-
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
NET-581 - Added vault namespace in helm #2841
NET-581 - Added vault namespace in helm #2841
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 good to me overall! Added some minor feedback inline.
One clarifying question about requirements: I think @david-yu 's example in the ticket had namespace
under the higher-level secretsBackend.vault
key. I'm not opposed to keeping it under connectCA
because that aligns w/ the existing agent config, but I could see a higher-level key being used for other namespaced Vault config, if we expect that one Vault namespace would always be used across Vault->Consul integrations (I'm not 100% certain whether that's the case).
- Do we think it's best to move the namespace KV up, or keep it under
connectCA
? - If we move it, a suggestion in one of the linked Slack 🧵s was to call it
vaultNamespace
to disambiguate from Consul. That would align w/ other names such asconsulServerRole
.
@curtbushko Is there a reason why you only added backport 1.2.x instead of 1.0.x, 1.1.x and 1.2.x? |
@david-yu No reason. I was just doing a passby label adding. I didn't look too deep to see where this should go. |
I would prefer it be under the
Behind the scenes it should also set the following annotation to
It would be best to call it |
Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
@david-yu I am not super familiar with vault namespace. but can this situation arise where user would want different vault namespace for agentAnnotation and different for connectCA? |
I don't think it's very likely that the Connect CA will live in a different place than the Vault KV store but you can have it separate if they want to do that. I would think we set the namespace to the value provided by |
Thanks for reply. Also can user give some other annotations as well for vault? we are going to remove agentAnnotations field right. |
@david-yu Removing agent annotations is probably incorrect as I see there are other annotations as well which users can add - https://developer.hashicorp.com/vault/docs/platform/k8s/injector/annotations. Let me know you thoughts. |
Added consul-docs for review of values.yaml |
@@ -72,6 +72,9 @@ data: | |||
"ca_file": "/consul/vault-ca/tls.crt", | |||
{{- end }} | |||
"intermediate_pki_path": "{{ .connectCA.intermediatePKIPath }}", | |||
{{- if (and (.vaultNamespace) (not (contains "namespace" (default "" .connectCA.additionalConfig)))) }} |
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.
@absolutelightning good catch, my example was off - but I think this is still possible, see this updated version.
From the link above, if you use:
{{- if (and (.vaultNamespace) (not (hasKey (default "" .connectCA.additionalConfig | fromJson).connect.ca_config "namespace"))) }}
"namespace": "{{ .vaultNamespace }}",
{{- end }}
does that work?
Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
Co-authored-by: David Yu <dyu@hashicorp.com>
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.
LGTM pending any input from docs team or @david-yu. Thank you again for working through my feedback and questions, @absolutelightning !
Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>
Thanks @zalimeni for review. |
* added namespace * namespace in connect ca * updated tests * fix test desc * changelog * Update .changelog/2841.txt Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * removed new line added * fix templates * bats test * fix double colon * fix template * added 2 more tests * fixes bats tests * fix json in api gateway * updated bats test * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * fix client daemon set bats * fix bats test * fix bats * api gateway fix * fix bats * fix clientdaemon set and api gateway controller * fix connect inject deployment * fix mesh gateway deployment * added tests for partition init job * server acl init job tests added * fix server stateful bats * fix sync catalog * fix includes check * bats test fixes * fix connect inject * fix yaml * fix yaml * fix assertions in bats * fix client daemon set bats * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/templates/server-config-configmap.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * change yaml * added addional config test * fix tests * added more tests * fix bats * Update charts/consul/test/unit/server-config-configmap.bats Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/test/unit/server-config-configmap.bats Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update .changelog/2841.txt Co-authored-by: David Yu <dyu@hashicorp.com> * Update .changelog/2841.txt Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * added dummy commit to run CI * fix change log * fix comment --------- Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> Co-authored-by: David Yu <dyu@hashicorp.com>
* added namespace * namespace in connect ca * updated tests * fix test desc * changelog * Update .changelog/2841.txt Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * removed new line added * fix templates * bats test * fix double colon * fix template * added 2 more tests * fixes bats tests * fix json in api gateway * updated bats test * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * fix client daemon set bats * fix bats test * fix bats * api gateway fix * fix bats * fix clientdaemon set and api gateway controller * fix connect inject deployment * fix mesh gateway deployment * added tests for partition init job * server acl init job tests added * fix server stateful bats * fix sync catalog * fix includes check * bats test fixes * fix connect inject * fix yaml * fix yaml * fix assertions in bats * fix client daemon set bats * Update charts/consul/values.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/templates/server-config-configmap.yaml Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * change yaml * added addional config test * fix tests * added more tests * fix bats * Update charts/consul/test/unit/server-config-configmap.bats Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update charts/consul/test/unit/server-config-configmap.bats Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * Update .changelog/2841.txt Co-authored-by: David Yu <dyu@hashicorp.com> * Update .changelog/2841.txt Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> * added dummy commit to run CI * fix change log * fix comment --------- Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> Co-authored-by: David Yu <dyu@hashicorp.com>
NET-581 - Added vault namespace in helm (#2841) * added namespace * namespace in connect ca * updated tests * fix test desc * changelog * Update .changelog/2841.txt * Update charts/consul/values.yaml * removed new line added * fix templates * bats test * fix double colon * fix template * added 2 more tests * fixes bats tests * fix json in api gateway * updated bats test * Update charts/consul/values.yaml * fix client daemon set bats * fix bats test * fix bats * api gateway fix * fix bats * fix clientdaemon set and api gateway controller * fix connect inject deployment * fix mesh gateway deployment * added tests for partition init job * server acl init job tests added * fix server stateful bats * fix sync catalog * fix includes check * bats test fixes * fix connect inject * fix yaml * fix yaml * fix assertions in bats * fix client daemon set bats * Update charts/consul/values.yaml * Update charts/consul/templates/server-config-configmap.yaml * change yaml * added addional config test * fix tests * added more tests * fix bats * Update charts/consul/test/unit/server-config-configmap.bats * Update charts/consul/test/unit/server-config-configmap.bats * Update .changelog/2841.txt * Update .changelog/2841.txt * added dummy commit to run CI * fix change log * fix comment --------- Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> Co-authored-by: David Yu <dyu@hashicorp.com>
NET-581 - Added vault namespace in helm (#2841) * added namespace * namespace in connect ca * updated tests * fix test desc * changelog * Update .changelog/2841.txt * Update charts/consul/values.yaml * removed new line added * fix templates * bats test * fix double colon * fix template * added 2 more tests * fixes bats tests * fix json in api gateway * updated bats test * Update charts/consul/values.yaml * fix client daemon set bats * fix bats test * fix bats * api gateway fix * fix bats * fix clientdaemon set and api gateway controller * fix connect inject deployment * fix mesh gateway deployment * added tests for partition init job * server acl init job tests added * fix server stateful bats * fix sync catalog * fix includes check * bats test fixes * fix connect inject * fix yaml * fix yaml * fix assertions in bats * fix client daemon set bats * Update charts/consul/values.yaml * Update charts/consul/templates/server-config-configmap.yaml * change yaml * added addional config test * fix tests * added more tests * fix bats * Update charts/consul/test/unit/server-config-configmap.bats * Update charts/consul/test/unit/server-config-configmap.bats * Update .changelog/2841.txt * Update .changelog/2841.txt * added dummy commit to run CI * fix change log * fix comment --------- Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com> Co-authored-by: David Yu <dyu@hashicorp.com>
Changes proposed in this PR:
vaultNamespace
insecretsBackend.vault
invalues.yaml
"{"connect": [{ "ca_config": [{ "namespace": "value"}]}]}"
inconnectCA.additionalConfig
vaultNamespace
is present, it automatically adds annotation below to the templates.How I've tested this PR:
a. CI
b. Updated test
TestVault_VaultNamespace
Test Steps -
Output -
How I expect reviewers to test this PR:
Checklist: