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 cf75b28c87..ab4e728e7f 100644 --- a/internal/operator-controller/applier/provider.go +++ b/internal/operator-controller/applier/provider.go @@ -16,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 @@ -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, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid ClusterExtension configuration: %w", err)) } if watchNS := bundleConfig.GetWatchNamespace(); watchNS != nil { 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/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/boxcutter_reconcile_steps.go b/internal/operator-controller/controllers/boxcutter_reconcile_steps.go index 5c746c7b0f..dfb5dad145 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,6 +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)) + // Only set Installed condition for retryable errors. + // 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) + } return nil, err } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index f089efcacb..a3fcf48f3f 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" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" k8sutil "github.com/operator-framework/operator-controller/internal/shared/util/k8s" ) @@ -455,6 +456,19 @@ func (r *ClusterExtensionReconciler) SetupWithManager(mgr ctrl.Manager, opts ... } func wrapErrorWithResolutionInfo(resolved ocv1.BundleMetadata, err error) error { + // Preserve TerminalError type and reason through wrapping + if errors.Is(err, reconcile.TerminalError(nil)) { + // 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 7fafc7bb7b..cb6834c6b6 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" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" ) const ( @@ -56,11 +57,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 +69,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{ @@ -119,12 +156,20 @@ 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 = 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 93fad962e8..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,14 +47,25 @@ 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{ Type: ocv1.TypeProgressing, Status: metav1.ConditionFalse, Reason: ocv1.ReasonBlocked, - Message: "terminal error: boom", + 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", }, }, } { @@ -240,3 +252,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) + }) + } +} diff --git a/internal/shared/util/error/terminal.go b/internal/shared/util/error/terminal.go index cd70d535f7..5c91012079 100644 --- a/internal/shared/util/error/terminal.go +++ b/internal/shared/util/error/terminal.go @@ -1,6 +1,61 @@ package error -import "sigs.k8s.io/controller-runtime/pkg/reconcile" +import ( + "errors" + + "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 { @@ -8,3 +63,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 ab87b5f31c..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 True with Reason Retrying 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 True with Reason Retrying 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 True with Reason Retrying 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'