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
jmurret
requested review from
kschoche,
curtbushko,
ndhanushkodi,
thisisnotashwin and
t-eckert
| @@ -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.
Should this be consul-tls-server-name as well?
There was a problem hiding this comment.
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.
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.
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.
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}} \
| @@ -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.
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.
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: