Allow configuring primary DC and gateways via designated Helm values #1038
Conversation
ishustava
requested review from
a team,
curtbushko and
t-eckert
and removed request for
a team and
t-eckert
ishustava
force-pushed
the
ishustava/wan-fed-helm-values
branch
2 times, most recently
from
346f5a3 to
8abf5c3
Compare
ishustava
force-pushed
the
ishustava/wan-fed-helm-values
branch
from
8abf5c3 to
93e6f56
Compare
| if t.EnterpriseLicense != "" { | ||
| setIfNotEmpty(helmValues, "global.enterpriseLicense.secretName", LicenseSecretName) | ||
| setIfNotEmpty(helmValues, "global.enterpriseLicense.secretKey", LicenseSecretKey) | ||
| } |
There was a problem hiding this comment.
we only want to set it when enterprise is enabled. Otherwise, we will always set enterpriseLicense helm values which will now break when running with Vault.
| @@ -229,7 +230,6 @@ func TestVault_WANFederationViaGateways(t *testing.T) { | |||
| "server.extraVolumes[0].type": "secret", | |||
| "server.extraVolumes[0].name": vaultCASecretName, | |||
| "server.extraVolumes[0].load": "false", | |||
| "server.extraConfig": serverExtraConfig, | |||
There was a problem hiding this comment.
I was wondering if it would be worthwhile to keep a test where this is set via extra config, but I don't really think it is because at that point it would just be testing something that should be covered by the bats tests. So I think it's the right move to just modify the test.
There was a problem hiding this comment.
We do use server extra config (via volumes) in the mesh_gateway_test which doesn't use vault:
consul-k8s/acceptance/tests/mesh-gateway/mesh_gateway_test.go
Lines 76 to 80 in ae206c9
| # The name of the primary datacenter. | ||
| primaryDatacenter: "" |
There was a problem hiding this comment.
Depending on how much context a user would have by the time they are setting this value, it may be valuable to note what the significance of the primary datacenter is to the behavior of federation. In the past, users have been capable of figuring this out and adding it via extra config, so it may not be a problem.
There was a problem hiding this comment.
It also may be worthwhile to add further context to the parent of this item linking the user to the federation documentation: https://www.consul.io/docs/k8s/installation/multi-cluster
There was a problem hiding this comment.
I think most users are familiar with the concept of datacenter in Consul and also that is the value they provide via global.datacenter Helm value in the primary. I don't know if it makes sense to reference this value here because their primary datacenter could be on VMs. wdyt?
| --set 'connectInject.enabled=true' \ | ||
| . | tee /dev/stderr | | ||
| yq '.data["federation-config.json"]' | tee /dev/stderr) | ||
| [ "${actual}" = '"{\n \"primary_datacenter\": \"dc1\",\n \"primary_gateways\": [\"1.1.1.1:443\",\"2.2.2.2:443\"]\n}"' ] | ||
| } |
There was a problem hiding this comment.
What is the behavior if the user sets this configuration using serverExtraConfig and sets it using these flags? Is it worth testing that scenario?
There was a problem hiding this comment.
The server extraConfig will override these settings (this is also the case in the general with extraConfig: it'll override any settings we set in the helm chart). Good question about testing - I don't think so and it'll also be hard to test with helm templates because these are provided via separate flags to the consul agent command. And so we'll be testing that one flag is provided after the other, and I don't know if there's a lot of value in that.
There was a problem hiding this comment.
This looks fantastic, Iryna. I had some hot cocoa and free time so I went ahead and gave this a look tonight.
My only concern is making sure we understand the behavior if the user sets primary_datacenter and primary_gateways using both serverExtraConfig and the new keys.
I made the relevant comment in the bats test, but it really makes more sense in the integration test. All the same, I think it's an edge case.
Co-authored-by: Thomas Eckert <[email protected]>
Changes proposed in this PR:
global.federation.primaryDatacenterandglobal.federation.primaryGatewayto allow users to specify these values directly rather than viaserver.extraConfigorserver.extraVolumesNote that we will keep the existing behavior of
create-federation-secretthat generates the serverExtraConfig with those values for the time being and will re-assess it when the ACL work to switch out tokens for auth methods will be complete as it will likely affect it. So at the moment we will allow using both methods, and if the user provides these values via any of theserver.extra*values, they will take precedence.How I've tested this PR:
vault acceptance tests
How I expect reviewers to test this PR:
👀
Checklist: