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

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"
@@ -229,37 +230,53 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
 		return ctrl.Result{}, err
 	}
 
+	// Defer deprecation updates until the end of reconcile. We only want to talk about
+	// catalog deprecation data after we know which bundle is actually installed.
+	// This always runs (success or failure) so install/validation errors never leak into
+	// the deprecation conditions.
+	//
+	// What the deferred update reports:
+	//   * Package deprecation: true if the package is marked deprecated in the catalog
+	//   * Channel deprecation: true if any requested channel is deprecated in the catalog
+	//   * Bundle deprecation: reflects the INSTALLED bundle
+	//       - no bundle installed -> status Unknown, reason Absent
+	//       - installed bundle deprecated -> status True, reason Deprecated
+	//       - installed bundle not deprecated -> status False, reason Deprecated
+	//
+	// If a bundle resolves but fails to install, package/channel results still show the
+	// catalog data, and the bundle condition stays Unknown because nothing is installed.
+	var resolvedDeprecation *declcfg.Deprecation
+	defer func() {
+		bundleName := ""
+		if revisionStates != nil && revisionStates.Installed != nil {
+			bundleName = revisionStates.Installed.Name
+		}
+		SetDeprecationStatus(ext, bundleName, 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 writes the failure message to every condition.
+			// The deferred SetDeprecationStatus call overwrites the deprecation conditions right
+			// away so they only reflect catalog data. This also supports the case where resolution
+			// returned deprecation info even though it could not find an installable bundle.
 			ensureAllConditionsWithReason(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 +343,143 @@ 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
+// 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 based on the
+// provided deprecation information and the currently installed bundle name. When bundleName
+// is empty, no bundle is considered installed and the bundle deprecation condition is
+// reported as Unknown with reason Absent.
+//
+// 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, bundleName string, deprecation *declcfg.Deprecation) {
-	deprecations := map[string][]declcfg.DeprecationEntry{}
-	channelSet := sets.New[string]()
+	info := buildDeprecationInfo(ext, bundleName, deprecation)
+	setDeprecatedConditions(ext, info)
+}
+
+func buildDeprecationInfo(ext *ocv1.ClusterExtension, bundleName 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 {
+				if bundleName == "" || entry.Reference.Name != bundleName {
 					continue
 				}
-				deprecations[ocv1.TypeBundleDeprecated] = []declcfg.DeprecationEntry{entry}
+				info.BundleEntries = append(info.BundleEntries, 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)
-			}
-		}
+	if bundleName == "" {
+		info.BundleStatus = metav1.ConditionUnknown
+	} else if len(info.BundleEntries) > 0 {
+		info.BundleStatus = metav1.ConditionTrue
+	} else {
+		info.BundleStatus = metav1.ConditionFalse
+	}
+
+	return info
+}
+
+func setDeprecatedConditions(ext *ocv1.ClusterExtension, info DeprecationInfo) {
+	packageMessages := collectDeprecationMessages(info.PackageEntries)
+	channelMessages := collectDeprecationMessages(info.ChannelEntries)
+	bundleMessages := collectDeprecationMessages(info.BundleEntries)
+
+	messages := make([]string, 0, len(packageMessages)+len(channelMessages)+len(bundleMessages))
+	messages = append(messages, packageMessages...)
+	messages = append(messages, channelMessages...)
+	if info.BundleStatus == metav1.ConditionTrue {
+		messages = append(messages, bundleMessages...)
 	}
 
-	// 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, ";")
+	status := metav1.ConditionFalse
+	if len(messages) > 0 {
+		status = metav1.ConditionTrue
 	}
+
 	SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
 		Type:               ocv1.TypeDeprecated,
-		Reason:             reason,
 		Status:             status,
-		Message:            message,
-		ObservedGeneration: ext.Generation,
+		Reason:             ocv1.ReasonDeprecated,
+		Message:            strings.Join(messages, "\n"),
+		ObservedGeneration: ext.GetGeneration(),
 	})
 
-	// 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)
-			}
+	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(),
+	})
+}
+
+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)