-
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
configure -tls-server-name when global.cloud.enabled=true so that it matches the server certificate minted from HCP #1591
Conversation
…tificate minted from 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.
Maybe at some point we could standardize the flag name to either -tls-server-name or -consul-tls-server-name across all the commands but that's definitely not blocking for this fix!
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.
As Nitya suggested, it is a little unclear where we use consul-tls-server-name
and tls-server-name
and we should put something in the backlog to clear that up.
@@ -213,6 +213,8 @@ spec: | |||
{{- end }} | |||
{{- if and .Values.externalServers.enabled .Values.externalServers.tlsServerName }} | |||
-tls-server-name={{.Values.externalServers.tlsServerName }} \ |
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.
Should this be consul-tls-server-name
as well?
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 think this should be -tls-server-name
for consul-dataplane
and -consul-tls-server-name
for other commands.
We should probably clear this up though.
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 just updated this to use tls-server-name everywhere. I think differentiating for dataplane vs other components is more of an implementation detail, where this setting in every case is meant to set the TLSConfig.
@@ -268,7 +268,9 @@ spec: | |||
-ca-certs=/consul/tls/ca/tls.crt \ | |||
{{- end }} | |||
{{- if and $root.Values.externalServers.enabled $root.Values.externalServers.tlsServerName }} | |||
-tls-server-name={{$root.Values.externalServers.tlsServerName }} \ | |||
-consul-tls-server-name={{ $root.Values.externalServers.tlsServerName }} \ |
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.
Should this be -tls-server-name
?
https://github.com/hashicorp/consul-dataplane/blob/8ef98f45aae440bfec68f422a98520861e2b1c3c/cmd/consul-dataplane/main.go#L115
Sorry, the different names are super confusing.
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.
yes. thank you!
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'm not sure the flags are all being passed correctly, happy to pair and take a look with you, but feel free to ignore if I'm actually wrong :)
One other thing, non-blocking, since this is repeated verbatim in so many places, should we move it into a template in _helpers.tpl
?
{{- if and $root.Values.externalServers.enabled $root.Values.externalServers.tlsServerName }}
-tls-server-name={{$root.Values.externalServers.tlsServerName }} \
{{- else if $root.Values.global.cloud.enabled }}
-consul-tls-server-name=server.{{ $root.Values.global.datacenter}}.{{ $root.Values.global.domain}} \
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!!
@@ -19,7 +19,7 @@ var ( | |||
// A pre-release marker for the version. If this is "" (empty string) | |||
// then it means that it is a final release. Otherwise, this is a pre-release | |||
// such as "dev" (in development), "beta", "rc1", etc. | |||
VersionPrerelease = "dev" | |||
VersionPrerelease = "beta1" |
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.
Is this correct? I think this should still be dev
?
@@ -19,7 +19,7 @@ var ( | |||
// A pre-release marker for the version. If this is "" (empty string) | |||
// then it means that it is a final release. Otherwise, this is a pre-release | |||
// such as "dev" (in development), "beta", "rc1", etc. | |||
VersionPrerelease = "dev" | |||
VersionPrerelease = "beta1" |
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 should be dev
too
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
👀
Checklist: