From 462098b3f53b559b49946474e8e3e335457d3917 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 8 Nov 2021 15:50:34 -0500 Subject: [PATCH] Add unit tests for the changes made. --- charts/consul/templates/client-daemonset.yaml | 5 +- .../templates/connect-inject-deployment.yaml | 11 +-- .../consul/templates/server-statefulset.yaml | 5 +- charts/consul/test/unit/client-daemonset.bats | 22 +++++ .../test/unit/connect-inject-deployment.bats | 24 ++++++ .../consul/test/unit/server-statefulset.bats | 22 +++++ charts/consul/values.yaml | 3 + control-plane/connect-inject/annotations.go | 6 ++ .../connect-inject/container_init.go | 35 +++++++- .../connect-inject/container_init_test.go | 83 +++++++++++++++++++ control-plane/connect-inject/handler.go | 7 +- .../subcommand/inject-connect/command.go | 10 ++- 12 files changed, 209 insertions(+), 24 deletions(-) diff --git a/charts/consul/templates/client-daemonset.yaml b/charts/consul/templates/client-daemonset.yaml index 1040d2d2e7..279ecfcf7f 100644 --- a/charts/consul/templates/client-daemonset.yaml +++ b/charts/consul/templates/client-daemonset.yaml @@ -202,9 +202,8 @@ spec: - | CONSUL_FULLNAME="{{template "consul.fullname" . }}" - {{- if .Values.dns.enabled }} + {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}} export kube_dns_service_ip=$(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2) - echo $kube_dns_service_ip > /consul/kubednsip if [[ -z $kube_dns_service_ip ]]; then echo "Unable to get kube-dns Service IP" fi @@ -283,7 +282,7 @@ spec: {{- range $value := .Values.global.recursors }} -recursor={{ quote $value }} \ {{- end }} - {{- if .Values.dns.enabled }} + {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}} -recursor="$kube_dns_service_ip" \ {{- end }} -config-file=/consul/extra-config/extra-from-values.json \ diff --git a/charts/consul/templates/connect-inject-deployment.yaml b/charts/consul/templates/connect-inject-deployment.yaml index 4817da0400..a81029a21b 100644 --- a/charts/consul/templates/connect-inject-deployment.yaml +++ b/charts/consul/templates/connect-inject-deployment.yaml @@ -84,13 +84,6 @@ spec: - | CONSUL_FULLNAME="{{template "consul.fullname" . }}" - {{- if .Values.dns.enabled }} - consul_fullname_with_underscore=$(echo $CONSUL_FULLNAME | tr '-' '_') - dns_var_name=${consul_fullname_with_underscore}_dns_service_host - dns_var_name_in_caps=$(echo $dns_var_name | tr 'a-z' 'A-Z') - export dns_ip=$(eval echo \$${dns_var_name_in_caps}) - {{- end }} - consul-k8s-control-plane inject-connect \ -log-level={{ default .Values.global.logLevel .Values.connectInject.logLevel }} \ -log-json={{ .Values.global.logJSON }} \ @@ -114,8 +107,8 @@ spec: {{- else }} -transparent-proxy-default-overwrite-probes=false \ {{- end }} - {{- if .Values.dns.enabled }} - -consul-dns-ip=$dns_ip \ + {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}} + -enable-consul-dns=true \ {{- end }} {{- if .Values.global.openshift.enabled }} -enable-openshift \ diff --git a/charts/consul/templates/server-statefulset.yaml b/charts/consul/templates/server-statefulset.yaml index 0386f63bba..0eb7353968 100644 --- a/charts/consul/templates/server-statefulset.yaml +++ b/charts/consul/templates/server-statefulset.yaml @@ -192,9 +192,8 @@ spec: - | CONSUL_FULLNAME="{{template "consul.fullname" . }}" - {{- if .Values.dns.enabled }} + {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}} export kube_dns_service_ip=$(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2) - echo $kube_dns_service_ip > /consul/kubednsip if [[ -z $kube_dns_service_ip ]]; then echo "Unable to get kube-dns Service IP" fi @@ -262,7 +261,7 @@ spec: {{- range $value := .Values.global.recursors }} -recursor={{ quote $value }} \ {{- end }} - {{- if .Values.dns.enabled }} + {{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}} -recursor="$kube_dns_service_ip" \ {{- end }} -config-file=/consul/extra-config/extra-from-values.json \ diff --git a/charts/consul/test/unit/client-daemonset.bats b/charts/consul/test/unit/client-daemonset.bats index 64a9d0e4c0..90208c31e6 100755 --- a/charts/consul/test/unit/client-daemonset.bats +++ b/charts/consul/test/unit/client-daemonset.bats @@ -1170,6 +1170,28 @@ load _helpers [ "${actual}" = "true" ] } +#-------------------------------------------------------------------- +# DNS + +@test "client/DaemonSet: recursor IP 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) + [ "${actual}" = "false" ] +} + +@test "client/DaemonSet: recursor IP set to kubeDNS IP if dns.enableRedirection" { + 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) + [ "${actual}" = "true" ] +} + #-------------------------------------------------------------------- # hostNetwork diff --git a/charts/consul/test/unit/connect-inject-deployment.bats b/charts/consul/test/unit/connect-inject-deployment.bats index a8e23d0c53..969a338b38 100755 --- a/charts/consul/test/unit/connect-inject-deployment.bats +++ b/charts/consul/test/unit/connect-inject-deployment.bats @@ -519,6 +519,30 @@ EOF [ "${actual}" = "true" ] } +#-------------------------------------------------------------------- +# DNS + +@test "connectInject/Deployment: -enable-consul-dns unset default" { + 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" ] +} + +@test "connectInject/Deployment: -enable-consul-dns is false if dns.enabled=false is disabled" { + cd `chart_dir` + local actual=$(helm template \ + -s templates/connect-inject-deployment.yaml \ + --set 'connectInject.enabled=true' \ + --set 'dns.enableRedirection=true' \ + . | tee /dev/stderr | + yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-enable-consul-dns=true")' | tee /dev/stderr) + [ "${actual}" = "true" ] +} + #-------------------------------------------------------------------- # global.tls.enabled diff --git a/charts/consul/test/unit/server-statefulset.bats b/charts/consul/test/unit/server-statefulset.bats index ea10cc163d..98369642fa 100755 --- a/charts/consul/test/unit/server-statefulset.bats +++ b/charts/consul/test/unit/server-statefulset.bats @@ -580,6 +580,28 @@ load _helpers [ "${actualBaz}" = "qux" ] } +#-------------------------------------------------------------------- +# DNS + +@test "server/StatefulSet: recursor IP is 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) + [ "${actual}" = "false" ] +} + +@test "server/StatefulSet: recursor IP set to kubeDNS IP 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) + [ "${actual}" = "true" ] +} + #-------------------------------------------------------------------- # annotations diff --git a/charts/consul/values.yaml b/charts/consul/values.yaml index 68ffbb6630..7bf05d77b4 100644 --- a/charts/consul/values.yaml +++ b/charts/consul/values.yaml @@ -1122,6 +1122,9 @@ dns: # @type: boolean enabled: "-" + # @type: boolean + enableRedirection: false + # Used to control the type of service created. For # example, setting this to "LoadBalancer" will create an external load # balancer (for supported K8S installations) diff --git a/control-plane/connect-inject/annotations.go b/control-plane/connect-inject/annotations.go index 929c9e4c69..ccc9ab6341 100644 --- a/control-plane/connect-inject/annotations.go +++ b/control-plane/connect-inject/annotations.go @@ -90,6 +90,12 @@ const ( // annotationConsulNamespace is the Consul namespace the service is registered into. annotationConsulNamespace = "consul.hashicorp.com/consul-namespace" + // keyConsulDNS enables or disables Consul DNS for a given pod. It can also be set as a label + // on a namespace to define the default behaviour for connect-injected pods which do not otherwise override this setting + // with their own annotation. + // This annotation/label takes a boolean value (true/false). + keyConsulDNS = "consul.hashicorp.com/consul-dns" + // keyTransparentProxy enables or disables transparent proxy for a given pod. It can also be set as a label // on a namespace to define the default behaviour for connect-injected pods which do not otherwise override this setting // with their own annotation. diff --git a/control-plane/connect-inject/container_init.go b/control-plane/connect-inject/container_init.go index d1880b21ff..7f05aed621 100644 --- a/control-plane/connect-inject/container_init.go +++ b/control-plane/connect-inject/container_init.go @@ -2,6 +2,7 @@ package connectinject import ( "bytes" + "os" "strconv" "strings" "text/template" @@ -16,6 +17,7 @@ const ( envoyUserAndGroupID = 5995 copyContainerUserAndGroupID = 5996 netAdminCapability = "NET_ADMIN" + dnsServiceHostEnvSuffix = "DNS_SERVICE_HOST" ) type initContainerCommandData struct { @@ -110,6 +112,21 @@ func (h *Handler) containerInit(namespace corev1.Namespace, pod corev1.Pod) (cor return corev1.Container{}, err } + dnsEnabled, err := consulDNSEnabled(namespace, pod, h.EnableConsulDNS) + if err != nil { + return corev1.Container{}, err + } + + var consulDNSIP string + if dnsEnabled { + for _, e := range os.Environ() { + if strings.Contains(e, dnsServiceHostEnvSuffix) { + dnsServiceHostEnv := strings.SplitN(e, "=", 2) + consulDNSIP = dnsServiceHostEnv[1] + } + } + } + data := initContainerCommandData{ AuthMethod: h.AuthMethod, ConsulPartition: h.ConsulPartition, @@ -121,7 +138,7 @@ func (h *Handler) containerInit(namespace corev1.Namespace, pod corev1.Pod) (cor TProxyExcludeOutboundPorts: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeOutboundPorts, pod), TProxyExcludeOutboundCIDRs: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeOutboundCIDRs, pod), TProxyExcludeUIDs: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeUIDs, pod), - ConsulDNSIP: h.ConsulDNSIP, + ConsulDNSIP: consulDNSIP, EnvoyUID: envoyUserAndGroupID, } @@ -243,6 +260,22 @@ func transparentProxyEnabled(namespace corev1.Namespace, pod corev1.Pod, globalE return globalEnabled, nil } +// consulDNSEnabled returns true if Consul DNS should be enabled for this pod. +// It returns an error when the annotation value cannot be parsed by strconv.ParseBool or if we are unable +// to read the pod's namespace label when it exists. +func consulDNSEnabled(namespace corev1.Namespace, pod corev1.Pod, globalEnabled bool) (bool, error) { + // First check to see if the pod annotation exists to override the namespace or global settings. + if raw, ok := pod.Annotations[keyConsulDNS]; ok { + return strconv.ParseBool(raw) + } + // Next see if the namespace has been defaulted. + if raw, ok := namespace.Labels[keyConsulDNS]; ok { + return strconv.ParseBool(raw) + } + // Else fall back to the global default. + return globalEnabled, nil +} + // pointerToInt64 takes an int64 and returns a pointer to it. func pointerToInt64(i int64) *int64 { return &i diff --git a/control-plane/connect-inject/container_init_test.go b/control-plane/connect-inject/container_init_test.go index e131ba20f7..d33800433e 100644 --- a/control-plane/connect-inject/container_init_test.go +++ b/control-plane/connect-inject/container_init_test.go @@ -2,6 +2,7 @@ package connectinject import ( "fmt" + "os" "strings" "testing" @@ -310,6 +311,88 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { } } +func TestHandlerContainerInit_consulDNS(t *testing.T) { + cases := map[string]struct { + globalEnabled bool + annotations map[string]string + expectedContainsCmd string + namespaceLabel map[string]string + }{ + "enabled globally, ns not set, annotation not provided": { + globalEnabled: true, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -consul-dns-ip="10.0.34.16" \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "enabled globally, ns not set, annotation is false": { + globalEnabled: true, + annotations: map[string]string{keyConsulDNS: "false"}, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "enabled globally, ns not set, annotation is true": { + globalEnabled: true, + annotations: map[string]string{keyConsulDNS: "true"}, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -consul-dns-ip="10.0.34.16" \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "disabled globally, ns not set, annotation not provided": { + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "disabled globally, ns not set, annotation is false": { + annotations: map[string]string{keyConsulDNS: "false"}, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "disabled globally, ns not set, annotation is true": { + annotations: map[string]string{keyConsulDNS: "true"}, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -consul-dns-ip="10.0.34.16" \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + }, + "disabled globally, ns enabled, annotation not set": { + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -consul-dns-ip="10.0.34.16" \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + namespaceLabel: map[string]string{keyConsulDNS: "true"}, + }, + "enabled globally, ns disabled, annotation not set": { + globalEnabled: true, + expectedContainsCmd: `/consul/connect-inject/consul connect redirect-traffic \ + -proxy-id="$(cat /consul/connect-inject/proxyid)" \ + -proxy-uid=5995`, + namespaceLabel: map[string]string{keyConsulDNS: "false"}, + }, + } + for name, c := range cases { + t.Run(name, func(t *testing.T) { + h := Handler{EnableConsulDNS: c.globalEnabled, EnableTransparentProxy: true} + os.Setenv(dnsServiceHostEnvSuffix, "10.0.34.16") + defer os.Unsetenv(dnsServiceHostEnvSuffix) + + pod := minimal() + pod.Annotations = c.annotations + + ns := testNS + ns.Labels = c.namespaceLabel + container, err := h.containerInit(ns, *pod) + require.NoError(t, err) + actualCmd := strings.Join(container.Command, " ") + + require.Contains(t, actualCmd, c.expectedContainsCmd) + }) + } +} + func TestHandlerContainerInit_namespacesAndPartitionsEnabled(t *testing.T) { minimal := func() *corev1.Pod { return &corev1.Pod{ diff --git a/control-plane/connect-inject/handler.go b/control-plane/connect-inject/handler.go index ec52918dad..f943dfadb8 100644 --- a/control-plane/connect-inject/handler.go +++ b/control-plane/connect-inject/handler.go @@ -134,10 +134,9 @@ type Handler struct { // to point them to the Envoy proxy. TProxyOverwriteProbes bool - // ConsulDNSIP is the IP of the Consul DNS service on Kubernetes. When this values is not empty, - // it should be passed to the redirect traffic command so DNS requests are directed to Consul from - // mesh services. - ConsulDNSIP string + // EnableConsulDNS enables traffic redirection so that DNS requests are directed to Consul + // from mesh services. + EnableConsulDNS bool // EnableOpenShift indicates that when tproxy is enabled, the security context for the Envoy and init // containers should not be added because OpenShift sets a random user for those and will not allow diff --git a/control-plane/subcommand/inject-connect/command.go b/control-plane/subcommand/inject-connect/command.go index b92fc41517..9798a4e49e 100644 --- a/control-plane/subcommand/inject-connect/command.go +++ b/control-plane/subcommand/inject-connect/command.go @@ -91,7 +91,9 @@ type Command struct { // Transparent proxy flags. flagDefaultEnableTransparentProxy bool flagTransparentProxyDefaultOverwriteProbes bool - flagConsulDNSIP string + + // Consul DNS flags. + flagEnableConsulDNS bool flagEnableOpenShift bool @@ -162,8 +164,8 @@ func (c *Command) init() { "Enable transparent proxy mode for all Consul service mesh applications by default.") c.flagSet.BoolVar(&c.flagTransparentProxyDefaultOverwriteProbes, "transparent-proxy-default-overwrite-probes", true, "Overwrite Kubernetes probes to point to Envoy by default when in Transparent Proxy mode.") - c.flagSet.StringVar(&c.flagConsulDNSIP, "consul-dns-ip", "", - "ClusterIP of the Consul DNS service.") + c.flagSet.BoolVar(&c.flagEnableConsulDNS, "enable-consul-dns", false, + "Enables Consul DNS lookup for services in the mesh.") c.flagSet.BoolVar(&c.flagEnableOpenShift, "enable-openshift", false, "Indicates that the command runs in an OpenShift cluster.") c.flagSet.StringVar(&c.flagLogLevel, "log-level", zapcore.InfoLevel.String(), @@ -474,7 +476,7 @@ func (c *Command) Run(args []string) int { CrossNamespaceACLPolicy: c.flagCrossNamespaceACLPolicy, EnableTransparentProxy: c.flagDefaultEnableTransparentProxy, TProxyOverwriteProbes: c.flagTransparentProxyDefaultOverwriteProbes, - ConsulDNSIP: c.flagConsulDNSIP, + EnableConsulDNS: c.flagEnableConsulDNS, EnableOpenShift: c.flagEnableOpenShift, Log: ctrl.Log.WithName("handler").WithName("connect"), LogLevel: c.flagLogLevel,