OCPBUGS-78455: Synchronize From Upstream Repositories

commitchecker.yaml

@@ -1,4 +1,4 @@
-expectedMergeBase: 253feaef5babbdabfb4b63d1cb3a60455947be3a
+expectedMergeBase: e709e65344e8e7bc23fc421b96587ee702f1e8e3
 upstreamBranch: main
 upstreamOrg: operator-framework
 upstreamRepo: operator-controller

internal/operator-controller/applier/provider.go

@@ -112,13 +112,13 @@ func (r *RegistryV1ManifestProvider) extractBundleConfigOptions(rv1 *bundle.Regi
 		opts = append(opts, render.WithTargetNamespaces(*watchNS))
 	}
 
-	// Extract and convert deploymentConfig if present and the feature gate is enabled.
+	// Extract deploymentConfig if present and the feature gate is enabled.
 	if r.IsDeploymentConfigEnabled {
-		if deploymentConfigMap := bundleConfig.GetDeploymentConfig(); deploymentConfigMap != nil {
-			deploymentConfig, err := convertToDeploymentConfig(deploymentConfigMap)
-			if err != nil {
-				return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err))
-			}
+		deploymentConfig, err := bundleConfig.GetDeploymentConfig()
+		if err != nil {
+			return nil, errorutil.NewTerminalError(ocv1.ReasonInvalidConfiguration, fmt.Errorf("invalid deploymentConfig: %w", err))
+		}
+		if deploymentConfig != nil {
 			opts = append(opts, render.WithDeploymentConfig(deploymentConfig))
 		}
 	}
@@ -187,29 +187,6 @@ func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte {
 	return nil
 }
 
-// convertToDeploymentConfig converts a map[string]any (from validated bundle config)
-// to a *config.DeploymentConfig struct that can be passed to the renderer.
-// Returns nil if the map is empty.
-func convertToDeploymentConfig(deploymentConfigMap map[string]any) (*config.DeploymentConfig, error) {
-	if len(deploymentConfigMap) == 0 {
-		return nil, nil
-	}
-
-	// Marshal the map to JSON
-	data, err := json.Marshal(deploymentConfigMap)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err)
-	}
-
-	// Unmarshal into the DeploymentConfig struct
-	var deploymentConfig config.DeploymentConfig
-	if err := json.Unmarshal(data, &deploymentConfig); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err)
-	}
-
-	return &deploymentConfig, nil
-}
-
 func getBundleAnnotations(bundleFS fs.FS) (map[string]string, error) {
 	// The need to get the underlying bundle in order to extract its annotations
 	// will go away once we have a bundle interface that can surface the annotations independently of the

internal/operator-controller/applier/provider_test.go

@@ -17,6 +17,7 @@ import (
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/config"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
@@ -599,8 +600,8 @@ func Test_RegistryV1ManifestProvider_DeploymentConfig(t *testing.T) {
 			BundleRenderer: render.BundleRenderer{
 				ResourceGenerators: []render.ResourceGenerator{
 					func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
-						t.Log("ensure deploymentConfig is nil for empty config object")
-						require.Nil(t, opts.DeploymentConfig)
+						t.Log("ensure deploymentConfig is empty for empty config object")
+						require.Equal(t, &config.DeploymentConfig{}, opts.DeploymentConfig)
 						return nil, nil
 					},
 				},

internal/operator-controller/config/config.go

@@ -106,44 +106,29 @@ func (c *Config) GetWatchNamespace() *string {
 }
 
 // GetDeploymentConfig returns the deploymentConfig value if present in the configuration.
-// Returns nil if deploymentConfig is not set or is explicitly set to null.
-// The returned value is a generic map[string]any that can be marshaled to JSON
-// for validation or conversion to specific types (like v1alpha1.SubscriptionConfig).
-//
-// Returns a defensive deep copy so callers can't mutate the internal Config state.
-func (c *Config) GetDeploymentConfig() map[string]any {
+// Returns (nil, nil) if deploymentConfig is not set or is explicitly set to null.
+// Returns a non-nil error if the value cannot be marshaled or unmarshaled into a DeploymentConfig.
+func (c *Config) GetDeploymentConfig() (*DeploymentConfig, error) {
 	if c == nil || *c == nil {
-		return nil
+		return nil, nil
 	}
 	val, exists := (*c)["deploymentConfig"]
 	if !exists {
-		return nil
+		return nil, nil
 	}
 	// User set deploymentConfig: null - treat as "not configured"
 	if val == nil {
-		return nil
-	}
-	// Schema validation ensures this is an object (map)
-	dcMap, ok := val.(map[string]any)
-	if !ok {
-		return nil
+		return nil, nil
 	}
-
-	// Return a defensive deep copy so callers can't mutate the internal Config state.
-	// We use JSON marshal/unmarshal because the data is already JSON-compatible and
-	// this handles nested structures correctly.
-	data, err := json.Marshal(dcMap)
+	data, err := json.Marshal(val)
 	if err != nil {
-		// This should never happen since the map came from validated JSON/YAML,
-		// but return nil as a safe fallback
-		return nil
+		return nil, fmt.Errorf("failed to marshal deploymentConfig: %w", err)
 	}
-	var copied map[string]any
-	if err := json.Unmarshal(data, &copied); err != nil {
-		// This should never happen for valid JSON
-		return nil
+	var dc DeploymentConfig
+	if err := json.Unmarshal(data, &dc); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal deploymentConfig: %w", err)
 	}
-	return copied
+	return &dc, nil
 }
 
 // UnmarshalConfig takes user configuration, validates it, and creates a Config object.

internal/operator-controller/config/config_test.go

@@ -5,6 +5,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/utils/ptr"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -596,7 +598,7 @@ func Test_GetDeploymentConfig(t *testing.T) {
 	tests := []struct {
 		name                        string
 		rawConfig                   []byte
-		expectedDeploymentConfig    map[string]any
+		expectedDeploymentConfig    *config.DeploymentConfig
 		expectedDeploymentConfigNil bool
 	}{
 		{
@@ -628,13 +630,13 @@ func Test_GetDeploymentConfig(t *testing.T) {
 					}
 				}
 			}`),
-			expectedDeploymentConfig: map[string]any{
-				"nodeSelector": map[string]any{
+			expectedDeploymentConfig: &config.DeploymentConfig{
+				NodeSelector: map[string]string{
 					"kubernetes.io/os": "linux",
 				},
-				"resources": map[string]any{
-					"requests": map[string]any{
-						"memory": "128Mi",
+				Resources: &corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceMemory: resource.MustParse("128Mi"),
 					},
 				},
 			},
@@ -650,7 +652,8 @@ func Test_GetDeploymentConfig(t *testing.T) {
 			cfg, err := config.UnmarshalConfig(tt.rawConfig, schema, "")
 			require.NoError(t, err)
 
-			result := cfg.GetDeploymentConfig()
+			result, err := cfg.GetDeploymentConfig()
+			require.NoError(t, err)
 			if tt.expectedDeploymentConfigNil {
 				require.Nil(t, result)
 			} else {
@@ -663,12 +666,13 @@ func Test_GetDeploymentConfig(t *testing.T) {
 	// Test nil config separately
 	t.Run("nil config returns nil", func(t *testing.T) {
 		var cfg *config.Config
-		result := cfg.GetDeploymentConfig()
+		result, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.Nil(t, result)
 	})
 
-	// Test that returned map is a defensive copy (mutations don't affect original)
-	t.Run("returned map is defensive copy - mutations don't affect original", func(t *testing.T) {
+	// Test that returned struct is a separate instance (mutations don't affect original)
+	t.Run("returned struct is independent copy - mutations don't affect original", func(t *testing.T) {
 		rawConfig := []byte(`{
 			"deploymentConfig": {
 				"nodeSelector": {
@@ -684,31 +688,29 @@ func Test_GetDeploymentConfig(t *testing.T) {
 		require.NoError(t, err)
 
 		// Get the deploymentConfig
-		result1 := cfg.GetDeploymentConfig()
+		result1, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.NotNil(t, result1)
 
-		// Mutate the returned map
-		result1["nodeSelector"] = map[string]any{
-			"mutated": "value",
-		}
-		result1["newField"] = "added"
+		// Mutate the returned struct
+		result1.NodeSelector["mutated"] = "value"
 
 		// Get deploymentConfig again - should be unaffected by mutations
-		result2 := cfg.GetDeploymentConfig()
+		result2, err := cfg.GetDeploymentConfig()
+		require.NoError(t, err)
 		require.NotNil(t, result2)
 
 		// Original values should be intact
-		require.Equal(t, map[string]any{
-			"nodeSelector": map[string]any{
-				"kubernetes.io/os": "linux",
-			},
-		}, result2)
-
-		// New field should not exist
-		_, exists := result2["newField"]
-		require.False(t, exists)
+		require.Equal(t, map[string]string{
+			"kubernetes.io/os": "linux",
+		}, result2.NodeSelector)
+	})
 
-		// result1 should have the mutations
-		require.Equal(t, "added", result1["newField"])
+	// Test that invalid deploymentConfig type returns an error
+	t.Run("invalid deploymentConfig type returns error", func(t *testing.T) {
+		cfg := config.Config{"deploymentConfig": "not-an-object"}
+		result, err := cfg.GetDeploymentConfig()
+		require.Error(t, err)
+		require.Nil(t, result)
 	})
 }

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -145,6 +145,12 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
 	}
 
+	siblings, err := c.siblingRevisionNames(ctx, cer)
+	if err != nil {
+		setRetryingConditions(cer, err.Error())
+		return ctrl.Result{}, fmt.Errorf("listing sibling revisions: %v", err)
+	}
+
 	revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer)
 	if err != nil {
 		setRetryingConditions(cer, err.Error())
@@ -207,6 +213,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
 			if ores.Action() == machinery.ActionCollision {
 				collidingObjs = append(collidingObjs, ores.String())
 			}
+			if ores.Action() == machinery.ActionProgressed && siblings != nil {
+				if ref := foreignRevisionController(ores.Object(), siblings); ref != nil {
+					collidingObjs = append(collidingObjs, ores.String()+fmt.Sprintf("\nConflicting Owner: %s", ref.String()))
+				}
+			}
 		}
 
 		if len(collidingObjs) > 0 {
@@ -517,6 +528,42 @@ func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.Collision
 	return ecp
 }
 
+// siblingRevisionNames returns the names of all ClusterExtensionRevisions that belong to
+// the same ClusterExtension as cer. Returns nil when cer has no owner label.
+func (c *ClusterExtensionRevisionReconciler) siblingRevisionNames(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (sets.Set[string], error) {
+	ownerLabel, ok := cer.Labels[labels.OwnerNameKey]
+	if !ok {
+		return nil, nil
+	}
+	revList := &ocv1.ClusterExtensionRevisionList{}
+	if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
+		labels.OwnerNameKey: ownerLabel,
+	}); err != nil {
+		return nil, fmt.Errorf("listing sibling revisions: %w", err)
+	}
+	names := sets.New[string]()
+	for i := range revList.Items {
+		names.Insert(revList.Items[i].Name)
+	}
+	return names, nil
+}
+
+// foreignRevisionController returns the controller OwnerReference when obj is owned by a
+// ClusterExtensionRevision that is not in siblings (i.e. belongs to a different ClusterExtension).
+// Returns nil when the controller is a sibling or is not a ClusterExtensionRevision.
+func foreignRevisionController(obj metav1.Object, siblings sets.Set[string]) *metav1.OwnerReference {
+	refs := obj.GetOwnerReferences()
+	for i := range refs {
+		if refs[i].Controller != nil && *refs[i].Controller &&
+			refs[i].Kind == ocv1.ClusterExtensionRevisionKind &&
+			refs[i].APIVersion == ocv1.GroupVersion.String() &&
+			!siblings.Has(refs[i].Name) {
+			return &refs[i]
+		}
+	}
+	return nil
+}
+
 // buildProgressionProbes creates a set of boxcutter probes from the fields provided in the CER's spec.progressionProbes.
 // Returns nil and an error if encountered while attempting to build the probes.
 func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing.And, error) {