From fcd36a9d67c35410ff8bf219fd7d0e4a85c71e79 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Wed, 24 May 2023 21:28:50 -0700 Subject: [PATCH] Disable DNS redirection when tproxy is disabled DNS redirection and the various settings that make that possible (like the dataplane binding to a port for DNS) is only useful if tproxy is enabled. Most of the code checked if tproxy was enabled but there was one location where we didn't check. This resulted in a bug with our multiport support where even though tproxy is disabled, we tried to setup the dataplane to proxy DNS. This meant each dataplane tried to bind to 8600 but because there are >1 dataplanes with multiport, there was a port conflict. This PR fixes the location where we didn't check if tproxy was enabled and as a result fixes the multiport issue. --- .../webhook/consul_dataplane_sidecar.go | 6 +- .../webhook/consul_dataplane_sidecar_test.go | 124 ++++++++++++++++-- .../connect-inject/webhook/container_init.go | 14 +- .../webhook/container_init_test.go | 3 +- .../connect-inject/webhook/mesh_webhook.go | 10 +- .../webhook/redirect_traffic.go | 2 +- 6 files changed, 137 insertions(+), 22 deletions(-) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 70f3be82da..f5134f208f 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -314,7 +314,11 @@ func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, mpi mu // If Consul DNS is enabled, we want to configure consul-dataplane to be the DNS proxy // for Consul DNS in the pod. - if w.EnableConsulDNS { + dnsEnabled, err := consulDNSEnabled(namespace, pod, w.EnableConsulDNS, w.EnableTransparentProxy) + if err != nil { + return nil, err + } + if dnsEnabled { args = append(args, "-consul-dns-bind-port="+strconv.Itoa(consulDataplaneDNSBindPort)) } diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go index f7cb7bc594..e759b627fe 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go @@ -290,24 +290,115 @@ func TestHandlerConsulDataplaneSidecar_Concurrency(t *testing.T) { } } +// Test that we pass the dns proxy flag to dataplane correctly. func TestHandlerConsulDataplaneSidecar_DNSProxy(t *testing.T) { - h := MeshWebhook{ - ConsulConfig: &consul.Config{HTTPPort: 8500, GRPCPort: 8502}, - EnableConsulDNS: true, + + // We only want the flag passed when DNS and tproxy are both enabled. DNS/tproxy can + // both be enabled/disabled with annotations/labels on the pod and namespace and then globally + // through the helm chart. To test this we use an outer loop with the possible DNS settings and then + // and inner loop with possible tproxy settings. + dnsCases := []struct { + GlobalConsulDNS bool + NamespaceDNS *bool + PodDNS *bool + ExpEnabled bool + }{ + { + GlobalConsulDNS: false, + ExpEnabled: false, + }, + { + GlobalConsulDNS: true, + ExpEnabled: true, + }, + { + GlobalConsulDNS: false, + NamespaceDNS: boolPtr(true), + ExpEnabled: true, + }, + { + GlobalConsulDNS: false, + PodDNS: boolPtr(true), + ExpEnabled: true, + }, } - pod := corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{}, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "web", - }, - }, + tproxyCases := []struct { + GlobalTProxy bool + NamespaceTProxy *bool + PodTProxy *bool + ExpEnabled bool + }{ + { + GlobalTProxy: false, + ExpEnabled: false, + }, + { + GlobalTProxy: true, + ExpEnabled: true, + }, + { + GlobalTProxy: false, + NamespaceTProxy: boolPtr(true), + ExpEnabled: true, + }, + { + GlobalTProxy: false, + PodTProxy: boolPtr(true), + ExpEnabled: true, }, } - container, err := h.consulDataplaneSidecar(testNS, pod, multiPortInfo{}) - require.NoError(t, err) - require.Contains(t, container.Args, "-consul-dns-bind-port=8600") + + // Outer loop is permutations of dns being enabled. Inner loop is permutations of tproxy being enabled. + // Both must be enabled for dns to be enabled. + for i, dnsCase := range dnsCases { + for j, tproxyCase := range tproxyCases { + t.Run(fmt.Sprintf("dns=%d,tproxy=%d", i, j), func(t *testing.T) { + + // Test setup. + h := MeshWebhook{ + ConsulConfig: &consul.Config{HTTPPort: 8500, GRPCPort: 8502}, + EnableTransparentProxy: tproxyCase.GlobalTProxy, + EnableConsulDNS: dnsCase.GlobalConsulDNS, + } + pod := corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "web", + }, + }, + }, + } + if dnsCase.PodDNS != nil { + pod.Annotations[constants.KeyConsulDNS] = strconv.FormatBool(*dnsCase.PodDNS) + } + if tproxyCase.PodTProxy != nil { + pod.Annotations[constants.KeyTransparentProxy] = strconv.FormatBool(*tproxyCase.PodTProxy) + } + + ns := testNS + if dnsCase.NamespaceDNS != nil { + ns.Labels[constants.KeyConsulDNS] = strconv.FormatBool(*dnsCase.NamespaceDNS) + } + if tproxyCase.NamespaceTProxy != nil { + ns.Labels[constants.KeyTransparentProxy] = strconv.FormatBool(*tproxyCase.NamespaceTProxy) + } + + // Actual test here. + container, err := h.consulDataplaneSidecar(ns, pod, multiPortInfo{}) + require.NoError(t, err) + // Flag should only be passed if both tproxy and dns are enabled. + if tproxyCase.ExpEnabled && dnsCase.ExpEnabled { + require.Contains(t, container.Args, "-consul-dns-bind-port=8600") + } else { + require.NotContains(t, container.Args, "-consul-dns-bind-port=8600") + } + }) + } + } } func TestHandlerConsulDataplaneSidecar_ProxyHealthCheck(t *testing.T) { @@ -1202,3 +1293,8 @@ func TestHandlerConsulDataplaneSidecar_Metrics(t *testing.T) { }) } } + +// boolPtr returns pointer to b. +func boolPtr(b bool) *bool { + return &b +} diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index b33c8f4d3e..f180de88a3 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -267,7 +267,17 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, // 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) { +func consulDNSEnabled(namespace corev1.Namespace, pod corev1.Pod, globalDNSEnabled bool, globalTProxyEnabled bool) (bool, error) { + // DNS is only possible when tproxy is also enabled because it relies + // on traffic being redirected. + tproxy, err := common.TransparentProxyEnabled(namespace, pod, globalTProxyEnabled) + if err != nil { + return false, err + } + if !tproxy { + return false, nil + } + // First check to see if the pod annotation exists to override the namespace or global settings. if raw, ok := pod.Annotations[constants.KeyConsulDNS]; ok { return strconv.ParseBool(raw) @@ -277,7 +287,7 @@ func consulDNSEnabled(namespace corev1.Namespace, pod corev1.Pod, globalEnabled return strconv.ParseBool(raw) } // Else fall back to the global default. - return globalEnabled, nil + return globalDNSEnabled, nil } // splitCommaSeparatedItemsFromAnnotation takes an annotation and a pod diff --git a/control-plane/connect-inject/webhook/container_init_test.go b/control-plane/connect-inject/webhook/container_init_test.go index 0e2de79bb1..fd89d7eba6 100644 --- a/control-plane/connect-inject/webhook/container_init_test.go +++ b/control-plane/connect-inject/webhook/container_init_test.go @@ -937,7 +937,8 @@ func TestHandlerContainerInit_Resources(t *testing.T) { var testNS = corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: k8sNamespace, + Name: k8sNamespace, + Labels: map[string]string{}, }, } diff --git a/control-plane/connect-inject/webhook/mesh_webhook.go b/control-plane/connect-inject/webhook/mesh_webhook.go index 7105e19e67..96c73d93d4 100644 --- a/control-plane/connect-inject/webhook/mesh_webhook.go +++ b/control-plane/connect-inject/webhook/mesh_webhook.go @@ -396,13 +396,17 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi pod.Annotations[constants.KeyTransparentProxyStatus] = constants.Enabled } - // If tproxy with DNS redirection is enabled, we want to configure dns on the pod. - if tproxyEnabled && w.EnableConsulDNS { + // If DNS redirection is enabled, we want to configure dns on the pod. + dnsEnabled, err := consulDNSEnabled(*ns, pod, w.EnableConsulDNS, w.EnableTransparentProxy) + if err != nil { + w.Log.Error(err, "error determining if dns redirection is enabled", "request name", req.Name) + return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error determining if dns redirection is enabled: %s", err)) + } + if dnsEnabled { if err = w.configureDNS(&pod, req.Namespace); err != nil { w.Log.Error(err, "error configuring DNS on the pod", "request name", req.Name) return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring DNS on the pod: %s", err)) } - } // Add annotations for metrics. diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index 7066929dae..b0cbefeeaa 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -98,7 +98,7 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s // Add init container user ID to exclude from traffic redirection. cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.Itoa(initContainersUserAndGroupID)) - dnsEnabled, err := consulDNSEnabled(ns, pod, w.EnableConsulDNS) + dnsEnabled, err := consulDNSEnabled(ns, pod, w.EnableConsulDNS, w.EnableTransparentProxy) if err != nil { return "", err }