diff --git a/cmd/k8s-operator/egress-eps.go b/cmd/k8s-operator/egress-eps.go index e596da6dd..75ce388c5 100644 --- a/cmd/k8s-operator/egress-eps.go +++ b/cmd/k8s-operator/egress-eps.go @@ -9,6 +9,7 @@ "context" "encoding/json" "fmt" + "maps" "net/netip" "reflect" "slices" @@ -73,12 +74,41 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ return res, fmt.Errorf("error retrieving ExternalName Service: %w", err) } + tailnetSvc := tailnetSvcName(svc) + lg = lg.With("tailnet-service-name", tailnetSvc) + + // Retrieve the current ClusterIP Service. egress-eps is triggered by the slice itself, so it can be handed an orphan - a + // slice left behind in some circumstances (for example, when the ProxyGroup on the parent Service was changed). + // In this case, the slice's service-name no longer matches the current ClusterIP Service, and writing to it would + // // rebind a stale slice into the live Service, unioning its stale endpoints into cluster traffic. + // Fetch the ClusterIP Service first and only proceed for slices bound to it. + // Note: editing a Service's ProxyGroup in place is not officially supported, but this ensures if a user + // does edit the proxygroup in place it will not erroneously try to migrate an orphaned EndpointSlice in place. + clusterIPSvc, err := getSingleObject[corev1.Service](ctx, er.Client, er.tsNamespace, egressSvcChildResourceLabels(svc)) + if err != nil { + return res, fmt.Errorf("error retrieving ClusterIP Service: %w", err) + } + if clusterIPSvc == nil { + lg.Debugf("ClusterIP Service not found, waiting...") + return res, nil + } + if eps.Labels[discoveryv1.LabelServiceName] != clusterIPSvc.Name { + lg.Debugf("EndpointSlice %s is bound to non-current ClusterIP Service %q (current %q); skipping", + eps.Name, eps.Labels[discoveryv1.LabelServiceName], clusterIPSvc.Name) + return res, nil + } + // TODO(irbekrm): currently this reconcile loop runs all the checks every time it's triggered, which is // wasteful. Once we have a Ready condition for ExternalName Services for ProxyGroup, use the condition to // determine if a reconcile is needed. - tailnetSvc := tailnetSvcName(svc) - lg = lg.With("tailnet-service-name", tailnetSvc) + // Snapshot the slice after the orphan guard and before any mutation. The + // change check below is a whole-object compare (reflect.DeepEqual(eps, oldEps)), + // so nothing between here and the Update may mutate eps except the fields this + // reconciler owns (labels, endpoints, ports) - otherwise an unrelated field + // difference would trigger a needless write which, because this reconciler + // watches EndpointSlices, would re-trigger itself (see tailscale/tailscale#20916). + oldEps := eps.DeepCopy() // Retrieve the desired tailnet service configuration from the ConfigMap. proxyGroupName := eps.Labels[labelProxyGroup] @@ -105,7 +135,12 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ if err := er.List(ctx, podList, client.MatchingLabels(pgLabels(proxyGroupName, nil))); err != nil { return res, fmt.Errorf("error listing Pods for ProxyGroup %s: %w", proxyGroupName, err) } - newEndpoints := make([]discoveryv1.Endpoint, 0) + // Leave newEndpoints nil (not an empty slice) so that when no Pod is ready it + // compares equal to a freshly created slice's nil Endpoints - otherwise the + // reflect.DeepEqual guard below would see nil != []Endpoint{} and, because this + // reconciler watches EndpointSlices, a needless write would re-trigger it. + //TODO(beckypauley): review swapping to nil vs empty. + var newEndpoints []discoveryv1.Endpoint for _, pod := range podList.Items { ready, err := er.podIsReadyToRouteTraffic(ctx, pod, &cfg, tailnetSvc, eps.AddressType, lg) if err != nil { @@ -131,20 +166,6 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ }, }) } - // This reconciler owns the mutable fields of an existing slice - ports and - // endpoints. The egress Services reconciler only ever creates the slice - // (with its immutable AddressType and labels) and never updates it, so a - // single reconciler writes any given slice and the two can't race Update - // calls on it (see tailscale/tailscale#20916). Ports are derived from the - // ClusterIP Service, so retrieve it to compute the desired state. - clusterIPSvc, err := getSingleObject[corev1.Service](ctx, er.Client, er.tsNamespace, egressSvcChildResourceLabels(svc)) - if err != nil { - return res, fmt.Errorf("error retrieving ClusterIP Service: %w", err) - } - if clusterIPSvc == nil { - 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: see tailscale/tailscale#20916.). @@ -153,20 +174,35 @@ func (er *egressEpsReconciler) Reconcile(ctx context.Context, req reconcile.Requ return strings.Compare(ptr.Deref(a.Hostname, ""), ptr.Deref(b.Hostname, "")) }) newPorts := epsPortsFromSvc(clusterIPSvc) - if reflect.DeepEqual(eps.Endpoints, newEndpoints) && reflect.DeepEqual(eps.Ports, newPorts) { + + // This reconciler is the sole writer of a slice after creation (the egress + // Services reconciler only ever Creates it), so it owns all mutable state: + // endpoints, ports and labels. Re-assert the owned labels to repair drift while + // preserving any labels added by other controllers - maps.Copy overwrites only + // the keys we own. Because the guard above established this slice is bound to the + // current ClusterIP Service, the identity labels we write equal what the slice + // already claims (repair, not rebind). + // eps.Labels is guaranteed non-nil here: the parent-Service lookup and the + // orphan guard above both early-return on a slice lacking our labels, so a + // slice that reaches this point carries them. maps.Copy into a nil map panics, + // so do not reorder those guards after this point. + // TODO(beckypauley): consider still having a nil guard here. + maps.Copy(eps.Labels, egressSvcEpsLabels(svc, clusterIPSvc)) + + // Apply the owned fields, then compare the whole object against the pre-mutation + // snapshot: if nothing this reconciler owns changed, skip the write. A needless + // write would re-trigger this reconciler via the EndpointSlice watch (#20916). + eps.Endpoints = newEndpoints + eps.Ports = newPorts + if reflect.DeepEqual(eps, oldEps) { return res, nil } - // Write only endpoints and ports. Labels and addressType are owned by - // the egress Services reconciler, which sets them once at creation time. - // Because this reconciler watches EndpointSlices, a needless write - // would re-trigger it. - patch := client.MergeFrom(eps.DeepCopy()) - eps.Endpoints = newEndpoints - eps.Ports = newPorts + // Single writer, so a plain resourceVersion-gated Update is safe (no two-reconciler + // Update race - see tailscale/tailscale#20916). 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) + if err = er.Update(ctx, eps); err != nil { + return res, fmt.Errorf("error updating 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 f0b4f8bfb..19b6ec495 100644 --- a/cmd/k8s-operator/egress-eps_test.go +++ b/cmd/k8s-operator/egress-eps_test.go @@ -94,18 +94,11 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "operator-ns", - // 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), + Labels: egressSvcEpsLabels(svc, clusterIPSvc), }, AddressType: discoveryv1.AddressTypeIPv4, } mustCreate(t, fc, eps) - // 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) { @@ -157,35 +150,52 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { 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. + t.Run("label_reasserted_when_dropped", func(t *testing.T) { + // egress-eps is the sole writer of the slice after creation, so it owns all + // mutable state: labels, endpoints and ports. It re-asserts the owned labels + // to repair drift (e.g. a managed label removed out of band) while preserving + // labels added by other controllers. Drop a managed label (LabelManagedBy) and + // add a foreign label, then reconcile: the managed label must be repaired and + // the foreign label must survive. + // + // This is a label-ONLY change (endpoints and ports are untouched), so it also + // exercises that the whole-object change guard detects a label diff: it must + // (a) write on the drift and (b) converge (no rewrite on the next reconcile). 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) + // Capture resourceVersion AFTER the mutate but BEFORE the reconcile, so a + // subsequent bump is attributable to the reconciler's write, not the mutate. + drifted := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: "foo", Namespace: "operator-ns"}, drifted); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } 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"]) + t.Errorf("foreign label not preserved: 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]) + if got.Labels[discoveryv1.LabelManagedBy] != "tailscale.com" { + t.Errorf("managed label %s not repaired: got %q, want %q", discoveryv1.LabelManagedBy, got.Labels[discoveryv1.LabelManagedBy], "tailscale.com") + } + // The label-only drift must have caused the reconciler to write (guards + // against a change guard that ignores labels). + if got.ResourceVersion == drifted.ResourceVersion { + t.Errorf("label drift did not trigger a write: resourceVersion unchanged at %s", got.ResourceVersion) + } + // A second reconcile with no further change must not rewrite the slice: the + // repair converged and the guard sees no diff. + rvAfterRepair := 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 != rvAfterRepair { + t.Errorf("label repair did not converge: resourceVersion %s -> %s on no-op reconcile", rvAfterRepair, got.ResourceVersion) } eps.Labels = got.Labels eps.Ports = epsPorts @@ -211,7 +221,9 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { mak.Set(&s.Data, egressservices.KeyEgressServices, stBs) }) expectReconciled(t, er, "operator-ns", "foo") - eps.Endpoints = []discoveryv1.Endpoint{} + // No ready Pod: egress-eps writes nil Endpoints (not an empty slice) so a + // no-ready-Pods result compares equal to a freshly created slice's nil field. + eps.Endpoints = nil expectEqual(t, fc, eps) }) @@ -220,10 +232,7 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "foo-ipv6", Namespace: "operator-ns", - // 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), + Labels: egressSvcEpsLabels(svc, clusterIPSvc), }, AddressType: discoveryv1.AddressTypeIPv6, } @@ -282,7 +291,7 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { }) expectReconciled(t, er, "operator-ns", "foo-ipv6") // IPv4-only pod should not appear in the IPv6 EndpointSlice. - epsV6.Endpoints = []discoveryv1.Endpoint{} + epsV6.Endpoints = nil expectEqual(t, fc, epsV6) }) ipv6Pod := &corev1.Pod{ @@ -304,7 +313,7 @@ func TestTailscaleEgressEndpointSlices(t *testing.T) { mak.Set(&s.Data, egressservices.KeyEgressServices, stBs) }) expectReconciled(t, er, "operator-ns", "foo-ipv6") - epsV6.Endpoints = []discoveryv1.Endpoint{} + epsV6.Endpoints = nil expectEqual(t, fc, epsV6) }) t.Run("ipv6_pod_ready_to_route", func(t *testing.T) { @@ -438,6 +447,135 @@ func TestEgressEndpointSliceEndpointsSorted(t *testing.T) { } } +// TestEgressEndpointSliceOrphanGuard verifies that the egress EndpointSlices +// reconciler does not write to an orphaned slice - one whose +// kubernetes.io/service-name no longer matches the current ClusterIP Service. +// Such a slice can be left behind when the ClusterIP Service is recreated (new +// GenerateName) or the ProxyGroup on the parent Service is changed. Writing to it +// would rebind a stale slice (with frozen endpoints) into the live ClusterIP +// Service, unioning dead endpoints into cluster traffic. The reconciler must skip +// it entirely, leaving it byte-for-byte unchanged. +func TestEgressEndpointSliceOrphanGuard(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) + // The current ClusterIP Service - its name is the slice identity the guard + // checks against. + 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"} + + // Seed a ready Pod so that, absent the guard, the reconciler WOULD write an + // endpoint - making an unchanged orphan slice a meaningful assertion. + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo-0", Namespace: "operator-ns", Labels: pgLabels("foo", nil), UID: types.UID("pod-uid")}, + Status: corev1.PodStatus{PodIPs: []corev1.PodIP{{IP: "10.0.0.1"}}}, + } + sec := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "foo-0", Namespace: "operator-ns", Labels: pgSecretLabels("foo", kubetypes.LabelSecretTypeState)}} + mustCreate(t, fc, pod) + mustCreate(t, fc, sec) + stBs := serviceStatusForPodIPs(t, svc, "10.0.0.1", "", port) + mustUpdate(t, fc, "operator-ns", "foo-0", func(s *corev1.Secret) { + mak.Set(&s.Data, egressservices.KeyEgressServices, stBs) + }) + + // assertUnchanged reconciles the named slice and asserts it was not written. + assertUnchanged := func(t *testing.T, name string) { + t.Helper() + before := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: name, Namespace: "operator-ns"}, before); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + expectReconciled(t, er, "operator-ns", name) + after := &discoveryv1.EndpointSlice{} + if err := fc.Get(t.Context(), types.NamespacedName{Name: name, Namespace: "operator-ns"}, after); err != nil { + t.Fatalf("getting EndpointSlice: %v", err) + } + if before.ResourceVersion != after.ResourceVersion { + t.Errorf("orphan slice %s was written: resourceVersion %s -> %s", name, before.ResourceVersion, after.ResourceVersion) + } + if len(after.Endpoints) != 0 { + t.Errorf("orphan slice %s gained endpoints: %+v", name, after.Endpoints) + } + } + + t.Run("orphan_by_service_name_not_touched", func(t *testing.T) { + // Same-PG ClusterIP recreation: proxy-group MATCHES the current PG, but + // service-name points at a dead ClusterIP Service. A proxy-group-keyed guard + // would miss this, so it is the load-bearing case. + orphan := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: "stale-clusterip-ipv4", + Namespace: "operator-ns", + Labels: map[string]string{ + LabelParentName: "test", + LabelParentNamespace: "default", + labelSvcType: typeEgress, + labelProxyGroup: "foo", // current PG + discoveryv1.LabelServiceName: "ts-test-clusterip-OLD", + }, + }, + AddressType: discoveryv1.AddressTypeIPv4, + } + mustCreate(t, fc, orphan) + assertUnchanged(t, orphan.Name) + }) + + t.Run("orphan_by_proxy_group_not_touched", func(t *testing.T) { + // ProxyGroup rename: both proxy-group and service-name diverge from the + // current identity. Documents the rename path explicitly. + orphan := &discoveryv1.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: "old-pg-clusterip-ipv4", + Namespace: "operator-ns", + Labels: map[string]string{ + LabelParentName: "test", + LabelParentNamespace: "default", + labelSvcType: typeEgress, + labelProxyGroup: "old-pg", + discoveryv1.LabelServiceName: "ts-test-clusterip-old-pg", + }, + }, + AddressType: discoveryv1.AddressTypeIPv4, + } + mustCreate(t, fc, orphan) + assertUnchanged(t, orphan.Name) + }) +} + 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 a4a07a1c8..b6f6d1473 100644 --- a/cmd/k8s-operator/egress-services.go +++ b/cmd/k8s-operator/egress-services.go @@ -246,11 +246,12 @@ 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, labels and -// (immutable) addressType, set once at creation. The slice's endpoints and ports -// are owned by the egress-eps reconciler and should not be modified here. -// Splitting ownership this way means no two reconcilers ever -// update the same slice, which avoids tailscale/tailscale#20916. +// This reconciler only creates each slice, setting its name, initial labels, +// addressType and initial ports. After creation the egress-eps reconciler +// is the sole writer of the slice's mutable state - labels, endpoints +// and ports - which it re-asserts to repair drift. This reconciler must not +// modify a slice it did not just create. Splitting ownership so that no two +// reconcilers ever update the same slice avoids tailscale/tailscale#20916. 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. @@ -278,6 +279,12 @@ func (esr *egressSvcsReconciler) ensureEndpointSlices(ctx context.Context, svc, AddressType: addrType, Ports: epsPortsFromSvc(clusterIPSvc), } + // NOTE(pr): this create-only path replaces a createOrUpdate whose mutate fn + // ran "for _, p := range e.Endpoints { p.Conditions.Ready = nil }". That loop + // was a no-op (range over a []Endpoint value slice - p was a copy, so the + // backing array was never modified), and egress-eps is the sole writer of + // endpoint Conditions anyway. Flagging in the PR description; safe to delete + // this note after review. if err := esr.Create(ctx, eps); err != nil && !apierrors.IsAlreadyExists(err) { return fmt.Errorf("error creating %s EndpointSlice: %w", addrType, err) }