From fcf3f54f7767d0e1ac12618ee97fdee5632f332e Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Mon, 17 Aug 2026 17:00:32 +0200 Subject: [PATCH] :bug: fix(webhook): keep KollectScope checks when the profile is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateScope` returned nil as soon as the referenced KollectProfile was absent, so a KollectTarget applied before its profile skipped the namespace allow/deny checks and the resourceRules GVK check entirely. PR #304 fixes the same skip on the cluster-scoped kind; this is the namespaced half. Severity is lower here than on the cluster kind: `scopeCheck.enforceTarget` already re-checks GVKs and namespaces at reconcile, so a target admitted this way degrades instead of collecting. What was wrong is that admission accepted a spec it can fully evaluate — `deniedNamespaces`, `allowedNamespaces` and `spec.resourceRules[].gvk` do not depend on the profile at all. Only the profile `targetGVK` does, and `CollectRuleGVKs` ignores it whenever resourceRules is non-empty. The resolve now runs as a switch: found resolves the GVK, NotFound continues with `profileResolved = false`, any other error still fails closed. GVK validation is skipped only in the one case where it would compare against a zero GVK — profile missing *and* no resourceRules — which would otherwise reject a legal create-before-profile against a non-empty allowedGVKs. Test first: `TestKollectTargetValidator_scopeAdmissionMissingProfile` covers denied namespace, out-of-allowlist namespace, and out-of-scope resourceRules GVK with no profile object present, plus the in-scope case that must stay admissible. Red before the change on the denied-namespace assertion. Gates: lint (golangci v2 + arch-lint) clean, verify ok, scrub ok, webhook unit tests green. envtest suites (TestWebhookEnvtest, TestSetupWithManager_*) do not run on this host — bin/k8s ships linux-amd64 assets only and setup-envtest is rate-limited — so they are CI-only, and they fail identically on an unmodified main checkout here. --- .../kollecttarget_scope_webhook_test.go | 59 +++++++++++++++++++ .../webhook/v1alpha1/kollecttarget_webhook.go | 22 ++++--- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/internal/webhook/v1alpha1/kollecttarget_scope_webhook_test.go b/internal/webhook/v1alpha1/kollecttarget_scope_webhook_test.go index 35dcef05..22757d23 100644 --- a/internal/webhook/v1alpha1/kollecttarget_scope_webhook_test.go +++ b/internal/webhook/v1alpha1/kollecttarget_scope_webhook_test.go @@ -81,6 +81,65 @@ func TestKollectTargetValidator_scopeAdmission(t *testing.T) { } } +func TestKollectTargetValidator_scopeAdmissionMissingProfile(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := kollectdevv1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + + scopeObj := &kollectdevv1alpha1.KollectScope{ + ObjectMeta: metav1.ObjectMeta{Name: "team-scope", Namespace: "sec-ops"}, + Spec: kollectdevv1alpha1.KollectScopeSpec{ + ScopeCeilingSpec: kollectdevv1alpha1.ScopeCeilingSpec{ + AllowedNamespaces: []string{"team-a"}, + DeniedNamespaces: []string{"kube-system"}, + AllowedGVKs: []kollectdevv1alpha1.GroupVersionKind{ + {Group: "apps", Version: "v1", Kind: "Deployment"}, + }, + }, + }, + } + + // No KollectProfile object: the Target references one that does not exist yet. + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(scopeObj).Build() + v := &kollectTargetValidator{client: cl} + + base := &kollectdevv1alpha1.KollectTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "pending", Namespace: "sec-ops"}, + Spec: kollectdevv1alpha1.KollectTargetSpec{ + ProfileRef: "not-created-yet", + CollectionFilterSpec: kollectdevv1alpha1.CollectionFilterSpec{ + IncludedNamespaces: []string{"team-a"}, + }, + }, + } + if err := v.validate(context.Background(), base); err != nil { + t.Fatalf("in-scope target must stay admissible before its profile exists: %v", err) + } + + deniedNS := base.DeepCopy() + deniedNS.Spec.IncludedNamespaces = []string{"kube-system"} + if err := v.validate(context.Background(), deniedNS); err == nil { + t.Fatal("expected reject for denied namespace when the profile is missing") + } + + outOfScope := base.DeepCopy() + outOfScope.Spec.IncludedNamespaces = []string{"team-b"} + if err := v.validate(context.Background(), outOfScope); err == nil { + t.Fatal("expected reject for namespace outside allowlist when the profile is missing") + } + + badGVK := base.DeepCopy() + badGVK.Spec.ResourceRules = []kollectdevv1alpha1.ResourceRule{ + {GVK: kollectdevv1alpha1.GroupVersionKind{Group: "batch", Version: "v1", Kind: "Job"}}, + } + if err := v.validate(context.Background(), badGVK); err == nil { + t.Fatal("expected reject for resourceRules GVK outside scope when the profile is missing") + } +} + func TestKollectTargetValidator_validateWatchMode(t *testing.T) { t.Parallel() diff --git a/internal/webhook/v1alpha1/kollecttarget_webhook.go b/internal/webhook/v1alpha1/kollecttarget_webhook.go index d643b025..eaa84b09 100644 --- a/internal/webhook/v1alpha1/kollecttarget_webhook.go +++ b/internal/webhook/v1alpha1/kollecttarget_webhook.go @@ -78,17 +78,23 @@ func (v *kollectTargetValidator) validateScope(ctx context.Context, target *koll var profile kollectdevv1alpha1.KollectProfile profileKey := client.ObjectKey{Namespace: target.Namespace, Name: target.Spec.ProfileRef} - if err := v.client.Get(ctx, profileKey, &profile); err != nil { - if apierrors.IsNotFound(err) { - return nil - } - + profileResolved := true + switch err := v.client.Get(ctx, profileKey, &profile); { + case err == nil: + case apierrors.IsNotFound(err): + // A Target may be applied before its KollectProfile. Only the profile + // targetGVK is unknowable here — it is re-checked at reconcile, which + // degrades with ScopeGVKDenied. Everything spec-derived stays enforced. + profileResolved = false + default: return fmt.Errorf("load KollectProfile: %w", err) } - gvks := scope.CollectRuleGVKs(target.Spec.CollectionFilterSpec, profile.Spec.TargetGVK) - if err := scope.ValidateResourceRuleGVKs(binding.Scope, gvks); err != nil { - return err + if profileResolved || len(target.Spec.ResourceRules) > 0 { + gvks := scope.CollectRuleGVKs(target.Spec.CollectionFilterSpec, profile.Spec.TargetGVK) + if err := scope.ValidateResourceRuleGVKs(binding.Scope, gvks); err != nil { + return err + } } intentNS := scope.NormalizeNamespaceList(target.Spec.IncludedNamespaces)