Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin/secrets/auth: enable multiplexing #19215

Merged
merged 5 commits into from
Feb 16, 2023
Merged

Conversation

fairclothjm
Copy link
Contributor

  • the plugin will be multiplexed when run as an external plugin by vault versions that support secrets/auth plugin multiplexing (> 1.12)
  • we continue to set the TLSProviderFunc to maintain backwards compatibility with vault versions that don't support AutoMTLS for secrets/auth plugins (< 1.12)

- the plugin will be multiplexed when run as an external plugin
  by vault versions that support secrets/auth plugin multiplexing (> 1.12)
- we continue to set the TLSProviderFunc to maintain backwards
  compatibility with vault versions that don't support AutoMTLS (< 1.12)
@fairclothjm fairclothjm added this to the 1.14 milestone Feb 16, 2023
@fairclothjm fairclothjm requested review from a team February 16, 2023 15:48
@fairclothjm fairclothjm changed the title plugin/auth: enable multiplexing plugin/secrets/auth: enable multiplexing Feb 16, 2023
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I did a spot check on the AWS and Transit.

It looks like the Transit plugin doesn't work as a standalone plugin (even before this change) :( But AWS worked fine.

@cipherboy
Copy link
Contributor

@swenson I think PKI and Transit will not work as a standalone plugin due to requiring managed key support among other things, which isn't exposed via the external plugin GRPC interface.

@fairclothjm
Copy link
Contributor Author

@swenson @cipherboy Thanks! I don't think there will be any problems with calling ServeMultiplex here for PKI and Transit. However, I am unsure if I should keep this change for them. I am not sure if it will cause any confusion for people reading the code? Any thoughts?

@cipherboy
Copy link
Contributor

cipherboy commented Feb 16, 2023

@fairclothjm Not sure, we're about to add these for SSH as well, so perhaps its just better to leave them consistent across all plugins and we can sort this out later? Are we expecting customers to attempt to run these externally, now?

@fairclothjm
Copy link
Contributor Author

@cipherboy

Are we expecting customers to attempt to run these externally, now?

No, I don't expect so.

so perhaps its just better to leave them consistent across all plugins and we ca sort this out later

sounds good to me

@fairclothjm fairclothjm enabled auto-merge (squash) February 16, 2023 21:55
@fairclothjm fairclothjm merged commit 4bfc649 into main Feb 16, 2023
@fairclothjm fairclothjm deleted the auth-enable-multiplexing branch February 16, 2023 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants