helm: default dns values inherit from connectInject.transparentProxy.defaultEnabled

CHANGELOG.md

@@ -9,6 +9,9 @@ BREAKING_CHANGES:
       * `client.snaphostAgent.replicas`
       * `client.snaphostAgent.serviceAccount`
     * Remove `global.secretsBackend.vault.consulSnapshotAgentRole` value. You should now use the `global.secretsBackend.vault.consulServerRole` for access to any Vault secrets. 
+  * Change`dns.enabled` and `dns.enableRedirection` to default to the value of `connectInject.transparentProxy.defaultEnabled`.
+    Previously, `dns.enabled` defaulted to the value of `global.enabled` and `dns.enableRedirection` defaulted to the
+    value to `false`. [[GH-1688](https://github.com/hashicorp/consul-k8s/pull/1688)]
 * Peering:
   * Remove support for customizing the server addresses in peering token generation. Instead, mesh gateways should be used
     to establish peering connections if the server pods are not directly reachable. [[GH-1610](https://github.com/hashicorp/consul-k8s/pull/1610)]

charts/consul/templates/connect-inject-deployment.yaml

@@ -5,10 +5,10 @@
 {{- if and .Values.global.adminPartitions.enabled (not .Values.global.enableConsulNamespaces) }}{{ fail "global.enableConsulNamespaces must be true if global.adminPartitions.enabled=true" }}{{ end }}
 {{ template "consul.validateVaultWebhookCertConfiguration" . }}
 {{- template "consul.reservedNamesFailer" (list .Values.connectInject.consulNamespaces.consulDestinationNamespace "connectInject.consulNamespaces.consulDestinationNamespace") }}
-{{- $serverEnabled := (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) -}}
-{{- $serverExposeServiceEnabled := (or (and (ne (.Values.server.exposeService.enabled | toString) "-") .Values.server.exposeService.enabled) (and (eq (.Values.server.exposeService.enabled | toString) "-") .Values.global.adminPartitions.enabled)) -}}
 {{- if and .Values.externalServers.enabled (not .Values.externalServers.hosts) }}{{ fail "externalServers.hosts must be set if externalServers.enabled is true" }}{{ end -}}
 {{- if and .Values.externalServers.skipServerWatch (not .Values.externalServers.enabled) }}{{ fail "externalServers.enabled must be set if externalServers.skipServerWatch is true" }}{{ end -}}
+{{- $dnsEnabled := (or (and (ne (.Values.dns.enabled | toString) "-") .Values.dns.enabled) (and (eq (.Values.dns.enabled | toString) "-") .Values.connectInject.transparentProxy.defaultEnabled)) -}}
+{{- $dnsRedirectionEnabled := (or (and (ne (.Values.dns.enableRedirection | toString) "-") .Values.dns.enableRedirection) (and (eq (.Values.dns.enableRedirection | toString) "-") .Values.connectInject.transparentProxy.defaultEnabled)) -}}
 {{ template "consul.validateRequiredCloudSecretsExist" . }}
 {{ template "consul.validateCloudSecretKeys" . }}
 # The deployment for running the Connect sidecar injector
@@ -153,7 +153,7 @@ spec:
                 {{- else }}
                 -transparent-proxy-default-overwrite-probes=false \
                 {{- end }}
-                {{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
+                {{- if (and $dnsEnabled $dnsRedirectionEnabled) }}
                 -enable-consul-dns=true \
                 {{- end }}
                 {{- if .Values.global.openshift.enabled }}

charts/consul/templates/dns-service.yaml

@@ -1,4 +1,4 @@
-{{- if (or (and (ne (.Values.dns.enabled | toString) "-") .Values.dns.enabled) (and (eq (.Values.dns.enabled | toString) "-") .Values.global.enabled)) }}
+{{- if (or (and (ne (.Values.dns.enabled | toString) "-") .Values.dns.enabled) (and (eq (.Values.dns.enabled | toString) "-") .Values.connectInject.transparentProxy.defaultEnabled)) }}
 # Service for Consul DNS.
 apiVersion: v1
 kind: Service

charts/consul/templates/server-acl-init-job.yaml

@@ -174,7 +174,7 @@ spec:
             {{- if .Values.global.peering.enabled }}
             -enable-peering=true \
             {{- end }}
-            {{- if (or (and (ne (.Values.dns.enabled | toString) "-") .Values.dns.enabled) (and (eq (.Values.dns.enabled | toString) "-") .Values.global.enabled)) }}
+            {{- if (or (and (ne (.Values.dns.enabled | toString) "-") .Values.dns.enabled) (and (eq (.Values.dns.enabled | toString) "-") .Values.connectInject.transparentProxy.defaultEnabled)) }}
             -allow-dns=true \
             {{- end }}
 

charts/consul/test/unit/connect-inject-deployment.bats

@@ -428,14 +428,14 @@ load _helpers
 #--------------------------------------------------------------------
 # DNS
 
-@test "connectInject/Deployment: -enable-consul-dns unset by default" {
+@test "connectInject/Deployment: -enable-consul-dns is set by default due to inheriting from connectInject.transparentProxy.defaultEnabled" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/connect-inject-deployment.yaml \
       --set 'connectInject.enabled=true' \
       . | tee /dev/stderr |
       yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr)
-  [ "${actual}" = "false" ]
+  [ "${actual}" = "true" ]
 }
 
 @test "connectInject/Deployment: -enable-consul-dns is true if dns.enabled=true and dns.enableRedirection=true" {
@@ -444,11 +444,42 @@ load _helpers
       -s templates/connect-inject-deployment.yaml \
       --set 'connectInject.enabled=true' \
       --set 'dns.enableRedirection=true' \
+      --set 'dns.enabled=true' \
       . | tee /dev/stderr |
       yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 
+@test "connectInject/Deployment: -enable-consul-dns is not set when connectInject.transparentProxy.defaultEnabled is false" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/connect-inject-deployment.yaml \
+      --set 'connectInject.enabled=true' \
+      --set 'connectInject.transparentProxy.defaultEnabled=false' \
+      . | tee /dev/stderr |
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+}
+
+@test "connectInject/Deployment: -enable-consul-dns is not set if dns.enabled is false or ens.enableRedirection is false" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/connect-inject-deployment.yaml \
+      --set 'connectInject.enabled=true' \
+      --set 'dns.enabled=false' \
+      . | tee /dev/stderr |
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+
+  local actual=$(helm template \
+      -s templates/connect-inject-deployment.yaml \
+      --set 'connectInject.enabled=true' \
+      --set 'dns.enableRedirection=false' \
+      . | tee /dev/stderr |
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+}
+
 @test "connectInject/Deployment: -resource-prefix always set" {
   cd `chart_dir`
   local actual=$(helm template \

charts/consul/test/unit/dns-service.bats

@@ -2,7 +2,7 @@
 
 load _helpers
 
-@test "dns/Service: enabled by default" {
+@test "dns/Service: enabled by default due to inheriting from connectInject.transparentProxy.defaultEnabled" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/dns-service.yaml  \
@@ -11,11 +11,11 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
-@test "dns/Service: enable with global.enabled false" {
+@test "dns/Service: enable with connectInject.transparentProxy.defaultEnabled false" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/dns-service.yaml  \
-      --set 'global.enabled=false' \
+      --set 'connectInject.transparentProxy.defaultEnabled=false' \
       --set 'dns.enabled=true' \
       . | tee /dev/stderr |
       yq 'length > 0' | tee /dev/stderr)
@@ -30,11 +30,11 @@ load _helpers
       .
 }
 
-@test "dns/Service: disable with global.enabled" {
+@test "dns/Service: disable with connectInject.transparentProxy.defaultEnabled false" {
   cd `chart_dir`
   assert_empty helm template \
       -s templates/dns-service.yaml  \
-      --set 'global.enabled=false' \
+      --set 'connectInject.transparentProxy.defaultEnabled=false' \
       .
 }
 

charts/consul/test/unit/server-acl-init-job.bats

@@ -114,7 +114,7 @@ load _helpers
 #--------------------------------------------------------------------
 # dns
 
-@test "serverACLInit/Job: dns acl option enabled with .dns.enabled=-" {
+@test "serverACLInit/Job: dns acl option enabled with .dns.enabled=- due to inheriting from connectInject.transparentProxy.defaultEnabled" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/server-acl-init-job.yaml  \
@@ -124,6 +124,29 @@ load _helpers
   [ "${actual}" = "true" ]
 }
 
+@test "serverACLInit/Job: dns acl option disabled with connectInject.transparentProxy.defaultEnabled=false" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/server-acl-init-job.yaml  \
+      --set 'global.acls.manageSystemACLs=true' \
+      --set 'connectInject.transparentProxy.defaultEnabled=false' \
+      . | tee /dev/stderr |
+      yq '.spec.template.spec.containers[0].command | any(contains("allow-dns"))' | tee /dev/stderr)
+  [ "${actual}" = "false" ]
+}
+
+@test "serverACLInit/Job: dns acl option enabled with .dns.enabled=true and connectInject.transparentProxy.defaultEnabled=false" {
+  cd `chart_dir`
+  local actual=$(helm template \
+      -s templates/server-acl-init-job.yaml  \
+      --set 'global.acls.manageSystemACLs=true' \
+      --set 'connectInject.transparentProxy.defaultEnabled=false' \
+      --set 'dns.enabled=true' \
+      . | tee /dev/stderr |
+      yq '.spec.template.spec.containers[0].command | any(contains("allow-dns"))' | tee /dev/stderr)
+  [ "${actual}" = "true" ]
+}
+
 @test "serverACLInit/Job: dns acl option enabled with .dns.enabled=true" {
   cd `chart_dir`
   local actual=$(helm template \

charts/consul/values.yaml

@@ -1480,7 +1480,7 @@ dns:
   # for default DNS resolution. The DNS lookups fall back to the nameserver IPs
   # listed in /etc/resolv.conf if not found in Consul.
   # @type: boolean
-  enableRedirection: false
+  enableRedirection: "-"
 
   # Used to control the type of service created. For
   # example, setting this to "LoadBalancer" will create an external load