Skip to content

Commit

Permalink
Backport of Prevent extra-config from being loaded twice (and errorin…
Browse files Browse the repository at this point in the history
…g 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 <john.murret@hashicorp.com>
Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>
  • Loading branch information
3 people authored Dec 14, 2023
1 parent e2dc674 commit 65d3b5a
Show file tree
Hide file tree
Showing 16 changed files with 251 additions and 38 deletions.
3 changes: 3 additions & 0 deletions .changelog/3337.txt
Original file line number Diff line number Diff line change
@@ -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.
```
18 changes: 18 additions & 0 deletions acceptance/tests/segments/main_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
75 changes: 75 additions & 0 deletions acceptance/tests/segments/segments_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
2 changes: 1 addition & 1 deletion charts/consul/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions charts/consul/templates/client-config-configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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. */}}
Expand Down
9 changes: 7 additions & 2 deletions charts/consul/templates/client-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
21 changes: 21 additions & 0 deletions charts/consul/templates/client-tmp-extra-config-configmap.yaml
Original file line number Diff line number Diff line change
@@ -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 }}
2 changes: 0 additions & 2 deletions charts/consul/templates/server-config-configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
{
Expand Down
10 changes: 7 additions & 3 deletions charts/consul/templates/server-statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 }}
Expand All @@ -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/
Expand Down
21 changes: 21 additions & 0 deletions charts/consul/templates/server-tmp-extra-config-configmap.yaml
Original file line number Diff line number Diff line change
@@ -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 }}
11 changes: 0 additions & 11 deletions charts/consul/test/unit/client-config-configmap.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
6 changes: 3 additions & 3 deletions charts/consul/test/unit/client-daemonset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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" {
Expand All @@ -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 ]
}

#--------------------------------------------------------------------
Expand Down
43 changes: 43 additions & 0 deletions charts/consul/test/unit/client-tmp-extra-config-configmap.bats
Original file line number Diff line number Diff line change
@@ -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" ]
}
10 changes: 0 additions & 10 deletions charts/consul/test/unit/server-config-configmap.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 3 additions & 3 deletions charts/consul/test/unit/server-statefulset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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" {
Expand All @@ -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 ]
}

#--------------------------------------------------------------------
Expand Down
Loading

0 comments on commit 65d3b5a

Please sign in to comment.