From 236564af757e2b6bce266b6278bbb203763e6be2 Mon Sep 17 00:00:00 2001 From: Tom Meadows Date: Tue, 14 Jul 2026 12:11:56 +0100 Subject: [PATCH] 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 --- cmd/k8s-operator/ingress-for-pg.go | 143 +++++++++++++++++------------ 1 file changed, 83 insertions(+), 60 deletions(-) diff --git a/cmd/k8s-operator/ingress-for-pg.go b/cmd/k8s-operator/ingress-for-pg.go index 6cacec61e..f76a7da2a 100644 --- 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 {