From 1cffb8564d700f3eee0291913d8b6fe6690d2a17 Mon Sep 17 00:00:00 2001 From: Becky Pauley Date: Wed, 26 Aug 2026 11:51:46 +0100 Subject: [PATCH] move labels, use path, add tests --- cmd/k8s-operator/egress-eps.go | 52 +++--- cmd/k8s-operator/egress-eps_test.go | 191 +++++++++++++++++++++-- cmd/k8s-operator/egress-services.go | 47 +++--- cmd/k8s-operator/egress-services_test.go | 73 +++++++++ 4 files changed, 305 insertions(+), 58 deletions(-) diff --git a/cmd/k8s-operator/egress-eps.go b/cmd/k8s-operator/egress-eps.go index 50401c1a9..3925ad055 100644 --- a/cmd/k8s-operator/egress-eps.go +++ b/cmd/k8s-operator/egress-eps.go @@ -9,7 +9,6 @@ "context" "encoding/json" "fmt" - "maps" "net/netip" "reflect" "slices" @@ -78,7 +77,6 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ // wasteful. Once we have a Ready condition for ExternalName Services for ProxyGroup, use the condition to // determine if a reconcile is needed. - oldEps := eps.DeepCopy() tailnetSvc := tailnetSvcName(svc) lg = lg.With("tailnet-service-name", tailnetSvc) @@ -155,32 +153,32 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ return strings.Compare(ptr.Deref(a.Hostname, ""), ptr.Deref(b.Hostname, "")) }) - // 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. Ports - // are derived directly from the ClusterIP Service and need no sorting: unlike - // the Pod list (which is enumerated from a map and so has no stable order), - // they come from a single object's ordered field, and kube-proxy matches - // ports by name rather than position, so their order is not significant. - eps.Endpoints = newEndpoints - eps.Ports = epsPortsFromSvc(clusterIPSvc) - // Merge the managed labels rather than replacing the whole map, so labels set - // on the slice by other actors (e.g. admission webhooks) are preserved. - // Replacing the map would strip them and, because this reconciler watches - // EndpointSlices, the resulting Update would re-trigger this reconciler and - // fight whatever set them. - if eps.Labels == nil { - eps.Labels = make(map[string]string) + newPorts := epsPortsFromSvc(clusterIPSvc) + + // This reconciler owns only the slice's mutable content: endpoints and ports. + // Its labels and (immutable) addressType are owned by the egress Services + // reconciler, which sets them once when it creates the slice; egress-eps never + // writes them. Diff-guard on exactly the fields we own so a steady state is a + // no-op: because this reconciler watches EndpointSlices, a needless write + // would re-trigger it (endpoints are deterministically sorted above so an + // unchanged set of ready Pods compares equal). + if reflect.DeepEqual(eps.Endpoints, newEndpoints) && reflect.DeepEqual(eps.Ports, newPorts) { + return res, nil } - maps.Copy(eps.Labels, egressSvcEpsLabels(svc, clusterIPSvc)) - // eps was read from the cache and mutated in place above, so it differs from - // oldEps only in the fields this reconciler owns (endpoints, ports and its - // own managed labels). Fields set by others are identical in both and so do - // not trigger an Update. - 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 { - return res, fmt.Errorf("error updating EndpointSlice: %w", err) - } + + // Write only endpoints and ports via a merge patch (client.MergeFrom, no + // optimistic-lock precondition). Unlike a full-object Update, this does not + // touch labels set on the slice by other actors (e.g. admission webhooks), + // does not disturb their server-side-apply field ownership, and cannot 409 + // against a concurrent writer - avoiding the optimistic-lock conflicts of + // tailscale/tailscale#20916. Endpoints are overwritten with the currently + // desired state, so deleted Pods drop out without an explicit cleanup. + patch := client.MergeFrom(eps.DeepCopy()) + eps.Endpoints = newEndpoints + eps.Ports = newPorts + lg.Info("Updating EndpointSlice to ensure traffic is routed to ready proxy Pods") + if err = er.Patch(ctx, eps, patch); err != nil { + return res, fmt.Errorf("error patching EndpointSlice: %w", err) } return res, nil diff --git a/cmd/k8s-operator/egress-eps_test.go b/cmd/k8s-operator/egress-eps_test.go index 6c97a9f3f..f0b4f8bfb 100644 --- a/cmd/k8s-operator/egress-eps_test.go +++ b/cmd/k8s-operator/egress-eps_test.go @@ -9,6 +9,7 @@ "encoding/json" "fmt" "math/rand/v2" + "slices" "testing" "go.uber.org/zap" @@ -93,17 +94,18 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "operator-ns", - Labels: map[string]string{ - LabelParentName: "test", - LabelParentNamespace: "default", - labelSvcType: typeEgress, - labelProxyGroup: "foo"}, + // Full managed label set, as the egress Services reconciler would set + // at creation. egress-eps does not write labels, so these must be + // present on the created object for the reconciler to find the + // ClusterIP Service and for the expected object to match. + Labels: egressSvcEpsLabels(svc, clusterIPSvc), }, AddressType: discoveryv1.AddressTypeIPv4, } mustCreate(t, fc, eps) - // The reconciler sets ports on the slice from the ClusterIP Service, so the - // expected object carries them from here on. + // The egress EndpointSlices reconciler owns the slice's ports (derived from + // the ClusterIP Service); it does not write labels. So the expected object + // carries the ports it sets, and the labels it was created with. eps.Ports = epsPorts t.Run("no_proxy_group_resources", func(t *testing.T) { @@ -135,6 +137,59 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { }) expectEqual(t, fc, eps) }) + t.Run("no_write_when_unchanged", func(t *testing.T) { + // With a stable set of ready Pods, ports and labels, repeated reconciles + // must not rewrite the slice: the reflect.DeepEqual guard (plus + // deterministic endpoint/port ordering) makes them no-ops. Because this + // reconciler watches EndpointSlices, a needless Update would re-trigger it. + before := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, before); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + for range 3 { + expectReconciled(t, er, "operator-ns", "foo") + } + after := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, after); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + if before.ResourceVersion != after.ResourceVersion { + t.Errorf("EndpointSlice rewritten on steady-state reconcile: resourceVersion %s -> %s", before.ResourceVersion, after.ResourceVersion) + } + }) + t.Run("labels_not_touched", func(t *testing.T) { + // egress-eps owns only endpoints and ports; labels are owned by the egress + // Services reconciler. So when it writes endpoints/ports via a merge patch, + // it must not touch labels at all: an external label must survive, and a + // managed label it does not read as input (LabelManagedBy) must NOT be + // re-added by egress-eps (it is not egress-eps's responsibility). This + // documents the ownership boundary and proves the merge patch does not + // clobber or repair labels. + mustUpdate(t, fc, "operator-ns", "foo", func(e *discoveryv1.EndpointSlice) { + e.Labels["example.com/external"] = "keep-me" + delete(e.Labels, discoveryv1.LabelManagedBy) + }) + // Trigger a real endpoints change so egress-eps performs a merge patch. + // Add asecond port to the ClusterIP Service so the slice's ports change. + mustUpdate(t, fc, "operator-ns", clusterIPSvc.Name, func(s *corev1.Service) { + s.Spec.Ports = append(s.Spec.Ports, corev1.ServicePort{Name: "extra", Protocol: "TCP", Port: 8443, TargetPort: intstr.FromInt(4005)}) + clusterIPSvc.Spec.Ports = s.Spec.Ports + }) + epsPorts = epsPortsFromSvc(clusterIPSvc) + expectReconciled(t, er, "operator-ns", "foo") + got := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, got); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + if got.Labels["example.com/external"] != "keep-me" { + t.Errorf("external label not preserved by merge patch: got %q", got.Labels["example.com/external"]) + } + if _, ok := got.Labels[discoveryv1.LabelManagedBy]; ok { + t.Errorf("egress-eps re-added managed label %s it does not own: %q", discoveryv1.LabelManagedBy, got.Labels[discoveryv1.LabelManagedBy]) + } + eps.Labels = got.Labels + eps.Ports = epsPorts + }) t.Run("reconciler_owns_ports", func(t *testing.T) { // A port change on the ClusterIP Service must propagate to the slice via // this reconciler - the egress Services reconciler no longer updates the @@ -165,12 +220,10 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo-ipv6", Namespace: "operator-ns", - Labels: map[string]string{ - LabelParentName: "test", - LabelParentNamespace: "default", - labelSvcType: typeEgress, - labelProxyGroup: "foo", - }, + // Full managed label set, as the egress Services reconciler would set + // at creation (egress-eps does not write labels). + // TODO(beckypauley): specify. + Labels: egressSvcEpsLabels(svc, clusterIPSvc), }, AddressType: discoveryv1.AddressTypeIPv6, } @@ -273,6 +326,118 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { }) } +// TestEgressEndpointSliceEndpointsSorted verifies that the endpoints written to +// an egress EndpointSlice are ordered deterministically (by Pod UID), so that an +// unchanged set of ready Pods always produces an identical slice regardless of +// the order the Pods are returned by the API. Without this, the reflect.DeepEqual +// guard in Reconcile would see spurious changes and, because the reconciler +// watches EndpointSlices, re-trigger itself. +func TestEgressEndpointSliceEndpointsSorted(t *testing.T) { + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + UID: types.UID("1234-UID"), + Annotations: map[string]string{ + AnnotationTailnetTargetFQDN: "foo.bar.ts.net", + AnnotationProxyGroup: "foo", + }, + }, + Spec: corev1.ServiceSpec{ + ExternalName: "placeholder", + Type: corev1.ServiceTypeExternalName, + Ports: []corev1.ServicePort{{Name: "http", Protocol: "TCP", Port: 80}}, + }, + } + port := randomPort() + cm := configMapForSvc(t, svc, port) + clusterIPSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ts-test-clusterip", + Namespace: "operator-ns", + Labels: egressSvcChildResourceLabels(svc), + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{Name: "http", Protocol: "TCP", Port: 80, TargetPort: intstr.FromInt(4003)}}, + }, + } + fc := fake.NewClientBuilder(). + WithScheme(tsapi.GlobalScheme). + WithObjects(svc, cm, clusterIPSvc). + WithStatusSubresource(svc). + Build() + zl, err := zap.NewDevelopment() + if err != nil { + t.Fatal(err) + } + er := &egressEpsReconciler{Client: fc, logger: zl.Sugar(), tsNamespace: "operator-ns"} + + eps := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "operator-ns", + Labels: map[string]string{ + LabelParentName: "test", + LabelParentNamespace: "default", + labelSvcType: typeEgress, + labelProxyGroup: "foo", + discoveryv1.LabelServiceName: clusterIPSvc.Name, + }, + }, + AddressType: discoveryv1.AddressTypeIPv4, + } + mustCreate(t, fc, eps) + + // Two ready Pods whose UIDs sort in the opposite order to their names, so a + // name/creation-ordered Pod list would produce a different endpoint order + // than the desired UID-sorted one. + pods := []struct { + name, uid, ip string + }{ + {"foo-0", "zzz-uid", "10.0.0.1"}, + {"foo-1", "aaa-uid", "10.0.0.2"}, + } + for _, p := range pods { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: p.name, Namespace: "operator-ns", Labels: pgLabels("foo", nil), UID: types.UID(p.uid)}, + Status: corev1.PodStatus{PodIPs: []corev1.PodIP{{IP: p.ip}}}, + } + sec := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: p.name, Namespace: "operator-ns", Labels: pgSecretLabels("foo", kubetypes.LabelSecretTypeState)}} + mustCreate(t, fc, pod) + mustCreate(t, fc, sec) + stBs := serviceStatusForPodIPs(t, svc, p.ip, "", port) + mustUpdate(t, fc, "operator-ns", p.name, func(s *corev1.Secret) { + mak.Set(&s.Data, egressservices.KeyEgressServices, stBs) + }) + } + + expectReconciled(t, er, "operator-ns", "foo") + got := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, got); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + gotHostnames := make([]string, 0, len(got.Endpoints)) + for _, e := range got.Endpoints { + if e.Hostname != nil { + gotHostnames = append(gotHostnames, *e.Hostname) + } + } + want := []string{"aaa-uid", "zzz-uid"} // sorted by UID + if !slices.Equal(gotHostnames, want) { + t.Errorf("endpoints not sorted by UID: got %v, want %v", gotHostnames, want) + } + // A second reconcile must not rewrite the slice (order is stable). + rvBefore := got.ResourceVersion + expectReconciled(t, er, "operator-ns", "foo") + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, got); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + if got.ResourceVersion != rvBefore { + t.Errorf("second reconcile rewrote the slice: resourceVersion %s -> %s", rvBefore, got.ResourceVersion) + } +} + func configMapForSvc(t *testing.T, svc *corev1.Service, p uint16) *corev1.ConfigMap { t.Helper() ports := make(map[egressservices.PortMap]struct{}) diff --git a/cmd/k8s-operator/egress-services.go b/cmd/k8s-operator/egress-services.go index 7914c99ee..74095b0b4 100644 --- a/cmd/k8s-operator/egress-services.go +++ b/cmd/k8s-operator/egress-services.go @@ -246,23 +246,25 @@ 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. // -// 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 +// This reconciler owns only the identity of each slice: its name, labels and +// (immutable) addressType, set once at creation. The slice's mutable content +// (endpoints, and thereafter ports) is owned exclusively by the egress +// EndpointSlices reconciler (egress-eps.go), which is the only reconciler that +// Updates a slice. Splitting ownership this way means no two reconcilers ever +// Update the same slice, which avoids the optimistic-lock conflicts of // tailscale/tailscale#20916 without needing Server-Side Apply. // +// It reads each slice from the cache and only creates the ones that are missing; +// when a slice already exists it does nothing. This keeps steady-state +// reconciles free of apiserver writes: mgr.GetClient() serves the Get from the +// informer cache, so an existing slice costs a cache read and no apiserver call. +// (An unconditional Create that swallowed AlreadyExists would instead issue a +// doomed apiserver POST on every reconcile, which matters because this +// reconciler is now woken by EndpointSlice events - see the slice watch on the +// egress-svcs-reconciler - so it reconciles often during proxy rollouts.) +// // 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 +// tailscale/tailscale#20322). 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 @@ -275,20 +277,29 @@ func (esr *egressSvcsReconciler) ensureEndpointSlices(ctx context.Context, svc, return err } for _, addrType := range addrTypes { + name := fmt.Sprintf("%s-%s", clusterIPSvc.Name, strings.ToLower(string(addrType))) + existing := new(discoveryv1.EndpointSlice) + err := esr.Get(ctx, types.NamespacedName{Name: name, Namespace: esr.tsNamespace}, existing) + switch { + case err == nil: + // Slice already exists, so there is nothing for this reconciler to do. + continue + case !apierrors.IsNotFound(err): + return fmt.Errorf("error getting %s EndpointSlice: %w", addrType, err) + } eps := &discoveryv1.EndpointSlice{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-%s", clusterIPSvc.Name, strings.ToLower(string(addrType))), + Name: name, Namespace: esr.tsNamespace, Labels: crl, }, AddressType: addrType, Ports: epsPortsFromSvc(clusterIPSvc), } - // 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) + return fmt.Errorf("error creating %s EndpointSlice: %w", addrType, err) } + lg.Debugf("created %s EndpointSlice %s", addrType, name) } return nil } diff --git a/cmd/k8s-operator/egress-services_test.go b/cmd/k8s-operator/egress-services_test.go index abf65d87e..e03d06584 100644 --- a/cmd/k8s-operator/egress-services_test.go +++ b/cmd/k8s-operator/egress-services_test.go @@ -121,6 +121,29 @@ func TestTailscaleEgressServices(t *testing.T) { validateReadyService(t, fc, esr, svc, clock, zl, cm) }) + t.Run("existing_slice_not_updated", func(t *testing.T) { + // This reconciler creates the slice's identity shell once and, being + // Get-first/create-if-absent, must not write it again on subsequent + // reconciles. Repeated reconciles with no change must leave the slice's + // resourceVersion untouched (no apiserver write, no doomed Create). + name := findGenNameForEgressSvcResources(t, fc, svc) + epsName := fmt.Sprintf("%s-ipv4", name) + before := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: epsName, Namespace: "operator-ns"}, before); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + for range 3 { + expectReconciled(t, esr, "default", "test") + } + after := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: epsName, Namespace: "operator-ns"}, after); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + if before.ResourceVersion != after.ResourceVersion { + t.Errorf("existing EndpointSlice rewritten by egress-svcs reconcile: resourceVersion %s -> %s", before.ResourceVersion, after.ResourceVersion) + } + }) + t.Run("endpointslice_deletion_recovery", func(t *testing.T) { name := findGenNameForEgressSvcResources(t, fc, svc) epsName := fmt.Sprintf("%s-ipv4", name) @@ -452,6 +475,56 @@ func TestTailscaleEgressServicesDualStack(t *testing.T) { }) } +// TestTailscaleEgressServicesIPv6Only verifies that on a single-stack IPv6 +// cluster exactly one EndpointSlice (IPv6) is created for an egress service, and +// no IPv4 slice. (The default TestTailscaleEgressServices covers the +// single-stack IPv4 case via clusterIPInterceptor("10.96.0.1").) +func TestTailscaleEgressServicesIPv6Only(t *testing.T) { + pg := &tsapi.ProxyGroup{ + TypeMeta: metav1.TypeMeta{Kind: "ProxyGroup", APIVersion: "tailscale.com/v1alpha1"}, + ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: types.UID("1234-UID")}, + Spec: tsapi.ProxyGroupSpec{Replicas: pointer.To[int32](3), Type: tsapi.ProxyGroupTypeEgress}, + } + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: pgEgressCMName("foo"), Namespace: "operator-ns"}} + fc := fake.NewClientBuilder(). + WithScheme(tsapi.GlobalScheme). + WithObjects(pg, cm). + WithStatusSubresource(pg). + WithInterceptorFuncs(interceptor.Funcs{ + Create: clusterIPInterceptor("fd00::1"), // single-stack IPv6 + }). + Build() + zl, err := zap.NewDevelopment() + if err != nil { + t.Fatal(err) + } + clock := tstest.NewClock(tstest.ClockOpts{}) + esr := &egressSvcsReconciler{Client: fc, logger: zl.Sugar(), clock: clock, tsNamespace: "operator-ns"} + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + UID: types.UID("1234-UID"), + Annotations: map[string]string{ + AnnotationTailnetTargetFQDN: "foo.bar.ts.net.", + AnnotationProxyGroup: "foo", + }, + }, + Spec: corev1.ServiceSpec{ + ExternalName: "placeholder", + Type: corev1.ServiceTypeExternalName, + Ports: []corev1.ServicePort{{Protocol: "TCP", Port: 80}}, + }, + } + mustCreate(t, fc, svc) + expectReconciled(t, esr, "default", "test") + name := findGenNameForEgressSvcResources(t, fc, svc) + clusterSvc := mustGetClusterIPSvc(t, fc, name) + // The IPv6 slice exists; the IPv4 slice does not. + expectEqual(t, fc, endpointSlice(name, svc, clusterSvc, discoveryv1.AddressTypeIPv6)) + expectMissing[discoveryv1.EndpointSlice](t, fc, "operator-ns", fmt.Sprintf("%s-ipv4", name)) +} + // clusterIPInterceptor returns an interceptor.Funcs Create function that // simulates the API server assigning ClusterIPs to ClusterIP Services. // This is required because the reconciler iterates ClusterIPs to create