diff --git a/internal/webhook/deletion_validator.go b/internal/webhook/deletion_validator.go index 1f7776a..54babd2 100644 --- a/internal/webhook/deletion_validator.go +++ b/internal/webhook/deletion_validator.go @@ -129,6 +129,10 @@ func (v *DeletionValidator) Handle(ctx context.Context, req admission.Request) a blockers = append(blockers, dependents...) } + // A dependent that references the deleted resource through more than one + // field matches once per field; collapse those into a single mention. + blockers = dedupeBlockers(blockers) + if len(blockers) > 0 { msg := fmt.Sprintf("cannot delete %s/%s: still referenced by %s", req.Resource.Resource, req.Name, strings.Join(blockers, ", ")) @@ -183,13 +187,54 @@ func (v *DeletionValidator) listDependents( for _, item := range list.Items { val := fieldpath.Resolve(item.Object, entry.MatchedField.FieldPath) if val == targetName { - blockers = append(blockers, fmt.Sprintf("%s/%s", entry.State.DependentGVK.Kind, item.GetName())) + // A namespaced target is listed within its own namespace, so names + // are already unique and the namespace segment would be redundant. + // A cluster-scoped target is listed across all namespaces, so qualify + // with the dependent's namespace to keep same-named objects distinct. + depNamespace := "" + if namespace == "" { + depNamespace = item.GetNamespace() + } + blockers = append(blockers, blockerID(entry.State.DependentGVK.Kind, depNamespace, item.GetName())) } } return blockers, nil } +// blockerID renders a stable, human-readable identifier for a dependent object. +// When a namespace is supplied it is included so that two objects sharing a +// name in different namespaces are not conflated — this matters for +// cluster-scoped deletions, where dependents are listed across all namespaces. +func blockerID(kind, namespace, name string) string { + if namespace != "" { + return fmt.Sprintf("%s/%s/%s", kind, namespace, name) + } + + return fmt.Sprintf("%s/%s", kind, name) +} + +// dedupeBlockers removes duplicate dependent identifiers while preserving +// first-seen order, so each referencing object is reported exactly once +// regardless of how many fields or rules reference the deleted resource. +func dedupeBlockers(blockers []string) []string { + if len(blockers) == 0 { + return blockers + } + + seen := make(map[string]bool, len(blockers)) + out := make([]string, 0, len(blockers)) + for _, b := range blockers { + if seen[b] { + continue + } + seen[b] = true + out = append(out, b) + } + + return out +} + // objectFromRequest extracts the unstructured object from the admission // request's OldObject (for DELETE) or Object (for other operations). func objectFromRequest(req admission.Request) (*unstructured.Unstructured, error) { diff --git a/internal/webhook/deletion_validator_test.go b/internal/webhook/deletion_validator_test.go index fb75f2e..a0855d8 100644 --- a/internal/webhook/deletion_validator_test.go +++ b/internal/webhook/deletion_validator_test.go @@ -4,11 +4,17 @@ package webhook import ( + "context" "encoding/json" + "reflect" + "sort" "testing" admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynamicfake "k8s.io/client-go/dynamic/fake" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -121,6 +127,159 @@ func TestSkipProtection_WrongValue(t *testing.T) { } } +func vpcPeering(namespace, name, targetVPC string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "network.test.io/v1", + "kind": "VPCPeering", + "metadata": map[string]any{"name": name, "namespace": namespace}, + "spec": map[string]any{"targetVpcRef": map[string]any{"name": targetVPC}}, + }} +} + +func TestListDependents_NamespaceQualification(t *testing.T) { + vpcGVR := schema.GroupVersionResource{Group: "network.test.io", Version: "v1", Resource: "vpcs"} + vpcPeeringGVR := schema.GroupVersionResource{Group: "network.test.io", Version: "v1", Resource: "vpcpeerings"} + + entry := RuleEntry{ + State: &RuleState{ + DependentGVR: vpcPeeringGVR, + DependentGVK: schema.GroupVersionKind{Group: "network.test.io", Version: "v1", Kind: "VPCPeering"}, + }, + MatchedField: IndexedField{FieldPath: ".spec.targetVpcRef.name", TargetGVR: vpcGVR}, + } + + newClient := func(objs ...runtime.Object) *dynamicfake.FakeDynamicClient { + return dynamicfake.NewSimpleDynamicClientWithCustomListKinds( + runtime.NewScheme(), + map[schema.GroupVersionResource]string{vpcPeeringGVR: "VPCPeeringList"}, + objs..., + ) + } + + v := &DeletionValidator{} + + t.Run("namespaced target renders clean Kind/name", func(t *testing.T) { + // Namespaced target: dependents are listed within the target's namespace + // only, so names are unique and the namespace segment is redundant. + dyn := newClient(vpcPeering("team-a", "peer1", "my-vpc")) + + got, err := v.listDependents(context.Background(), dyn, entry, "my-vpc", "team-a") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"VPCPeering/peer1"} + assertBlockers(t, got, want) + }) + + t.Run("cluster-scoped target keeps same-named dependents distinct", func(t *testing.T) { + // Cluster-scoped target (namespace ""): dependents are listed across all + // namespaces, so two same-named objects must not be conflated. + dyn := newClient( + vpcPeering("team-a", "peer1", "global-vpc"), + vpcPeering("team-b", "peer1", "global-vpc"), + ) + + got, err := v.listDependents(context.Background(), dyn, entry, "global-vpc", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := []string{"VPCPeering/team-a/peer1", "VPCPeering/team-b/peer1"} + assertBlockers(t, got, want) + }) +} + +func assertBlockers(t *testing.T, got, want []string) { + t.Helper() + sort.Strings(got) + sort.Strings(want) + if !reflect.DeepEqual(got, want) { + t.Errorf("blockers = %v, want %v", got, want) + } +} + +func TestBlockerID(t *testing.T) { + tests := []struct { + name string + kind string + namespace string + objName string + want string + }{ + { + name: "cluster-scoped dependent has no namespace segment", + kind: "VPCPeering", + objName: "peer1", + want: "VPCPeering/peer1", + }, + { + name: "namespaced dependent includes its namespace", + kind: "Order", + namespace: "team-a", + objName: "order1", + want: "Order/team-a/order1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := blockerID(tt.kind, tt.namespace, tt.objName); got != tt.want { + t.Errorf("blockerID(%q, %q, %q) = %q, want %q", tt.kind, tt.namespace, tt.objName, got, tt.want) + } + }) + } + + // Same name in different namespaces must not collapse to one identity, + // which would hide a genuine blocker for cluster-scoped deletions. + if a, b := blockerID("Order", "team-a", "order1"), blockerID("Order", "team-b", "order1"); a == b { + t.Errorf("distinct dependents collided: both rendered %q", a) + } +} + +func TestDedupeBlockers(t *testing.T) { + tests := []struct { + name string + in []string + want []string + }{ + { + name: "nil", + in: nil, + want: nil, + }, + { + name: "no duplicates preserves order", + in: []string{"VPCPeering/peer1", "VPCPeering/peer2"}, + want: []string{"VPCPeering/peer1", "VPCPeering/peer2"}, + }, + { + // A single dependent referencing the target via multiple fields + // must be listed once. + name: "same dependent collapsed to one", + in: []string{"VPCPeering/peer1", "VPCPeering/peer1", "VPCPeering/peer1"}, + want: []string{"VPCPeering/peer1"}, + }, + { + name: "keeps distinct dependents, first-seen order", + in: []string{"VPCPeering/peer1", "VPCPeering/peer2", "VPCPeering/peer1"}, + want: []string{"VPCPeering/peer1", "VPCPeering/peer2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := dedupeBlockers(tt.in) + if len(got) != len(tt.want) { + t.Fatalf("dedupeBlockers(%v) = %v, want %v", tt.in, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("index %d = %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + func FuzzObjectFromRequest(f *testing.F) { for _, seed := range [][]byte{ nil, diff --git a/internal/webhook/rule_registry.go b/internal/webhook/rule_registry.go index 50128b5..51177e4 100644 --- a/internal/webhook/rule_registry.go +++ b/internal/webhook/rule_registry.go @@ -142,10 +142,20 @@ func (r *RuleRegistry) AllTargetGVRs() []schema.GroupVersionResource { // rebuildTargetIndex rebuilds the byTarget reverse index from scratch. // Must be called with the write lock held. +// +// A rule key is added to a GVR's list at most once, even when the rule +// references that GVR via multiple fields. FindByTargetGVR expands the matching +// fields per key; indexing the key once per field would otherwise produce an +// N-squared cross product of entries for a single rule. func (r *RuleRegistry) rebuildTargetIndex() { r.byTarget = make(map[schema.GroupVersionResource][]string) for key, state := range r.rules { + seen := make(map[schema.GroupVersionResource]bool) for _, f := range state.IndexFields { + if seen[f.TargetGVR] { + continue + } + seen[f.TargetGVR] = true r.byTarget[f.TargetGVR] = append(r.byTarget[f.TargetGVR], key) } } diff --git a/internal/webhook/rule_registry_test.go b/internal/webhook/rule_registry_test.go index 0a9550b..9acd221 100644 --- a/internal/webhook/rule_registry_test.go +++ b/internal/webhook/rule_registry_test.go @@ -115,6 +115,39 @@ func TestRuleRegistry_MultipleRulesSameTarget(t *testing.T) { } } +func TestRuleRegistry_MultipleFieldsSameTarget(t *testing.T) { + r := NewRuleRegistry() + + vpcGVR := schema.GroupVersionResource{Group: "network.test.io", Version: "v1", Resource: "vpcs"} + + // A VPCPeering references the same target type (vpcs) via two fields. + state := &RuleState{ + IndexFields: []IndexedField{ + {FieldPath: ".spec.sourceVpcRef.name", TargetGVR: vpcGVR}, + {FieldPath: ".spec.targetVpcRef.name", TargetGVR: vpcGVR}, + }, + } + + r.Register("cluster1/vpcpeering-deps", state) + + // FindByTargetGVR must return exactly one entry per matching field — not the + // N-squared cross product produced when the rule key is indexed once per field. + entries := r.FindByTargetGVR(vpcGVR) + if len(entries) != 2 { + t.Fatalf("expected 2 entries (one per field), got %d", len(entries)) + } + + seen := map[string]int{} + for _, e := range entries { + seen[e.MatchedField.FieldPath]++ + } + for _, f := range state.IndexFields { + if seen[f.FieldPath] != 1 { + t.Errorf("field %q appeared %d times, want 1", f.FieldPath, seen[f.FieldPath]) + } + } +} + func TestRuleRegistry_UnregisterNonexistent(t *testing.T) { r := NewRuleRegistry() // Should not panic.