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