ad protections against hot loop

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)

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)
 		}
 	}