cmd/k8s-operator: validate Service tags, catch duplicate Tailscale Services (#16058)

Validate that any tags that users have specified via tailscale.com/tags
annotation are valid Tailscale ACL tags.
Validate that no more than one HA Tailscale Kubernetes Services in a single cluster refer
to the same Tailscale Service.

Updates tailscale/tailscale#16054
Updates tailscale/tailscale#16035

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

cmd/k8s-operator/ingress-for-pg.go

@@ -660,14 +660,9 @@ func (r *HAIngressReconciler) validateIngress(ctx context.Context, ing *networki
 	var errs []error
 
 	// Validate tags if present
-	if tstr, ok := ing.Annotations[AnnotationTags]; ok {
+	violations := tagViolations(ing)
-		tags := strings.Split(tstr, ",")
+	if len(violations) > 0 {
-		for _, tag := range tags {
+		errs = append(errs, fmt.Errorf("Ingress contains invalid tags: %v", strings.Join(violations, ",")))
-			tag = strings.TrimSpace(tag)
-			if err := tailcfg.CheckTag(tag); err != nil {
-				errs = append(errs, fmt.Errorf("tailscale.com/tags annotation contains invalid tag %q: %w", tag, err))
-			}
-		}
 	}
 
 	// Validate TLS configuration
@@ -699,8 +694,8 @@ func (r *HAIngressReconciler) validateIngress(ctx context.Context, ing *networki
 		return errors.Join(errs...)
 	}
 	for _, i := range ingList.Items {
-		if r.shouldExpose(&i) && hostnameForIngress(&i) == hostname && i.Name != ing.Name {
+		if r.shouldExpose(&i) && hostnameForIngress(&i) == hostname && i.UID != ing.UID {
-			errs = append(errs, fmt.Errorf("found duplicate Ingress %q for hostname %q - multiple Ingresses for the same hostname in the same cluster are not allowed", i.Name, hostname))
+			errs = append(errs, fmt.Errorf("found duplicate Ingress %q for hostname %q - multiple Ingresses for the same hostname in the same cluster are not allowed", client.ObjectKeyFromObject(&i), hostname))
 		}
 	}
 	return errors.Join(errs...)
@@ -1113,3 +1108,22 @@ func isErrorTailscaleServiceNotFound(err error) bool {
 	ok := errors.As(err, &errResp)
 	return ok && errResp.Status == http.StatusNotFound
 }
+
+func tagViolations(obj client.Object) []string {
+	var violations []string
+	if obj == nil {
+		return nil
+	}
+	tags, ok := obj.GetAnnotations()[AnnotationTags]
+	if !ok {
+		return nil
+	}
+
+	for _, tag := range strings.Split(tags, ",") {
+		tag = strings.TrimSpace(tag)
+		if err := tailcfg.CheckTag(tag); err != nil {
+			violations = append(violations, fmt.Sprintf("invalid tag %q: %v", tag, err))
+		}
+	}
+	return violations
+}

cmd/k8s-operator/ingress-for-pg_test.go

@@ -272,6 +272,7 @@ func TestValidateIngress(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-ingress",
 			Namespace: "default",
+			UID:       types.UID("1234-UID"),
 			Annotations: map[string]string{
 				AnnotationProxyGroup: "test-pg",
 			},
@@ -339,7 +340,7 @@ func TestValidateIngress(t *testing.T) {
 				},
 			},
 			pg:      readyProxyGroup,
-			wantErr: "tailscale.com/tags annotation contains invalid tag \"tag:invalid!\": tag names can only contain numbers, letters, or dashes",
+			wantErr: "Ingress contains invalid tags: invalid tag \"tag:invalid!\": tag names can only contain numbers, letters, or dashes",
 		},
 		{
 			name: "multiple_TLS_entries",
@@ -417,7 +418,7 @@ func TestValidateIngress(t *testing.T) {
 					},
 				},
 			}},
-			wantErr: `found duplicate Ingress "existing-ingress" for hostname "test" - multiple Ingresses for the same hostname in the same cluster are not allowed`,
+			wantErr: `found duplicate Ingress "default/existing-ingress" for hostname "test" - multiple Ingresses for the same hostname in the same cluster are not allowed`,
 		},
 	}
 

cmd/k8s-operator/operator_test.go

@@ -1804,7 +1804,7 @@ func Test_metricsResourceCreation(t *testing.T) {
 
 func TestIgnorePGService(t *testing.T) {
 	// NOTE: creating proxygroup stuff just to be sure that it's all ignored
-	_, _, fc, _ := setupServiceTest(t)
+	_, _, fc, _, _ := setupServiceTest(t)
 
 	ft := &fakeTSClient{}
 	zl, err := zap.NewDevelopment()

cmd/k8s-operator/svc-for-pg.go

@@ -169,12 +169,9 @@ func (r *HAServiceReconciler) maybeProvision(ctx context.Context, hostname strin
 		return false, nil
 	}
 
-	// Validate Service configuration
+	if err := r.validateService(ctx, svc, pg); err != nil {
-	if violations := validateService(svc); len(violations) > 0 {
+		r.recorder.Event(svc, corev1.EventTypeWarning, reasonIngressSvcInvalid, err.Error())
-		msg := fmt.Sprintf("unable to provision proxy resources: invalid Service: %s", strings.Join(violations, ", "))
+		tsoperator.SetServiceCondition(svc, tsapi.IngressSvcValid, metav1.ConditionFalse, reasonIngressSvcInvalid, err.Error(), r.clock, logger)
-		r.recorder.Event(svc, corev1.EventTypeWarning, "INVALIDSERVICE", msg)
-		r.logger.Error(msg)
-		tsoperator.SetServiceCondition(svc, tsapi.IngressSvcValid, metav1.ConditionFalse, reasonIngressSvcInvalid, msg, r.clock, logger)
 		return false, nil
 	}
 
@@ -857,3 +854,26 @@ func (r *HAServiceReconciler) checkEndpointsReady(ctx context.Context, svc *core
 	logger.Debugf("could not find any ready Endpoints in EndpointSlice")
 	return false, nil
 }
+
+func (r *HAServiceReconciler) validateService(ctx context.Context, svc *corev1.Service, pg *tsapi.ProxyGroup) error {
+	var errs []error
+	if pg.Spec.Type != tsapi.ProxyGroupTypeIngress {
+		errs = append(errs, fmt.Errorf("ProxyGroup %q is of type %q but must be of type %q",
+			pg.Name, pg.Spec.Type, tsapi.ProxyGroupTypeIngress))
+	}
+	if violations := validateService(svc); len(violations) > 0 {
+		errs = append(errs, fmt.Errorf("invalid Service: %s", strings.Join(violations, ", ")))
+	}
+	svcList := &corev1.ServiceList{}
+	if err := r.List(ctx, svcList); err != nil {
+		errs = append(errs, fmt.Errorf("[unexpected] error listing Services: %w", err))
+		return errors.Join(errs...)
+	}
+	svcName := nameForService(svc)
+	for _, s := range svcList.Items {
+		if r.shouldExpose(&s) && nameForService(&s) == svcName && s.UID != svc.UID {
+			errs = append(errs, fmt.Errorf("found duplicate Service %q for hostname %q - multiple HA Services for the same hostname in the same cluster are not allowed", client.ObjectKeyFromObject(&s), svcName))
+		}
+	}
+	return errors.Join(errs...)
+}

cmd/k8s-operator/svc-for-pg_test.go

@@ -12,6 +12,7 @@ import (
 	"math/rand/v2"
 	"net/netip"
 	"testing"
+	"time"
 
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
@@ -33,7 +34,7 @@ import (
 )
 
 func TestServicePGReconciler(t *testing.T) {
-	svcPGR, stateSecret, fc, ft := setupServiceTest(t)
+	svcPGR, stateSecret, fc, ft, _ := setupServiceTest(t)
 	svcs := []*corev1.Service{}
 	config := []string{}
 	for i := range 4 {
@@ -79,7 +80,7 @@ func TestServicePGReconciler(t *testing.T) {
 }
 
 func TestServicePGReconciler_UpdateHostname(t *testing.T) {
-	svcPGR, stateSecret, fc, ft := setupServiceTest(t)
+	svcPGR, stateSecret, fc, ft, _ := setupServiceTest(t)
 
 	cip := "4.1.6.7"
 	svc, _ := setupTestService(t, "test-service", "", cip, fc, stateSecret)
@@ -110,7 +111,7 @@ func TestServicePGReconciler_UpdateHostname(t *testing.T) {
 	}
 }
 
-func setupServiceTest(t *testing.T) (*HAServiceReconciler, *corev1.Secret, client.Client, *fakeTSClient) {
+func setupServiceTest(t *testing.T) (*HAServiceReconciler, *corev1.Secret, client.Client, *fakeTSClient, *tstest.Clock) {
 	// Pre-create the ProxyGroup
 	pg := &tsapi.ProxyGroup{
 		ObjectMeta: metav1.ObjectMeta{
@@ -215,14 +216,74 @@ func setupServiceTest(t *testing.T) (*HAServiceReconciler, *corev1.Secret, clien
 		lc:          lc,
 	}
 
-	return svcPGR, pgStateSecret, fc, ft
+	return svcPGR, pgStateSecret, fc, ft, cl
+}
+
+func TestValidateService(t *testing.T) {
+	// Test that no more than one Kubernetes Service in a cluster refers to the same Tailscale Service.
+	pgr, _, lc, _, cl := setupServiceTest(t)
+	svc := &corev1.Service{
+		TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "v1"},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-app",
+			Namespace: "ns-1",
+			UID:       types.UID("1234-UID"),
+			Annotations: map[string]string{
+				"tailscale.com/proxy-group": "test-pg",
+				"tailscale.com/hostname":    "my-app",
+			},
+		},
+		Spec: corev1.ServiceSpec{
+			ClusterIP:         "1.2.3.4",
+			Type:              corev1.ServiceTypeLoadBalancer,
+			LoadBalancerClass: ptr.To("tailscale"),
+		},
+	}
+	svc2 := &corev1.Service{
+		TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "v1"},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "my-app2",
+			Namespace: "ns-2",
+			UID:       types.UID("1235-UID"),
+			Annotations: map[string]string{
+				"tailscale.com/proxy-group": "test-pg",
+				"tailscale.com/hostname":    "my-app",
+			},
+		},
+		Spec: corev1.ServiceSpec{
+			ClusterIP:         "1.2.3.5",
+			Type:              corev1.ServiceTypeLoadBalancer,
+			LoadBalancerClass: ptr.To("tailscale"),
+		},
+	}
+	wantSvc := &corev1.Service{
+		ObjectMeta: svc.ObjectMeta,
+		TypeMeta:   svc.TypeMeta,
+		Spec:       svc.Spec,
+		Status: corev1.ServiceStatus{
+			Conditions: []metav1.Condition{
+				{
+					Type:               string(tsapi.IngressSvcValid),
+					Status:             metav1.ConditionFalse,
+					Reason:             reasonIngressSvcInvalid,
+					LastTransitionTime: metav1.NewTime(cl.Now().Truncate(time.Second)),
+					Message:            `found duplicate Service "ns-2/my-app2" for hostname "my-app" - multiple HA Services for the same hostname in the same cluster are not allowed`,
+				},
+			},
+		},
+	}
+
+	mustCreate(t, lc, svc)
+	mustCreate(t, lc, svc2)
+	expectReconciled(t, pgr, svc.Namespace, svc.Name)
+	expectEqual(t, lc, wantSvc)
 }
 
 func TestServicePGReconciler_MultiCluster(t *testing.T) {
 	var ft *fakeTSClient
 	var lc localClient
 	for i := 0; i <= 10; i++ {
-		pgr, stateSecret, fc, fti := setupServiceTest(t)
+		pgr, stateSecret, fc, fti, _ := setupServiceTest(t)
 		if i == 0 {
 			ft = fti
 			lc = pgr.lc
@@ -250,7 +311,7 @@ func TestServicePGReconciler_MultiCluster(t *testing.T) {
 }
 
 func TestIgnoreRegularService(t *testing.T) {
-	pgr, _, fc, ft := setupServiceTest(t)
+	pgr, _, fc, ft, _ := setupServiceTest(t)
 
 	svc := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{

cmd/k8s-operator/svc.go

@@ -392,6 +392,7 @@ func validateService(svc *corev1.Service) []string {
 			violations = append(violations, fmt.Sprintf("invalid Tailscale hostname %q, use %q annotation to override: %s", svcName, AnnotationHostname, err))
 		}
 	}
+	violations = append(violations, tagViolations(svc)...)
 	return violations
 }