From 65d3b5a4863cd5e47c7cceb78ebcad43f7f759ad Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Thu, 14 Dec 2023 11:58:00 -0600 Subject: [PATCH] Backport of Prevent extra-config from being loaded twice (and erroring for segment config) on clients and servers into release/1.2.x (#3373) Prevent extra-config from being loaded twice (and erroring for segment config) on clients and servers (#3337) * wip: testing with server works when you add segments as extraValues. Todos: * make similar changes to clients * potentially upgrade test? * consider locality having its own volume, rather than 2 volumes with extra in them * move extra-config out of /consul/config so it does not get applied twice * add comments about use of additional config maps * remove temporary inclusion of values.yaml in root that was used for hand off * get rid of temporary config.file * add segments test * test using 3 servers in a single cluster * add changelog * fix linting issues. * add comment to test. remove extra lines from config map. * fix bats tests --------- Co-authored-by: John Murret Co-authored-by: Nitya Dhanushkodi --- .changelog/3337.txt | 3 + acceptance/tests/segments/main_test.go | 18 +++++ acceptance/tests/segments/segments_test.go | 75 +++++++++++++++++++ charts/consul/templates/_helpers.tpl | 2 +- .../templates/client-config-configmap.yaml | 3 - charts/consul/templates/client-daemonset.yaml | 9 ++- .../client-tmp-extra-config-configmap.yaml | 21 ++++++ .../templates/server-config-configmap.yaml | 2 - .../consul/templates/server-statefulset.yaml | 10 ++- .../server-tmp-extra-config-configmap.yaml | 21 ++++++ .../test/unit/client-config-configmap.bats | 11 --- charts/consul/test/unit/client-daemonset.bats | 6 +- .../client-tmp-extra-config-configmap.bats | 43 +++++++++++ .../test/unit/server-config-configmap.bats | 10 --- .../consul/test/unit/server-statefulset.bats | 6 +- .../server-tmp-extra-config-configmap.bats | 49 ++++++++++++ 16 files changed, 251 insertions(+), 38 deletions(-) create mode 100644 .changelog/3337.txt create mode 100644 acceptance/tests/segments/main_test.go create mode 100644 acceptance/tests/segments/segments_test.go create mode 100644 charts/consul/templates/client-tmp-extra-config-configmap.yaml create mode 100644 charts/consul/templates/server-tmp-extra-config-configmap.yaml create mode 100755 charts/consul/test/unit/client-tmp-extra-config-configmap.bats create mode 100755 charts/consul/test/unit/server-tmp-extra-config-configmap.bats diff --git a/.changelog/3337.txt b/.changelog/3337.txt new file mode 100644 index 0000000000..da9e7d5c70 --- /dev/null +++ b/.changelog/3337.txt @@ -0,0 +1,3 @@ +```release-note:bug-fix +mesh: prevent extra-config from being loaded twice (and erroring for segment config) on clients and servers. +``` \ No newline at end of file diff --git a/acceptance/tests/segments/main_test.go b/acceptance/tests/segments/main_test.go new file mode 100644 index 0000000000..4d66c9ae4f --- /dev/null +++ b/acceptance/tests/segments/main_test.go @@ -0,0 +1,18 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package segments + +import ( + "os" + "testing" + + testsuite "github.com/hashicorp/consul-k8s/acceptance/framework/suite" +) + +var suite testsuite.Suite + +func TestMain(m *testing.M) { + suite = testsuite.NewSuite(m) + os.Exit(suite.Run()) +} diff --git a/acceptance/tests/segments/segments_test.go b/acceptance/tests/segments/segments_test.go new file mode 100644 index 0000000000..53e8d7cdcf --- /dev/null +++ b/acceptance/tests/segments/segments_test.go @@ -0,0 +1,75 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package segments + +import ( + "testing" + + "github.com/hashicorp/consul-k8s/acceptance/framework/connhelper" + "github.com/hashicorp/consul-k8s/acceptance/framework/consul" + "github.com/hashicorp/consul-k8s/acceptance/framework/helpers" +) + +// TestSegments_MeshWithAgentfulClients is a simple test that verifies that +// the Consul service mesh can be configured to use segments with: +// - one cluster with an alpha segment configured on the servers. +// - clients enabled and joining the alpha segment. +// - static client can communicate with static server. +func TestSegments_MeshWithAgentfulClients(t *testing.T) { + cases := map[string]struct { + secure bool + }{ + "not-secure": {secure: false}, + "secure": {secure: true}, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + cfg := suite.Config() + if !cfg.EnableEnterprise { + t.Skipf("skipping this test because -enable-enterprise is not set") + } + ctx := suite.Environment().DefaultContext(t) + + releaseName := helpers.RandomName() + + helmValues := map[string]string{ + "connectInject.enabled": "true", + + "server.replicas": "3", + "server.extraConfig": `"{\"segments\": [{\"name\":\"alpha1\"\,\"bind\":\"0.0.0.0\"\,\"port\":8303}]}"`, + + "client.enabled": "true", + // need to configure clients to connect to port 8303 that the alpha segment was configured on rather than + // the standard serf LAN port. + "client.join[0]": "${CONSUL_FULLNAME}-server-0.${CONSUL_FULLNAME}-server.${NAMESPACE}.svc:8303", + "client.join[1]": "${CONSUL_FULLNAME}-server-1.${CONSUL_FULLNAME}-server.${NAMESPACE}.svc:8303", + "client.join[2]": "${CONSUL_FULLNAME}-server-2.${CONSUL_FULLNAME}-server.${NAMESPACE}.svc:8303", + "client.extraConfig": `"{\"segment\": \"alpha1\"}"`, + } + + connHelper := connhelper.ConnectHelper{ + ClusterKind: consul.Helm, + Secure: c.secure, + ReleaseName: releaseName, + Ctx: ctx, + UseAppNamespace: cfg.EnableRestrictedPSAEnforcement, + Cfg: cfg, + HelmValues: helmValues, + } + + connHelper.Setup(t) + + connHelper.Install(t) + connHelper.DeployClientAndServer(t) + if c.secure { + connHelper.TestConnectionFailureWithoutIntention(t) + connHelper.CreateIntention(t) + } + + connHelper.TestConnectionSuccess(t) + connHelper.TestConnectionFailureWhenUnhealthy(t) + }) + } +} diff --git a/charts/consul/templates/_helpers.tpl b/charts/consul/templates/_helpers.tpl index 4248b8f64e..8507b2103a 100644 --- a/charts/consul/templates/_helpers.tpl +++ b/charts/consul/templates/_helpers.tpl @@ -145,7 +145,7 @@ substitution for HOST_IP/POD_IP/HOSTNAME. Useful for dogstats telemetry. The out is passed to consul as a -config-file param on command line. */}} {{- define "consul.extraconfig" -}} - cp /consul/config/extra-from-values.json /consul/extra-config/extra-from-values.json + cp /consul/tmp/extra-config/extra-from-values.json /consul/extra-config/extra-from-values.json [ -n "${HOST_IP}" ] && sed -Ei "s|HOST_IP|${HOST_IP?}|g" /consul/extra-config/extra-from-values.json [ -n "${POD_IP}" ] && sed -Ei "s|POD_IP|${POD_IP?}|g" /consul/extra-config/extra-from-values.json [ -n "${HOSTNAME}" ] && sed -Ei "s|HOSTNAME|${HOSTNAME?}|g" /consul/extra-config/extra-from-values.json diff --git a/charts/consul/templates/client-config-configmap.yaml b/charts/consul/templates/client-config-configmap.yaml index d91a4d21bf..cab2c7c043 100644 --- a/charts/consul/templates/client-config-configmap.yaml +++ b/charts/consul/templates/client-config-configmap.yaml @@ -25,13 +25,10 @@ data: "log_level": "{{ .Values.client.logLevel | upper }}" {{- end }} } - extra-from-values.json: |- -{{ tpl .Values.client.extraConfig . | trimAll "\"" | indent 4 }} central-config.json: |- { "enable_central_service_config": true } - {{- if .Values.connectInject.enabled }} {{/* We set check_update_interval to 0s so that check output is immediately viewable in the UI. */}} diff --git a/charts/consul/templates/client-daemonset.yaml b/charts/consul/templates/client-daemonset.yaml index 61425cfdb8..2812417dbf 100644 --- a/charts/consul/templates/client-daemonset.yaml +++ b/charts/consul/templates/client-daemonset.yaml @@ -86,7 +86,7 @@ spec: {{- end }} {{- end }} "consul.hashicorp.com/connect-inject": "false" - "consul.hashicorp.com/config-checksum": {{ include (print $.Template.BasePath "/client-config-configmap.yaml") . | sha256sum }} + "consul.hashicorp.com/config-checksum": {{ print (include (print $.Template.BasePath "/client-config-configmap.yaml") .) (include (print $.Template.BasePath "/client-tmp-extra-config-configmap.yaml") .) | sha256sum }} {{- if .Values.client.annotations }} {{- tpl .Values.client.annotations . | nindent 8 }} {{- end }} @@ -141,6 +141,9 @@ spec: - name: consul-data emptyDir: medium: "Memory" + - name: tmp-extra-config + configMap: + name: {{ template "consul.fullname" . }}-client-tmp-extra-config {{- if .Values.global.tls.enabled }} {{- if not .Values.global.secretsBackend.vault.enabled }} - name: consul-ca-cert @@ -389,7 +392,7 @@ spec: {{- range $value := .Values.global.recursors }} -recursor={{ quote $value }} \ {{- end }} - -config-file=/consul/extra-config/extra-from-values.json \ + -config-dir=/consul/extra-config \ -domain={{ .Values.global.domain }} volumeMounts: - name: data @@ -398,6 +401,8 @@ spec: mountPath: /consul/config - name: extra-config mountPath: /consul/extra-config + - name: tmp-extra-config + mountPath: /consul/tmp/extra-config - mountPath: /consul/login name: consul-data readOnly: true diff --git a/charts/consul/templates/client-tmp-extra-config-configmap.yaml b/charts/consul/templates/client-tmp-extra-config-configmap.yaml new file mode 100644 index 0000000000..a379157f57 --- /dev/null +++ b/charts/consul/templates/client-tmp-extra-config-configmap.yaml @@ -0,0 +1,21 @@ +{{- if (or (and (ne (.Values.client.enabled | toString) "-") .Values.client.enabled) (and (eq (.Values.client.enabled | toString) "-") .Values.global.enabled)) }} +# ConfigMap that is used as a temporary landing spot so that the container command +# in the client-daemonset where it needs to be transformed. ConfigMaps create +# read only volumes so it needs to be copied and transformed to the extra-config +# emptyDir volume where all final extra cofngi lives for use in consul. (locality-init +# also writes to extra-config volume.) +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ template "consul.fullname" . }}-client-tmp-extra-config + namespace: {{ .Release.Namespace }} + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: client +data: + extra-from-values.json: |- +{{ tpl .Values.client.extraConfig . | trimAll "\"" | indent 4 }} +{{- end }} diff --git a/charts/consul/templates/server-config-configmap.yaml b/charts/consul/templates/server-config-configmap.yaml index 9ebfbd2571..8cd726f445 100644 --- a/charts/consul/templates/server-config-configmap.yaml +++ b/charts/consul/templates/server-config-configmap.yaml @@ -95,8 +95,6 @@ data: {{- end }} {{- end }} {{- end }} - extra-from-values.json: |- -{{ tpl .Values.server.extraConfig . | trimAll "\"" | indent 4 }} {{- if .Values.global.acls.manageSystemACLs }} acl-config.json: |- { diff --git a/charts/consul/templates/server-statefulset.yaml b/charts/consul/templates/server-statefulset.yaml index a224340d77..7421fbc4a6 100644 --- a/charts/consul/templates/server-statefulset.yaml +++ b/charts/consul/templates/server-statefulset.yaml @@ -118,7 +118,7 @@ spec: {{- end }} {{- end }} "consul.hashicorp.com/connect-inject": "false" - "consul.hashicorp.com/config-checksum": {{ include (print $.Template.BasePath "/server-config-configmap.yaml") . | sha256sum }} + "consul.hashicorp.com/config-checksum": {{ print (include (print $.Template.BasePath "/server-config-configmap.yaml") .) (include (print $.Template.BasePath "/server-tmp-extra-config-configmap.yaml") .) | sha256sum }} {{- if .Values.server.annotations }} {{- tpl .Values.server.annotations . | nindent 8 }} {{- end }} @@ -158,6 +158,9 @@ spec: name: {{ template "consul.fullname" . }}-server-config - name: extra-config emptyDir: {} + - name: tmp-extra-config + configMap: + name: {{ template "consul.fullname" . }}-server-tmp-extra-config {{- if (and .Values.global.tls.enabled (not .Values.global.secretsBackend.vault.enabled)) }} - name: consul-ca-cert secret: @@ -421,8 +424,7 @@ spec: -config-dir=/consul/userconfig/{{ .name }} \ {{- end }} {{- end }} - -config-file=/consul/extra-config/extra-from-values.json \ - -config-file=/consul/extra-config/locality.json + -config-dir=/consul/extra-config \ {{- if and .Values.global.cloud.enabled .Values.global.cloud.resourceId.secretName }} -hcl="cloud { resource_id = \"${HCP_RESOURCE_ID}\" }" {{- end }} @@ -433,6 +435,8 @@ spec: mountPath: /consul/config - name: extra-config mountPath: /consul/extra-config + - name: tmp-extra-config + mountPath: /consul/tmp/extra-config {{- if (and .Values.global.tls.enabled (not .Values.global.secretsBackend.vault.enabled)) }} - name: consul-ca-cert mountPath: /consul/tls/ca/ diff --git a/charts/consul/templates/server-tmp-extra-config-configmap.yaml b/charts/consul/templates/server-tmp-extra-config-configmap.yaml new file mode 100644 index 0000000000..a42d6d09f6 --- /dev/null +++ b/charts/consul/templates/server-tmp-extra-config-configmap.yaml @@ -0,0 +1,21 @@ +{{- if (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) }} +# ConfigMap that is used as a temporary landing spot so that the container command +# in the server-stateful set where it needs to be transformed. ConfigMaps create +# read only volumes so it needs to be copied and transformed to the extra-config +# emptyDir volume where all final extra cofngi lives for use in consul. (locality-init +# also writes to extra-config volume.) +apiVersion: v1 +kind: ConfigMap +metadata: + name: {{ template "consul.fullname" . }}-server-tmp-extra-config + namespace: {{ .Release.Namespace }} + labels: + app: {{ template "consul.name" . }} + chart: {{ template "consul.chart" . }} + heritage: {{ .Release.Service }} + release: {{ .Release.Name }} + component: server +data: + extra-from-values.json: |- +{{ tpl .Values.server.extraConfig . | trimAll "\"" | indent 4 }} +{{- end }} \ No newline at end of file diff --git a/charts/consul/test/unit/client-config-configmap.bats b/charts/consul/test/unit/client-config-configmap.bats index 1f1443a156..7d0d424499 100755 --- a/charts/consul/test/unit/client-config-configmap.bats +++ b/charts/consul/test/unit/client-config-configmap.bats @@ -31,17 +31,6 @@ load _helpers . } -@test "client/ConfigMap: extraConfig is set" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/client-config-configmap.yaml \ - --set 'client.enabled=true' \ - --set 'client.extraConfig="{\"hello\": \"world\"}"' \ - . | tee /dev/stderr | - yq '.data["extra-from-values.json"] | match("world") | length > 1' | tee /dev/stderr) - [ "${actual}" = "true" ] -} - #-------------------------------------------------------------------- # connectInject.centralConfig [DEPRECATED] diff --git a/charts/consul/test/unit/client-daemonset.bats b/charts/consul/test/unit/client-daemonset.bats index ff9288a51d..0a69df37a4 100755 --- a/charts/consul/test/unit/client-daemonset.bats +++ b/charts/consul/test/unit/client-daemonset.bats @@ -621,7 +621,7 @@ load _helpers --set 'client.enabled=true' \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = 4fa9ddc3abc4c79eafccb19e5beef80006b7c9736b867d8873554ca03f42a6b3 ] + [ "${actual}" = 678c5c1c2ca0f8cb1464d38636f12714c05df26fab1a101e43ce619fdbc2e7d1 ] } @test "client/DaemonSet: config-checksum annotation changes when extraConfig is provided" { @@ -632,7 +632,7 @@ load _helpers --set 'client.extraConfig="{\"hello\": \"world\"}"' \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = 42b99932385e7a0580b134fe36a9bda405aab2e375593326677b9838708f0796 ] + [ "${actual}" = 0ef58da6fd14fb57c702a2a0d631c4eecacff152fe3a36836a23283b19d8dbe1 ] } @test "client/DaemonSet: config-checksum annotation changes when connectInject.enabled=true" { @@ -643,7 +643,7 @@ load _helpers --set 'connectInject.enabled=true' \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = 4fa9ddc3abc4c79eafccb19e5beef80006b7c9736b867d8873554ca03f42a6b3 ] + [ "${actual}" = 678c5c1c2ca0f8cb1464d38636f12714c05df26fab1a101e43ce619fdbc2e7d1 ] } #-------------------------------------------------------------------- diff --git a/charts/consul/test/unit/client-tmp-extra-config-configmap.bats b/charts/consul/test/unit/client-tmp-extra-config-configmap.bats new file mode 100755 index 0000000000..435ed54ce9 --- /dev/null +++ b/charts/consul/test/unit/client-tmp-extra-config-configmap.bats @@ -0,0 +1,43 @@ +#!/usr/bin/env bats + +load _helpers + +@test "client/TmpExtraConfigMap: enable with global.enabled false" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/client-tmp-extra-config-configmap.yaml \ + --set 'client.enabled=true' \ + --set 'global.enabled=false' \ + --set 'client.enabled=true' \ + . | tee /dev/stderr | + yq 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "client/TmpExtraConfigMap: disable with client.enabled false" { + cd `chart_dir` + assert_empty helm template \ + -s templates/client-tmp-extra-config-configmap.yaml \ + --set 'client.enabled=true' \ + --set 'client.enabled=false' \ + . +} + +@test "client/TmpExtraConfigMap: disable with global.enabled" { + cd `chart_dir` + assert_empty helm template \ + -s templates/client-tmp-extra-config-configmap.yaml \ + --set 'global.enabled=false' \ + . +} + +@test "client/TmpExtraConfigMap: extraConfig is set" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/client-tmp-extra-config-configmap.yaml \ + --set 'client.enabled=true' \ + --set 'client.extraConfig="{\"hello\": \"world\"}"' \ + . | tee /dev/stderr | + yq '.data["extra-from-values.json"] | match("world") | length > 1' | tee /dev/stderr) + [ "${actual}" = "true" ] +} diff --git a/charts/consul/test/unit/server-config-configmap.bats b/charts/consul/test/unit/server-config-configmap.bats index e074c3b1ae..5a9d07c7de 100755 --- a/charts/consul/test/unit/server-config-configmap.bats +++ b/charts/consul/test/unit/server-config-configmap.bats @@ -38,16 +38,6 @@ load _helpers . } -@test "server/ConfigMap: extraConfig is set" { - cd `chart_dir` - local actual=$(helm template \ - -s templates/server-config-configmap.yaml \ - --set 'server.extraConfig="{\"hello\": \"world\"}"' \ - . | tee /dev/stderr | - yq '.data["extra-from-values.json"] | match("world") | length' | tee /dev/stderr) - [ ! -z "${actual}" ] -} - #-------------------------------------------------------------------- # retry-join diff --git a/charts/consul/test/unit/server-statefulset.bats b/charts/consul/test/unit/server-statefulset.bats index 449657bc71..124c83306c 100755 --- a/charts/consul/test/unit/server-statefulset.bats +++ b/charts/consul/test/unit/server-statefulset.bats @@ -788,7 +788,7 @@ load _helpers -s templates/server-statefulset.yaml \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = 3714bf1fbca840dcd10b5aeb40c6ef35e349bd534727abe80b831c77f88da7da ] + [ "${actual}" = 0e599137f8357c786d46e1b694d7d867c541cb34d6056241a037afd0de14866b ] } @test "server/StatefulSet: adds config-checksum annotation when extraConfig is provided" { @@ -798,7 +798,7 @@ load _helpers --set 'server.extraConfig="{\"hello\": \"world\"}"' \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = 87126fac3c7704fba1d4265201ad0345f4d972d2123df6e6fb416b40c5823d80 ] + [ "${actual}" = 3f54c51be3473d7ae4cb91c24ba03263b7700d9a3dc3196f624ce3c6c8e93b8f ] } @test "server/StatefulSet: adds config-checksum annotation when config is updated" { @@ -808,7 +808,7 @@ load _helpers --set 'global.acls.manageSystemACLs=true' \ . | tee /dev/stderr | yq -r '.spec.template.metadata.annotations."consul.hashicorp.com/config-checksum"' | tee /dev/stderr) - [ "${actual}" = d98ff135c5dee661058f33e29970675761ecf235676db0d4d24f354908eee425 ] + [ "${actual}" = b44c82c9e4732433f54eeed8a299f11de0bad82a920047c8a3ad039e512ba281 ] } #-------------------------------------------------------------------- diff --git a/charts/consul/test/unit/server-tmp-extra-config-configmap.bats b/charts/consul/test/unit/server-tmp-extra-config-configmap.bats new file mode 100755 index 0000000000..213cc0cbc8 --- /dev/null +++ b/charts/consul/test/unit/server-tmp-extra-config-configmap.bats @@ -0,0 +1,49 @@ +#!/usr/bin/env bats + +load _helpers + +@test "server/TmpConfigMap: enabled by default" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-tmp-extra-config-configmap.yaml \ + . | tee /dev/stderr | + yq 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "server/TmpConfigMap: enable with global.enabled false" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-tmp-extra-config-configmap.yaml \ + --set 'global.enabled=false' \ + --set 'server.enabled=true' \ + . | tee /dev/stderr | + yq 'length > 0' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + +@test "server/TmpConfigMap: disable with server.enabled" { + cd `chart_dir` + assert_empty helm template \ + -s templates/server-tmp-extra-config-configmap.yaml \ + --set 'server.enabled=false' \ + . +} + +@test "server/TmpConfigMap: disable with global.enabled" { + cd `chart_dir` + assert_empty helm template \ + -s templates/server-tmp-extra-config-configmap.yaml \ + --set 'global.enabled=false' \ + . +} + +@test "server/TmpConfigMap: extraConfig is set" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/server-tmp-extra-config-configmap.yaml \ + --set 'server.extraConfig="{\"hello\": \"world\"}"' \ + . | tee /dev/stderr | + yq '.data["extra-from-values.json"] | match("world") | length' | tee /dev/stderr) + [ ! -z "${actual}" ] +}