Disable DNS redirection when tproxy is disabled

.changelog/2176.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+control-plane: fix issue with multiport pods crashlooping due to dataplane port conflicts by ensuring dns redirection is disabled for non-tproxy pods
+```

acceptance/tests/connect/connect_inject_test.go

@@ -136,13 +136,10 @@ func TestConnectInject_MultiportServices(t *testing.T) {
 			cfg := suite.Config()
 			ctx := suite.Environment().DefaultContext(t)
 
-			// Multi port apps don't work with transparent proxy.
-			if cfg.EnableTransparentProxy {
-				t.Skipf("skipping this test because transparent proxy is enabled")
-			}
-
 			helmValues := map[string]string{
 				"connectInject.enabled": "true",
+				// Enable DNS so we can test that DNS redirection _isn't_ set in the pod.
+				"dns.enabled": "true",
 
 				"global.tls.enabled":           strconv.FormatBool(secure),
 				"global.acls.manageSystemACLs": strconv.FormatBool(secure),

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/mesh_webhook_test.go

@@ -811,6 +811,145 @@ func TestHandlerHandle(t *testing.T) {
 				},
 			},
 		},
+		{
+			"dns redirection enabled",
+			MeshWebhook{
+				Log:                    logrtest.TestLogger{T: t},
+				AllowK8sNamespacesSet:  mapset.NewSetWith("*"),
+				DenyK8sNamespacesSet:   mapset.NewSet(),
+				EnableTransparentProxy: true,
+				TProxyOverwriteProbes:  true,
+				decoder:                decoder,
+				Clientset:              defaultTestClientWithNamespace(),
+			},
+			admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Namespace: namespaces.DefaultNamespace,
+					Object: encodeRaw(t, &corev1.Pod{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels:      map[string]string{},
+							Annotations: map[string]string{constants.KeyConsulDNS: "true"},
+						},
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Name: "web",
+								},
+							},
+						},
+					}),
+				},
+			},
+			"",
+			[]jsonpatch.Operation{
+				{
+					Operation: "add",
+					Path:      "/spec/volumes",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/initContainers",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/containers/1",
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/labels",
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.KeyInjectStatus),
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.KeyTransparentProxyStatus),
+				},
+
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationOriginalPod),
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationConsulK8sVersion),
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/dnsPolicy",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/dnsConfig",
+				},
+			},
+		},
+		{
+			"dns redirection only enabled if tproxy enabled",
+			MeshWebhook{
+				Log:                    logrtest.TestLogger{T: t},
+				AllowK8sNamespacesSet:  mapset.NewSetWith("*"),
+				DenyK8sNamespacesSet:   mapset.NewSet(),
+				EnableTransparentProxy: true,
+				TProxyOverwriteProbes:  true,
+				decoder:                decoder,
+				Clientset:              defaultTestClientWithNamespace(),
+			},
+			admission.Request{
+				AdmissionRequest: admissionv1.AdmissionRequest{
+					Namespace: namespaces.DefaultNamespace,
+					Object: encodeRaw(t, &corev1.Pod{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{},
+							Annotations: map[string]string{
+								constants.KeyConsulDNS:        "true",
+								constants.KeyTransparentProxy: "false",
+							},
+						},
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{
+								{
+									Name: "web",
+								},
+							},
+						},
+					}),
+				},
+			},
+			"",
+			[]jsonpatch.Operation{
+				{
+					Operation: "add",
+					Path:      "/spec/volumes",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/initContainers",
+				},
+				{
+					Operation: "add",
+					Path:      "/spec/containers/1",
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/labels",
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.KeyInjectStatus),
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationOriginalPod),
+				},
+				{
+					Operation: "add",
+					Path:      "/metadata/annotations/" + escapeJSONPointer(constants.AnnotationConsulK8sVersion),
+				},
+				// Note: no DNS policy/config additions.
+			},
+		},
 	}
 
 	for _, tt := range cases {

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
 	}