Address PR #2595 review feedback

- Fix duplicate key size inflation in SecretPacker by only incrementing
  size for new content hash keys
- Add io.LimitReader (10 MiB cap) for gzip decompression to prevent
  gzip bombs in controller and e2e helpers
- Add doc comment clarifying ObjectSourceRef.Namespace defaults to OLM
  system namespace during ref resolution
- Fix docs: orphan cleanup uses ownerReference GC, ref resolution
  failures are retried (not terminal)
- Remove unused ClusterExtensionRevisionReasonRefResolutionFailed constant
- Add default error branch in e2e listExtensionRevisionResources for
  objects missing both ref and inline content

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: pedjak

api/v1/clusterextensionrevision_types.go

@@ -30,13 +30,12 @@ const (
 	ClusterExtensionRevisionTypeSucceeded   = "Succeeded"
 
 	// Condition Reasons
-	ClusterExtensionRevisionReasonArchived            = "Archived"
-	ClusterExtensionRevisionReasonBlocked             = "Blocked"
-	ClusterExtensionRevisionReasonProbeFailure        = "ProbeFailure"
-	ClusterExtensionRevisionReasonProbesSucceeded     = "ProbesSucceeded"
-	ClusterExtensionRevisionReasonReconciling         = "Reconciling"
-	ClusterExtensionRevisionReasonRefResolutionFailed = "RefResolutionFailed"
-	ClusterExtensionRevisionReasonRetrying            = "Retrying"
+	ClusterExtensionRevisionReasonArchived        = "Archived"
+	ClusterExtensionRevisionReasonBlocked         = "Blocked"
+	ClusterExtensionRevisionReasonProbeFailure    = "ProbeFailure"
+	ClusterExtensionRevisionReasonProbesSucceeded = "ProbesSucceeded"
+	ClusterExtensionRevisionReasonReconciling     = "Reconciling"
+	ClusterExtensionRevisionReasonRetrying        = "Retrying"
 )
 
 // ClusterExtensionRevisionSpec defines the desired state of ClusterExtensionRevision.
@@ -452,6 +451,7 @@ type ObjectSourceRef struct {
 	Name string `json:"name"`
 
 	// namespace is the namespace of the referenced Secret.
+	// When empty, defaults to the OLM system namespace during ref resolution.
 	//
 	// +optional
 	// +kubebuilder:validation:MaxLength=63

applyconfigurations/api/v1/objectsourceref.go

@@ -364,9 +364,12 @@ Key properties:
 - **No reconciler churn**: Referenced Secrets exist before the CER is created.
   The CER reconciler never encounters missing Secrets during normal operation.
 - **Orphan cleanup**: Secrets created in step 1 carry the revision label
-  (`olm.operatorframework.io/revision-name`). If a crash leaves Secrets without
-  a corresponding CER, the ClusterExtension controller detects and deletes them
-  on its next reconciliation.
+  (`olm.operatorframework.io/revision-name`). Once the CER is created in step 2
+  and ownerReferences are patched in step 3, Kubernetes garbage collection
+  automatically removes the Secrets when the CER is deleted. If a crash occurs
+  between steps 1 and 2, the ClusterExtension controller detects orphaned
+  Secrets (those with the revision label but no corresponding CER) and deletes
+  them on its next reconciliation.
 - **Idempotent retry**: Secrets are immutable in data. Re-creation of an
   existing Secret returns AlreadyExists and is skipped. ownerReference patching
   is idempotent — patching an already-set ownerReference is a no-op.
@@ -389,7 +392,9 @@ Under normal operation, referenced Secrets are guaranteed to exist before the
 CER is created (see [Crash-safe creation sequence](#crash-safe-creation-sequence)).
 If a referenced Secret or key is not found — indicating an inconsistent state
 caused by external modification or a partially completed creation sequence —
-the reconciler sets a terminal error condition on the CER.
+the reconciler returns a retryable error, allowing the controller to retry on
+subsequent reconciliation attempts. This handles transient issues such as
+informer cache lag after Secret creation.
 
 Secrets are fetched using the typed client served from the informer cache.
 

docs/concepts/large-bundle-support.md

@@ -221,8 +221,9 @@ spec:
                                 minLength: 1
                                 type: string
                               namespace:
-                                description: namespace is the namespace of the referenced
-                                  Secret.
+                                description: |-
+                                  namespace is the namespace of the referenced Secret.
+                                  When empty, defaults to the OLM system namespace during ref resolution.
                                 maxLength: 63
                                 type: string
                             required:

helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml

@@ -115,13 +115,15 @@ func (p *SecretPacker) Pack(phases []ocv1.ClusterExtensionRevisionPhase) (*PackR
 
 			key := contentHash(data)
 
-			// If adding this entry would exceed the limit, finalize the current Secret.
-			if currentSize+len(data) > maxSecretDataSize && len(currentData) > 0 {
-				finalizeCurrent()
+			// Only add data and increment size for new keys. Duplicate content
+			// (same hash) reuses the existing entry without inflating the size.
+			if _, exists := currentData[key]; !exists {
+				if currentSize+len(data) > maxSecretDataSize && len(currentData) > 0 {
+					finalizeCurrent()
+				}
+				currentData[key] = data
+				currentSize += len(data)
 			}
-
-			currentData[key] = data
-			currentSize += len(data)
 			currentPending = append(currentPending, pendingRef{pos: [2]int{phaseIdx, objIdx}, key: key})
 		}
 	}

internal/operator-controller/applier/secretpacker.go

@@ -171,6 +171,28 @@ func TestSecretPacker_Pack(t *testing.T) {
 		defer reader.Close()
 	})
 
+	t.Run("duplicate content objects share key and do not inflate size", func(t *testing.T) {
+		// Two identical objects should produce the same content hash key.
+		// The second occurrence must not double-count the size.
+		phases := []ocv1.ClusterExtensionRevisionPhase{{
+			Name: "deploy",
+			Objects: []ocv1.ClusterExtensionRevisionObject{
+				{Object: testConfigMap("same-cm", "default")},
+				{Object: testConfigMap("same-cm", "default")},
+			},
+		}}
+
+		result, err := packer.Pack(phases)
+		require.NoError(t, err)
+		require.Len(t, result.Secrets, 1)
+		// Only one data entry despite two objects.
+		assert.Len(t, result.Secrets[0].Data, 1)
+		// Both positions get refs.
+		assert.Len(t, result.Refs, 2)
+		// Both refs point to the same key.
+		assert.Equal(t, result.Refs[[2]int{0, 0}].Key, result.Refs[[2]int{0, 1}].Key)
+	})
+
 	t.Run("key is SHA-256 base64url", func(t *testing.T) {
 		obj := testConfigMap("test-cm", "default")
 		rawData, err := json.Marshal(obj.Object)

internal/operator-controller/applier/secretpacker_test.go

@@ -554,10 +554,15 @@ func (c *ClusterExtensionRevisionReconciler) resolveObjectRef(ctx context.Contex
 			return nil, fmt.Errorf("creating gzip reader for key %q in Secret %s/%s: %w", ref.Key, ref.Namespace, ref.Name, err)
 		}
 		defer reader.Close()
-		decompressed, err := io.ReadAll(reader)
+		const maxDecompressedSize = 10 * 1024 * 1024 // 10 MiB
+		limited := io.LimitReader(reader, maxDecompressedSize+1)
+		decompressed, err := io.ReadAll(limited)
 		if err != nil {
 			return nil, fmt.Errorf("decompressing key %q in Secret %s/%s: %w", ref.Key, ref.Namespace, ref.Name, err)
 		}
+		if len(decompressed) > maxDecompressedSize {
+			return nil, fmt.Errorf("decompressed data for key %q in Secret %s/%s exceeds maximum size (%d bytes)", ref.Key, ref.Namespace, ref.Name, maxDecompressedSize)
+		}
 		data = decompressed
 	}
 

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -833,8 +833,9 @@ spec:
                                 minLength: 1
                                 type: string
                               namespace:
-                                description: namespace is the namespace of the referenced
-                                  Secret.
+                                description: |-
+                                  namespace is the namespace of the referenced Secret.
+                                  When empty, defaults to the OLM system namespace during ref resolution.
                                 maxLength: 63
                                 type: string
                             required:

manifests/experimental-e2e.yaml

@@ -794,8 +794,9 @@ spec:
                                 minLength: 1
                                 type: string
                               namespace:
-                                description: namespace is the namespace of the referenced
-                                  Secret.
+                                description: |-
+                                  namespace is the namespace of the referenced Secret.
+                                  When empty, defaults to the OLM system namespace during ref resolution.
                                 maxLength: 63
                                 type: string
                             required:

manifests/experimental.yaml

@@ -1577,6 +1577,8 @@ func listExtensionRevisionResources(extName string) ([]client.Object, error) {
 				objs = append(objs, resolved)
 			case len(specObj.Object.Object) > 0:
 				objs = append(objs, &specObj.Object)
+			default:
+				return nil, fmt.Errorf("object %d in phase %q has neither object nor ref", j, phase.Name)
 			}
 		}
 	}
@@ -1605,10 +1607,15 @@ func resolveObjectRef(ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, err
 			return nil, fmt.Errorf("creating gzip reader for key %q in Secret %s/%s: %w", ref.Key, ref.Namespace, ref.Name, err)
 		}
 		defer reader.Close()
-		decompressed, err := io.ReadAll(reader)
+		const maxDecompressedSize = 10 * 1024 * 1024 // 10 MiB
+		limited := io.LimitReader(reader, maxDecompressedSize+1)
+		decompressed, err := io.ReadAll(limited)
 		if err != nil {
 			return nil, fmt.Errorf("decompressing key %q in Secret %s/%s: %w", ref.Key, ref.Namespace, ref.Name, err)
 		}
+		if len(decompressed) > maxDecompressedSize {
+			return nil, fmt.Errorf("decompressed data for key %q in Secret %s/%s exceeds maximum size (%d bytes)", ref.Key, ref.Namespace, ref.Name, maxDecompressedSize)
+		}
 		data = decompressed
 	}
 	obj := &unstructured.Unstructured{}