From 00a7dd180a7582502773c71a7ea52e051dbc67cd Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Fri, 23 May 2025 12:23:58 +0100 Subject: [PATCH] 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 --- cmd/k8s-operator/ingress-for-pg.go | 34 ++++++++---- cmd/k8s-operator/ingress-for-pg_test.go | 5 +- cmd/k8s-operator/operator_test.go | 2 +- cmd/k8s-operator/svc-for-pg.go | 32 +++++++++-- cmd/k8s-operator/svc-for-pg_test.go | 73 +++++++++++++++++++++++-- cmd/k8s-operator/svc.go | 1 + 6 files changed, 122 insertions(+), 25 deletions(-) diff --git a/cmd/k8s-operator/ingress-for-pg.go b/cmd/k8s-operator/ingress-for-pg.go index 9cdd9cba9..4779014f3 100644 --- a/cmd/k8s-operator/ingress-for-pg.go +++ b/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 { - tags := strings.Split(tstr, ",") - for _, tag := range tags { - 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)) - } - } + violations := tagViolations(ing) + if len(violations) > 0 { + errs = append(errs, fmt.Errorf("Ingress contains invalid tags: %v", strings.Join(violations, ","))) } // 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 { - 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)) + 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", 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 +} diff --git a/cmd/k8s-operator/ingress-for-pg_test.go b/cmd/k8s-operator/ingress-for-pg_test.go index 3330da8d0..9ce90f771 100644 --- a/cmd/k8s-operator/ingress-for-pg_test.go +++ b/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`, }, } diff --git a/cmd/k8s-operator/operator_test.go b/cmd/k8s-operator/operator_test.go index f4b0db01c..33bf23e84 100644 --- a/cmd/k8s-operator/operator_test.go +++ b/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() diff --git a/cmd/k8s-operator/svc-for-pg.go b/cmd/k8s-operator/svc-for-pg.go index 779f2714e..c9b5b8ae6 100644 --- a/cmd/k8s-operator/svc-for-pg.go +++ b/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 violations := validateService(svc); len(violations) > 0 { - msg := fmt.Sprintf("unable to provision proxy resources: invalid Service: %s", strings.Join(violations, ", ")) - r.recorder.Event(svc, corev1.EventTypeWarning, "INVALIDSERVICE", msg) - r.logger.Error(msg) - tsoperator.SetServiceCondition(svc, tsapi.IngressSvcValid, metav1.ConditionFalse, reasonIngressSvcInvalid, msg, r.clock, logger) + if err := r.validateService(ctx, svc, pg); err != nil { + r.recorder.Event(svc, corev1.EventTypeWarning, reasonIngressSvcInvalid, err.Error()) + tsoperator.SetServiceCondition(svc, tsapi.IngressSvcValid, metav1.ConditionFalse, reasonIngressSvcInvalid, err.Error(), 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...) +} diff --git a/cmd/k8s-operator/svc-for-pg_test.go b/cmd/k8s-operator/svc-for-pg_test.go index 4bb633cb8..ecd60af50 100644 --- a/cmd/k8s-operator/svc-for-pg_test.go +++ b/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{ diff --git a/cmd/k8s-operator/svc.go b/cmd/k8s-operator/svc.go index d6a6f440f..c880f59f5 100644 --- a/cmd/k8s-operator/svc.go +++ b/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 }