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>
This commit is contained in:
Tom Proctor 2025-03-18 05:48:59 -07:00 committed by GitHub
parent b413b70ae2
commit ef1e14250c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 60 additions and 17 deletions

View File

@ -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

View File

@ -8,8 +8,10 @@ package main
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{