From a611a2942285bcb1be98cd0b7cf7cf1b475de528 Mon Sep 17 00:00:00 2001 From: Zach Smith Date: Tue, 3 Mar 2026 13:43:58 -0800 Subject: [PATCH] feat: annotation strategy over labels due to char length --- .../dnsrecordset_replicator_controller.go | 2 +- ...dnsrecordset_replicator_controller_test.go | 190 ++++++++++++++++++ .../dnszone_replicator_controller.go | 2 +- .../enqueue_upstream_owner.go | 27 ++- .../enqueue_upstream_owner_test.go | 158 +++++++++++++++ internal/downstreamclient/mappednamespace.go | 58 +++--- 6 files changed, 400 insertions(+), 37 deletions(-) create mode 100644 internal/controller/dnsrecordset_replicator_controller_test.go create mode 100644 internal/downstreamclient/enqueue_upstream_owner_test.go diff --git a/internal/controller/dnsrecordset_replicator_controller.go b/internal/controller/dnsrecordset_replicator_controller.go index 1682ce7..d62182d 100644 --- a/internal/controller/dnsrecordset_replicator_controller.go +++ b/internal/controller/dnsrecordset_replicator_controller.go @@ -222,7 +222,7 @@ func (r *DNSRecordSetReplicator) ensureDownstreamRecordSet(ctx context.Context, shadow.SetName(md.Name) res, cErr := controllerutil.CreateOrPatch(ctx, r.DownstreamClient, &shadow, func() error { - shadow.Labels = md.Labels + shadow.Annotations = md.Annotations if !equality.Semantic.DeepEqual(shadow.Spec, upstream.Spec) { shadow.Spec = upstream.Spec } diff --git a/internal/controller/dnsrecordset_replicator_controller_test.go b/internal/controller/dnsrecordset_replicator_controller_test.go new file mode 100644 index 0000000..9caf4ab --- /dev/null +++ b/internal/controller/dnsrecordset_replicator_controller_test.go @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "testing" + + dnsv1alpha1 "go.miloapis.com/dns-operator/api/v1alpha1" + "go.miloapis.com/dns-operator/internal/downstreamclient" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +type annotatingStrategy struct { + fakeStrategy +} + +func (s annotatingStrategy) ObjectMetaFromUpstreamObject(_ context.Context, obj metav1.Object) (metav1.ObjectMeta, error) { + return metav1.ObjectMeta{ + Namespace: s.namespace, + Name: obj.GetName(), + Annotations: map[string]string{ + downstreamclient.UpstreamOwnerNamespaceAnnotation: obj.GetNamespace(), + }, + }, nil +} + +func (s annotatingStrategy) SetControllerReference(_ context.Context, owner, controlled metav1.Object, _ ...controllerutil.OwnerReferenceOption) error { + annotations := controlled.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[downstreamclient.UpstreamOwnerClusterNameAnnotation] = "cluster-test" + annotations[downstreamclient.UpstreamOwnerGroupAnnotation] = "dns.networking.miloapis.com" + annotations[downstreamclient.UpstreamOwnerKindAnnotation] = "DNSRecordSet" + annotations[downstreamclient.UpstreamOwnerNameAnnotation] = owner.GetName() + annotations[downstreamclient.UpstreamOwnerNamespaceAnnotation] = owner.GetNamespace() + controlled.SetAnnotations(annotations) + return nil +} + +func newTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{ + dnsv1alpha1.AddToScheme, + corev1.AddToScheme, + } { + if err := add(s); err != nil { + t.Fatalf("add scheme: %v", err) + } + } + return s +} + +func TestEnsureDownstreamRecordSet_NewShadowGetsAnnotations(t *testing.T) { + t.Parallel() + scheme := newTestScheme(t) + + longName := "v4-2ed208a11f54412a92e8a2619eb662ea-prism-staging-env-datum-net-a-a59c082e" + + upstream := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: longName, + Namespace: "default", + UID: "upstream-uid-1", + }, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + DNSZoneRef: corev1.LocalObjectReference{Name: "zone-a"}, + RecordType: dnsv1alpha1.RRTypeA, + }, + } + + downstreamClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + strategy := annotatingStrategy{fakeStrategy{ + namespace: "ns-downstream", + client: downstreamClient, + }} + + r := &DNSRecordSetReplicator{DownstreamClient: downstreamClient} + + res, err := r.ensureDownstreamRecordSet(context.Background(), strategy, upstream) + if err != nil { + t.Fatalf("ensureDownstreamRecordSet: %v", err) + } + if res != controllerutil.OperationResultCreated { + t.Fatalf("expected Created, got %s", res) + } + + var shadow dnsv1alpha1.DNSRecordSet + if err := downstreamClient.Get(context.Background(), types.NamespacedName{ + Namespace: "ns-downstream", + Name: longName, + }, &shadow); err != nil { + t.Fatalf("get shadow: %v", err) + } + + wantAnnotations := map[string]string{ + downstreamclient.UpstreamOwnerClusterNameAnnotation: "cluster-test", + downstreamclient.UpstreamOwnerGroupAnnotation: "dns.networking.miloapis.com", + downstreamclient.UpstreamOwnerKindAnnotation: "DNSRecordSet", + downstreamclient.UpstreamOwnerNameAnnotation: longName, + downstreamclient.UpstreamOwnerNamespaceAnnotation: "default", + } + for k, want := range wantAnnotations { + if got := shadow.Annotations[k]; got != want { + t.Errorf("annotation %s = %q, want %q", k, got, want) + } + } + + if len(shadow.Labels) != 0 { + t.Errorf("expected no labels on new shadow, got %v", shadow.Labels) + } +} + +func TestEnsureDownstreamRecordSet_ExistingLabelsPreserved(t *testing.T) { + t.Parallel() + scheme := newTestScheme(t) + + upstream := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "short-name", + Namespace: "default", + UID: "upstream-uid-2", + }, + Spec: dnsv1alpha1.DNSRecordSetSpec{ + DNSZoneRef: corev1.LocalObjectReference{Name: "zone-a"}, + RecordType: dnsv1alpha1.RRTypeA, + }, + } + + oldLabels := map[string]string{ + downstreamclient.UpstreamOwnerClusterNameAnnotation: "cluster-test", + downstreamclient.UpstreamOwnerGroupAnnotation: "dns.networking.miloapis.com", + downstreamclient.UpstreamOwnerKindAnnotation: "DNSRecordSet", + downstreamclient.UpstreamOwnerNameAnnotation: "short-name", + downstreamclient.UpstreamOwnerNamespaceAnnotation: "default", + } + + existingShadow := &dnsv1alpha1.DNSRecordSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "short-name", + Namespace: "ns-downstream", + Labels: oldLabels, + }, + Spec: upstream.Spec, + } + + downstreamClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(existingShadow). + Build() + + strategy := annotatingStrategy{fakeStrategy{ + namespace: "ns-downstream", + client: downstreamClient, + }} + + r := &DNSRecordSetReplicator{DownstreamClient: downstreamClient} + + _, err := r.ensureDownstreamRecordSet(context.Background(), strategy, upstream) + if err != nil { + t.Fatalf("ensureDownstreamRecordSet: %v", err) + } + + var shadow dnsv1alpha1.DNSRecordSet + if err := downstreamClient.Get(context.Background(), types.NamespacedName{ + Namespace: "ns-downstream", + Name: "short-name", + }, &shadow); err != nil { + t.Fatalf("get shadow: %v", err) + } + + for k, want := range oldLabels { + if got := shadow.Labels[k]; got != want { + t.Errorf("old label %s = %q, want %q (should be preserved)", k, got, want) + } + } + + if shadow.Annotations[downstreamclient.UpstreamOwnerNameAnnotation] != "short-name" { + t.Errorf("expected name annotation to be set, got %q", + shadow.Annotations[downstreamclient.UpstreamOwnerNameAnnotation]) + } +} diff --git a/internal/controller/dnszone_replicator_controller.go b/internal/controller/dnszone_replicator_controller.go index 58e3729..2baf729 100644 --- a/internal/controller/dnszone_replicator_controller.go +++ b/internal/controller/dnszone_replicator_controller.go @@ -273,7 +273,7 @@ func (r *DNSZoneReplicator) ensureDownstreamZone(ctx context.Context, strategy d shadow.SetName(md.Name) res, cErr := controllerutil.CreateOrPatch(ctx, strategy.GetClient(), &shadow, func() error { - shadow.Labels = md.Labels + shadow.Annotations = md.Annotations if !equality.Semantic.DeepEqual(shadow.Spec, upstream.Spec) { shadow.Spec = upstream.Spec } diff --git a/internal/downstreamclient/enqueue_upstream_owner.go b/internal/downstreamclient/enqueue_upstream_owner.go index 5ef257a..17a5730 100644 --- a/internal/downstreamclient/enqueue_upstream_owner.go +++ b/internal/downstreamclient/enqueue_upstream_owner.go @@ -25,7 +25,7 @@ type empty struct{} // TypedEnqueueRequestForUpstreamOwner enqueues Requests for the upstream Owners of an object. // -// This handler depends on the `compute.datumapis.com/upstream-namespace` label +// This handler depends on the `meta.datumapis.com/upstream-*` annotations // to exist on the resource for the event. func TypedEnqueueRequestForUpstreamOwner[object client.Object](ownerType client.Object) mchandler.TypedEventHandlerFunc[object, mcreconcile.Request] { @@ -106,17 +106,32 @@ func (e *enqueueRequestForOwner[object]) parseOwnerTypeGroupKind(scheme *runtime // getOwnerReconcileRequest looks at object and builds a map of reconcile.Request to reconcile // owners of object that match e.OwnerType. func (e *enqueueRequestForOwner[object]) getOwnerReconcileRequest(obj metav1.Object, result map[mcreconcile.Request]empty) { - labels := obj.GetLabels() - if labels[UpstreamOwnerKindLabel] == e.groupKind.Kind && labels[UpstreamOwnerGroupLabel] == e.groupKind.Group { + meta := upstreamOwnerMeta(obj) + + if meta[UpstreamOwnerKindAnnotation] == e.groupKind.Kind && meta[UpstreamOwnerGroupAnnotation] == e.groupKind.Group { request := mcreconcile.Request{ Request: reconcile.Request{ NamespacedName: types.NamespacedName{ - Name: labels[UpstreamOwnerNameLabel], - Namespace: labels[UpstreamOwnerNamespaceLabel], + Name: meta[UpstreamOwnerNameAnnotation], + Namespace: meta[UpstreamOwnerNamespaceAnnotation], }, }, - ClusterName: strings.TrimPrefix(strings.ReplaceAll(labels[UpstreamOwnerClusterNameLabel], "_", "/"), "cluster-"), + ClusterName: strings.TrimPrefix(strings.ReplaceAll(meta[UpstreamOwnerClusterNameAnnotation], "_", "/"), "cluster-"), } result[request] = empty{} } } + +// upstreamOwnerMeta returns upstream owner metadata from the object, +// preferring annotations but falling back to labels for backwards +// compatibility with resources created before the labels-to-annotations +// migration. +func upstreamOwnerMeta(obj metav1.Object) map[string]string { + if annotations := obj.GetAnnotations(); annotations[UpstreamOwnerKindAnnotation] != "" { + return annotations + } + if labels := obj.GetLabels(); labels[UpstreamOwnerKindAnnotation] != "" { + return labels + } + return nil +} diff --git a/internal/downstreamclient/enqueue_upstream_owner_test.go b/internal/downstreamclient/enqueue_upstream_owner_test.go new file mode 100644 index 0000000..aa6c7b3 --- /dev/null +++ b/internal/downstreamclient/enqueue_upstream_owner_test.go @@ -0,0 +1,158 @@ +package downstreamclient + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile" +) + +func ownerAnnotations(cluster, group, kind, name, namespace string) map[string]string { + return map[string]string{ + UpstreamOwnerClusterNameAnnotation: cluster, + UpstreamOwnerGroupAnnotation: group, + UpstreamOwnerKindAnnotation: kind, + UpstreamOwnerNameAnnotation: name, + UpstreamOwnerNamespaceAnnotation: namespace, + } +} + +func TestUpstreamOwnerMeta(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + labels map[string]string + wantKind string + }{ + { + name: "prefers annotations", + annotations: ownerAnnotations("cluster-c1", "dns.example.com", "DNSRecordSet", "my-record", "default"), + labels: ownerAnnotations("cluster-c1", "dns.example.com", "DNSRecordSet", "stale", "stale-ns"), + wantKind: "DNSRecordSet", + }, + { + name: "falls back to labels", + labels: ownerAnnotations("cluster-c1", "dns.example.com", "DNSRecordSet", "my-record", "default"), + wantKind: "DNSRecordSet", + }, + { + name: "returns nil when empty", + wantKind: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + obj := &metav1.ObjectMeta{ + Annotations: tt.annotations, + Labels: tt.labels, + } + got := upstreamOwnerMeta(obj) + if gotKind := got[UpstreamOwnerKindAnnotation]; gotKind != tt.wantKind { + t.Errorf("upstreamOwnerMeta() kind = %q, want %q", gotKind, tt.wantKind) + } + }) + } +} + +func TestGetOwnerReconcileRequest_Annotations(t *testing.T) { + e := &enqueueRequestForOwner[client.Object]{ + groupKind: schema.GroupKind{Group: "dns.example.com", Kind: "DNSRecordSet"}, + } + + obj := &metav1.ObjectMeta{ + Annotations: ownerAnnotations( + "cluster-_my_cluster", + "dns.example.com", + "DNSRecordSet", + "v4-2ed208a11f54412a92e8a2619eb662ea-prism-staging-env-datum-net-a-a59c082e", + "my-namespace", + ), + } + + result := map[mcreconcile.Request]empty{} + e.getOwnerReconcileRequest(obj, result) + + if len(result) != 1 { + t.Fatalf("expected 1 request, got %d", len(result)) + } + + want := mcreconcile.Request{ + Request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "v4-2ed208a11f54412a92e8a2619eb662ea-prism-staging-env-datum-net-a-a59c082e", + Namespace: "my-namespace", + }, + }, + ClusterName: "/my/cluster", + } + + for req := range result { + if req != want { + t.Errorf("got request %+v, want %+v", req, want) + } + } +} + +func TestGetOwnerReconcileRequest_LabelsFallback(t *testing.T) { + e := &enqueueRequestForOwner[client.Object]{ + groupKind: schema.GroupKind{Group: "dns.example.com", Kind: "DNSRecordSet"}, + } + + obj := &metav1.ObjectMeta{ + Labels: ownerAnnotations( + "cluster-_my_cluster", + "dns.example.com", + "DNSRecordSet", + "short-name", + "my-namespace", + ), + } + + result := map[mcreconcile.Request]empty{} + e.getOwnerReconcileRequest(obj, result) + + if len(result) != 1 { + t.Fatalf("expected 1 request, got %d", len(result)) + } + + want := mcreconcile.Request{ + Request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "short-name", + Namespace: "my-namespace", + }, + }, + ClusterName: "/my/cluster", + } + + for req := range result { + if req != want { + t.Errorf("got request %+v, want %+v", req, want) + } + } +} + +func TestGetOwnerReconcileRequest_NoMatch(t *testing.T) { + e := &enqueueRequestForOwner[client.Object]{ + groupKind: schema.GroupKind{Group: "dns.example.com", Kind: "DNSRecordSet"}, + } + + result := map[mcreconcile.Request]empty{} + + e.getOwnerReconcileRequest(&metav1.ObjectMeta{}, result) + if len(result) != 0 { + t.Errorf("expected 0 requests for empty object, got %d", len(result)) + } + + e.getOwnerReconcileRequest(&metav1.ObjectMeta{ + Annotations: ownerAnnotations("cluster-c1", "other.group", "OtherKind", "name", "ns"), + }, result) + if len(result) != 0 { + t.Errorf("expected 0 requests for wrong group/kind, got %d", len(result)) + } +} diff --git a/internal/downstreamclient/mappednamespace.go b/internal/downstreamclient/mappednamespace.go index 6e779ee..bcb02ea 100644 --- a/internal/downstreamclient/mappednamespace.go +++ b/internal/downstreamclient/mappednamespace.go @@ -54,8 +54,8 @@ func (c *mappedNamespaceResourceStrategy) ObjectMetaFromUpstreamObject(ctx conte return metav1.ObjectMeta{ Name: obj.GetName(), Namespace: downstreamNamespaceName, - Labels: map[string]string{ - UpstreamOwnerNamespaceLabel: obj.GetNamespace(), + Annotations: map[string]string{ + UpstreamOwnerNamespaceAnnotation: obj.GetNamespace(), }, }, nil } @@ -93,15 +93,15 @@ func (c *mappedNamespaceResourceStrategy) ensureDownstreamNamespace(ctx context. } _, err := controllerutil.CreateOrUpdate(ctx, c.downstreamClient, downstreamNamespace, func() error { - if downstreamNamespace.Labels == nil { - downstreamNamespace.Labels = make(map[string]string) + if downstreamNamespace.Annotations == nil { + downstreamNamespace.Annotations = make(map[string]string) } - downstreamNamespace.Labels[UpstreamOwnerClusterNameLabel] = fmt.Sprintf("cluster-%s", strings.ReplaceAll(c.upstreamClusterName, "/", "_")) + downstreamNamespace.Annotations[UpstreamOwnerClusterNameAnnotation] = fmt.Sprintf("cluster-%s", strings.ReplaceAll(c.upstreamClusterName, "/", "_")) - labels := obj.GetLabels() - if v, ok := labels[UpstreamOwnerNamespaceLabel]; ok { - downstreamNamespace.Labels[UpstreamOwnerNamespaceLabel] = v + annotations := obj.GetAnnotations() + if v, ok := annotations[UpstreamOwnerNamespaceAnnotation]; ok { + downstreamNamespace.Annotations[UpstreamOwnerNamespaceAnnotation] = v } return nil @@ -114,11 +114,11 @@ func (c *mappedNamespaceResourceStrategy) ensureDownstreamNamespace(ctx context. } const ( - UpstreamOwnerClusterNameLabel = "meta.datumapis.com/upstream-cluster-name" - UpstreamOwnerGroupLabel = "meta.datumapis.com/upstream-group" - UpstreamOwnerKindLabel = "meta.datumapis.com/upstream-kind" - UpstreamOwnerNameLabel = "meta.datumapis.com/upstream-name" - UpstreamOwnerNamespaceLabel = "meta.datumapis.com/upstream-namespace" + UpstreamOwnerClusterNameAnnotation = "meta.datumapis.com/upstream-cluster-name" + UpstreamOwnerGroupAnnotation = "meta.datumapis.com/upstream-group" + UpstreamOwnerKindAnnotation = "meta.datumapis.com/upstream-kind" + UpstreamOwnerNameAnnotation = "meta.datumapis.com/upstream-name" + UpstreamOwnerNamespaceAnnotation = "meta.datumapis.com/upstream-namespace" ) func (c *mappedNamespaceResourceStrategy) SetControllerReference(ctx context.Context, owner, controlled metav1.Object, opts ...controllerutil.OwnerReferenceOption) error { @@ -138,12 +138,12 @@ func (c *mappedNamespaceResourceStrategy) SetControllerReference(ctx context.Con anchorName := fmt.Sprintf("anchor-%s", owner.GetUID()) - anchorLabels := map[string]string{ - UpstreamOwnerClusterNameLabel: fmt.Sprintf("cluster-%s", strings.ReplaceAll(c.upstreamClusterName, "/", "_")), - UpstreamOwnerGroupLabel: gvk.Group, - UpstreamOwnerKindLabel: gvk.Kind, - UpstreamOwnerNameLabel: owner.GetName(), - UpstreamOwnerNamespaceLabel: owner.GetNamespace(), + anchorAnnotations := map[string]string{ + UpstreamOwnerClusterNameAnnotation: fmt.Sprintf("cluster-%s", strings.ReplaceAll(c.upstreamClusterName, "/", "_")), + UpstreamOwnerGroupAnnotation: gvk.Group, + UpstreamOwnerKindAnnotation: gvk.Kind, + UpstreamOwnerNameAnnotation: owner.GetName(), + UpstreamOwnerNamespaceAnnotation: owner.GetNamespace(), } downstreamClient := c.GetClient() @@ -155,7 +155,7 @@ func (c *mappedNamespaceResourceStrategy) SetControllerReference(ctx context.Con if anchorConfigMap.CreationTimestamp.IsZero() { anchorConfigMap.Name = anchorName - anchorConfigMap.Labels = anchorLabels + anchorConfigMap.Annotations = anchorAnnotations anchorConfigMap.Namespace = controlled.GetNamespace() if err := downstreamClient.Create(ctx, &anchorConfigMap); err != nil { return fmt.Errorf("failed creating anchor configmap: %w", err) @@ -166,17 +166,17 @@ func (c *mappedNamespaceResourceStrategy) SetControllerReference(ctx context.Con return fmt.Errorf("failed setting anchor owner reference: %w", err) } - labels := controlled.GetLabels() - if labels == nil { - labels = map[string]string{} + annotations := controlled.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} } - labels[UpstreamOwnerClusterNameLabel] = anchorLabels[UpstreamOwnerClusterNameLabel] - labels[UpstreamOwnerGroupLabel] = anchorLabels[UpstreamOwnerGroupLabel] - labels[UpstreamOwnerKindLabel] = anchorLabels[UpstreamOwnerKindLabel] - labels[UpstreamOwnerNameLabel] = anchorLabels[UpstreamOwnerNameLabel] - labels[UpstreamOwnerNamespaceLabel] = anchorLabels[UpstreamOwnerNamespaceLabel] - controlled.SetLabels(labels) + annotations[UpstreamOwnerClusterNameAnnotation] = anchorAnnotations[UpstreamOwnerClusterNameAnnotation] + annotations[UpstreamOwnerGroupAnnotation] = anchorAnnotations[UpstreamOwnerGroupAnnotation] + annotations[UpstreamOwnerKindAnnotation] = anchorAnnotations[UpstreamOwnerKindAnnotation] + annotations[UpstreamOwnerNameAnnotation] = anchorAnnotations[UpstreamOwnerNameAnnotation] + annotations[UpstreamOwnerNamespaceAnnotation] = anchorAnnotations[UpstreamOwnerNamespaceAnnotation] + controlled.SetAnnotations(annotations) return nil }