move labels, use path, add tests

This commit is contained in:
Becky Pauley committed 2026-08-26 11:51:46 +01:00
1 parent 51c85f345a
commit 1cffb8564d
4 files changed
+305 -58

No files matched your search

+25 -27
View File
@@ -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
+178 -13
View File
@@ -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{})
+29 -18
View File
@@ -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
}
+73
View File
@@ -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