From 36289298717118bfa74d58a67e2426b547f58f8f Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 21 Jun 2026 13:26:23 +0100 Subject: [PATCH] refactor(controller): extract shared mirror-deletion and naming helpers Replace the triplicated ownership-verified delete guard with deleteOwnedMirror, and hand-built controller-name strings with gvkControllerName. Use constants.Domain/ControllerName over magic strings and maps.Copy/Clone for plain copy loops. Behaviour-preserving; tests green. --- pkg/controller/mirror.go | 33 +++++---------- pkg/controller/mirror_deletion.go | 65 +++++++++++++++++++++++++++++ pkg/controller/mirror_reconciler.go | 6 +-- pkg/controller/naming.go | 18 ++++++++ 4 files changed, 95 insertions(+), 27 deletions(-) create mode 100644 pkg/controller/mirror_deletion.go create mode 100644 pkg/controller/naming.go diff --git a/pkg/controller/mirror.go b/pkg/controller/mirror.go index d3b2751..570f882 100644 --- a/pkg/controller/mirror.go +++ b/pkg/controller/mirror.go @@ -3,6 +3,7 @@ package controller import ( "fmt" + "maps" "strings" "time" @@ -96,7 +97,7 @@ func filterKubeMirrorMetadata(metadata map[string]string) map[string]string { filtered := make(map[string]string) for k, v := range metadata { // Skip all kubemirror.raczylo.com keys - if !strings.HasPrefix(k, "kubemirror.raczylo.com/") { + if !strings.HasPrefix(k, constants.Domain+"/") { filtered[k] = v } } @@ -136,9 +137,7 @@ func createUnstructuredMirror(source runtime.Object, targetNamespace, sourceHash // Add mirror-specific annotations annotations := buildMirrorAnnotations(source, sourceHash) - for k, v := range annotations { - existingAnnotations[k] = v - } + maps.Copy(existingAnnotations, annotations) mirror.SetAnnotations(existingAnnotations) // Remove status (never mirror status) @@ -272,8 +271,8 @@ func UpdateMirror(mirror, source runtime.Object) error { return nil } -// convertToStringMap converts map[string]interface{} to map[string]string. -func convertToStringMap(data map[string]interface{}) map[string]string { +// convertToStringMap converts map[string]any to map[string]string. +func convertToStringMap(data map[string]any) map[string]string { result := make(map[string]string) for k, v := range data { if s, ok := v.(string); ok { @@ -283,8 +282,8 @@ func convertToStringMap(data map[string]interface{}) map[string]string { return result } -// convertToByteMap converts map[string]interface{} to map[string][]byte. -func convertToByteMap(data map[string]interface{}) map[string][]byte { +// convertToByteMap converts map[string]any to map[string][]byte. +func convertToByteMap(data map[string]any) map[string][]byte { result := make(map[string][]byte) for k, v := range data { switch val := v.(type) { @@ -445,13 +444,7 @@ func applyTransformations(source, mirror runtime.Object, targetNamespace string) // Save original annotations to restore on failure originalAnnotations := mirrorObj.GetAnnotations() - var savedAnnotations map[string]string - if originalAnnotations != nil { - savedAnnotations = make(map[string]string, len(originalAnnotations)) - for k, v := range originalAnnotations { - savedAnnotations[k] = v - } - } + savedAnnotations := maps.Clone(originalAnnotations) mirrorAnnotations := mirrorObj.GetAnnotations() if mirrorAnnotations == nil { @@ -504,18 +497,12 @@ func buildTransformContext(source, mirror runtime.Object, targetNamespace string // Copy labels (if any) if labels := sourceObj.GetLabels(); labels != nil { - ctx.Labels = make(map[string]string) - for k, v := range labels { - ctx.Labels[k] = v - } + ctx.Labels = maps.Clone(labels) } // Copy annotations (if any) if annotations := sourceObj.GetAnnotations(); annotations != nil { - ctx.Annotations = make(map[string]string) - for k, v := range annotations { - ctx.Annotations[k] = v - } + ctx.Annotations = maps.Clone(annotations) } return ctx diff --git a/pkg/controller/mirror_deletion.go b/pkg/controller/mirror_deletion.go new file mode 100644 index 0000000..2f2cc2c --- /dev/null +++ b/pkg/controller/mirror_deletion.go @@ -0,0 +1,65 @@ +// Package controller implements the kubemirror reconciliation logic. +package controller + +import ( + "context" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// mirrorDeletionOutcome describes the result of an ownership-verified mirror +// deletion attempt. +type mirrorDeletionOutcome int + +const ( + // mirrorAbsent means no resource exists at the requested namespace/name. + mirrorAbsent mirrorDeletionOutcome = iota + // mirrorNotOwned means a resource exists but is not a kubemirror-managed + // mirror of the given source, so it was left untouched. + mirrorNotOwned + // mirrorDeleted means an owned mirror was found and deleted. + mirrorDeleted +) + +// deleteOwnedMirror deletes the mirror at gvk/ns/name only if it is managed by +// kubemirror AND its source reference matches srcNs/srcName. This ownership +// guard is the single safety gate that prevents deleting an unrelated resource +// that merely shares the same name. All mirror-cleanup paths route through here +// so the guard cannot be forgotten in one place and drift in another. +// +// A NotFound on delete is treated as success (the mirror is already gone). +func deleteOwnedMirror( + ctx context.Context, + c client.Client, + gvk schema.GroupVersionKind, + ns, name, srcNs, srcName string, +) (mirrorDeletionOutcome, error) { + mirror := &unstructured.Unstructured{} + mirror.SetGroupVersionKind(gvk) + + if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: name}, mirror); err != nil { + if errors.IsNotFound(err) { + return mirrorAbsent, nil + } + return mirrorAbsent, err + } + + // Verify this is actually our mirror (not someone else's resource with the same name). + if !IsManagedByUs(mirror) { + return mirrorNotOwned, nil + } + + // Verify this mirror points to our source. + refNs, refName, _, found := GetSourceReference(mirror) + if !found || refNs != srcNs || refName != srcName { + return mirrorNotOwned, nil + } + + if err := c.Delete(ctx, mirror); err != nil && !errors.IsNotFound(err) { + return mirrorNotOwned, err + } + return mirrorDeleted, nil +} diff --git a/pkg/controller/mirror_reconciler.go b/pkg/controller/mirror_reconciler.go index 7693300..84777e4 100644 --- a/pkg/controller/mirror_reconciler.go +++ b/pkg/controller/mirror_reconciler.go @@ -149,7 +149,7 @@ func (r *MirrorReconciler) SetupWithManager(mgr ctrl.Manager, gvk schema.GroupVe return false } managedBy, exists := labels[constants.LabelManagedBy] - return exists && managedBy == "kubemirror" + return exists && managedBy == constants.ControllerName }) // Convert GVK to resource object for watching @@ -157,9 +157,7 @@ func (r *MirrorReconciler) SetupWithManager(mgr ctrl.Manager, gvk schema.GroupVe gv := schema.GroupVersion{Group: gvk.Group, Version: gvk.Version} obj.SetGroupVersionKind(gv.WithKind(gvk.Kind)) - // Set custom controller name to avoid conflicts with source reconciler and multiple API versions - // Include group and version to make it truly unique - controllerName := gvk.Kind + "." + gvk.Version + "." + gvk.Group + "-mirror" + controllerName := gvkControllerName(gvk, true) return ctrl.NewControllerManagedBy(mgr). For(obj). diff --git a/pkg/controller/naming.go b/pkg/controller/naming.go new file mode 100644 index 0000000..7570693 --- /dev/null +++ b/pkg/controller/naming.go @@ -0,0 +1,18 @@ +// Package controller implements the kubemirror reconciliation logic. +package controller + +import "k8s.io/apimachinery/pkg/runtime/schema" + +// gvkControllerName builds the unique controller-runtime controller name for a +// GVK. Including version and group avoids collisions across API versions, e.g. +// "HorizontalPodAutoscaler.v1.autoscaling" or "Secret.v1." (empty group for +// core resources). Source and mirror reconcilers for the same GVK must differ, +// so mirror controllers get a "-mirror" suffix. This is the single source of +// truth for the convention shared by both reconcilers. +func gvkControllerName(gvk schema.GroupVersionKind, mirror bool) string { + name := gvk.Kind + "." + gvk.Version + "." + gvk.Group + if mirror { + name += "-mirror" + } + return name +}