-
Notifications
You must be signed in to change notification settings - Fork 321
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
vault: allow providing auth method path to the connect CA configuration #1029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Iryna!!
"global.secretsBackend.vault.connectCA.authMethodPath": "kubernetes-dc2", | ||
"global.secretsBackend.vault.connectCA.rootPKIPath": "connect_root", | ||
"global.secretsBackend.vault.connectCA.intermediatePKIPath": "dc2/connect_inter", | ||
"global.secretsBackend.vault.connectCA.additionalConfig": fmt.Sprintf(`"{"connect": [{"ca_config": [{"tls_server_name": "%s-vault"}]}]}"`, vaultReleaseName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be worth having a global.secretsBackend.vault.connectCA.dc value and then generating an authMethodPath and intermediatePKIPath based on that? Or would most folks want to just configure those paths themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize intermediatePKIPath is already a configurable value so maybe it has to be this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think in most cases we'd want the user to decide how these paths should be configured in their Vault. They'd have to have different intermediate paths per datacenter, but we don't want to necessarily enforce a naming convention for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE!!
3f5383c
to
43f6307
Compare
b02ab29
to
4afdd37
Compare
4afdd37
to
94094e2
Compare
Changes proposed in this PR:
authMethodPath
to the connect CA vault configuration so that you can use a different kubernetes auth method for a case when you are using a single vault server for multiple kubernetes clusters (WAN federation case).How I've tested this PR:
acceptance tests
How I expect reviewers to test this PR:
👀
Checklist: