From ef1e14250c40b28c68691f88dc1b6d1cc33425c0 Mon Sep 17 00:00:00 2001
From: Tom Proctor <tomhjp@users.noreply.github.com>
Date: Tue, 18 Mar 2025 05:48:59 -0700
Subject: [PATCH] cmd/k8s-operator: ensure old VIPServices are cleaned up
 (#15344)

When the Ingress is updated to a new hostname, the controller does not
currently clean up the old VIPService from control. Fix this up to parse
the ownership comment correctly and write a test to enforce the improved
behaviour

Updates tailscale/corp#24795

Change-Id: I792ae7684807d254bf2d3cc7aa54aa04a582d1f5

Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
---
 cmd/k8s-operator/ingress-for-pg.go      | 20 ++-------
 cmd/k8s-operator/ingress-for-pg_test.go | 57 +++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/cmd/k8s-operator/ingress-for-pg.go b/cmd/k8s-operator/ingress-for-pg.go
index 85a64a336..cdbfecb35 100644
--- a/cmd/k8s-operator/ingress-for-pg.go
+++ b/cmd/k8s-operator/ingress-for-pg.go
@@ -402,16 +402,9 @@ func (r *HAIngressReconciler) maybeCleanupProxyGroup(ctx context.Context, proxyG
 			logger.Infof("VIPService %q is not owned by any Ingress, cleaning up", vipServiceName)
 
 			// Delete the VIPService from control if necessary.
-			svc, _ := r.tsClient.GetVIPService(ctx, vipServiceName)
-			if svc != nil && isVIPServiceForAnyIngress(svc) {
-				logger.Infof("cleaning up orphaned VIPService %q", vipServiceName)
-				svcsChanged, err = r.cleanupVIPService(ctx, vipServiceName, logger)
-				if err != nil {
-					errResp := &tailscale.ErrResponse{}
-					if !errors.As(err, &errResp) || errResp.Status != http.StatusNotFound {
-						return false, fmt.Errorf("deleting VIPService %q: %w", vipServiceName, err)
-					}
-				}
+			svcsChanged, err = r.cleanupVIPService(ctx, vipServiceName, logger)
+			if err != nil {
+				return false, fmt.Errorf("deleting VIPService %q: %w", vipServiceName, err)
 			}
 
 			// Make sure the VIPService is not advertised in tailscaled or serve config.
@@ -570,13 +563,6 @@ func (r *HAIngressReconciler) shouldExpose(ing *networkingv1.Ingress) bool {
 	return isTSIngress && pgAnnot != ""
 }
 
-func isVIPServiceForAnyIngress(svc *tailscale.VIPService) bool {
-	if svc == nil {
-		return false
-	}
-	return strings.HasPrefix(svc.Comment, "tailscale.com/k8s-operator:owned-by:")
-}
-
 // validateIngress validates that the Ingress is properly configured.
 // Currently validates:
 // - Any tags provided via tailscale.com/tags annotation are valid Tailscale ACL tags
diff --git a/cmd/k8s-operator/ingress-for-pg_test.go b/cmd/k8s-operator/ingress-for-pg_test.go
index 7a995e169..2f675337e 100644
--- a/cmd/k8s-operator/ingress-for-pg_test.go
+++ b/cmd/k8s-operator/ingress-for-pg_test.go
@@ -8,8 +8,10 @@
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"maps"
+	"net/http"
 	"reflect"
 	"testing"
 
@@ -186,6 +188,61 @@ func TestIngressPGReconciler(t *testing.T) {
 	verifyTailscaledConfig(t, fc, nil)
 }
 
+func TestIngressPGReconciler_UpdateIngressHostname(t *testing.T) {
+	ingPGR, fc, ft := setupIngressTest(t)
+
+	ing := &networkingv1.Ingress{
+		TypeMeta: metav1.TypeMeta{Kind: "Ingress", APIVersion: "networking.k8s.io/v1"},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-ingress",
+			Namespace: "default",
+			UID:       types.UID("1234-UID"),
+			Annotations: map[string]string{
+				"tailscale.com/proxy-group": "test-pg",
+			},
+		},
+		Spec: networkingv1.IngressSpec{
+			IngressClassName: ptr.To("tailscale"),
+			DefaultBackend: &networkingv1.IngressBackend{
+				Service: &networkingv1.IngressServiceBackend{
+					Name: "test",
+					Port: networkingv1.ServiceBackendPort{
+						Number: 8080,
+					},
+				},
+			},
+			TLS: []networkingv1.IngressTLS{
+				{Hosts: []string{"my-svc.tailnetxyz.ts.net"}},
+			},
+		},
+	}
+	mustCreate(t, fc, ing)
+
+	// Verify initial reconciliation
+	expectReconciled(t, ingPGR, "default", "test-ingress")
+	verifyServeConfig(t, fc, "svc:my-svc", false)
+	verifyVIPService(t, ft, "svc:my-svc", []string{"443"})
+	verifyTailscaledConfig(t, fc, []string{"svc:my-svc"})
+
+	// Update the Ingress hostname and make sure the original VIPService is deleted.
+	mustUpdate(t, fc, "default", "test-ingress", func(ing *networkingv1.Ingress) {
+		ing.Spec.TLS[0].Hosts[0] = "updated-svc.tailnetxyz.ts.net"
+	})
+	expectReconciled(t, ingPGR, "default", "test-ingress")
+	verifyServeConfig(t, fc, "svc:updated-svc", false)
+	verifyVIPService(t, ft, "svc:updated-svc", []string{"443"})
+	verifyTailscaledConfig(t, fc, []string{"svc:updated-svc"})
+
+	_, err := ft.GetVIPService(context.Background(), tailcfg.ServiceName("svc:my-svc"))
+	if err == nil {
+		t.Fatalf("svc:my-svc not cleaned up")
+	}
+	var errResp *tailscale.ErrResponse
+	if !errors.As(err, &errResp) || errResp.Status != http.StatusNotFound {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
 func TestValidateIngress(t *testing.T) {
 	baseIngress := &networkingv1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{