From 7cd5abe905708e85a7b73ebf25c91a05c231b275 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Thu, 5 Feb 2026 15:17:59 +0100 Subject: [PATCH] feat: Add validation framework with ServiceAccount validator to ClusterExtension Introduces an extensible validation framework that runs early in the ClusterExtension reconciliation pipeline (after finalizer handling, before revision state retrieval) to catch configuration errors before expensive operations begin. Key changes: - New ClusterExtensionValidator type and ValidateClusterExtension reconcile step that executes all validators and collects all errors (no fail-fast behavior) - ServiceAccountValidator checks SA existence via direct CoreV1 API Get call, providing clear "not found" feedback - Sets Installed=Unknown and Progressing=Retrying conditions on validation failure - Removed SA-specific error handling from RetrieveRevisionStates, since validation now catches these errors earlier - Added RBAC permission (get serviceaccounts) to the operator-controller manager ClusterRole and regenerated manifests Testing: - Table-driven unit tests covering 7 scenarios (all pass, single fail, multiple fail, mixed, SA not found, SA found) - E2E scenario for missing ServiceAccount validation error Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Per Goncalves da Silva Signed-off-by: Per G. da Silva --- cmd/operator-controller/main.go | 6 + ...rrole-operator-controller-manager-role.yml | 6 + .../clusterextension_controller_test.go | 215 ++++++++++++++---- .../clusterextension_reconcile_steps.go | 58 ++++- .../controllers/suite_test.go | 7 +- manifests/experimental-e2e.yaml | 6 + manifests/experimental.yaml | 6 + manifests/standard-e2e.yaml | 6 + manifests/standard.yaml | 6 + test/e2e/features/install.feature | 24 ++ 10 files changed, 287 insertions(+), 53 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index dd0bc98f6..759056fb5 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -627,6 +627,9 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl } ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{ controllers.HandleFinalizers(c.finalizers), + controllers.ValidateClusterExtension( + controllers.ServiceAccountValidator(coreClient), + ), controllers.MigrateStorage(storageMigrator), controllers.RetrieveRevisionStates(revisionStatesGetter), controllers.ResolveBundle(c.resolver, c.mgr.GetClient()), @@ -747,6 +750,9 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster revisionStatesGetter := &controllers.HelmRevisionStatesGetter{ActionClientGetter: acg} ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{ controllers.HandleFinalizers(c.finalizers), + controllers.ValidateClusterExtension( + controllers.ServiceAccountValidator(coreClient), + ), controllers.RetrieveRevisionStates(revisionStatesGetter), controllers.ResolveBundle(c.resolver, c.mgr.GetClient()), controllers.UnpackBundle(c.imagePuller, c.imageCache), diff --git a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml index 2049532be..9b942febf 100644 --- a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml +++ b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml @@ -15,6 +15,12 @@ rules: - serviceaccounts/token verbs: - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - get - apiGroups: - apiextensions.k8s.io resources: diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 28d766ace..0c7b4050e 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -15,11 +15,13 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/client-go/kubernetes/fake" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" @@ -29,7 +31,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" @@ -767,60 +768,177 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } -func TestClusterExtensionServiceAccountNotFound(t *testing.T) { - cl, reconciler := newClientAndReconciler(t, func(d *deps) { - d.RevisionStatesGetter = &MockRevisionStatesGetter{ - Err: &authentication.ServiceAccountNotFoundError{ - ServiceAccountName: "missing-sa", - ServiceAccountNamespace: "default", - }} - }) - - ctx := context.Background() - extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - - t.Log("Given a cluster extension with a missing service account") - clusterExtension := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, - Spec: ocv1.ClusterExtensionSpec{ - Source: ocv1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1.CatalogFilter{ - PackageName: "test-package", +func TestValidateClusterExtension(t *testing.T) { + tests := []struct { + name string + validators []controllers.ClusterExtensionValidator + expectError bool + errorMessageIncludes string + }{ + { + name: "all validators pass", + validators: []controllers.ClusterExtensionValidator{ + // Validator that always passes + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil }, }, - Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "missing-sa", + expectError: false, + }, + { + name: "validator fails - sets Progressing condition", + validators: []controllers.ClusterExtensionValidator{ + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("generic validation error") + }, + }, + expectError: true, + errorMessageIncludes: "generic validation error", + }, + { + name: "multiple validators - collects all failures", + validators: []controllers.ClusterExtensionValidator{ + // First validator fails + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("first validator failed") + }, + // Second validator also fails + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("second validator failed") + }, + // Third validator fails too + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("third validator failed") + }, + }, + expectError: true, + errorMessageIncludes: "first validator failed\nsecond validator failed\nthird validator failed", + }, + { + name: "multiple validators - all pass", + validators: []controllers.ClusterExtensionValidator{ + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil + }, + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil + }, + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil + }, + }, + expectError: false, + }, + { + name: "multiple validators - some pass, some fail", + validators: []controllers.ClusterExtensionValidator{ + // First validator passes + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil + }, + // Second validator fails + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("validation error 1") + }, + // Third validator passes + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return nil + }, + // Fourth validator fails + func(_ context.Context, _ *ocv1.ClusterExtension) error { + return errors.New("validation error 2") + }, + }, + expectError: true, + errorMessageIncludes: "validation error 1\nvalidation error 2", + }, + { + name: "service account not found", + validators: []controllers.ClusterExtensionValidator{ + serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-sa", + Namespace: "test-ns", + }, + }), + }, + expectError: true, + errorMessageIncludes: `service account "test-sa" not found in namespace "test-ns"`, + }, + { + name: "service account found", + validators: []controllers.ClusterExtensionValidator{ + serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sa", + Namespace: "test-ns", + }, + }), }, + expectError: false, }, } - require.NoError(t, cl.Create(ctx, clusterExtension)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() - t.Log("When reconciling the cluster extension") - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + cl, reconciler := newClientAndReconciler(t, func(d *deps) { + d.RevisionStatesGetter = &MockRevisionStatesGetter{ + RevisionStates: &controllers.RevisionStates{}, + } + d.Validators = tt.validators + }) - require.Equal(t, ctrl.Result{}, res) - require.Error(t, err) - var saErr *authentication.ServiceAccountNotFoundError - require.ErrorAs(t, err, &saErr) - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - t.Log("By checking the status conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionUnknown, installedCond.Status) - require.Contains(t, installedCond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.", - "missing-sa", "default")) + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "test-package", + }, + }, + Namespace: "test-ns", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "test-sa", + }, + }, + } - progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason) - require.Contains(t, progressingCond.Message, "installation cannot proceed due to missing ServiceAccount") - require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) + require.NoError(t, cl.Create(ctx, clusterExtension)) + + t.Log("When reconciling the cluster extension") + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + if tt.expectError { + require.Error(t, err) + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + t.Log("By checking the status conditions") + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) + require.NotNil(t, installedCond) + require.Equal(t, metav1.ConditionUnknown, installedCond.Status) + require.Contains(t, installedCond.Message, "installation cannot proceed due to the following validation error(s)") + require.Contains(t, installedCond.Message, tt.errorMessageIncludes) + + progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) + require.NotNil(t, progressingCond) + require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason) + require.Contains(t, progressingCond.Message, "installation cannot proceed due to the following validation error(s)") + require.Contains(t, progressingCond.Message, tt.errorMessageIncludes) + } else { + require.NoError(t, err) + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + require.Empty(t, clusterExtension.Status.Conditions) + } + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) + }) + } } func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { @@ -2878,3 +2996,10 @@ func TestCheckCatalogsExist(t *testing.T) { require.False(t, exists) }) } + +func serviceAccountValidatorWithFakeClient(serviceAccount *corev1.ServiceAccount) controllers.ClusterExtensionValidator { + if serviceAccount == nil { + return controllers.ServiceAccountValidator(fake.NewClientset().CoreV1()) + } + return controllers.ServiceAccountValidator(fake.NewClientset(serviceAccount).CoreV1()) +} diff --git a/internal/operator-controller/controllers/clusterextension_reconcile_steps.go b/internal/operator-controller/controllers/clusterextension_reconcile_steps.go index 16d691f8b..44e71a717 100644 --- a/internal/operator-controller/controllers/clusterextension_reconcile_steps.go +++ b/internal/operator-controller/controllers/clusterextension_reconcile_steps.go @@ -21,15 +21,16 @@ import ( "errors" "fmt" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/log" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" "github.com/operator-framework/operator-controller/internal/operator-controller/resolve" @@ -63,6 +64,55 @@ func HandleFinalizers(f finalizer.Finalizer) ReconcileStepFunc { } } +// ClusterExtensionValidator is a function that validates a ClusterExtension. +// It returns an error if validation fails. Validators are executed sequentially +// in the order they are registered. +type ClusterExtensionValidator func(context.Context, *ocv1.ClusterExtension) error + +// ValidateClusterExtension returns a ReconcileStepFunc that executes all +// validators sequentially. All validators are executed even if some fail, +// and all errors are collected and returned as a joined error. +// This provides complete validation feedback in a single reconciliation cycle. +func ValidateClusterExtension(validators ...ClusterExtensionValidator) ReconcileStepFunc { + return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) { + l := log.FromContext(ctx) + + l.Info("validating cluster extension") + var validationErrors []error + for _, validator := range validators { + if err := validator(ctx, ext); err != nil { + validationErrors = append(validationErrors, err) + } + } + + // If there are no validation errors, continue reconciliation + if len(validationErrors) == 0 { + return nil, nil + } + + // Set status condition with the validation error for other validation failures + err := fmt.Errorf("installation cannot proceed due to the following validation error(s): %w", errors.Join(validationErrors...)) + setInstalledStatusConditionUnknown(ext, err.Error()) + setStatusProgressing(ext, err) + return nil, err + } +} + +// ServiceAccountValidator returns a validator that checks if the specified +// ServiceAccount exists in the cluster by performing a direct Get call. +func ServiceAccountValidator(saClient corev1client.ServiceAccountsGetter) ClusterExtensionValidator { + return func(ctx context.Context, ext *ocv1.ClusterExtension) error { + _, err := saClient.ServiceAccounts(ext.Spec.Namespace).Get(ctx, ext.Spec.ServiceAccount.Name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return fmt.Errorf("service account %q not found in namespace %q", ext.Spec.ServiceAccount.Name, ext.Spec.Namespace) + } + return fmt.Errorf("error validating service account: %w", err) + } + return nil + } +} + func RetrieveRevisionStates(r RevisionStatesGetter) ReconcileStepFunc { return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) { l := log.FromContext(ctx) @@ -70,12 +120,6 @@ func RetrieveRevisionStates(r RevisionStatesGetter) ReconcileStepFunc { revisionStates, err := r.GetRevisionStates(ctx, ext) if err != nil { setInstallStatus(ext, nil) - var saerr *authentication.ServiceAccountNotFoundError - if errors.As(err, &saerr) { - setInstalledStatusConditionUnknown(ext, saerr.Error()) - setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) - return nil, err - } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) return nil, err diff --git a/internal/operator-controller/controllers/suite_test.go b/internal/operator-controller/controllers/suite_test.go index e60d231f3..109f0b66c 100644 --- a/internal/operator-controller/controllers/suite_test.go +++ b/internal/operator-controller/controllers/suite_test.go @@ -88,6 +88,7 @@ type deps struct { ImagePuller image.Puller ImageCache image.Cache Applier controllers.Applier + Validators []controllers.ClusterExtensionValidator } func newClientAndReconciler(t *testing.T, opts ...reconcilerOption) (client.Client, *controllers.ClusterExtensionReconciler) { @@ -105,7 +106,11 @@ func newClientAndReconciler(t *testing.T, opts ...reconcilerOption) (client.Clie for _, opt := range opts { opt(d) } - reconciler.ReconcileSteps = []controllers.ReconcileStepFunc{controllers.HandleFinalizers(d.Finalizers), controllers.RetrieveRevisionStates(d.RevisionStatesGetter)} + reconciler.ReconcileSteps = []controllers.ReconcileStepFunc{ + controllers.HandleFinalizers(d.Finalizers), + controllers.ValidateClusterExtension(d.Validators...), + controllers.RetrieveRevisionStates(d.RevisionStatesGetter), + } if r := d.Resolver; r != nil { reconciler.ReconcileSteps = append(reconciler.ReconcileSteps, controllers.ResolveBundle(r, cl)) } diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index c6e370cda..ed2b467b8 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1777,6 +1777,12 @@ rules: - serviceaccounts/token verbs: - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - get - apiGroups: - apiextensions.k8s.io resources: diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 46ca67c91..51791d70d 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1738,6 +1738,12 @@ rules: - serviceaccounts/token verbs: - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - get - apiGroups: - apiextensions.k8s.io resources: diff --git a/manifests/standard-e2e.yaml b/manifests/standard-e2e.yaml index 3ae2938d8..34679e1cb 100644 --- a/manifests/standard-e2e.yaml +++ b/manifests/standard-e2e.yaml @@ -1383,6 +1383,12 @@ rules: - serviceaccounts/token verbs: - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - get - apiGroups: - apiextensions.k8s.io resources: diff --git a/manifests/standard.yaml b/manifests/standard.yaml index 2fc75569c..819f07c40 100644 --- a/manifests/standard.yaml +++ b/manifests/standard.yaml @@ -1344,6 +1344,12 @@ rules: - serviceaccounts/token verbs: - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: + - get - apiGroups: - apiextensions.k8s.io resources: diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index 23f75c546..51bebe3a4 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -89,6 +89,30 @@ Feature: Install ClusterExtension found bundles for package "test" in multiple catalogs with the same priority [extra-catalog test-catalog] """ + Scenario: Report validation error when ServiceAccount does not exist + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: non-existent-sa + source: + sourceType: Catalog + catalog: + packageName: test + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": test-catalog + """ + Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + installation cannot proceed due to the following validation error(s): service account "non-existent-sa" not found in namespace "${TEST_NAMESPACE}" + """ + @SingleOwnNamespaceInstallSupport Scenario: watchNamespace config is required for extension supporting single namespace Given ServiceAccount "olm-admin" in test namespace is cluster admin