-
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
Mount certs when using clients even with external servers #1759
Conversation
@@ -149,7 +149,7 @@ spec: | |||
- name: consul-bin | |||
mountPath: /consul-bin | |||
{{- end }} | |||
{{- if not (and .Values.externalServers.enabled .Values.externalServers.useSystemRoots) }} | |||
{{- if or (not (and .Values.externalServers.enabled .Values.externalServers.useSystemRoots)) .Values.client.enabled }} |
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.
This fix is repeated in 3 spots. It allows the use of certs to talk to local clients when connected to an external server like HCP.
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.
Update: this is added only here in order to have an auto-encrypt-based cert mounted to establish proper client-node communication if clients are explicitly enabled (in which case our controller still leverages the agent node) -- otherwise we'll try and use the system roots to verify a connection to the client node with a cert provisioned through the auto-encrypt process and the TLS handshake will fail.
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.
Approving so that we can get it in the release for tomorrow
@andrewstucki I will be watching this to see if there are any issues. If you don't see any, feel free to do the merge. |
@ishustava Looks like the enterprise-control-plane tests are failing, not sure if that's flaky or something just currently broken in
but other than that I think this is probably good to go with the smaller-scoped change given the earlier discussion. Let me know if there are any other concerns that you have and thanks again for reviewing. |
@andrewstucki looks like this one is a flake. It should be safe to re-run, and if it passes, we're good. |
Mount certs when using clients even with external servers
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: