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.

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))
 	}
 

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
+}

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

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{},
 	},
 }
 

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.

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
 	}