From 8b0dbf002043e6eaa2021b75afc94b39abf70ef2 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sun, 16 Aug 2026 21:59:08 -0700 Subject: [PATCH] :bug: fix(webhook): enforce cluster-scope when the profile is missing Keep namespace and profileRef checks on ClusterTarget admission even when the profile does not exist yet. Re-check allowedGVKs at reconcile once the profile loads. Signed-off-by: Sebastien Tardif --- internal/controller/cluster_scope_enforce.go | 24 +++++-- .../kollectclustertarget_controller.go | 2 +- .../kollectclustertarget_unit_test.go | 62 +++++++++++++++++++ ...tclustertarget_scope_webhook_extra_test.go | 15 +++++ .../v1alpha1/kollectclustertarget_webhook.go | 7 ++- 5 files changed, 104 insertions(+), 6 deletions(-) diff --git a/internal/controller/cluster_scope_enforce.go b/internal/controller/cluster_scope_enforce.go index 31305a95..ea198376 100644 --- a/internal/controller/cluster_scope_enforce.go +++ b/internal/controller/cluster_scope_enforce.go @@ -70,19 +70,35 @@ func (r *KollectClusterTargetReconciler) resolveProfileOrDegrade( func (r *KollectClusterTargetReconciler) loadClusterScopeBinding( ctx context.Context, ct *kollectdevv1alpha1.KollectClusterTarget, + profile *kollectdevv1alpha1.KollectProfile, ) (scope.ClusterBinding, bool, error) { clusterBinding, loadErr := scope.LoadCluster(ctx, r.Client) if loadErr != nil { return scope.ClusterBinding{}, false, loadErr } - if clusterBinding.Enforced { - if scopeErr := scope.ValidateClusterScopeStaticRefNamespace(clusterBinding.Scope, ct.Spec.ProfileRef.Namespace); scopeErr != nil { + if !clusterBinding.Enforced { + return clusterBinding, false, nil + } + + if scopeErr := scope.ValidateClusterScopeStaticRefNamespace(clusterBinding.Scope, ct.Spec.ProfileRef.Namespace); scopeErr != nil { + r.unregisterAll(ct) + recordWarning(r.Recorder, ct, scopeReasonNSDenied, scopeErr.Error()) + if degErr := r.setDegraded(ctx, ct, scopeReasonNSDenied, scopeErr.Error()); degErr != nil { + return clusterBinding, false, degErr + } + + return clusterBinding, true, nil + } + + for _, gvk := range scope.CollectRuleGVKs(ct.Spec.CollectionFilterSpec, profile.Spec.TargetGVK) { + if scopeErr := scope.ValidateClusterScopeGVKs(clusterBinding.Scope, gvk); scopeErr != nil { r.unregisterAll(ct) - recordWarning(r.Recorder, ct, scopeReasonNSDenied, scopeErr.Error()) - if degErr := r.setDegraded(ctx, ct, scopeReasonNSDenied, scopeErr.Error()); degErr != nil { + recordWarning(r.Recorder, ct, scopeReasonGVKDenied, scopeErr.Error()) + if degErr := r.setDegraded(ctx, ct, scopeReasonGVKDenied, scopeErr.Error()); degErr != nil { return clusterBinding, false, degErr } + return clusterBinding, true, nil } } diff --git a/internal/controller/kollectclustertarget_controller.go b/internal/controller/kollectclustertarget_controller.go index 5938285d..0cce13d2 100644 --- a/internal/controller/kollectclustertarget_controller.go +++ b/internal/controller/kollectclustertarget_controller.go @@ -103,7 +103,7 @@ func (r *KollectClusterTargetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - clusterBinding, scopeDegraded, scopeErr := r.loadClusterScopeBinding(ctx, &ct) + clusterBinding, scopeDegraded, scopeErr := r.loadClusterScopeBinding(ctx, &ct, profile) if scopeErr != nil { retErr = scopeErr return ctrl.Result{}, scopeErr diff --git a/internal/controller/kollectclustertarget_unit_test.go b/internal/controller/kollectclustertarget_unit_test.go index 4b7e7631..e22a82d4 100644 --- a/internal/controller/kollectclustertarget_unit_test.go +++ b/internal/controller/kollectclustertarget_unit_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + corev1 "k8s.io/api/core/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -75,3 +76,64 @@ func TestKollectClusterTargetReconciler_suspend(t *testing.T) { t.Fatalf("Degraded = %+v", cond) } } + +func TestKollectClusterTargetReconciler_deniedGVKDegrades(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: "secrets", Namespace: "kollect-system"}, + Spec: kollectdevv1alpha1.KollectProfileSpec{ + TargetGVK: kollectdevv1alpha1.GroupVersionKind{Version: "v1", Kind: "Secret"}, + }, + } + ct := &kollectdevv1alpha1.KollectClusterTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster-secrets", Generation: 1}, + Spec: kollectdevv1alpha1.KollectClusterTargetSpec{ + ProfileRef: kollectdevv1alpha1.NamespacedObjectReference{Name: "secrets", Namespace: "kollect-system"}, + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "platform"}, + }, + }, + } + + 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 134ab703..a0588ce7 100644 --- a/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go +++ b/internal/webhook/v1alpha1/kollectclustertarget_scope_webhook_extra_test.go @@ -78,4 +78,19 @@ func TestKollectClusterTargetValidator_validateClusterScope(t *testing.T) { if err := v.validateClusterScope(context.Background(), badNS); err == nil { t.Fatal("expected denied namespace violation") } + + missingProfile := target.DeepCopy() + missingProfile.Spec.ProfileRef.Name = "does-not-exist" + missingProfile.Spec.CollectionFilterSpec = kollectdevv1alpha1.CollectionFilterSpec{ + IncludedNamespaces: []string{"kube-system"}, + } + if err := v.validateClusterScope(context.Background(), missingProfile); err == nil { + t.Fatal("expected denied namespace violation when the profile is missing") + } + + missingOK := target.DeepCopy() + missingOK.Spec.ProfileRef.Name = "does-not-exist" + if err := v.validateClusterScope(context.Background(), missingOK); err != nil { + t.Fatalf("missing in-scope profile should still admit: %v", err) + } } diff --git a/internal/webhook/v1alpha1/kollectclustertarget_webhook.go b/internal/webhook/v1alpha1/kollectclustertarget_webhook.go index f1e78f2f..c8142d7c 100644 --- a/internal/webhook/v1alpha1/kollectclustertarget_webhook.go +++ b/internal/webhook/v1alpha1/kollectclustertarget_webhook.go @@ -86,7 +86,12 @@ func (v *kollectClusterTargetValidator) validateClusterScope( profile, err := resolveClusterTargetProfileForWebhook(ctx, v.client, target.Spec.ProfileRef) if err != nil { if apierrors.IsNotFound(err) { - return nil + 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) } return err