diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 4fb70bd3..bc927ef2 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -232,7 +232,7 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( clusterRule.Spec.GitTargetRef.Namespace, clusterRule.Spec.GitTargetRef.Name, ) - return r.commitRule(ctx, st, ruleReadiness(clusterRule.Status.Conditions, "ClusterWatchRule", msg)) + return commitRule(ctx, st, ruleReadiness(clusterRule.Status.Conditions, "ClusterWatchRule", msg)) } // gateClusterWatchRule is the ClusterWatchRule gate and the ONE place this controller compiles a @@ -327,29 +327,7 @@ func (r *ClusterWatchRuleReconciler) stallRule( ) (ctrl.Result, error) { rd := newRuleReadiness("ClusterWatchRule", "") rd.stalled(reason, message) - return r.commitRule(ctx, st, rd) -} - -// commitRule writes the trio, persists the status, and picks the requeue cadence from the same -// verdict, so the cadence can never disagree with what status says. -// -// Only a CONVERGING rule takes the fast loop. A stalled one waits for an event — a ClusterProvider -// policy change, a source-cluster Namespace label change, an edit to the rule — and every one of -// those has a watch edge registered in SetupWithManager, so polling it would find nothing. -func (r *ClusterWatchRuleReconciler) commitRule( - ctx context.Context, - st *reconcileStatus, - rd *readiness, -) (ctrl.Result, error) { - st.applyReadiness(rd) - if err := st.commit(ctx); err != nil { - return ctrl.Result{}, err - } - cadence := RequeueSteadyInterval - if rd.converging() { - cadence = RequeueStreamSettleInterval - } - return ctrl.Result{RequeueAfter: st.requeueAfter(cadence)}, nil + return commitRule(ctx, st, rd) } func (r *ClusterWatchRuleReconciler) setGitTargetReadyCondition( diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index 3548d510..b59281d3 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -3,7 +3,10 @@ package controller import ( + "context" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/watch" @@ -130,3 +133,28 @@ func gitTargetReadyReasonIsStalled(reason string) bool { return false } } + +// commitRule writes the trio, persists the status, and picks the requeue cadence from the same +// verdict, so the cadence can never disagree with what status says. +// +// Only a CONVERGING rule takes the fast loop; a stalled one falls back to RequeueSteadyInterval, +// which is a backstop rather than the mechanism. What actually clears a stall — a ClusterProvider +// policy change, a source-cluster Namespace label change, an edit to the rule — has a watch edge +// registered in SetupWithManager, so the rule is woken by an event long before the steady tick; +// polling it faster would find nothing. +// +// It is one function for both rule kinds. It was two identical methods, neither of which used its +// receiver, and the pairing of a cadence with a verdict is exactly the decision that must not be +// allowed to differ between them: the whole point of deriving it from the readiness is that one +// rule kind cannot end up polling on a schedule its own status contradicts. +func commitRule(ctx context.Context, st *reconcileStatus, rd *readiness) (ctrl.Result, error) { + st.applyReadiness(rd) + if err := st.commit(ctx); err != nil { + return ctrl.Result{}, err + } + cadence := RequeueSteadyInterval + if rd.converging() { + cadence = RequeueStreamSettleInterval + } + return ctrl.Result{RequeueAfter: st.requeueAfter(cadence)}, nil +} diff --git a/internal/controller/stream_status_test.go b/internal/controller/stream_status_test.go index a86bd983..f84d47dc 100644 --- a/internal/controller/stream_status_test.go +++ b/internal/controller/stream_status_test.go @@ -13,7 +13,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" @@ -53,8 +52,10 @@ func TestRuleReadiness_GitTargetReadyStalledBlocksRule(t *testing.T) { // rejected, and a rule stuck on the older answer is exactly what a spec waiting on its Ready reason // reads. // -// Both rule kinds carry their own copy of commitRule, so both are checked here: fixing one and -// leaving the other is the shape of mistake this table exists to catch. +// Both rule kinds go through it, which is now one shared function rather than a copy each. The +// table stays because the two kinds still reach it over different objects — one namespaced, one +// cluster-scoped, each with its own status subresource — and it is the whole path from a verdict to +// a requeue that is under test, not the arithmetic in the middle. func TestCommitRule_LostWriteBeatsTheConvergingLoop(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) @@ -74,12 +75,10 @@ func TestCommitRule_LostWriteBeatsTheConvergingLoop(t *testing.T) { // no-op path and never reach the patch this test is about. tests := []struct { name string - commit func(context.Context, *reconcileStatus, *readiness) (ctrl.Result, error) newRule func() statusObject }{ { - name: "WatchRule", - commit: (&WatchRuleReconciler{}).commitRule, + name: "WatchRule", newRule: func() statusObject { return &configbutleraiv1alpha3.WatchRule{ ObjectMeta: metav1.ObjectMeta{Name: "rule", Namespace: "tenant-acme", ResourceVersion: "1"}, @@ -87,8 +86,7 @@ func TestCommitRule_LostWriteBeatsTheConvergingLoop(t *testing.T) { }, }, { - name: "ClusterWatchRule", - commit: (&ClusterWatchRuleReconciler{}).commitRule, + name: "ClusterWatchRule", newRule: func() statusObject { return &configbutleraiv1alpha3.ClusterWatchRule{ ObjectMeta: metav1.ObjectMeta{Name: "rule", ResourceVersion: "1"}, @@ -108,7 +106,7 @@ func TestCommitRule_LostWriteBeatsTheConvergingLoop(t *testing.T) { rule := tc.newRule() landed := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rule). WithStatusSubresource(rule).Build() - result, err := tc.commit(context.Background(), beginStatus(landed, nil, rule), converging()) + result, err := commitRule(context.Background(), beginStatus(landed, nil, rule), converging()) require.NoError(t, err) assert.Equal(t, RequeueStreamSettleInterval, result.RequeueAfter, "a converging rule that published its status keeps the stream-settle loop") @@ -116,7 +114,7 @@ func TestCommitRule_LostWriteBeatsTheConvergingLoop(t *testing.T) { rule = tc.newRule() lost := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rule). WithInterceptorFuncs(conflict).Build() - result, err = tc.commit(context.Background(), beginStatus(lost, nil, rule), converging()) + result, err = commitRule(context.Background(), beginStatus(lost, nil, rule), converging()) require.NoError(t, err) assert.Equal(t, RequeueWriteLostInterval, result.RequeueAfter, "a converging rule whose status never landed must come back promptly, not on the settle loop") diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index 8c5cd0d1..f06c09d6 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -217,7 +217,7 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( targetNS, watchRule.Spec.GitTargetRef.Name, ) - return r.commitRule(ctx, st, ruleReadiness(watchRule.Status.Conditions, "WatchRule", msg)) + return commitRule(ctx, st, ruleReadiness(watchRule.Status.Conditions, "WatchRule", msg)) } // stallRule publishes a terminal WatchRule outcome and ends the reconcile. @@ -228,29 +228,7 @@ func (r *WatchRuleReconciler) stallRule( ) (ctrl.Result, error) { rd := newRuleReadiness("WatchRule", "") rd.stalled(reason, message) - return r.commitRule(ctx, st, rd) -} - -// commitRule writes the trio, persists the status, and picks the requeue cadence from the same -// verdict, so the cadence can never disagree with what status says. -// -// Only a CONVERGING rule takes the fast loop. A stalled one waits for an event — a ClusterProvider -// policy change, a source-cluster Namespace label change, an edit to the rule — and every one of -// those has a watch edge registered in SetupWithManager, so polling it would find nothing. -func (r *WatchRuleReconciler) commitRule( - ctx context.Context, - st *reconcileStatus, - rd *readiness, -) (ctrl.Result, error) { - st.applyReadiness(rd) - if err := st.commit(ctx); err != nil { - return ctrl.Result{}, err - } - cadence := RequeueSteadyInterval - if rd.converging() { - cadence = RequeueStreamSettleInterval - } - return ctrl.Result{RequeueAfter: st.requeueAfter(cadence)}, nil + return commitRule(ctx, st, rd) } func (r *WatchRuleReconciler) setResourceResolutionCondition( diff --git a/internal/rulestore/store.go b/internal/rulestore/store.go index 508ab4a0..5272cc24 100644 --- a/internal/rulestore/store.go +++ b/internal/rulestore/store.go @@ -38,8 +38,15 @@ type CompiledRule struct { ResourceRules []CompiledResourceRule } -// CompiledResourceRule represents a single resource matching rule with all its filters. -type CompiledResourceRule struct { +// compiledSelector is the TYPE filter both rule kinds compile to: the operations, groups, versions +// and resource plurals an item selects, with nothing about where the object lives. +// +// It is one type because the answer has to be one answer. The namespaced and cluster-scoped rules +// carried byte-identical copies of all four checks, so a fix to how a wildcard or a subresource +// pattern is read landed on one kind and silently not the other — and the two kinds are routed by +// different call paths, so nothing failed loudly when they disagreed. Scope is the only axis the +// two rules genuinely differ on, and it stays outside: see CompiledResourceRule.SourceNamespaces. +type compiledSelector struct { // Operations specifies which operations trigger this rule. Operations []configv1alpha3.OperationType // APIGroups specifies which API groups this rule matches. @@ -48,6 +55,27 @@ type CompiledResourceRule struct { APIVersions []string // Resources specifies which resource types this rule matches. Resources []string +} + +// clone returns a selector sharing no backing array with the receiver. +// +// The store hands compiled rules to callers and takes them from a CR it does not own, so every +// crossing of that boundary copies. Struct assignment is not enough: it duplicates four slice +// HEADERS, leaving the store's rule and the caller's snapshot writing through to the same arrays — +// and a caller writing there reaches store state behind the mutex entirely. An empty slice clones +// to nil, which every reader here already treats as "match all", exactly as before. +func (s *compiledSelector) clone() compiledSelector { + return compiledSelector{ + Operations: append([]configv1alpha3.OperationType(nil), s.Operations...), + APIGroups: append([]string(nil), s.APIGroups...), + APIVersions: append([]string(nil), s.APIVersions...), + Resources: append([]string(nil), s.Resources...), + } +} + +// CompiledResourceRule represents a single resource matching rule with all its filters. +type CompiledResourceRule struct { + compiledSelector // SourceNamespaces is this item's RESOLVED source-namespace set IN THE SOURCE CLUSTER — // spec.rules[i].sourceNamespace fully expanded to concrete names at compile time. Neither a @@ -93,14 +121,7 @@ type CompiledClusterRule struct { // a scope field here would let a pruned or absent value widen a stream, which is the failure the // narrowing exists to prevent. type CompiledClusterResourceRule struct { - // Operations specifies which operations trigger this rule. - Operations []configv1alpha3.OperationType - // APIGroups specifies which API groups this rule matches. - APIGroups []string - // APIVersions specifies which API versions this rule matches. - APIVersions []string - // Resources specifies which resource types this rule matches. - Resources []string + compiledSelector } // RuleStore holds the in-memory representation of all active watch rules. @@ -175,11 +196,14 @@ func (s *RuleStore) AddOrUpdateWatchRule( if i < len(sourceNamespaces) { namespaces = append([]string(nil), sourceNamespaces[i]...) } + selector := compiledSelector{ + Operations: r.Operations, + APIGroups: r.APIGroups, + APIVersions: r.APIVersions, + Resources: r.Resources, + } compiled.ResourceRules = append(compiled.ResourceRules, CompiledResourceRule{ - Operations: r.Operations, - APIGroups: r.APIGroups, - APIVersions: r.APIVersions, - Resources: r.Resources, + compiledSelector: selector.clone(), SourceNamespaces: namespaces, }) } @@ -242,11 +266,14 @@ func (s *RuleStore) AddOrUpdateClusterWatchRule( } for _, r := range rule.Spec.Rules { - compiled.Rules = append(compiled.Rules, CompiledClusterResourceRule{ + selector := compiledSelector{ Operations: r.Operations, APIGroups: r.APIGroups, APIVersions: r.APIVersions, Resources: r.Resources, + } + compiled.Rules = append(compiled.Rules, CompiledClusterResourceRule{ + compiledSelector: selector.clone(), }) } @@ -375,7 +402,7 @@ func (s *RuleStore) clusterRuleMatches( return false } for _, rule := range clusterRule.Rules { - if rule.matchesCluster(resourcePlural, operation, apiGroup, apiVersion) { + if rule.matchesType(resourcePlural, operation, apiGroup, apiVersion) { return true } } @@ -418,23 +445,7 @@ func (r *CompiledResourceRule) matches( return false } - // Match operations (empty = match all) - if !r.matchesOperations(operation) { - return false - } - - // Match API groups (empty = match all) - if !r.matchesAPIGroups(apiGroup) { - return false - } - - // Match API versions (empty = match all) - if !r.matchesAPIVersions(apiVersion) { - return false - } - - // Match resource plural (required) - return r.resourceMatches(resourcePlural) + return r.matchesType(resourcePlural, operation, apiGroup, apiVersion) } // matchesSourceNamespace checks the event's namespace against this item's RESOLVED set. An event @@ -457,125 +468,33 @@ func (r *CompiledResourceRule) matchesSourceNamespace(eventNamespace string) boo return false } -// matchesOperations checks if the operation matches any in the rule. -func (r *CompiledResourceRule) matchesOperations(operation configv1alpha3.OperationType) bool { - if len(r.Operations) == 0 { - return true // Empty = match all - } - - for _, op := range r.Operations { - if op == configv1alpha3.OperationAll || op == operation { - return true - } - } - return false -} - -// matchesAPIGroups checks if the API group matches any in the rule. -func (r *CompiledResourceRule) matchesAPIGroups(apiGroup string) bool { - if len(r.APIGroups) == 0 { - return true // Empty = match all - } - - for _, group := range r.APIGroups { - if group == "*" || group == apiGroup { - return true - } - } - return false -} - -// matchesAPIVersions checks if the API version matches any in the rule. -func (r *CompiledResourceRule) matchesAPIVersions(apiVersion string) bool { - if len(r.APIVersions) == 0 { - return true // Empty = match all - } - - for _, version := range r.APIVersions { - if version == "*" || version == apiVersion { - return true - } - } - return false -} - -// resourceMatches checks if the resource plural matches any of the rule patterns. -func (r *CompiledResourceRule) resourceMatches(resourcePlural string) bool { - for _, ruleResource := range r.Resources { - if r.singleResourceMatches(ruleResource, resourcePlural) { - return true - } - } - return false -} - -// singleResourceMatches checks if a single rule pattern matches the given resource plural. -// Supports: -// - "*" - matches all resources -// - "pods" - exact match (case-insensitive) -// - "pods/*" - matches all pod subresources (pods/log, pods/status, etc.) -// - "pods/log" - matches specific subresource -// -// Does NOT support: -// - Prefix wildcards: "pod*" (removed per enhancement plan) -// - Suffix wildcards: "*.example.com" (removed per enhancement plan) -func (r *CompiledResourceRule) singleResourceMatches(ruleResource, resourcePlural string) bool { - if ruleResource == "" { - return false - } - - // Match wildcard for all resources - if ruleResource == "*" { - return true - } - - // Exact match (case-insensitive) - if strings.EqualFold(ruleResource, resourcePlural) { - return true - } - - // Subresource wildcard: "pods/*" matches "pods/log", "pods/status", etc. - if strings.HasSuffix(ruleResource, "/*") { - prefix := ruleResource[:len(ruleResource)-2] // Remove "/*" - return strings.HasPrefix(strings.ToLower(resourcePlural), strings.ToLower(prefix)+"/") - } - - return false -} - -// matchesCluster checks if a cluster resource rule matches the given filters. -func (r *CompiledClusterResourceRule) matchesCluster( +// matchesType answers the whole type filter: operations, group, version, resource plural. An empty +// list means "match all" on every axis except the resource plural, which is required. +func (s *compiledSelector) matchesType( resourcePlural string, operation configv1alpha3.OperationType, apiGroup string, apiVersion string, ) bool { - // Match operations (empty = match all) - if !r.matchesOperations(operation) { + if !s.matchesOperations(operation) { return false } - - // Match API groups (empty = match all) - if !r.matchesAPIGroups(apiGroup) { + if !matchesAny(s.APIGroups, apiGroup) { return false } - - // Match API versions (empty = match all) - if !r.matchesAPIVersions(apiVersion) { + if !matchesAny(s.APIVersions, apiVersion) { return false } - - // Match resource plural (required) - return r.resourceMatches(resourcePlural) + return s.resourceMatches(resourcePlural) } // matchesOperations checks if the operation matches any in the rule. -func (r *CompiledClusterResourceRule) matchesOperations(operation configv1alpha3.OperationType) bool { - if len(r.Operations) == 0 { +func (s *compiledSelector) matchesOperations(operation configv1alpha3.OperationType) bool { + if len(s.Operations) == 0 { return true // Empty = match all } - for _, op := range r.Operations { + for _, op := range s.Operations { if op == configv1alpha3.OperationAll || op == operation { return true } @@ -583,28 +502,15 @@ func (r *CompiledClusterResourceRule) matchesOperations(operation configv1alpha3 return false } -// matchesAPIGroups checks if the API group matches any in the rule. -func (r *CompiledClusterResourceRule) matchesAPIGroups(apiGroup string) bool { - if len(r.APIGroups) == 0 { +// matchesAny is the group and version test, which are the same test: an empty list matches all, and +// "*" is the only wildcard either axis accepts. +func matchesAny(patterns []string, value string) bool { + if len(patterns) == 0 { return true // Empty = match all } - for _, group := range r.APIGroups { - if group == "*" || group == apiGroup { - return true - } - } - return false -} - -// matchesAPIVersions checks if the API version matches any in the rule. -func (r *CompiledClusterResourceRule) matchesAPIVersions(apiVersion string) bool { - if len(r.APIVersions) == 0 { - return true // Empty = match all - } - - for _, version := range r.APIVersions { - if version == "*" || version == apiVersion { + for _, pattern := range patterns { + if pattern == "*" || pattern == value { return true } } @@ -612,9 +518,9 @@ func (r *CompiledClusterResourceRule) matchesAPIVersions(apiVersion string) bool } // resourceMatches checks if the resource plural matches any of the rule patterns. -func (r *CompiledClusterResourceRule) resourceMatches(resourcePlural string) bool { - for _, ruleResource := range r.Resources { - if r.singleResourceMatches(ruleResource, resourcePlural) { +func (s *compiledSelector) resourceMatches(resourcePlural string) bool { + for _, ruleResource := range s.Resources { + if singleResourceMatches(ruleResource, resourcePlural) { return true } } @@ -622,7 +528,16 @@ func (r *CompiledClusterResourceRule) resourceMatches(resourcePlural string) boo } // singleResourceMatches checks if a single rule pattern matches the given resource plural. -func (r *CompiledClusterResourceRule) singleResourceMatches(ruleResource, resourcePlural string) bool { +// Supports: +// - "*" - matches all resources +// - "pods" - exact match (case-insensitive) +// - "pods/*" - matches all pod subresources (pods/log, pods/status, etc.) +// - "pods/log" - matches specific subresource +// +// Does NOT support: +// - Prefix wildcards: "pod*" (removed per enhancement plan) +// - Suffix wildcards: "*.example.com" (removed per enhancement plan) +func singleResourceMatches(ruleResource, resourcePlural string) bool { if ruleResource == "" { return false } @@ -679,6 +594,7 @@ func deepCopyCompiledRule(in CompiledRule) CompiledRule { cp.ResourceRules = make([]CompiledResourceRule, len(in.ResourceRules)) copy(cp.ResourceRules, in.ResourceRules) for i := range cp.ResourceRules { + cp.ResourceRules[i].compiledSelector = in.ResourceRules[i].clone() cp.ResourceRules[i].SourceNamespaces = append([]string(nil), in.ResourceRules[i].SourceNamespaces...) } @@ -692,6 +608,9 @@ func deepCopyCompiledClusterRule(in CompiledClusterRule) CompiledClusterRule { if len(in.Rules) > 0 { cp.Rules = make([]CompiledClusterResourceRule, len(in.Rules)) copy(cp.Rules, in.Rules) + for i := range cp.Rules { + cp.Rules[i].compiledSelector = in.Rules[i].clone() + } } return cp } diff --git a/internal/rulestore/store_test.go b/internal/rulestore/store_test.go index 61ad94d0..0c157567 100644 --- a/internal/rulestore/store_test.go +++ b/internal/rulestore/store_test.go @@ -634,7 +634,8 @@ func TestGetMatchingClusterRules(t *testing.T) { } } -// TestResourceMatching verifies different resource matching patterns. +// TestResourceMatching verifies different resource matching patterns. It covers both rule kinds: +// the pattern test is one function, shared through compiledSelector. func TestResourceMatching(t *testing.T) { tests := []struct { name string @@ -657,11 +658,7 @@ func TestResourceMatching(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rule := CompiledResourceRule{ - Resources: []string{tt.ruleResource}, - } - - result := rule.singleResourceMatches(tt.ruleResource, tt.resourcePlural) + result := singleResourceMatches(tt.ruleResource, tt.resourcePlural) if result != tt.shouldMatch { t.Errorf("Expected match=%v for pattern '%s' against '%s', got %v", tt.shouldMatch, tt.ruleResource, tt.resourcePlural, result) @@ -670,7 +667,7 @@ func TestResourceMatching(t *testing.T) { } } -// TestOperationMatching verifies operation matching logic. +// TestOperationMatching verifies operation matching logic, for both rule kinds at once. func TestOperationMatching(t *testing.T) { tests := []struct { name string @@ -713,11 +710,9 @@ func TestOperationMatching(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rule := CompiledResourceRule{ - Operations: tt.ruleOperations, - } + selector := compiledSelector{Operations: tt.ruleOperations} - result := rule.matchesOperations(tt.operation) + result := selector.matchesOperations(tt.operation) if result != tt.shouldMatch { t.Errorf("Expected match=%v, got %v", tt.shouldMatch, result) } @@ -725,7 +720,7 @@ func TestOperationMatching(t *testing.T) { } } -// TestAPIGroupMatching verifies API group matching. +// TestAPIGroupMatching verifies API group matching, for both rule kinds at once. func TestAPIGroupMatching(t *testing.T) { tests := []struct { name string @@ -744,11 +739,7 @@ func TestAPIGroupMatching(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rule := CompiledResourceRule{ - APIGroups: tt.ruleGroups, - } - - result := rule.matchesAPIGroups(tt.apiGroup) + result := matchesAny(tt.ruleGroups, tt.apiGroup) if result != tt.shouldMatch { t.Errorf("Expected match=%v, got %v", tt.shouldMatch, result) } @@ -756,7 +747,8 @@ func TestAPIGroupMatching(t *testing.T) { } } -// TestAPIVersionMatching verifies API version matching. +// TestAPIVersionMatching verifies API version matching. Groups and versions run the same test, so +// this and TestAPIGroupMatching are two corpora over one function. func TestAPIVersionMatching(t *testing.T) { tests := []struct { name string @@ -773,11 +765,7 @@ func TestAPIVersionMatching(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rule := CompiledResourceRule{ - APIVersions: tt.ruleVersions, - } - - result := rule.matchesAPIVersions(tt.apiVersion) + result := matchesAny(tt.ruleVersions, tt.apiVersion) if result != tt.shouldMatch { t.Errorf("Expected match=%v, got %v", tt.shouldMatch, result) } @@ -1163,3 +1151,267 @@ func TestGetMatchingRules_WildcardItemMatchesEveryResolvedNamespace(t *testing.T "implicitly included, got %d matches", len(got)) } } + +// TestSharedSelector_RoutesAlikeThroughBothEntryPoints is the regression the extraction exists to +// prevent. Each rule kind used to carry its own copy of the four type-filter checks, and the two +// are reached over different call paths — a WatchRule through CompiledRule.matches, which layers +// the namespace gate on top, and a ClusterWatchRule through clusterRuleMatches, which does not — so +// a divergence produced no failure anywhere: a wildcard or subresource pattern honoured for one +// kind and quietly ignored for the other. +// +// It therefore routes ONE selector through both public entry points rather than calling the shared +// method twice, which post-extraction could only ever agree with itself. The last case is the axis +// on which the two kinds legitimately differ, and shows it stayed outside the shared selector. +func TestSharedSelector_RoutesAlikeThroughBothEntryPoints(t *testing.T) { + t.Parallel() + + operations := []configv1alpha3.OperationType{configv1alpha3.OperationCreate} + store := NewStore() + + watchRule := configv1alpha3.WatchRule{ + Spec: configv1alpha3.WatchRuleSpec{ + Rules: []configv1alpha3.ResourceRule{{ + Operations: operations, + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }}, + }, + } + watchRule.Name = "rule" + watchRule.Namespace = "tenant-acme" + store.AddOrUpdateWatchRule(watchRule, ownNamespaceScope(watchRule), "target", "tenant-acme", + "provider", "tenant-acme", "main", "clusters") + + // The same selector on the cluster-scoped kind. A ClusterWatchRule may legally name a + // namespaced plural (see TestGetMatchingClusterRules); what bounds it is the OBJECT's scope. + clusterRule := configv1alpha3.ClusterWatchRule{ + Spec: configv1alpha3.ClusterWatchRuleSpec{ + Rules: []configv1alpha3.ClusterResourceRule{{ + Operations: operations, + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + }}, + }, + } + clusterRule.Name = "cluster-rule" + store.AddOrUpdateClusterWatchRule(clusterRule, "target", "tenant-acme", "provider", "tenant-acme", + "main", "clusters") + + inScope := &unstructured.Unstructured{} + inScope.SetNamespace("tenant-acme") + foreign := &unstructured.Unstructured{} + foreign.SetNamespace("tenant-other") + + probes := []struct { + name string + object *unstructured.Unstructured + resource string + operation configv1alpha3.OperationType + group string + version string + want bool + // clusterWant differs only where scope, not the selector, decides. + clusterWant *bool + }{ + {name: "exact hit", object: inScope, resource: "deployments", + operation: configv1alpha3.OperationCreate, group: "apps", version: "v1", want: true}, + {name: "operation not selected", object: inScope, resource: "deployments", + operation: configv1alpha3.OperationDelete, group: "apps", version: "v1", want: false}, + {name: "wrong group", object: inScope, resource: "deployments", + operation: configv1alpha3.OperationCreate, group: "batch", version: "v1", want: false}, + {name: "wrong version", object: inScope, resource: "deployments", + operation: configv1alpha3.OperationCreate, group: "apps", version: "v2", want: false}, + {name: "wrong resource", object: inScope, resource: "statefulsets", + operation: configv1alpha3.OperationCreate, group: "apps", version: "v1", want: false}, + {name: "unauthorized namespace", object: foreign, resource: "deployments", + operation: configv1alpha3.OperationCreate, group: "apps", version: "v1", + want: false, clusterWant: ptr(true)}, + } + + for _, probe := range probes { + t.Run(probe.name, func(t *testing.T) { + t.Parallel() + + namespaced := store.GetMatchingRules( + probe.object, probe.resource, probe.operation, probe.group, probe.version, false) + if got := len(namespaced) == 1; got != probe.want { + t.Errorf("WatchRule routing: want match=%v, got %v", probe.want, got) + } + + // The cluster path is probed with isClusterScoped=true, since that is the only way a + // ClusterWatchRule ever matches; every other axis is the shared selector's answer. + wantCluster := probe.want + if probe.clusterWant != nil { + wantCluster = *probe.clusterWant + } + cluster := store.GetMatchingClusterRules( + probe.resource, probe.operation, probe.group, probe.version, true, nil) + if got := len(cluster) == 1; got != wantCluster { + t.Errorf("ClusterWatchRule routing: want match=%v, got %v", wantCluster, got) + } + }) + } +} + +// ptr is a local helper for the one probe whose two kinds legitimately disagree. +func ptr[T any](v T) *T { return &v } + +// TestSnapshotWatchRules_SourceNamespacesStayDefensivelyCopied pins the source-namespace slice, +// which the store has always copied per item rather than by struct assignment. A copy that silently +// became shallow would hand every caller a slice aliasing the stored rule. +func TestSnapshotWatchRules_SourceNamespacesStayDefensivelyCopied(t *testing.T) { + t.Parallel() + + store := NewStore() + rule := configv1alpha3.WatchRule{ + Spec: configv1alpha3.WatchRuleSpec{ + Rules: []configv1alpha3.ResourceRule{{Resources: []string{"pods"}}}, + }, + } + rule.Name = "rule" + rule.Namespace = "tenant-acme" + store.AddOrUpdateWatchRule(rule, ownNamespaceScope(rule), "target", "tenant-acme", "provider", "tenant-acme", + "main", "clusters") + + snapshot := store.SnapshotWatchRules() + if len(snapshot) != 1 || len(snapshot[0].ResourceRules) != 1 { + t.Fatalf("expected one compiled item, got %#v", snapshot) + } + snapshot[0].ResourceRules[0].SourceNamespaces[0] = "mutated-by-caller" + + again := store.SnapshotWatchRules() + if got := again[0].ResourceRules[0].SourceNamespaces[0]; got != "tenant-acme" { + t.Errorf("a caller mutating its snapshot reached the stored rule: got %q", got) + } +} + +// TestStore_OwnsItsSelectorSlices covers the two boundaries a compiled selector crosses, on both +// rule kinds and all four of its slices. +// +// Struct assignment copies slice HEADERS, so before the selector owned its arrays the store shared +// them in both directions: with the CR it was compiled from, and with every snapshot it handed out. +// Either aliasing lets a write outside the store change what the store matches on, with no lock +// held and no reconcile — a rule silently starts or stops selecting a type. Snapshot* documents the +// opposite: callers may freely modify what they get back. +func TestStore_OwnsItsSelectorSlices(t *testing.T) { + t.Parallel() + + // selectorOf reads the four slices back off whichever kind the case compiled, so one table can + // assert the same property for both. + type selectors struct { + operations []configv1alpha3.OperationType + apiGroups []string + apiVersions []string + resources []string + } + + tests := []struct { + name string + // install compiles a rule whose selector slices the test still holds a reference to, and + // returns those slices for mutation plus a reader for the store's own copy. + install func(store *RuleStore) (mutate func(), stored func() selectors) + }{ + { + name: "WatchRule", + install: func(store *RuleStore) (func(), func() selectors) { + item := configv1alpha3.ResourceRule{ + Operations: []configv1alpha3.OperationType{configv1alpha3.OperationCreate}, + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + } + rule := configv1alpha3.WatchRule{ + Spec: configv1alpha3.WatchRuleSpec{Rules: []configv1alpha3.ResourceRule{item}}, + } + rule.Name = "rule" + rule.Namespace = "tenant-acme" + store.AddOrUpdateWatchRule(rule, ownNamespaceScope(rule), "target", "tenant-acme", + "provider", "tenant-acme", "main", "clusters") + + return func() { + item.Operations[0] = configv1alpha3.OperationDelete + item.APIGroups[0] = "batch" + item.APIVersions[0] = "v2" + item.Resources[0] = "statefulsets" + }, func() selectors { + snap := store.SnapshotWatchRules()[0].ResourceRules[0] + return selectors{snap.Operations, snap.APIGroups, snap.APIVersions, snap.Resources} + } + }, + }, + { + name: "ClusterWatchRule", + install: func(store *RuleStore) (func(), func() selectors) { + item := configv1alpha3.ClusterResourceRule{ + Operations: []configv1alpha3.OperationType{configv1alpha3.OperationCreate}, + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + } + rule := configv1alpha3.ClusterWatchRule{ + Spec: configv1alpha3.ClusterWatchRuleSpec{ + Rules: []configv1alpha3.ClusterResourceRule{item}, + }, + } + rule.Name = "cluster-rule" + store.AddOrUpdateClusterWatchRule(rule, "target", "tenant-acme", "provider", + "tenant-acme", "main", "clusters") + + return func() { + item.Operations[0] = configv1alpha3.OperationDelete + item.APIGroups[0] = "batch" + item.APIVersions[0] = "v2" + item.Resources[0] = "statefulsets" + }, func() selectors { + snap := store.SnapshotClusterWatchRules()[0].Rules[0] + return selectors{snap.Operations, snap.APIGroups, snap.APIVersions, snap.Resources} + } + }, + }, + } + + want := selectors{ + operations: []configv1alpha3.OperationType{configv1alpha3.OperationCreate}, + apiGroups: []string{"apps"}, + apiVersions: []string{"v1"}, + resources: []string{"deployments"}, + } + assertSelectors := func(t *testing.T, boundary string, got selectors) { + t.Helper() + if got.operations[0] != want.operations[0] { + t.Errorf("%s: operations reached the store: %v", boundary, got.operations) + } + if got.apiGroups[0] != want.apiGroups[0] { + t.Errorf("%s: apiGroups reached the store: %v", boundary, got.apiGroups) + } + if got.apiVersions[0] != want.apiVersions[0] { + t.Errorf("%s: apiVersions reached the store: %v", boundary, got.apiVersions) + } + if got.resources[0] != want.resources[0] { + t.Errorf("%s: resources reached the store: %v", boundary, got.resources) + } + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + store := NewStore() + mutateSource, stored := tc.install(store) + + // Boundary one: the CR the rule was compiled from is still mutable by its owner. + mutateSource() + assertSelectors(t, "compiled-from CR", stored()) + + // Boundary two: a snapshot the store handed out, which callers may freely modify. + snapshot := stored() + snapshot.operations[0] = configv1alpha3.OperationDelete + snapshot.apiGroups[0] = "batch" + snapshot.apiVersions[0] = "v2" + snapshot.resources[0] = "statefulsets" + assertSelectors(t, "returned snapshot", stored()) + }) + } +}