Skip to content

Commit

Permalink
Add unit tests for the changes made.
Browse files Browse the repository at this point in the history
  • Loading branch information
thisisnotashwin committed Nov 9, 2021
1 parent 2299c2a commit 462098b
Show file tree
Hide file tree
Showing 12 changed files with 209 additions and 24 deletions.
5 changes: 2 additions & 3 deletions charts/consul/templates/client-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 \
Expand Down
11 changes: 2 additions & 9 deletions charts/consul/templates/connect-inject-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }} \
Expand All @@ -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 \
Expand Down
5 changes: 2 additions & 3 deletions charts/consul/templates/server-statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 \
Expand Down
22 changes: 22 additions & 0 deletions charts/consul/test/unit/client-daemonset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
24 changes: 24 additions & 0 deletions charts/consul/test/unit/connect-inject-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
22 changes: 22 additions & 0 deletions charts/consul/test/unit/server-statefulset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions charts/consul/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions control-plane/connect-inject/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
35 changes: 34 additions & 1 deletion control-plane/connect-inject/container_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package connectinject

import (
"bytes"
"os"
"strconv"
"strings"
"text/template"
Expand All @@ -16,6 +17,7 @@ const (
envoyUserAndGroupID = 5995
copyContainerUserAndGroupID = 5996
netAdminCapability = "NET_ADMIN"
dnsServiceHostEnvSuffix = "DNS_SERVICE_HOST"
)

type initContainerCommandData struct {
Expand Down Expand Up @@ -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,
Expand All @@ -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,
}

Expand Down Expand Up @@ -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
Expand Down
83 changes: 83 additions & 0 deletions control-plane/connect-inject/container_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package connectinject

import (
"fmt"
"os"
"strings"
"testing"

Expand Down Expand Up @@ -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{
Expand Down
7 changes: 3 additions & 4 deletions control-plane/connect-inject/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions control-plane/subcommand/inject-connect/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ type Command struct {
// Transparent proxy flags.
flagDefaultEnableTransparentProxy bool
flagTransparentProxyDefaultOverwriteProbes bool
flagConsulDNSIP string

// Consul DNS flags.
flagEnableConsulDNS bool

flagEnableOpenShift bool

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 462098b

Please sign in to comment.