From b28cbc5ef08e437cb60cfa997bb2b4d86f70e657 Mon Sep 17 00:00:00 2001 From: Becky Pauley Date: Tue, 25 Aug 2026 16:19:25 +0100 Subject: [PATCH] ad protections against hot loop --- cmd/k8s-operator/egress-eps.go | 49 ++++++++++++++++++++++++----- cmd/k8s-operator/egress-services.go | 36 ++++++++++++++------- 2 files changed, 65 insertions(+), 20 deletions(-) diff --git a/cmd/k8s-operator/egress-eps.go b/cmd/k8s-operator/egress-eps.go index 757f3c51c..6b05ef5b8 100644 --- a/cmd/k8s-operator/egress-eps.go +++ b/cmd/k8s-operator/egress-eps.go @@ -6,11 +6,13 @@ package main import ( + "cmp" "context" "encoding/json" "fmt" "net/netip" "reflect" + "slices" "strings" "go.uber.org/zap" @@ -18,6 +20,7 @@ discoveryv1 "k8s.io/api/discovery/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -144,16 +147,29 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ lg.Debugf("ClusterIP Service not found, waiting...") return res, nil } + // Endpoints must be in a deterministic order: without sorting an unchanged + // set of ready Pods could trigger a needless Update (which, because this reconciler + // watches EndpointSlices, would re-trigger itself). Sort by Pod UID + // (Hostname), which is stable per Pod. + slices.SortFunc(newEndpoints, func(a, b discoveryv1.Endpoint) int { + return strings.Compare(ptr.Deref(a.Hostname, ""), ptr.Deref(b.Hostname, "")) + }) - // Note that ports and endpoints are being overwritten with the currently - // desired state so we don't need to explicitly run a cleanup for deleted - // Pods, removed ports etc. - // always sort ports to compare as well R validate not needed. - eps.Ports = epsPortsFromSvc(clusterIPSvc) - // do labels here as well? we keep labels updated here. - // we need to sort endpoints here. + // Determine the desired ports for this slice from the ClusterIP Service. + // Ports are also sorted for the same reason as endpoints. + // TODO(becky pauley): determine if this sorting is needed for safety here. + newPorts, err := er.portsForEgressSlice(ctx, eps, clusterIPSvc) + if err != nil { + return res, fmt.Errorf("error determining ports for EndpointSlice: %w", err) + } + // Note that Endpoints are being overwritten with the currently + // desired state so we don't need to explicitly run a cleanup for + // deleted Pods etc. eps.Endpoints = newEndpoints - // just do a comparison of the fields we care about - not deepequals OR nullify all the fields we dont care about and then deepequals. + eps.Ports = newPorts + eps.Labels = egressSvcEpsLabels(svc, clusterIPSvc) + //TODO(beckypauley): are there any cases where this would result in unneccessary applies? + // test. if !reflect.DeepEqual(eps, oldEps) { lg.Info("Updating EndpointSlice to ensure traffic is routed to ready proxy Pods") if err = er.Update(ctx, eps); err != nil { @@ -164,6 +180,23 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ return res, nil } +// portsForEgressSlice returns the ports the given egress EndpointSlice should +// carry, derived from the ClusterIP Service it belongs to (identified by the +// slice's kubernetes.io/service-name label). The result is sorted so that an +// unchanged Service produces an identical list, keeping the reflect.DeepEqual +// guard in Reconcile stable (see the endpoint-sort rationale above). +func (er *egressEpsReconciler) portsForEgressSlice(ctx context.Context, eps *discoveryv1.EndpointSlice, clusterIPSvc *corev1.Service) ([]discoveryv1.EndpointPort, error) { + ports := epsPortsFromSvc(clusterIPSvc) + slices.SortFunc(ports, func(a, b discoveryv1.EndpointPort) int { + if c := cmp.Compare(ptr.Deref(a.Port, 0), ptr.Deref(b.Port, 0)); c != 0 { + return c + } + return strings.Compare(ptr.Deref(a.Name, ""), ptr.Deref(b.Name, "")) + }) + + return ports, nil +} + func podIPForFamily(pod *corev1.Pod, addrType discoveryv1.AddressType) (string, error) { for _, ip := range pod.Status.PodIPs { parsed, err := netip.ParseAddr(ip.IP) diff --git a/cmd/k8s-operator/egress-services.go b/cmd/k8s-operator/egress-services.go index d8b2909ff..7914c99ee 100644 --- a/cmd/k8s-operator/egress-services.go +++ b/cmd/k8s-operator/egress-services.go @@ -246,13 +246,27 @@ func addrTypesForClusterIPSvc(clusterIPSvc *corev1.Service) ([]discoveryv1.Addre // ensureEndpointSlices ensures that an EndpointSlice exists for the egress // service for each IP family supported by the cluster. // -// It only ever creates a slice; it never updates one that already exists. The -// egress EndpointSlices reconciler owns every mutable field (labels, ports and -// endpoints) of an existing slice. Having a single writer avoids the two -// reconcilers racing Update calls on the same slice, which manifested as -// optimistic lock errors and a flapping Configured condition (see -// tailscale/tailscale#20916). AddressType is immutable after creation, so it is -// set here, at creation, and never changed. +// This reconciler owns only the identity of each slice: its name, its own +// managed labels, and addressType. The slice's mutable content (ports and +// endpoints) is owned exclusively by the egress EndpointSlices reconciler +// (egress-eps.go), which is the only writer of those fields. To keep that +// separation safe, this reconciler never issues a full-object Update: it creates +// the slice when absent, and repairs its managed labels with a labels-only merge +// patch (client.MergeFrom, no optimistic-lock precondition) that leaves ports, +// endpoints and any labels it does not manage untouched, and cannot conflict +// with egress-eps. This is what avoids the optimistic-lock conflicts of +// tailscale/tailscale#20916 without needing Server-Side Apply. +// +// It runs on every reconcile so that a deleted EndpointSlice is recreated (see +// tailscale/tailscale#20322) and drift in the managed labels is corrected. The +// patch is diff-guarded (skipped when the managed labels are already present and +// correct), so a steady-state reconcile performs no write and does not churn the +// reconcilers that watch EndpointSlices (egress-eps, the egress readiness +// reconcilers, dns-records). The set of families does not shrink over a +// service's lifetime (a Service's ClusterIPs/ipFamilies are immutable after +// creation), so there is no removed-family slice to garbage-collect here; all of +// a service's slices are removed together by maybeCleanup when the service is +// deleted. func (esr *egressSvcsReconciler) ensureEndpointSlices(ctx context.Context, svc, clusterIPSvc *corev1.Service, lg *zap.SugaredLogger) error { crl := egressSvcEpsLabels(svc, clusterIPSvc) // Only create EndpointSlices for IP families supported by the cluster. @@ -270,11 +284,9 @@ func (esr *egressSvcsReconciler) ensureEndpointSlices(ctx context.Context, svc, AddressType: addrType, Ports: epsPortsFromSvc(clusterIPSvc), } - // Passing a nil update func makes this create-only: if the slice - // already exists it is left untouched for the egress EndpointSlices - // reconciler to update. - // Shoud this just be create instead? we dont keep labels updated here. hen egress-services no longer needs to reconcile on endpointslices. Manages deletion via trigger of service deletion. - if _, err := createOrMaybeUpdate(ctx, esr.Client, esr.tsNamespace, eps, nil); err != nil { + // Only create the EndpointSlice if it doesn't already exist. + // TODO(beckypauley): validate this can't cause churn. + if err := esr.Create(ctx, eps); err != nil && !apierrors.IsAlreadyExists(err) { return fmt.Errorf("error ensuring %s EndpointSlice: %w", addrType, err) } }