Add feedback from Iryna's review

hashicorp · Nov 9, 2021 · f29938f · f29938f
1 parent 462098b
commit f29938f
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 19 deletions.
diff --git a/charts/consul/templates/_helpers.tpl b/charts/consul/templates/_helpers.tpl
@@ -28,6 +28,18 @@ is passed to consul as a -config-file param on command line.
               [ -n "${HOSTNAME}" ] && sed -Ei "s|HOSTNAME|${HOSTNAME?}|g" /consul/extra-config/extra-from-values.json
 {{- end -}}
 
+{{/*
+Sets up a list of recusor flags for Consul agents by iterating over the IPs of every nameserver
+in /etc/resolv.conf and concatenating them into a string of arguments that can be passed directly
+to the consul agent command.
+*/}}
+{{- define "consul.recursors" -}}
+              recursor_flags=""
+              for ip in $(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2)
+              do
+                 recursor_flags=$recursor_flags" -recursor=$ip"
+              done
+{{- end -}}
 
 {{/*
 Create chart name and version as used by the chart label.

diff --git a/charts/consul/templates/client-daemonset.yaml b/charts/consul/templates/client-daemonset.yaml
@@ -203,10 +203,7 @@ spec:
               CONSUL_FULLNAME="{{template "consul.fullname" . }}"
 
               {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
-              export kube_dns_service_ip=$(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2)
-              if [[ -z $kube_dns_service_ip ]]; then
-                  echo "Unable to get kube-dns Service IP"
-              fi
+              {{ template "consul.recursors" }}
               {{- end }}
 
               {{ template "consul.extraconfig" }}
@@ -283,7 +280,7 @@ spec:
                 -recursor={{ quote $value }} \
                 {{- end }}
                 {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
-                -recursor="$kube_dns_service_ip" \
+                $recursor_flags \
                 {{- end }}
                 -config-file=/consul/extra-config/extra-from-values.json \
                 -domain={{ .Values.global.domain }}

diff --git a/charts/consul/templates/server-statefulset.yaml b/charts/consul/templates/server-statefulset.yaml
@@ -193,10 +193,7 @@ spec:
               CONSUL_FULLNAME="{{template "consul.fullname" . }}"
 
               {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
-              export kube_dns_service_ip=$(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2)
-              if [[ -z $kube_dns_service_ip ]]; then
-                  echo "Unable to get kube-dns Service IP"
-              fi
+              {{ template "consul.recursors" }}
               {{- end }}
 
               {{ template "consul.extraconfig" }}
@@ -262,7 +259,7 @@ spec:
                 -recursor={{ quote $value }} \
                 {{- end }}
                 {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
-                -recursor="$kube_dns_service_ip" \
+                $recursor_flags \
                 {{- end }}
                 -config-file=/consul/extra-config/extra-from-values.json \
                 -server

diff --git a/charts/consul/test/unit/client-daemonset.bats b/charts/consul/test/unit/client-daemonset.bats
@@ -1173,22 +1173,22 @@ load _helpers
 #--------------------------------------------------------------------
 # DNS
 
-@test "client/DaemonSet: recursor IP is not set by default" {
+@test "client/DaemonSet: recursor flags is not set by default" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/client-daemonset.yaml \
       . | tee /dev/stderr |
-      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-recursor=\"$kube_dns_service_ip\"")' | tee /dev/stderr)
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("$recursor_flags")' | tee /dev/stderr)
   [ "${actual}" = "false" ]
 }
 
-@test "client/DaemonSet: recursor IP set to kubeDNS IP if dns.enableRedirection" {
+@test "client/DaemonSet: add recursor flags if dns.enableRedirection is true" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/client-daemonset.yaml \
       --set 'dns.enableRedirection=true' \
       . | tee /dev/stderr |
-      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-recursor=\"$kube_dns_service_ip\"")' | tee /dev/stderr)
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("$recursor_flags")' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 

diff --git a/charts/consul/test/unit/connect-inject-deployment.bats b/charts/consul/test/unit/connect-inject-deployment.bats
@@ -532,7 +532,7 @@ EOF
   [ "${actual}" = "false" ]
 }
 
-@test "connectInject/Deployment: -enable-consul-dns is false if dns.enabled=false is disabled" {
+@test "connectInject/Deployment: -enable-consul-dns is true if dns.enabled=true and dns.enableRedirection=true" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/connect-inject-deployment.yaml \

diff --git a/charts/consul/test/unit/server-statefulset.bats b/charts/consul/test/unit/server-statefulset.bats
@@ -583,22 +583,22 @@ load _helpers
 #--------------------------------------------------------------------
 # DNS
 
-@test "server/StatefulSet: recursor IP is unset by default" {
+@test "server/StatefulSet: recursor flags unset by default" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/server-statefulset.yaml \
       . | tee /dev/stderr |
-      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-recursor=\"$kube_dns_service_ip\"")' | tee /dev/stderr)
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("$recursor_flags")' | tee /dev/stderr)
   [ "${actual}" = "false" ]
 }
 
-@test "server/StatefulSet: recursor IP set to kubeDNS IP if dns.enableRedirection is true" {
+@test "server/StatefulSet: add recursor flags if dns.enableRedirection is true" {
   cd `chart_dir`
   local actual=$(helm template \
       -s templates/server-statefulset.yaml \
       --set 'dns.enableRedirection=true' \
       . | tee /dev/stderr |
-      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-recursor=\"$kube_dns_service_ip\"")' | tee /dev/stderr)
+      yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("$recursor_flags")' | tee /dev/stderr)
   [ "${actual}" = "true" ]
 }
 

diff --git a/charts/consul/values.yaml b/charts/consul/values.yaml
@@ -1122,6 +1122,9 @@ dns:
   # @type: boolean
   enabled: "-"
 
+  # Used to determine if services using Consul Connect use Consul 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