Skip to content
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.

Sign up for GitHub

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

Allow configuring primary DC and gateways via designated Helm values #1038

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Allow configuring primary DC and gateways via designated Helm values #1038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93e6f56
Allow configuring primary DC and gateways via designated Helm values
ishustava Feb 18, 2022
4a1a4c9
Apply suggestions from code review
ishustava Feb 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions acceptance/framework/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ func (t *TestConfig) HelmValuesFromConfig() (map[string]string, error) {
return nil, err
}
setIfNotEmpty(helmValues, "global.image", entImage)
}

if t.EnterpriseLicense != "" {
setIfNotEmpty(helmValues, "global.enterpriseLicense.secretName", LicenseSecretName)
setIfNotEmpty(helmValues, "global.enterpriseLicense.secretKey", LicenseSecretKey)
if t.EnterpriseLicense != "" {
setIfNotEmpty(helmValues, "global.enterpriseLicense.secretName", LicenseSecretName)
setIfNotEmpty(helmValues, "global.enterpriseLicense.secretKey", LicenseSecretKey)
}
Comment on lines +67 to +70
Copy link
Contributor Author

@ishustava ishustava Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if t.EnableOpenshift {
Expand Down
5 changes: 4 additions & 1 deletion acceptance/framework/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,15 @@ func TestConfig_HelmValuesFromConfig(t *testing.T) {
{
"sets ent license secret",
TestConfig{
EnableEnterprise: true,
EnterpriseLicense: "ent-license",
ConsulImage: "consul:test-version",
},
map[string]string{
"global.enterpriseLicense.secretName": "license",
"global.enterpriseLicense.secretKey": "key",
"connectInject.transparentProxy.defaultEnabled": "false",
"global.image": "consul:test-version",
},
},
{
Expand Down Expand Up @@ -109,7 +112,7 @@ func TestConfig_HelmValuesFromConfig(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
values, err := tt.testConfig.HelmValuesFromConfig()
require.NoError(t, err)
require.Equal(t, values, tt.want)
require.Equal(t, tt.want, values)
})
}
}
Expand Down
6 changes: 3 additions & 3 deletions acceptance/tests/vault/vault_wan_fed_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,12 @@ func TestVault_WANFederationViaGateways(t *testing.T) {

// Get the address of the mesh gateway.
primaryMeshGWAddress := meshGatewayAddress(t, cfg, primaryCtx, consulReleaseName)
serverExtraConfig := fmt.Sprintf(`"{\"primary_gateways\":[\"%s\"]\,\"primary_datacenter\":\"dc1\"}"`, primaryMeshGWAddress)
secondaryConsulHelmValues := map[string]string{
"global.datacenter": "dc2",

"global.federation.enabled": "true",
"global.federation.enabled": "true",
"global.federation.primaryDatacenter": "dc1",
"global.federation.primaryGateways[0]": primaryMeshGWAddress,

// TLS config.
"global.tls.enabled": "true",
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ishustava ishustava Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"server.extraVolumes[0].type": "secret",
"server.extraVolumes[0].name": federationSecretName,
"server.extraVolumes[0].load": "true",
"server.extraVolumes[0].items[0].key": "serverConfigJSON",
"server.extraVolumes[0].items[0].path": "config.json",


// Vault config.
"global.secretsBackend.vault.enabled": "true",
Expand Down
7 changes: 7 additions & 0 deletions charts/consul/templates/server-config-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,11 @@ data:
{
"enable_central_service_config": true
}
{{- if .Values.global.federation.enabled }}
federation-config.json: |-
{
"primary_datacenter": "{{ .Values.global.federation.primaryDatacenter }}",
"primary_gateways": {{ .Values.global.federation.primaryGateways | toJson }}
}
{{- end }}
{{- end }}
38 changes: 38 additions & 0 deletions charts/consul/test/unit/server-config-configmap.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,4 +492,42 @@ load _helpers
. | tee /dev/stderr |
yq '.data["connect-ca-config.json"] | contains("\"ca_file\": \"/consul/vault-ca/tls.crt\"")' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

@test "server/ConfigMap: doesn't add federation config by default" {
cd `chart_dir`
local actual=$(helm template \
-s templates/server-config-configmap.yaml \
. | tee /dev/stderr |
yq '.data["federation-config.json"] | length > 0' | tee /dev/stderr)
[ "${actual}" = "false" ]
}

@test "server/ConfigMap: adds empty federation config when global.federation.enabled is true" {
cd `chart_dir`
local actual=$(helm template \
-s templates/server-config-configmap.yaml \
--set 'global.federation.enabled=true' \
--set 'global.tls.enabled=true' \
--set 'meshGateway.enabled=true' \
--set 'connectInject.enabled=true' \
. | tee /dev/stderr |
yq '.data["federation-config.json"]' | tee /dev/stderr)
[ "${actual}" = '"{\n \"primary_datacenter\": \"\",\n \"primary_gateways\": []\n}"' ]
}

@test "server/ConfigMap: can set primary dc and gateways when global.federation.enabled is true" {
cd `chart_dir`
local actual=$(helm template \
-s templates/server-config-configmap.yaml \
--set 'global.federation.enabled=true' \
--set 'global.federation.primaryDatacenter=dc1' \
--set 'global.federation.primaryGateways[0]=1.1.1.1:443' \
--set 'global.federation.primaryGateways[1]=2.2.2.2:443' \
--set 'global.tls.enabled=true' \
--set 'meshGateway.enabled=true' \
--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}"' ]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behavior if the user sets this configuration using serverExtraConfig and sets it using these flags? Is it worth testing that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

8 changes: 8 additions & 0 deletions charts/consul/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,14 @@ global:
# `<helm-release-name>-consul-federation`.
createFederationSecret: false

# The name of the primary datacenter.
primaryDatacenter: ""
Comment on lines +433 to +434
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# A list of addresses of the primary mesh gateways in the form <ip>:<port>.
ishustava marked this conversation as resolved.
Show resolved Hide resolved
# (e.g. ["1.1.1.1:443", "2.3.4.5:443"]
# @type: array<string>
primaryGateways: []

# Configures metrics for Consul service mesh
metrics:
# Configures the Helm chart’s components
Expand Down