cmd/k8s-operator: fix port name change bug for egress ProxyGroup proxies (#14247)

Ensure that the ExternalName Service port names are always synced to the
ClusterIP Service, to fix a bug where if users created a Service with
a single unnamed port and later changed to 1+ named ports, the operator
attempted to apply an invalid multi-port Service with an unnamed port.
Also, fixes a small internal issue where not-yet Service status conditons
were lost on a spec update.

Updates tailscale/tailscale#10102

Signed-off-by: Irbe Krumina <irbe@tailscale.com>

cmd/k8s-operator/egress-services.go

@@ -136,9 +136,8 @@ func (esr *egressSvcsReconciler) Reconcile(ctx context.Context, req reconcile.Re
 	}
 
 	if !slices.Contains(svc.Finalizers, FinalizerName) {
-		l.Infof("configuring tailnet service") // logged exactly once
 		svc.Finalizers = append(svc.Finalizers, FinalizerName)
-		if err := esr.Update(ctx, svc); err != nil {
+		if err := esr.updateSvcSpec(ctx, svc); err != nil {
 			err := fmt.Errorf("failed to add finalizer: %w", err)
 			r := svcConfiguredReason(svc, false, l)
 			tsoperator.SetServiceCondition(svc, tsapi.EgressSvcConfigured, metav1.ConditionFalse, r, err.Error(), esr.clock, l)
@@ -198,7 +197,7 @@ func (esr *egressSvcsReconciler) maybeProvision(ctx context.Context, svc *corev1
 	if svc.Spec.ExternalName != clusterIPSvcFQDN {
 		l.Infof("Configuring ExternalName Service to point to ClusterIP Service %s", clusterIPSvcFQDN)
 		svc.Spec.ExternalName = clusterIPSvcFQDN
-		if err = esr.Update(ctx, svc); err != nil {
+		if err = esr.updateSvcSpec(ctx, svc); err != nil {
 			err = fmt.Errorf("error updating ExternalName Service: %w", err)
 			return err
 		}
@@ -222,6 +221,15 @@ func (esr *egressSvcsReconciler) provision(ctx context.Context, proxyGroupName s
 		found := false
 		for _, wantsPM := range svc.Spec.Ports {
 			if wantsPM.Port == pm.Port && strings.EqualFold(string(wantsPM.Protocol), string(pm.Protocol)) {
+				// We don't use the port name to distinguish this port internally, but Kubernetes
+				// require that, for Service ports with more than one name each port is uniquely named.
+				// So we can always pick the port name from the ExternalName Service as at this point we
+				// know that those are valid names because Kuberentes already validated it once. Note
+				// that users could have changed an unnamed port to a named port and might have changed
+				// port names- this should still work.
+				// https://kubernetes.io/docs/concepts/services-networking/service/#multi-port-services
+				// See also https://github.com/tailscale/tailscale/issues/13406#issuecomment-2507230388
+				clusterIPSvc.Spec.Ports[i].Name = wantsPM.Name
 				found = true
 				break
 			}
@@ -714,3 +722,13 @@ func epsPortsFromSvc(svc *corev1.Service) (ep []discoveryv1.EndpointPort) {
 	}
 	return ep
 }
+
+// updateSvcSpec ensures that the given Service's spec is updated in cluster, but the local Service object still retains
+// the not-yet-applied status.
+// TODO(irbekrm): once we do SSA for these patch updates, this will no longer be needed.
+func (esr *egressSvcsReconciler) updateSvcSpec(ctx context.Context, svc *corev1.Service) error {
+	st := svc.Status.DeepCopy()
+	err := esr.Update(ctx, svc)
+	svc.Status = *st
+	return err
+}

cmd/k8s-operator/egress-services_test.go

@@ -105,10 +105,57 @@ func TestTailscaleEgressServices(t *testing.T) {
 				condition(tsapi.ProxyGroupReady, metav1.ConditionTrue, "", "", clock),
 			}
 		})
-		// Quirks of the fake client.
-		mustUpdateStatus(t, fc, "default", "test", func(svc *corev1.Service) {
-			svc.Status.Conditions = []metav1.Condition{}
+		expectReconciled(t, esr, "default", "test")
+		validateReadyService(t, fc, esr, svc, clock, zl, cm)
+	})
+	t.Run("service_retain_one_unnamed_port", func(t *testing.T) {
+		svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80}}
+		mustUpdate(t, fc, "default", "test", func(s *corev1.Service) {
+			s.Spec.Ports = svc.Spec.Ports
+		})
+		expectReconciled(t, esr, "default", "test")
+		validateReadyService(t, fc, esr, svc, clock, zl, cm)
+	})
+	t.Run("service_add_two_named_ports", func(t *testing.T) {
+		svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80, Name: "http"}, {Protocol: "TCP", Port: 443, Name: "https"}}
+		mustUpdate(t, fc, "default", "test", func(s *corev1.Service) {
+			s.Spec.Ports = svc.Spec.Ports
+		})
+		expectReconciled(t, esr, "default", "test")
+		validateReadyService(t, fc, esr, svc, clock, zl, cm)
+	})
+	t.Run("service_add_udp_port", func(t *testing.T) {
+		svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{Port: 53, Protocol: "UDP", Name: "dns"})
+		mustUpdate(t, fc, "default", "test", func(s *corev1.Service) {
+			s.Spec.Ports = svc.Spec.Ports
+		})
+		expectReconciled(t, esr, "default", "test")
+		validateReadyService(t, fc, esr, svc, clock, zl, cm)
+	})
+	t.Run("service_change_protocol", func(t *testing.T) {
+		svc.Spec.Ports = []corev1.ServicePort{{Protocol: "TCP", Port: 80, Name: "http"}, {Protocol: "TCP", Port: 443, Name: "https"}, {Port: 53, Protocol: "TCP", Name: "tcp_dns"}}
+		mustUpdate(t, fc, "default", "test", func(s *corev1.Service) {
+			s.Spec.Ports = svc.Spec.Ports
 		})
+		expectReconciled(t, esr, "default", "test")
+		validateReadyService(t, fc, esr, svc, clock, zl, cm)
+	})
+
+	t.Run("delete_external_name_service", func(t *testing.T) {
+		name := findGenNameForEgressSvcResources(t, fc, svc)
+		if err := fc.Delete(context.Background(), svc); err != nil {
+			t.Fatalf("error deleting ExternalName Service: %v", err)
+		}
+		expectReconciled(t, esr, "default", "test")
+		// Verify that ClusterIP Service and EndpointSlice have been deleted.
+		expectMissing[corev1.Service](t, fc, "operator-ns", name)
+		expectMissing[discoveryv1.EndpointSlice](t, fc, "operator-ns", fmt.Sprintf("%s-ipv4", name))
+		// Verify that service config has been deleted from the ConfigMap.
+		mustNotHaveConfigForSvc(t, fc, svc, cm)
+	})
+}
+
+func validateReadyService(t *testing.T, fc client.WithWatch, esr *egressSvcsReconciler, svc *corev1.Service, clock *tstest.Clock, zl *zap.Logger, cm *corev1.ConfigMap) {
 	expectReconciled(t, esr, "default", "test")
 	// Verify that a ClusterIP Service has been created.
 	name := findGenNameForEgressSvcResources(t, fc, svc)
@@ -122,25 +169,13 @@ func TestTailscaleEgressServices(t *testing.T) {
 	// Verify that the user-created ExternalName Service has Configured set to true and ExternalName pointing to the
 	// CluterIP Service.
 	svc.Status.Conditions = []metav1.Condition{
+		condition(tsapi.EgressSvcValid, metav1.ConditionTrue, "EgressSvcValid", "EgressSvcValid", clock),
 		condition(tsapi.EgressSvcConfigured, metav1.ConditionTrue, r, r, clock),
 	}
 	svc.ObjectMeta.Finalizers = []string{"tailscale.com/finalizer"}
 	svc.Spec.ExternalName = fmt.Sprintf("%s.operator-ns.svc.cluster.local", name)
 	expectEqual(t, fc, svc, nil)
-	})
 
-	t.Run("delete_external_name_service", func(t *testing.T) {
-		name := findGenNameForEgressSvcResources(t, fc, svc)
-		if err := fc.Delete(context.Background(), svc); err != nil {
-			t.Fatalf("error deleting ExternalName Service: %v", err)
-		}
-		expectReconciled(t, esr, "default", "test")
-		// Verify that ClusterIP Service and EndpointSlice have been deleted.
-		expectMissing[corev1.Service](t, fc, "operator-ns", name)
-		expectMissing[discoveryv1.EndpointSlice](t, fc, "operator-ns", fmt.Sprintf("%s-ipv4", name))
-		// Verify that service config has been deleted from the ConfigMap.
-		mustNotHaveConfigForSvc(t, fc, svc, cm)
-	})
 }
 
 func condition(typ tsapi.ConditionType, st metav1.ConditionStatus, r, msg string, clock tstime.Clock) metav1.Condition {

cmd/k8s-operator/testutils_test.go

@@ -650,7 +650,7 @@ func removeHashAnnotation(sts *appsv1.StatefulSet) {
 func removeTargetPortsFromSvc(svc *corev1.Service) {
 	newPorts := make([]corev1.ServicePort, 0)
 	for _, p := range svc.Spec.Ports {
-		newPorts = append(newPorts, corev1.ServicePort{Protocol: p.Protocol, Port: p.Port})
+		newPorts = append(newPorts, corev1.ServicePort{Protocol: p.Protocol, Port: p.Port, Name: p.Name})
 	}
 	svc.Spec.Ports = newPorts
 }