cmd/k8s-operator: reorder Ingress cleanup so cert loop stops before VIPService delete (#20426)

The cert loop only stops when the domain leaves the ServeConfig. Deleting the VIPService first left the loop hammering ACME for a domain the control plane no longer recognised, burning retry slots. Reorder to: remove from serve config, unadvertise, delete VIPService, clean cert resources. Updates #20288 Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
2026-07-20 21:23:07 +08:00 · 2026-07-14 12:11:56 +01:00 · 2026-07-14 12:11:56 +01:00 · 236564af75
commit 236564af75
parent 58fcaaf9a5
1 changed files with 83 additions and 60 deletions
--- a/cmd/k8s-operator/ingress-for-pg.go
+++ b/cmd/k8s-operator/ingress-for-pg.go
@ -455,8 +455,10 @@ func (r *HAIngressReconciler) maybeCleanupProxyGroup(ctx context.Context, logger
 	if err := r.List(ctx, ingList); err != nil {
 		return false, fmt.Errorf("listing Ingresses: %w", err)
 	}
-	serveConfigChanged := false
-	// For each Tailscale Service in serve config...
+
+	// Collect orphans first so we are not mutating cfg.Services during
+	// iteration.
+	var orphans []tailcfg.ServiceName
 	for tsSvcName := range cfg.Services {
 		// ...check if there is currently an Ingress with this hostname
 		found := false
@ -469,40 +471,23 @@ func (r *HAIngressReconciler) maybeCleanupProxyGroup(ctx context.Context, logger
 		}

 		if !found {
-			logger.Infof("Tailscale Service %q is not owned by any Ingress, cleaning up", tsSvcName)
-			tsService, err := tsClient.VIPServices().Get(ctx, tsSvcName.String())
-			switch {
-			case tailscale.IsNotFound(err):
-				return false, nil
-			case err != nil:
-				return false, fmt.Errorf("getting Tailscale Service %q: %w", tsSvcName, err)
-			}
-
-			// Delete the Tailscale Service from control if necessary.
-			svcsChanged, err = r.cleanupTailscaleService(ctx, tsService, logger, tsClient)
-			if err != nil {
-				return false, fmt.Errorf("deleting Tailscale Service %q: %w", tsSvcName, err)
-			}
-
-			// Make sure the Tailscale Service is not advertised in tailscaled or serve config.
-			if err = r.maybeUpdateAdvertiseServicesConfig(ctx, tsSvcName, serviceAdvertisementOff, pg); err != nil {
-				return false, fmt.Errorf("failed to update tailscaled config services: %w", err)
-			}
-
-			_, ok := cfg.Services[tsSvcName]
-			if ok {
-				logger.Infof("Removing Tailscale Service %q from serve config", tsSvcName)
-				delete(cfg.Services, tsSvcName)
-				serveConfigChanged = true
-			}
-
-			if err = cleanupCertResources(ctx, r.Client, r.tsNamespace, tsSvcName, pg); err != nil {
-				return false, fmt.Errorf("failed to clean up cert resources: %w", err)
-			}
+			orphans = append(orphans, tsSvcName)
 		}
 	}

-	if serveConfigChanged {
+	// 1. Remove all orphans from serve config in a single ConfigMap Update
+	// so the proxy cancels every cert loop before we start deleting
+	// VIPServices, and we only pay one fsnotify propagation window.
+	updated := false
+	for _, tsSvcName := range orphans {
+		logger.Infof("Tailscale Service %q is not owned by any Ingress, cleaning up", tsSvcName)
+		_, ok := cfg.Services[tsSvcName]
+		if ok {
+			delete(cfg.Services, tsSvcName)
+			updated = true
+		}
+	}
+	if updated {
 		cfgBytes, err := json.Marshal(cfg)
 		if err != nil {
 			return false, fmt.Errorf("marshaling serve config: %w", err)
@ -511,7 +496,37 @@ func (r *HAIngressReconciler) maybeCleanupProxyGroup(ctx context.Context, logger
 		if err := r.Update(ctx, cm); err != nil {
 			return false, fmt.Errorf("updating serve config: %w", err)
 		}
+		logger.Infof("Removed Tailscale Services from serve config: %v", orphans)
 	}
+
+	for _, tsSvcName := range orphans {
+		// 2. Unadvertise the Tailscale Service in tailscaled config.
+		if err := r.maybeUpdateAdvertiseServicesConfig(ctx, tsSvcName, serviceAdvertisementOff, pg); err != nil {
+			return svcsChanged, fmt.Errorf("failed to update tailscaled config services: %w", err)
+		}
+
+		// 3. Delete the Tailscale Service from the control plane.
+		tsService, err := tsClient.VIPServices().Get(ctx, tsSvcName.String())
+		switch {
+		case tailscale.IsNotFound(err):
+			// Already gone at the control plane; continue with cluster
+			// cleanup rather than aborting the sweep.
+		case err != nil:
+			return svcsChanged, fmt.Errorf("getting Tailscale Service %q: %w", tsSvcName, err)
+		default:
+			updated, err := r.cleanupTailscaleService(ctx, tsService, logger, tsClient)
+			if err != nil {
+				return svcsChanged, fmt.Errorf("deleting Tailscale Service %q: %w", tsSvcName, err)
+			}
+			svcsChanged = svcsChanged || updated
+		}
+
+		// 4. Clean up cluster cert resources.
+		if err := cleanupCertResources(ctx, r.Client, r.tsNamespace, tsSvcName, pg); err != nil {
+			return svcsChanged, fmt.Errorf("failed to clean up cert resources: %w", err)
+		}
+	}
+
 	return svcsChanged, nil
 }

@ -519,6 +534,10 @@ func (r *HAIngressReconciler) maybeCleanupProxyGroup(ctx context.Context, logger
 // Ingress is being deleted or is unexposed. The cleanup is safe for a multi-cluster setup- the Tailscale Service is only
 // deleted if it does not contain any other owner references. If it does the cleanup only removes the owner reference
 // corresponding to this Ingress.
+//
+// Steps are ordered so the proxy cancels its cert loop (via serve config
+// removal) before the VIPService is deleted; otherwise the loop retries
+// against a domain the control plane no longer recognises.
 func (r *HAIngressReconciler) maybeCleanup(ctx context.Context, hostname string, ing *networkingv1.Ingress, logger *zap.SugaredLogger, tsClient tsclient.Client, pg *tsapi.ProxyGroup) (svcChanged bool, err error) {
 	logger.Debugf("Ensuring any resources for Ingress are cleaned up")
 	ix := slices.Index(ing.Finalizers, FinalizerNamePG)
@ -543,49 +562,53 @@ func (r *HAIngressReconciler) maybeCleanup(ctx context.Context, hostname string,
 		err = r.deleteFinalizer(ctx, ing, logger)
 	}()

-	// 1. Check if there is a Tailscale Service associated with this Ingress.
 	cm, cfg, err := r.proxyGroupServeConfig(ctx, pg.Name)
 	if err != nil {
 		return false, fmt.Errorf("error getting ProxyGroup serve config: %w", err)
 	}

-	// Tailscale Service is always first added to serve config and only then created in the Tailscale API, so if it is not
-	// found in the serve config, we can assume that there is no Tailscale Service. (If the serve config does not exist at
-	// all, it is possible that the ProxyGroup has been deleted before cleaning up the Ingress, so carry on with
-	// cleanup).
-	if cfg != nil && cfg.Services != nil && cfg.Services[serviceName] == nil {
-		return false, nil
+	// 1. Remove the Tailscale Service from the proxy's serve config. The proxy
+	// picks up the change via fsnotify on the mounted ConfigMap and cancels
+	// its cert loop for this domain before we proceed to delete the
+	// VIPService.
+	if cfg != nil && cfg.Services != nil {
+		if _, ok := cfg.Services[serviceName]; ok {
+			logger.Infof("Removing TailscaleService %q from serve config for ProxyGroup %q", hostname, pg.Name)
+			delete(cfg.Services, serviceName)
+			cfgBytes, err := json.Marshal(cfg)
+			if err != nil {
+				return false, fmt.Errorf("error marshaling serve config: %w", err)
+			}
+			mak.Set(&cm.BinaryData, serveConfigKey, cfgBytes)
+			if err := r.Update(ctx, cm); err != nil {
+				return false, fmt.Errorf("error updating serve config: %w", err)
+			}
+		}
 	}

-	// 2. Clean up the Tailscale Service resources.
+	// 2. Unadvertise the Tailscale Service in each proxy's tailscaled config.
+	// Skipped if the ProxyGroup itself has been deleted (no config Secrets to
+	// update).
+	if cfg != nil {
+		if err = r.maybeUpdateAdvertiseServicesConfig(ctx, serviceName, serviceAdvertisementOff, pg); err != nil {
+			return false, fmt.Errorf("failed to update tailscaled config services: %w", err)
+		}
+	}
+
+	// 3. Delete the Tailscale Service from the control plane. By now the
+	// proxy has stopped serving HTTPS for the domain and stopped trying to
+	// renew its cert.
 	svcChanged, err = r.cleanupTailscaleService(ctx, svc, logger, tsClient)
 	if err != nil {
 		return false, fmt.Errorf("error deleting Tailscale Service: %w", err)
 	}

-	// 3. Clean up any cluster resources
+	// 4. Clean up cluster cert resources (TLS Secret + RBAC).
 	if err = cleanupCertResources(ctx, r.Client, r.tsNamespace, serviceName, pg); err != nil {
 		return false, fmt.Errorf("failed to clean up cert resources: %w", err)
 	}

-	if cfg == nil || cfg.Services == nil { // user probably deleted the ProxyGroup
-		return svcChanged, nil
-	}
-
-	// 4. Unadvertise the Tailscale Service in tailscaled config.
-	if err = r.maybeUpdateAdvertiseServicesConfig(ctx, serviceName, serviceAdvertisementOff, pg); err != nil {
-		return false, fmt.Errorf("failed to update tailscaled config services: %w", err)
-	}
-
-	// 5. Remove the Tailscale Service from the serve config for the ProxyGroup.
-	logger.Infof("Removing TailscaleService %q from serve config for ProxyGroup %q", hostname, pg.Name)
-	delete(cfg.Services, serviceName)
-	cfgBytes, err := json.Marshal(cfg)
-	if err != nil {
-		return false, fmt.Errorf("error marshaling serve config: %w", err)
-	}
-	mak.Set(&cm.BinaryData, serveConfigKey, cfgBytes)
-	return svcChanged, r.Update(ctx, cm)
+	return svcChanged, nil
 }

 func (r *HAIngressReconciler) deleteFinalizer(ctx context.Context, ing *networkingv1.Ingress, logger *zap.SugaredLogger) error {