From c13d30c110bedf77f5350f658b282e2986d6edbc Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 17 Aug 2026 17:11:03 +0200 Subject: [PATCH] :bug: fix(webhook): validate spec-derived GVKs when the profile is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #304, which is the parent commit of this branch. #304 defers the whole GVK check when profileRef does not resolve, but only the profile targetGVK actually depends on the profile. `CollectRuleGVKs` returns the `spec.resourceRules[].gvk` list and ignores the profile GVK entirely whenever resourceRules is non-empty, so those GVKs are knowable at admission and were being waved through. Not an exploit path — a target cannot collect while its profile is missing, and reconcile now catches it once the profile appears — but admission should reject a spec it can fully evaluate. The NotFound branch also duplicated the happy path's two namespace checks. That is the divergence shape that caused the bug #304 fixes: a third check added to the tail later would silently not run for missing profiles. Resolve is now a switch that yields `profileGVK` plus `profileResolved`, and the whole tail runs once for both paths. GVK validation is skipped only when the profile is missing *and* resourceRules is empty, where the alternative is comparing a zero GVK against allowedGVKs and rejecting a legal create-before-profile. Tests: - `missingProfileSpecDerivedChecks` — denied resourceRules GVK and denied profileRef namespace, both with no profile object; red on the first before this change. It also covers the `ValidateClusterScopeStaticRefNamespace` call #304 added, which its own fixture could not exercise: allowedStaticRefNamespaces was unset there, so the call returned nil unconditionally. - `deniedRuleGVKDegrades` — reconcile side, permitted profile targetGVK plus a denied resourceRules GVK. Green before this commit; it locks the rules path through CollectRuleGVKs that #304's test did not reach. - ScopeCeiling's doc comment claimed "namespace and GVK checks" while the struct only carries namespaces, which points readers at the wrong enforcement point. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook and controller unit tests green. envtest suites are CI-only on this host; a GVK-denied spec in cluster_scope_enforce_envtest_test.go is left as follow-up rather than added unverified. --- internal/collect/collection_filter.go | 4 +- .../kollectclustertarget_unit_test.go | 69 +++++++++++++++++++ ...tclustertarget_scope_webhook_extra_test.go | 57 +++++++++++++++ .../v1alpha1/kollectclustertarget_webhook.go | 31 +++++---- 4 files changed, 146 insertions(+), 15 deletions(-) diff --git a/internal/collect/collection_filter.go b/internal/collect/collection_filter.go index fc7065ca..2c17b97f 100644 --- a/internal/collect/collection_filter.go +++ b/internal/collect/collection_filter.go @@ -25,7 +25,9 @@ type NamespaceDefaults struct { Excluded []string } -// ScopeCeiling is the resolved tenancy guardrail for namespace and GVK checks. +// ScopeCeiling is the resolved tenancy guardrail for namespace checks at collect +// time. GVKs are not filtered here: they are enforced at admission and, as +// backstop, at reconcile (ScopeGVKDenied). type ScopeCeiling struct { AllowedNamespaces []string DeniedNamespaces []string diff --git a/internal/controller/kollectclustertarget_unit_test.go b/internal/controller/kollectclustertarget_unit_test.go index e22a82d4..1763d58e 100644 --- a/internal/controller/kollectclustertarget_unit_test.go +++ b/internal/controller/kollectclustertarget_unit_test.go @@ -137,3 +137,72 @@ func TestKollectClusterTargetReconciler_deniedGVKDegrades(t *testing.T) { t.Fatalf("Degraded = %+v, want reason %s", cond, scopeReasonGVKDenied) } } + +// A permitted profile targetGVK does not launder a denied resourceRules GVK: +// CollectRuleGVKs returns the rule GVKs and ignores the profile GVK entirely +// once resourceRules is set, so the reconcile check has to see the rules. +func TestKollectClusterTargetReconciler_deniedRuleGVKDegrades(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := kollectdevv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + clusterScope := &kollectdevv1alpha1.KollectClusterScope{ + ObjectMeta: metav1.ObjectMeta{Name: "platform"}, + Spec: kollectdevv1alpha1.KollectClusterScopeSpec{ + ScopeCeilingSpec: kollectdevv1alpha1.ScopeCeilingSpec{ + AllowedGVKs: []kollectdevv1alpha1.GroupVersionKind{ + {Group: "apps", Version: "v1", Kind: "Deployment"}, + }, + }, + }, + } + profile := &kollectdevv1alpha1.KollectProfile{ + ObjectMeta: metav1.ObjectMeta{Name: "deployments", Namespace: "kollect-system"}, + Spec: kollectdevv1alpha1.KollectProfileSpec{ + TargetGVK: kollectdevv1alpha1.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, + }, + } + ct := &kollectdevv1alpha1.KollectClusterTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-rules", Generation: 1}, + Spec: kollectdevv1alpha1.KollectClusterTargetSpec{ + ProfileRef: kollectdevv1alpha1.NamespacedObjectReference{Name: "deployments", Namespace: "kollect-system"}, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "platform"}, + }, + CollectionFilterSpec: kollectdevv1alpha1.CollectionFilterSpec{ + ResourceRules: []kollectdevv1alpha1.ResourceRule{ + {GVK: kollectdevv1alpha1.GroupVersionKind{Version: "v1", Kind: "Secret"}}, + }, + }, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithStatusSubresource(ct). + WithObjects(clusterScope, profile, ct). + Build() + + r := &KollectClusterTargetReconciler{Client: c, Scheme: scheme} + if _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: ct.Name}, + }); err != nil { + t.Fatalf("reconcile: %v", err) + } + + var got kollectdevv1alpha1.KollectClusterTarget + if err := c.Get(context.Background(), types.NamespacedName{Name: ct.Name}, &got); err != nil { + t.Fatal(err) + } + + cond := apimeta.FindStatusCondition(got.Status.Conditions, conditionDegraded) + if cond == nil || cond.Reason != scopeReasonGVKDenied { + t.Fatalf("Degraded = %+v, want reason %s", cond, scopeReasonGVKDenied) + } +} diff --git a/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go b/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go index a0588ce7..ebb57332 100644 --- a/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go +++ b/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go @@ -94,3 +94,60 @@ func TestKollectClusterTargetValidator_validateClusterScope(t *testing.T) { t.Fatalf("missing in-scope profile should still admit: %v", err) } } + +func TestKollectClusterTargetValidator_missingProfileSpecDerivedChecks(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := kollectdevv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + clusterScope := &kollectdevv1alpha1.KollectClusterScope{ + ObjectMeta: metav1.ObjectMeta{Name: "platform"}, + Spec: kollectdevv1alpha1.KollectClusterScopeSpec{ + ScopeCeilingSpec: kollectdevv1alpha1.ScopeCeilingSpec{ + AllowedGVKs: []kollectdevv1alpha1.GroupVersionKind{ + {Group: "apps", Version: "v1", Kind: "Deployment"}, + }, + }, + AllowedStaticRefNamespaces: []string{"kollect-system"}, + }, + } + + // No KollectProfile object: profileRef points at one that does not exist yet. + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(clusterScope).Build() + v := &kollectClusterTargetValidator{client: cl} + + base := &kollectdevv1alpha1.KollectClusterTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "ct"}, + Spec: kollectdevv1alpha1.KollectClusterTargetSpec{ + ProfileRef: kollectdevv1alpha1.NamespacedObjectReference{Name: "not-created-yet", Namespace: "kollect-system"}, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "platform"}, + }, + }, + } + if err := v.validateClusterScope(context.Background(), base); err != nil { + t.Fatalf("in-scope target must stay admissible before its profile exists: %v", err) + } + + // resourceRules GVKs come from the spec, not the profile — CollectRuleGVKs + // ignores the profile GVK entirely once resourceRules is set, so a missing + // profile is no reason to skip them. + deniedRuleGVK := base.DeepCopy() + deniedRuleGVK.Spec.ResourceRules = []kollectdevv1alpha1.ResourceRule{ + {GVK: kollectdevv1alpha1.GroupVersionKind{Version: "v1", Kind: "Secret"}}, + } + if err := v.validateClusterScope(context.Background(), deniedRuleGVK); err == nil { + t.Fatal("expected resourceRules GVK violation when the profile is missing") + } + + // allowedStaticRefNamespaces is spec-derived too; #304 added the check but no + // fixture exercised it, because allowedStaticRefNamespaces was unset there. + deniedRefNS := base.DeepCopy() + deniedRefNS.Spec.ProfileRef.Namespace = "tenant-b" + if err := v.validateClusterScope(context.Background(), deniedRefNS); err == nil { + t.Fatal("expected profileRef namespace violation when the profile is missing") + } +} diff --git a/internal/webhook/v1alpha1/kollectclustertarget_webhook.go b/internal/webhook/v1alpha1/kollectclustertarget_webhook.go index c8142d7c..1f1680ca 100644 --- a/internal/webhook/v1alpha1/kollectclustertarget_webhook.go +++ b/internal/webhook/v1alpha1/kollectclustertarget_webhook.go @@ -83,24 +83,27 @@ func (v *kollectClusterTargetValidator) validateClusterScope( return nil } + var profileGVK kollectdevv1alpha1.GroupVersionKind + profileResolved := true profile, err := resolveClusterTargetProfileForWebhook(ctx, v.client, target.Spec.ProfileRef) - if err != nil { - if apierrors.IsNotFound(err) { - intentNS := scope.NormalizeNamespaceList(target.Spec.IncludedNamespaces) - if nsErr := scope.ValidateClusterScopeNamespaces(binding.Scope, intentNS); nsErr != nil { - return nsErr - } - - return scope.ValidateClusterScopeStaticRefNamespace(binding.Scope, target.Spec.ProfileRef.Namespace) - } - + switch { + case err == nil: + profileGVK = profile.Spec.TargetGVK + case apierrors.IsNotFound(err): + // A ClusterTarget may be applied before its KollectProfile. Only the + // profile targetGVK is unknowable here; reconcile re-checks it and + // degrades with ScopeGVKDenied. Everything below is spec-derived and + // stays enforced so the checks cannot silently diverge per branch. + profileResolved = false + default: return err } - gvks := scope.CollectRuleGVKs(target.Spec.CollectionFilterSpec, profile.Spec.TargetGVK) - for _, gvk := range gvks { - if err := scope.ValidateClusterScopeGVKs(binding.Scope, gvk); err != nil { - return err + if profileResolved || len(target.Spec.ResourceRules) > 0 { + for _, gvk := range scope.CollectRuleGVKs(target.Spec.CollectionFilterSpec, profileGVK) { + if err := scope.ValidateClusterScopeGVKs(binding.Scope, gvk); err != nil { + return err + } } }