Fix deprecation conditions leaking install errors

- stop copying install/validation errors into deprecation conditions
- base bundle deprecation on the installed bundle (Unknown when none)
- extend unit tests to cover resolver failures with catalog deprecations

Assisted-by: Cursor

Author: camilamacedo86

api/v1/clusterextension_types.go

@@ -483,12 +483,12 @@ type ClusterExtensionStatus struct {
 	// When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.
 	// When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.
 	//
-	// When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.
+	// When the ClusterExtension is sourced from a catalog, it surfaces deprecation conditions based on catalog metadata.
 	// These are indications from a package owner to guide users away from a particular package, channel, or bundle.
-	// BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.
-	// ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.
-	// PackageDeprecated is set if the requested package is marked deprecated in the catalog.
-	// Deprecated is a rollup condition that is present when any of the deprecated conditions are present.
+	// PackageDeprecated becomes True when the catalog marks the requested package deprecated; otherwise it stays False.
+	// ChannelDeprecated becomes True when any requested channel is marked deprecated; otherwise it stays False.
+	// BundleDeprecated reports the catalog status of the installed bundle: it remains Unknown until a bundle installs, then becomes True or False depending on whether the catalog marks that bundle deprecated.
+	// Deprecated is a rollup that mirrors True whenever any of the specific deprecation conditions are True.
 	//
 	// +listType=map
 	// +listMapKey=type

internal/operator-controller/controllers/clusterextension_admission_test.go

@@ -13,20 +13,20 @@ import (
 )
 
 func TestClusterExtensionSourceConfig(t *testing.T) {
-	sourceTypeEmptyError := "Invalid value: null"
+	sourceTypeEmptyErrors := []string{"Invalid value: \"null\"", "Invalid value: null"}
 	sourceTypeMismatchError := "spec.source.sourceType: Unsupported value"
 	sourceConfigInvalidError := "spec.source: Invalid value"
 	// unionField represents the required Catalog or (future) Bundle field required by SourceConfig
 	testCases := []struct {
 		name       string
 		sourceType string
 		unionField string
-		errMsg     string
+		errMsgs    []string
 	}{
-		{"sourceType is null", "", "Catalog", sourceTypeEmptyError},
-		{"sourceType is invalid", "Invalid", "Catalog", sourceTypeMismatchError},
-		{"catalog field does not exist", "Catalog", "", sourceConfigInvalidError},
-		{"sourceConfig has required fields", "Catalog", "Catalog", ""},
+		{"sourceType is null", "", "Catalog", sourceTypeEmptyErrors},
+		{"sourceType is invalid", "Invalid", "Catalog", []string{sourceTypeMismatchError}},
+		{"catalog field does not exist", "Catalog", "", []string{sourceConfigInvalidError}},
+		{"sourceConfig has required fields", "Catalog", "Catalog", nil},
 	}
 
 	t.Parallel()
@@ -62,12 +62,20 @@ func TestClusterExtensionSourceConfig(t *testing.T) {
 				}))
 			}
 
-			if tc.errMsg == "" {
+			if len(tc.errMsgs) == 0 {
 				require.NoError(t, err, "unexpected error for sourceType %q: %w", tc.sourceType, err)
-			} else {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), tc.errMsg)
+				return
+			}
+
+			require.Error(t, err)
+			matched := false
+			for _, msg := range tc.errMsgs {
+				if strings.Contains(err.Error(), msg) {
+					matched = true
+					break
+				}
 			}
+			require.True(t, matched, "expected one of %v in error %q", tc.errMsgs, err)
 		})
 	}
 }

internal/operator-controller/controllers/clusterextension_controller.go

@@ -25,6 +25,7 @@ import (
 	"slices"
 	"strings"
 
+	bsemver "github.com/blang/semver/v4"
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage/driver"
@@ -139,13 +140,16 @@ func (r *ClusterExtensionReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	return res, reconcileErr
 }
 
-// ensureAllConditionsWithReason checks that all defined condition types exist in the given ClusterExtension,
-// and assigns a specified reason and custom message to any missing condition.
-func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.ConditionReason, message string) {
+// ensureFailureConditionsWithReason keeps every non-deprecation condition present.
+// If one is missing, we add it with the given reason and message so users see why
+// reconcile failed. Deprecation conditions are handled later by SetDeprecationStatus.
+func ensureFailureConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.ConditionReason, message string) {
 	for _, condType := range conditionsets.ConditionTypes {
+		if isDeprecationCondition(condType) {
+			continue
+		}
 		cond := apimeta.FindStatusCondition(ext.Status.Conditions, condType)
 		if cond == nil {
-			// Create a new condition with a valid reason and add it
 			SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
 				Type:               condType,
 				Status:             metav1.ConditionFalse,
@@ -157,6 +161,17 @@ func ensureAllConditionsWithReason(ext *ocv1.ClusterExtension, reason v1alpha1.C
 	}
 }
 
+// isDeprecationCondition reports whether the given type is one of the deprecation
+// conditions we manage separately.
+func isDeprecationCondition(condType string) bool {
+	switch condType {
+	case ocv1.TypeDeprecated, ocv1.TypePackageDeprecated, ocv1.TypeChannelDeprecated, ocv1.TypeBundleDeprecated:
+		return true
+	default:
+		return false
+	}
+}
+
 // Compare resources - ignoring status & metadata.finalizers
 func checkForUnexpectedClusterExtensionFieldChange(a, b ocv1.ClusterExtension) bool {
 	a.Status, b.Status = ocv1.ClusterExtensionStatus{}, ocv1.ClusterExtensionStatus{}
@@ -229,37 +244,41 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
 		return ctrl.Result{}, err
 	}
 
+	// Hold deprecation updates until the end. That way:
+	//   * if nothing installs, BundleDeprecated stays Unknown/Absent
+	//   * if a bundle installs, we report its real deprecation status
+	//   * install errors never leak into the deprecation conditions
+	var resolvedDeprecation *declcfg.Deprecation
+	defer func(resolvedDeprecationCopy *declcfg.Deprecation) {
+		installedBundleName := ""
+		if revisionStates != nil && revisionStates.Installed != nil {
+			installedBundleName = revisionStates.Installed.Name
+		}
+		SetDeprecationStatus(ext, installedBundleName, resolvedDeprecationCopy)
+	}(resolvedDeprecation)
+
 	var resolvedRevisionMetadata *RevisionMetadata
 	if len(revisionStates.RollingOut) == 0 {
 		l.Info("resolving bundle")
 		var bm *ocv1.BundleMetadata
 		if revisionStates.Installed != nil {
 			bm = &revisionStates.Installed.BundleMetadata
 		}
-		resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, bm)
+		var resolvedBundle *declcfg.Bundle
+		var resolvedBundleVersion *bsemver.Version
+		resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err = r.Resolver.Resolve(ctx, ext, bm)
+		// Keep any deprecation data the resolver returned. The deferred update will use it
+		// even if installation later fails or never begins.
 		if err != nil {
 			// Note: We don't distinguish between resolution-specific errors and generic errors
 			setStatusProgressing(ext, err)
 			setInstalledStatusFromRevisionStates(ext, revisionStates)
-			ensureAllConditionsWithReason(ext, ocv1.ReasonFailed, err.Error())
+			// Ensure non-deprecation conditions capture the failure immediately. The deferred
+			// SetDeprecationStatus call is responsible for updating the deprecation conditions
+			// based on any catalog data returned by the resolver.
+			ensureFailureConditionsWithReason(ext, ocv1.ReasonFailed, err.Error())
 			return ctrl.Result{}, err
 		}
-
-		// set deprecation status after _successful_ resolution
-		// TODO:
-		//  1. It seems like deprecation status should reflect the currently installed bundle, not the resolved
-		//     bundle. So perhaps we should set package and channel deprecations directly after resolution, but
-		//     defer setting the bundle deprecation until we successfully install the bundle.
-		//  2. If resolution fails because it can't find a bundle, that doesn't mean we wouldn't be able to find
-		//     a deprecation for the ClusterExtension's spec.packageName. Perhaps we should check for a non-nil
-		//     resolvedDeprecation even if resolution returns an error. If present, we can still update some of
-		//     our deprecation status.
-		//       - Open question though: what if different catalogs have different opinions of what's deprecated.
-		//         If we can't resolve a bundle, how do we know which catalog to trust for deprecation information?
-		//         Perhaps if the package shows up in multiple catalogs and deprecations don't match, we can set
-		//         the deprecation status to unknown? Or perhaps we somehow combine the deprecation information from
-		//         all catalogs?
-		SetDeprecationStatus(ext, resolvedBundle.Name, resolvedDeprecation)
 		resolvedRevisionMetadata = &RevisionMetadata{
 			Package:        resolvedBundle.Package,
 			Image:          resolvedBundle.Image,
@@ -326,83 +345,141 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
 	return ctrl.Result{}, nil
 }
 
-// SetDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension
-// based on the provided bundle
-func SetDeprecationStatus(ext *ocv1.ClusterExtension, bundleName string, deprecation *declcfg.Deprecation) {
-	deprecations := map[string][]declcfg.DeprecationEntry{}
-	channelSet := sets.New[string]()
+// DeprecationInfo captures the deprecation data needed to update condition status.
+type DeprecationInfo struct {
+	PackageEntries []declcfg.DeprecationEntry
+	ChannelEntries []declcfg.DeprecationEntry
+	BundleEntries  []declcfg.DeprecationEntry
+	BundleStatus   metav1.ConditionStatus
+}
+
+// SetDeprecationStatus updates the ClusterExtension deprecation conditions using the
+// catalog data from resolve plus the name of the bundle that actually landed. Examples:
+//   - no bundle installed -> bundle status stays Unknown/Absent
+//   - installed bundle marked deprecated -> bundle status True/Deprecated
+//   - installed bundle not deprecated -> bundle status False/Deprecated
+//
+// This keeps the deprecation conditions focused on catalog information:
+//   - PackageDeprecated: true if the catalog marks the package deprecated
+//   - ChannelDeprecated: true if any requested channel is marked deprecated
+//   - BundleDeprecated: reflects the installed bundle (Unknown/Absent when nothing installed)
+//   - Deprecated (rollup): true if any of the above signals a deprecation
+//
+// Install or validation errors never appear here because they belong on the
+// Progressing/Installed conditions instead. Callers should invoke this after reconcile
+// finishes (for example via a defer) so catalog data replaces any transient error messages.
+func SetDeprecationStatus(ext *ocv1.ClusterExtension, installedBundleName string, deprecation *declcfg.Deprecation) {
+	info := buildDeprecationInfo(ext, installedBundleName, deprecation)
+
+	packageMessages := collectDeprecationMessages(info.PackageEntries)
+	channelMessages := collectDeprecationMessages(info.ChannelEntries)
+	bundleMessages := collectDeprecationMessages(info.BundleEntries)
+
+	messages := slices.Concat(packageMessages, channelMessages, bundleMessages)
+
+	status := metav1.ConditionFalse
+	if len(messages) > 0 {
+		status = metav1.ConditionTrue
+	}
+
+	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
+		Type:               ocv1.TypeDeprecated,
+		Status:             status,
+		Reason:             ocv1.ReasonDeprecated,
+		Message:            strings.Join(messages, "\n"),
+		ObservedGeneration: ext.GetGeneration(),
+	})
+
+	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
+		Type:               ocv1.TypePackageDeprecated,
+		Status:             conditionStatus(len(packageMessages) > 0),
+		Reason:             ocv1.ReasonDeprecated,
+		Message:            strings.Join(packageMessages, "\n"),
+		ObservedGeneration: ext.GetGeneration(),
+	})
+
+	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
+		Type:               ocv1.TypeChannelDeprecated,
+		Status:             conditionStatus(len(channelMessages) > 0),
+		Reason:             ocv1.ReasonDeprecated,
+		Message:            strings.Join(channelMessages, "\n"),
+		ObservedGeneration: ext.GetGeneration(),
+	})
+
+	bundleReason := ocv1.ReasonDeprecated
+	bundleMessage := strings.Join(bundleMessages, "\n")
+	if info.BundleStatus == metav1.ConditionUnknown {
+		bundleReason = ocv1.ReasonAbsent
+		bundleMessage = ""
+	}
+
+	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
+		Type:               ocv1.TypeBundleDeprecated,
+		Status:             info.BundleStatus,
+		Reason:             bundleReason,
+		Message:            bundleMessage,
+		ObservedGeneration: ext.GetGeneration(),
+	})
+}
+
+// buildDeprecationInfo filters the catalog deprecation data down to the package, channel,
+// and bundle entries that matter for this ClusterExtension. An empty bundle name means
+// nothing is installed yet, so we leave bundle status Unknown/Absent.
+func buildDeprecationInfo(ext *ocv1.ClusterExtension, installedBundleName string, deprecation *declcfg.Deprecation) DeprecationInfo {
+	info := DeprecationInfo{BundleStatus: metav1.ConditionUnknown}
+	var channelSet sets.Set[string]
 	if ext.Spec.Source.Catalog != nil {
-		for _, channel := range ext.Spec.Source.Catalog.Channels {
-			channelSet.Insert(channel)
-		}
+		channelSet = sets.New(ext.Spec.Source.Catalog.Channels...)
+	} else {
+		channelSet = sets.New[string]()
 	}
+
 	if deprecation != nil {
 		for _, entry := range deprecation.Entries {
 			switch entry.Reference.Schema {
 			case declcfg.SchemaPackage:
-				deprecations[ocv1.TypePackageDeprecated] = []declcfg.DeprecationEntry{entry}
+				info.PackageEntries = append(info.PackageEntries, entry)
 			case declcfg.SchemaChannel:
 				if channelSet.Has(entry.Reference.Name) {
-					deprecations[ocv1.TypeChannelDeprecated] = append(deprecations[ocv1.TypeChannelDeprecated], entry)
+					info.ChannelEntries = append(info.ChannelEntries, entry)
 				}
 			case declcfg.SchemaBundle:
-				if bundleName != entry.Reference.Name {
-					continue
+				if installedBundleName != "" && entry.Reference.Name == installedBundleName {
+					info.BundleEntries = append(info.BundleEntries, entry)
 				}
-				deprecations[ocv1.TypeBundleDeprecated] = []declcfg.DeprecationEntry{entry}
 			}
 		}
 	}
 
-	// first get ordered deprecation messages that we'll join in the Deprecated condition message
-	var deprecationMessages []string
-	for _, conditionType := range []string{
-		ocv1.TypePackageDeprecated,
-		ocv1.TypeChannelDeprecated,
-		ocv1.TypeBundleDeprecated,
-	} {
-		if entries, ok := deprecations[conditionType]; ok {
-			for _, entry := range entries {
-				deprecationMessages = append(deprecationMessages, entry.Message)
-			}
+	// installedBundleName is empty when nothing is installed. In that case we want
+	// to report the bundle deprecation condition as Unknown/Absent.
+	if installedBundleName != "" {
+		if len(info.BundleEntries) > 0 {
+			info.BundleStatus = metav1.ConditionTrue
+		} else {
+			info.BundleStatus = metav1.ConditionFalse
 		}
 	}
 
-	// next, set the Deprecated condition
-	status, reason, message := metav1.ConditionFalse, ocv1.ReasonDeprecated, ""
-	if len(deprecationMessages) > 0 {
-		status, reason, message = metav1.ConditionTrue, ocv1.ReasonDeprecated, strings.Join(deprecationMessages, ";")
-	}
-	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
-		Type:               ocv1.TypeDeprecated,
-		Reason:             reason,
-		Status:             status,
-		Message:            message,
-		ObservedGeneration: ext.Generation,
-	})
+	return info
+}
 
-	// finally, set the individual deprecation conditions for package, channel, and bundle
-	for _, conditionType := range []string{
-		ocv1.TypePackageDeprecated,
-		ocv1.TypeChannelDeprecated,
-		ocv1.TypeBundleDeprecated,
-	} {
-		entries, ok := deprecations[conditionType]
-		status, reason, message := metav1.ConditionFalse, ocv1.ReasonDeprecated, ""
-		if ok {
-			status, reason = metav1.ConditionTrue, ocv1.ReasonDeprecated
-			for _, entry := range entries {
-				message = fmt.Sprintf("%s\n%s", message, entry.Message)
-			}
+// collectDeprecationMessages collects the non-empty deprecation messages from the provided entries.
+func collectDeprecationMessages(entries []declcfg.DeprecationEntry) []string {
+	messages := make([]string, 0, len(entries))
+	for _, entry := range entries {
+		if entry.Message != "" {
+			messages = append(messages, entry.Message)
 		}
-		SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
-			Type:               conditionType,
-			Reason:             reason,
-			Status:             status,
-			Message:            message,
-			ObservedGeneration: ext.Generation,
-		})
 	}
+	return messages
+}
+
+func conditionStatus(ok bool) metav1.ConditionStatus {
+	if ok {
+		return metav1.ConditionTrue
+	}
+	return metav1.ConditionFalse
 }
 
 type ControllerBuilderOption func(builder *ctrl.Builder)