From cea1d533b9bb02ff59fa39da838e9349fc75c4a5 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:21:50 +0000 Subject: [PATCH 1/7] Make config errors permanent instead of retrying When configuration is invalid, mark it as a permanent error so the system doesn't keep trying to install it over and over. This saves resources and makes it clear the user needs to fix their config. Assisted-by: Claude --- internal/operator-controller/applier/provider.go | 3 ++- .../controllers/clusterextension_controller.go | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/operator-controller/applier/provider.go b/internal/operator-controller/applier/provider.go index cf75b28c87..2bcd1cdbd2 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -9,6 +9,7 @@ import ( "helm.sh/helm/v3/pkg/chart" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -77,7 +78,7 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens bundleConfigBytes := extensionConfigBytes(ext) bundleConfig, err := config.UnmarshalConfig(bundleConfigBytes, schema, ext.Spec.Namespace) if err != nil { - return nil, fmt.Errorf("invalid ClusterExtension configuration: %w", err) + return nil, reconcile.TerminalError(fmt.Errorf("invalid ClusterExtension configuration: %w", err)) } if watchNS := bundleConfig.GetWatchNamespace(); watchNS != nil { diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index f089efcacb..d280eb238c 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -455,7 +455,12 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ... } func wrapErrorWithResolutionInfo(resolved ocv1.BundleMetadata, err error) error { - return fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, err) + wrappedErr := fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, err) + // Preserve TerminalError type if the original error was terminal + if errors.Is(err, reconcile.TerminalError(nil)) { + return reconcile.TerminalError(wrappedErr) + } + return wrappedErr } // Generate reconcile requests for all cluster extensions affected by a catalog change From 86ba3ae3ec77b0ea629fe5f25571e7c2052822d1 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:21:57 +0000 Subject: [PATCH 2/7] Show "Failed" status when config is wrong Before: Invalid config showed status as "Absent" which was confusing After: Shows "Failed" so users know something is wrong Only checks the latest attempt, not old ones that don't matter anymore. Assisted-by: Claude --- .../controllers/common_controller.go | 46 ++++- .../controllers/common_controller_test.go | 157 ++++++++++++++++++ 2 files changed, 198 insertions(+), 5 deletions(-) diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index 7fafc7bb7b..4c337f614f 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -56,11 +56,8 @@ func setInstalledStatusFromRevisionStates(ext *ocv1.ClusterExtension, revisionSt // Nothing is installed if revisionStates.Installed == nil { setInstallStatus(ext, nil) - if len(revisionStates.RollingOut) == 0 { - setInstalledStatusConditionFalse(ext, ocv1.ReasonFailed, "No bundle installed") - } else { - setInstalledStatusConditionFalse(ext, ocv1.ReasonAbsent, "No bundle installed") - } + reason := determineFailureReason(revisionStates.RollingOut) + setInstalledStatusConditionFalse(ext, reason, "No bundle installed") return } // Something is installed @@ -71,6 +68,45 @@ func setInstalledStatusFromRevisionStates(ext *ocv1.ClusterExtension, revisionSt setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", revisionStates.Installed.Image)) } +// determineFailureReason determines the appropriate reason for the Installed condition +// when no bundle is installed (Installed: False). +// +// Returns Failed when: +// - No rolling revisions exist (nothing to install) +// - The latest rolling revision has Reason: Retrying (indicates an error occurred) +// +// Returns Absent when: +// - Rolling revisions exist with the latest having Reason: RollingOut (healthy phased rollout in progress) +// +// Rationale: +// - Failed: Semantically indicates an error prevented installation +// - Absent: Semantically indicates "not there yet" (neutral state, e.g., during healthy rollout) +// - Retrying reason indicates an error (config validation, apply failure, etc.) +// - RollingOut reason indicates healthy progress (not an error) +// - Only the LATEST revision matters - old errors superseded by newer healthy revisions should not cause Failed +// +// Note: rollingRevisions are sorted in ascending order by Spec.Revision (oldest to newest), +// so the latest revision is the LAST element in the array. +func determineFailureReason(rollingRevisions []*RevisionMetadata) string { + if len(rollingRevisions) == 0 { + return ocv1.ReasonFailed + } + + // Check if the LATEST rolling revision indicates an error (Retrying reason) + // Latest revision is the last element in the array (sorted ascending by Spec.Revision) + latestRevision := rollingRevisions[len(rollingRevisions)-1] + progressingCond := apimeta.FindStatusCondition(latestRevision.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing) + if progressingCond != nil && progressingCond.Reason == string(ocv1.ClusterExtensionRevisionReasonRetrying) { + // Retrying indicates an error occurred (config, apply, validation, etc.) + // Use Failed for semantic correctness: installation failed due to error + return ocv1.ReasonFailed + } + + // No error detected in latest revision - it's progressing healthily (RollingOut) or no conditions set + // Use Absent for neutral "not installed yet" state + return ocv1.ReasonAbsent +} + // setInstalledStatusConditionSuccess sets the installed status condition to success. func setInstalledStatusConditionSuccess(ext *ocv1.ClusterExtension, message string) { SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 93fad962e8..06f4b89a23 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -240,3 +240,160 @@ func TestSetStatusConditionWrapper(t *testing.T) { }) } } + +func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T) { + tests := []struct { + name string + revisionStates *RevisionStates + expectedInstalledCond metav1.Condition + }{ + { + name: "no revisions at all - uses Failed", + revisionStates: &RevisionStates{ + Installed: nil, + RollingOut: nil, + }, + expectedInstalledCond: metav1.Condition{ + Type: ocv1.TypeInstalled, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonFailed, + }, + }, + { + name: "rolling revision with error (Retrying) - uses Failed", + revisionStates: &RevisionStates{ + Installed: nil, + RollingOut: []*RevisionMetadata{ + { + RevisionName: "rev-1", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ClusterExtensionRevisionReasonRetrying, + Message: "some error occurred", + }, + }, + }, + }, + }, + expectedInstalledCond: metav1.Condition{ + Type: ocv1.TypeInstalled, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonFailed, + }, + }, + { + name: "multiple rolling revisions with one Retrying - uses Failed", + revisionStates: &RevisionStates{ + Installed: nil, + RollingOut: []*RevisionMetadata{ + { + RevisionName: "rev-1", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonRollingOut, + Message: "Revision is rolling out", + }, + }, + }, + { + RevisionName: "rev-2", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ClusterExtensionRevisionReasonRetrying, + Message: "validation error occurred", + }, + }, + }, + }, + }, + expectedInstalledCond: metav1.Condition{ + Type: ocv1.TypeInstalled, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonFailed, + }, + }, + { + name: "rolling revision with RollingOut reason - uses Absent", + revisionStates: &RevisionStates{ + Installed: nil, + RollingOut: []*RevisionMetadata{ + { + RevisionName: "rev-1", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonRollingOut, + Message: "Revision is rolling out", + }, + }, + }, + }, + }, + expectedInstalledCond: metav1.Condition{ + Type: ocv1.TypeInstalled, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonAbsent, + }, + }, + { + name: "old revision with Retrying superseded by latest healthy - uses Absent", + revisionStates: &RevisionStates{ + Installed: nil, + RollingOut: []*RevisionMetadata{ + { + RevisionName: "rev-1", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ClusterExtensionRevisionReasonRetrying, + Message: "old error that was superseded", + }, + }, + }, + { + RevisionName: "rev-2", + Conditions: []metav1.Condition{ + { + Type: ocv1.ClusterExtensionRevisionTypeProgressing, + Status: metav1.ConditionTrue, + Reason: ocv1.ReasonRollingOut, + Message: "Latest revision is rolling out healthy", + }, + }, + }, + }, + }, + expectedInstalledCond: metav1.Condition{ + Type: ocv1.TypeInstalled, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonAbsent, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + Generation: 1, + }, + } + + setInstalledStatusFromRevisionStates(ext, tt.revisionStates) + + cond := meta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled) + require.NotNil(t, cond) + require.Equal(t, tt.expectedInstalledCond.Status, cond.Status) + require.Equal(t, tt.expectedInstalledCond.Reason, cond.Reason) + }) + } +} From 5304ad7cb32cf889515d17ffe32f8dd9c57251e6 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:22:02 +0000 Subject: [PATCH 3/7] Fix status not updating when BOXCUTTER config fails BOXCUTTER runtime was returning early on config errors without setting the status. Now it updates the status first so users can see what went wrong. Assisted-by: Claude --- .../controllers/boxcutter_reconcile_steps.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 5c746c7b0f..026c72426b 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -114,6 +114,8 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e // If there was an error applying the resolved bundle, // report the error via the Progressing condition. setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err)) + // Set the Installed condition based on revision states even when there's an error + setInstalledStatusFromRevisionStates(ext, state.revisionStates) return nil, err } From 8f42220f602e9fc70bd5512d8c704e5824588c28 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:22:14 +0000 Subject: [PATCH 4/7] Add tests for config error handling Tests check: - Config errors are permanent (not retried) - Status shows "Failed" when config is wrong - Namespace names are validated - AllNamespaces operators reject watchNamespace config Includes both unit tests and end-to-end tests. Assisted-by: Claude --- .../applier/provider_test.go | 32 +++++++ test/e2e/features/install.feature | 96 ++++++++++++++++++- 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/internal/operator-controller/applier/provider_test.go b/internal/operator-controller/applier/provider_test.go index 4138284a05..49a7c653d9 100644 --- a/internal/operator-controller/applier/provider_test.go +++ b/internal/operator-controller/applier/provider_test.go @@ -11,6 +11,7 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -100,6 +101,37 @@ func Test_RegistryV1ManifestProvider_Integration(t *testing.T) { require.Contains(t, err.Error(), "invalid ClusterExtension configuration") }) + t.Run("returns terminal error for invalid config", func(t *testing.T) { + provider := applier.RegistryV1ManifestProvider{ + BundleRenderer: render.BundleRenderer{ + ResourceGenerators: []render.ResourceGenerator{ + func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) { + return nil, nil + }, + }, + }, + IsSingleOwnNamespaceEnabled: true, + } + + // Bundle with SingleNamespace install mode requiring watchNamespace config + bundleFS := bundlefs.Builder().WithPackageName("test"). + WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build() + + // ClusterExtension without required config + ext := &ocv1.ClusterExtension{ + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "install-namespace", + // No config provided - should fail validation + }, + } + + _, err := provider.Get(bundleFS, ext) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid ClusterExtension configuration") + // Assert that config validation errors are terminal (not retriable) + require.ErrorIs(t, err, reconcile.TerminalError(nil), "config validation errors should be terminal") + }) + t.Run("returns rendered manifests", func(t *testing.T) { provider := applier.RegistryV1ManifestProvider{ BundleRenderer: registryv1.Renderer, diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index ab87b5f31c..605f2ba6a8 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -117,11 +117,15 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as True with Reason Retrying and Message: + And ClusterExtension reports Progressing as False with Reason Blocked and Message: """ error for resolved bundle "single-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: required field "watchNamespace" is missing """ + And ClusterExtension reports Installed as False with Reason Failed and Message: + """ + No bundle installed + """ When ClusterExtension is updated to set config.watchNamespace field """ apiVersion: olm.operatorframework.io/v1 @@ -169,12 +173,16 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as True with Reason Retrying and Message: + And ClusterExtension reports Progressing as False with Reason Blocked and Message: """ error for resolved bundle "own-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: required field "watchNamespace" is missing """ + And ClusterExtension reports Installed as False with Reason Failed and Message: + """ + No bundle installed + """ And ClusterExtension is updated to include the watchNamespace configuration """ apiVersion: olm.operatorframework.io/v1 @@ -197,7 +205,7 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as True with Reason Retrying and Message: + And ClusterExtension reports Progressing as False with Reason Blocked and Message: """ error for resolved bundle "own-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: 'some-ns' @@ -205,6 +213,10 @@ Feature: Install ClusterExtension must be "${TEST_NAMESPACE}" (the namespace where the operator is installed) because this operator only supports OwnNamespace install mode """ + And ClusterExtension reports Installed as False with Reason Failed and Message: + """ + No bundle installed + """ When ClusterExtension is updated to set watchNamespace to own namespace value """ apiVersion: olm.operatorframework.io/v1 @@ -231,6 +243,84 @@ Feature: Install ClusterExtension And ClusterExtension is available And operator "own-namespace-operator" target namespace is "${TEST_NAMESPACE}" + @SingleOwnNamespaceInstallSupport + Scenario: Reject invalid watchNamespace format (DNS-1123 violation) + Given ServiceAccount "olm-admin" in test namespace is cluster admin + And resource is applied + """ + apiVersion: v1 + kind: Namespace + metadata: + name: single-namespace-operator-target + """ + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-admin + config: + configType: Inline + inline: + watchNamespace: ${TEST_NAMESPACE}- # trailing '-' violates DNS-1123 + source: + sourceType: Catalog + catalog: + packageName: single-namespace-operator + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": test-catalog + """ + Then ClusterExtension reports Progressing as False with Reason Blocked and Message includes: + """ + invalid ClusterExtension configuration: invalid configuration: invalid namespace name + """ + And ClusterExtension reports Installed as False with Reason Failed and Message: + """ + No bundle installed + """ + + @SingleOwnNamespaceInstallSupport + @WebhookProviderCertManager + Scenario: Reject watchNamespace for operator that does not support Single/OwnNamespace install modes + Given ServiceAccount "olm-admin" in test namespace is cluster admin + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-admin + config: + configType: Inline + inline: + watchNamespace: ${TEST_NAMESPACE} + source: + sourceType: Catalog + catalog: + packageName: webhook-operator + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": test-catalog + """ + Then ClusterExtension reports Progressing as False with Reason Blocked and Message includes: + """ + error for resolved bundle "webhook-operator.1.0.0" with version "1.0.0": + invalid ClusterExtension configuration: invalid configuration: + additionalProperties 'watchNamespace' not allowed + """ + And ClusterExtension reports Installed as False with Reason Failed and Message: + """ + No bundle installed + """ + @WebhookProviderCertManager Scenario: Install operator having webhooks Given ServiceAccount "olm-admin" in test namespace is cluster admin From f805c0532f6176702c1d04bd90606744cedbf15d Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Thu, 29 Jan 2026 15:07:32 +0000 Subject: [PATCH 5/7] Remove 'terminal error:' prefix from status messages When TerminalError is used, unwrap it before showing the error message in status conditions. This prevents the 'terminal error:' prefix from appearing in user-facing messages. Changes: - Add UnwrapTerminal() helper function to cleanly unwrap terminal errors - Use the helper in setStatusProgressing and wrapErrorWithResolutionInfo - Don't set Installed condition when Progressing is Blocked (terminal error) since the Progressing condition already provides all necessary information Co-Authored-By: Claude Sonnet 4.5 --- .../controllers/boxcutter_reconcile_steps.go | 10 ++- .../clusterextension_controller.go | 9 +- .../controllers/common_controller.go | 4 +- .../controllers/common_controller_test.go | 2 +- internal/shared/util/error/terminal.go | 25 +++++- test/e2e/features/install.feature | 90 ------------------- 6 files changed, 42 insertions(+), 98 deletions(-) diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 026c72426b..f92ee28f49 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -19,6 +19,7 @@ package controllers import ( "cmp" "context" + "errors" "fmt" "io/fs" "slices" @@ -27,6 +28,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" @@ -114,8 +116,12 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e // If there was an error applying the resolved bundle, // report the error via the Progressing condition. setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err)) - // Set the Installed condition based on revision states even when there's an error - setInstalledStatusFromRevisionStates(ext, state.revisionStates) + // Only set Installed condition for retryable errors. + // For terminal errors (Progressing: False/Blocked), the Progressing condition + // already provides all necessary information about the failure. + if !errors.Is(err, reconcile.TerminalError(nil)) { + setInstalledStatusFromRevisionStates(ext, state.revisionStates) + } return nil, err } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index d280eb238c..fe8489e555 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -50,6 +50,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" + ocerror "github.com/operator-framework/operator-controller/internal/shared/util/error" k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s" ) @@ -455,12 +456,14 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ... } func wrapErrorWithResolutionInfo(resolved ocv1.BundleMetadata, err error) error { - wrappedErr := fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, err) - // Preserve TerminalError type if the original error was terminal + // Preserve TerminalError type through wrapping by re-wrapping after adding context if errors.Is(err, reconcile.TerminalError(nil)) { + // Unwrap to get the inner error, add context, then re-wrap as TerminalError + innerErr := ocerror.UnwrapTerminal(err) + wrappedErr := fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, innerErr) return reconcile.TerminalError(wrappedErr) } - return wrappedErr + return fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, err) } // Generate reconcile requests for all cluster extensions affected by a catalog change diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index 4c337f614f..a25490e5d6 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -25,6 +25,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" + ocerror "github.com/operator-framework/operator-controller/internal/shared/util/error" ) const ( @@ -155,7 +156,8 @@ func setStatusProgressing(ext *ocv1.ClusterExtension, err error) { if err != nil { progressingCond.Reason = ocv1.ReasonRetrying - progressingCond.Message = err.Error() + // Unwrap TerminalError to avoid "terminal error:" prefix in message + progressingCond.Message = ocerror.UnwrapTerminal(err).Error() } if errors.Is(err, reconcile.TerminalError(nil)) { diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index 06f4b89a23..dd4cbeada0 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -53,7 +53,7 @@ func TestSetStatusProgressing(t *testing.T) { Type: ocv1.TypeProgressing, Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, - Message: "terminal error: boom", + Message: "boom", }, }, } { diff --git a/internal/shared/util/error/terminal.go b/internal/shared/util/error/terminal.go index cd70d535f7..ecb0353522 100644 --- a/internal/shared/util/error/terminal.go +++ b/internal/shared/util/error/terminal.go @@ -1,6 +1,10 @@ package error -import "sigs.k8s.io/controller-runtime/pkg/reconcile" +import ( + "errors" + + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) func WrapTerminal(err error, isTerminal bool) error { if !isTerminal || err == nil { @@ -8,3 +12,22 @@ func WrapTerminal(err error, isTerminal bool) error { } return reconcile.TerminalError(err) } + +// UnwrapTerminal unwraps a TerminalError to get the underlying error without +// the "terminal error:" prefix that reconcile.TerminalError adds to the message. +// This is useful when displaying error messages in status conditions where the +// terminal/blocked nature is already conveyed by the condition Reason field. +// +// If err is not a TerminalError, it returns err unchanged. +// If err is nil, it returns nil. +func UnwrapTerminal(err error) error { + if err == nil { + return nil + } + if errors.Is(err, reconcile.TerminalError(nil)) { + if unwrapped := errors.Unwrap(err); unwrapped != nil { + return unwrapped + } + } + return err +} diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index 605f2ba6a8..91c2af62e8 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -122,10 +122,6 @@ Feature: Install ClusterExtension error for resolved bundle "single-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: required field "watchNamespace" is missing """ - And ClusterExtension reports Installed as False with Reason Failed and Message: - """ - No bundle installed - """ When ClusterExtension is updated to set config.watchNamespace field """ apiVersion: olm.operatorframework.io/v1 @@ -179,10 +175,6 @@ Feature: Install ClusterExtension "1.0.0": invalid ClusterExtension configuration: invalid configuration: required field "watchNamespace" is missing """ - And ClusterExtension reports Installed as False with Reason Failed and Message: - """ - No bundle installed - """ And ClusterExtension is updated to include the watchNamespace configuration """ apiVersion: olm.operatorframework.io/v1 @@ -213,10 +205,6 @@ Feature: Install ClusterExtension must be "${TEST_NAMESPACE}" (the namespace where the operator is installed) because this operator only supports OwnNamespace install mode """ - And ClusterExtension reports Installed as False with Reason Failed and Message: - """ - No bundle installed - """ When ClusterExtension is updated to set watchNamespace to own namespace value """ apiVersion: olm.operatorframework.io/v1 @@ -243,84 +231,6 @@ Feature: Install ClusterExtension And ClusterExtension is available And operator "own-namespace-operator" target namespace is "${TEST_NAMESPACE}" - @SingleOwnNamespaceInstallSupport - Scenario: Reject invalid watchNamespace format (DNS-1123 violation) - Given ServiceAccount "olm-admin" in test namespace is cluster admin - And resource is applied - """ - apiVersion: v1 - kind: Namespace - metadata: - name: single-namespace-operator-target - """ - When ClusterExtension is applied - """ - apiVersion: olm.operatorframework.io/v1 - kind: ClusterExtension - metadata: - name: ${NAME} - spec: - namespace: ${TEST_NAMESPACE} - serviceAccount: - name: olm-admin - config: - configType: Inline - inline: - watchNamespace: ${TEST_NAMESPACE}- # trailing '-' violates DNS-1123 - source: - sourceType: Catalog - catalog: - packageName: single-namespace-operator - selector: - matchLabels: - "olm.operatorframework.io/metadata.name": test-catalog - """ - Then ClusterExtension reports Progressing as False with Reason Blocked and Message includes: - """ - invalid ClusterExtension configuration: invalid configuration: invalid namespace name - """ - And ClusterExtension reports Installed as False with Reason Failed and Message: - """ - No bundle installed - """ - - @SingleOwnNamespaceInstallSupport - @WebhookProviderCertManager - Scenario: Reject watchNamespace for operator that does not support Single/OwnNamespace install modes - Given ServiceAccount "olm-admin" in test namespace is cluster admin - When ClusterExtension is applied - """ - apiVersion: olm.operatorframework.io/v1 - kind: ClusterExtension - metadata: - name: ${NAME} - spec: - namespace: ${TEST_NAMESPACE} - serviceAccount: - name: olm-admin - config: - configType: Inline - inline: - watchNamespace: ${TEST_NAMESPACE} - source: - sourceType: Catalog - catalog: - packageName: webhook-operator - selector: - matchLabels: - "olm.operatorframework.io/metadata.name": test-catalog - """ - Then ClusterExtension reports Progressing as False with Reason Blocked and Message includes: - """ - error for resolved bundle "webhook-operator.1.0.0" with version "1.0.0": - invalid ClusterExtension configuration: invalid configuration: - additionalProperties 'watchNamespace' not allowed - """ - And ClusterExtension reports Installed as False with Reason Failed and Message: - """ - No bundle installed - """ - @WebhookProviderCertManager Scenario: Install operator having webhooks Given ServiceAccount "olm-admin" in test namespace is cluster admin From 511c8c1d52043bae49183c6d59a9d2e400e4875e Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 30 Jan 2026 10:45:33 +0000 Subject: [PATCH 6/7] Add custom TerminalError with granular Reason support Previously, all terminal errors used the generic "Blocked" reason in the Progressing condition. This wasn't semantically correct - "Blocked" was intended for external blockers like resource collisions, not for user configuration errors. This commit introduces a custom TerminalError type that can carry a specific Reason field, allowing more precise categorization of terminal errors in status conditions. Changes: - Add ReasonInvalidConfiguration constant for configuration errors - Create NewTerminalError(reason, err) to wrap errors with a reason - Add ExtractTerminalReason(err) to retrieve the reason from errors - Update setStatusProgressing to use extracted reason when available - Fall back to ReasonBlocked for backward compatibility with plain reconcile.TerminalError() usage - Update config validation to use ReasonInvalidConfiguration - Update tests and e2e expectations accordingly Result: - Config errors now show: Progressing: False, Reason: InvalidConfiguration - Generic terminal errors show: Progressing: False, Reason: Blocked - More helpful and semantically correct status reporting Co-Authored-By: Claude Sonnet 4.5 --- api/v1/common_types.go | 7 +-- .../operator-controller/applier/provider.go | 4 +- .../conditionsets/conditionsets.go | 1 + .../clusterextension_controller.go | 14 +++-- .../controllers/common_controller.go | 13 +++-- .../controllers/common_controller_test.go | 14 ++++- internal/shared/util/error/terminal.go | 51 +++++++++++++++++++ test/e2e/features/install.feature | 6 +-- 8 files changed, 94 insertions(+), 16 deletions(-) diff --git a/api/v1/common_types.go b/api/v1/common_types.go index 863336e0ca..53215dbdee 100644 --- a/api/v1/common_types.go +++ b/api/v1/common_types.go @@ -24,9 +24,10 @@ const ( ReasonAbsent = "Absent" // Progressing reasons - ReasonRollingOut = "RollingOut" - ReasonRetrying = "Retrying" - ReasonBlocked = "Blocked" + ReasonRollingOut = "RollingOut" + ReasonRetrying = "Retrying" + ReasonBlocked = "Blocked" + ReasonInvalidConfiguration = "InvalidConfiguration" // Deprecation reasons ReasonDeprecated = "Deprecated" diff --git a/internal/operator-controller/applier/provider.go b/internal/operator-controller/applier/provider.go index 2bcd1cdbd2..ab4e728e7f 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -9,7 +9,6 @@ import ( "helm.sh/helm/v3/pkg/chart" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -17,6 +16,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/config" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" ) // ManifestProvider returns the manifests that should be applied by OLM given a bundle and its associated ClusterExtension @@ -78,7 +78,7 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens bundleConfigBytes := extensionConfigBytes(ext) bundleConfig, err := config.UnmarshalConfig(bundleConfigBytes, schema, ext.Spec.Namespace) if err != nil { - return nil, reconcile.TerminalError(fmt.Errorf("invalid ClusterExtension configuration: %w", err)) + return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid ClusterExtension configuration: %w", err)) } if watchNS := bundleConfig.GetWatchNamespace(); watchNS != nil { diff --git a/internal/operator-controller/conditionsets/conditionsets.go b/internal/operator-controller/conditionsets/conditionsets.go index 0a741ed8d3..97073a02d8 100644 --- a/internal/operator-controller/conditionsets/conditionsets.go +++ b/internal/operator-controller/conditionsets/conditionsets.go @@ -40,6 +40,7 @@ var ConditionReasons = []string{ ocv1.ReasonDeprecationStatusUnknown, ocv1.ReasonFailed, ocv1.ReasonBlocked, + ocv1.ReasonInvalidConfiguration, ocv1.ReasonRetrying, ocv1.ReasonAbsent, ocv1.ReasonRollingOut, diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index fe8489e555..a3fcf48f3f 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -50,7 +50,7 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" - ocerror "github.com/operator-framework/operator-controller/internal/shared/util/error" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s" ) @@ -456,11 +456,17 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ... } func wrapErrorWithResolutionInfo(resolved ocv1.BundleMetadata, err error) error { - // Preserve TerminalError type through wrapping by re-wrapping after adding context + // Preserve TerminalError type and reason through wrapping if errors.Is(err, reconcile.TerminalError(nil)) { - // Unwrap to get the inner error, add context, then re-wrap as TerminalError - innerErr := ocerror.UnwrapTerminal(err) + // Extract the reason if one was provided + reason, hasReason := errorutil.ExtractTerminalReason(err) + // Unwrap to get the inner error, add context + innerErr := errorutil.UnwrapTerminal(err) wrappedErr := fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, innerErr) + // Re-wrap preserving the reason if it existed + if hasReason { + return errorutil.NewTerminalError(reason, wrappedErr) + } return reconcile.TerminalError(wrappedErr) } return fmt.Errorf("error for resolved bundle %q with version %q: %w", resolved.Name, resolved.Version, err) diff --git a/internal/operator-controller/controllers/common_controller.go b/internal/operator-controller/controllers/common_controller.go index a25490e5d6..cb6834c6b6 100644 --- a/internal/operator-controller/controllers/common_controller.go +++ b/internal/operator-controller/controllers/common_controller.go @@ -25,7 +25,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" - ocerror "github.com/operator-framework/operator-controller/internal/shared/util/error" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" ) const ( @@ -157,12 +157,19 @@ func setStatusProgressing(ext *ocv1.ClusterExtension, err error) { if err != nil { progressingCond.Reason = ocv1.ReasonRetrying // Unwrap TerminalError to avoid "terminal error:" prefix in message - progressingCond.Message = ocerror.UnwrapTerminal(err).Error() + progressingCond.Message = errorutil.UnwrapTerminal(err).Error() } if errors.Is(err, reconcile.TerminalError(nil)) { progressingCond.Status = metav1.ConditionFalse - progressingCond.Reason = ocv1.ReasonBlocked + // Try to extract a specific reason from the terminal error. + // If the error was created with NewTerminalError(reason, err), use that reason. + // Otherwise, fall back to the generic "Blocked" reason. + if reason, ok := errorutil.ExtractTerminalReason(err); ok { + progressingCond.Reason = reason + } else { + progressingCond.Reason = ocv1.ReasonBlocked + } } SetStatusCondition(&ext.Status.Conditions, progressingCond) diff --git a/internal/operator-controller/controllers/common_controller_test.go b/internal/operator-controller/controllers/common_controller_test.go index dd4cbeada0..3cc7575430 100644 --- a/internal/operator-controller/controllers/common_controller_test.go +++ b/internal/operator-controller/controllers/common_controller_test.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" ) func TestSetStatusProgressing(t *testing.T) { @@ -46,7 +47,7 @@ func TestSetStatusProgressing(t *testing.T) { }, }, { - name: "non-nil ClusterExtension, terminal error, Progressing condition has status False with reason Blocked", + name: "non-nil ClusterExtension, terminal error without reason, Progressing condition has status False with reason Blocked", err: reconcile.TerminalError(errors.New("boom")), clusterExtension: &ocv1.ClusterExtension{}, expected: metav1.Condition{ @@ -56,6 +57,17 @@ func TestSetStatusProgressing(t *testing.T) { Message: "boom", }, }, + { + name: "non-nil ClusterExtension, terminal error with InvalidConfiguration reason, Progressing condition has status False with that reason", + err: errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, errors.New("missing required field")), + clusterExtension: &ocv1.ClusterExtension{}, + expected: metav1.Condition{ + Type: ocv1.TypeProgressing, + Status: metav1.ConditionFalse, + Reason: ocv1.ReasonInvalidConfiguration, + Message: "missing required field", + }, + }, } { t.Run(tc.name, func(t *testing.T) { setStatusProgressing(tc.clusterExtension, tc.err) diff --git a/internal/shared/util/error/terminal.go b/internal/shared/util/error/terminal.go index ecb0353522..5c91012079 100644 --- a/internal/shared/util/error/terminal.go +++ b/internal/shared/util/error/terminal.go @@ -6,6 +6,57 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) +// terminalErrorWithReason is an internal error type that carries a Reason field +// to provide more granular categorization of terminal errors for status conditions. +type terminalErrorWithReason struct { + reason string + err error +} + +func (e *terminalErrorWithReason) Error() string { + return e.err.Error() +} + +func (e *terminalErrorWithReason) Unwrap() error { + return e.err +} + +// NewTerminalError creates a terminal error with a specific reason. +// The error is wrapped with reconcile.TerminalError so controller-runtime +// recognizes it as terminal, while preserving the reason for status reporting. +// +// Example usage: +// +// return error.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("missing required field")) +// +// The reason can be extracted later using ExtractTerminalReason() when setting +// status conditions to provide more specific feedback than just "Blocked". +func NewTerminalError(reason string, err error) error { + return reconcile.TerminalError(&terminalErrorWithReason{ + reason: reason, + err: err, + }) +} + +// ExtractTerminalReason extracts the reason from a terminal error created with +// NewTerminalError. Returns the reason and true if found, or empty string and +// false if the error was not created with NewTerminalError. +// +// This allows setStatusProgressing to use specific reasons like "InvalidConfiguration" +// instead of the generic "Blocked" for all terminal errors. +func ExtractTerminalReason(err error) (string, bool) { + if err == nil { + return "", false + } + // Unwrap the reconcile.TerminalError wrapper first + unwrapped := errors.Unwrap(err) + var terr *terminalErrorWithReason + if errors.As(unwrapped, &terr) { + return terr.reason, true + } + return "", false +} + func WrapTerminal(err error, isTerminal bool) error { if !isTerminal || err == nil { return err diff --git a/test/e2e/features/install.feature b/test/e2e/features/install.feature index 91c2af62e8..fdc8782ccd 100644 --- a/test/e2e/features/install.feature +++ b/test/e2e/features/install.feature @@ -117,7 +117,7 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as False with Reason Blocked and Message: + And ClusterExtension reports Progressing as False with Reason InvalidConfiguration and Message: """ error for resolved bundle "single-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: required field "watchNamespace" is missing @@ -169,7 +169,7 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as False with Reason Blocked and Message: + And ClusterExtension reports Progressing as False with Reason InvalidConfiguration and Message: """ error for resolved bundle "own-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: required @@ -197,7 +197,7 @@ Feature: Install ClusterExtension matchLabels: "olm.operatorframework.io/metadata.name": test-catalog """ - And ClusterExtension reports Progressing as False with Reason Blocked and Message: + And ClusterExtension reports Progressing as False with Reason InvalidConfiguration and Message: """ error for resolved bundle "own-namespace-operator.1.0.0" with version "1.0.0": invalid ClusterExtension configuration: invalid configuration: 'some-ns' From c2807f5341bc982e4acfd6b0cc66c1ec6f7c11eb Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:31:58 +0000 Subject: [PATCH 7/7] Update internal/operator-controller/controllers/boxcutter_reconcile_steps.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../controllers/boxcutter_reconcile_steps.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index f92ee28f49..dfb5dad145 100644 --- a/internal/operator-controller/controllers/boxcutter_reconcile_steps.go +++ b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go @@ -117,8 +117,8 @@ func ApplyBundleWithBoxcutter(apply func(ctx context.Context, contentFS fs.FS, e // report the error via the Progressing condition. setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err)) // Only set Installed condition for retryable errors. - // For terminal errors (Progressing: False/Blocked), the Progressing condition - // already provides all necessary information about the failure. + // For terminal errors (Progressing: False with a terminal reason such as Blocked or InvalidConfiguration), + // the Progressing condition already provides all necessary information about the failure. if !errors.Is(err, reconcile.TerminalError(nil)) { setInstalledStatusFromRevisionStates(ext, state.revisionStates) }