OCPBUGS-123707: prevent ClusterCatalog catalog rollbacks

Record a positive catalog publication version from the OCI image config label alongside cached catalog content and in ClusterCatalog status.

Reject a newly resolved digest when the currently accepted catalog is versioned and the new catalog has no version or a non-increasing version. Preserve the last accepted version when content is unavailable so subsequent reconciles continue to reject the rollback.

Add controller, cache, and puller coverage for version parsing, metadata persistence and garbage collection, and rollback rejection.

Validation: make test-unit
Signed-off-by: Todd Short <tshort@redhat.com>

Author: tmshort

api/v1/clustercatalog_types.go

@@ -265,6 +265,13 @@ type ResolvedImageSource struct {
 	// +kubebuilder:validation:XValidation:rule="self.find('(@.*:)') != \"\" ? self.find(':.*$').substring(1).size() >= 32 : true",message="digest is not valid. the encoded string must be at least 32 characters"
 	// +kubebuilder:validation:XValidation:rule="self.find('(@.*:)') != \"\" ? self.find(':.*$').matches(':[0-9A-Fa-f]*$') : true",message="digest is not valid. the encoded string must only contain hex characters (A-F, a-f, 0-9)"
 	Ref string `json:"ref"`
+	// catalogVersion is the monotonic version assigned to the catalog publication by the image publisher.
+	// It is read from the OCI image config label "olm.operatorframework.io/catalog-version".
+	// Older images without this label have no version. After a version is recorded, a different digest must have a
+	// greater version before its content can replace the currently served catalog.
+	// +kubebuilder:validation:Minimum:=1
+	// +optional
+	CatalogVersion int64 `json:"catalogVersion,omitempty"`
 }
 
 // ImageSource enables users to define the information required for sourcing a Catalog from an OCI image

applyconfigurations/api/v1/resolvedimagesource.go

@@ -545,6 +545,7 @@ _Appears in:_
 | Field | Description | Default | Validation |
 | --- | --- | --- | --- |
 | `ref` _string_ | ref contains the resolved image digest-based reference.<br />The digest format allows you to use other tooling to fetch the exact OCI manifests<br />that were used to extract the catalog contents. |  | MaxLength: 1000 <br />Required: \{\} <br /> |
+| `catalogVersion` _integer_ | catalogVersion is the monotonic version assigned to the catalog publication by the image publisher.<br />It is read from the OCI image config label "olm.operatorframework.io/catalog-version".<br />Older images without this label have no version. After a version is recorded, a different digest must have a<br />greater version before its content can replace the currently served catalog. |  | Minimum: 1 <br />Optional: \{\} <br /> |
 
 
 #### RevisionStatus

applyconfigurations/internal/internal.go

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.20.1
+    controller-gen.kubebuilder.io/version: v0.21.0
     olm.operatorframework.io/generator: experimental
   name: clustercatalogs.olm.operatorframework.io
 spec:
@@ -332,6 +332,15 @@ spec:
                       image contains resolution information for a catalog sourced from an image.
                       It must be set when type is Image, and forbidden otherwise.
                     properties:
+                      catalogVersion:
+                        description: |-
+                          catalogVersion is the monotonic version assigned to the catalog publication by the image publisher.
+                          It is read from the OCI image config label "olm.operatorframework.io/catalog-version".
+                          Older images without this label have no version. After a version is recorded, a different digest must have a
+                          greater version before its content can replace the currently served catalog.
+                        format: int64
+                        minimum: 1
+                        type: integer
                       ref:
                         description: |-
                           ref contains the resolved image digest-based reference.

docs/api-reference/olmv1-api-reference.md

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.20.1
+    controller-gen.kubebuilder.io/version: v0.21.0
     olm.operatorframework.io/generator: standard
   name: clustercatalogs.olm.operatorframework.io
 spec:
@@ -332,6 +332,15 @@ spec:
                       image contains resolution information for a catalog sourced from an image.
                       It must be set when type is Image, and forbidden otherwise.
                     properties:
+                      catalogVersion:
+                        description: |-
+                          catalogVersion is the monotonic version assigned to the catalog publication by the image publisher.
+                          It is read from the OCI image config label "olm.operatorframework.io/catalog-version".
+                          Older images without this label have no version. After a version is recorded, a different digest must have a
+                          greater version before its content can replace the currently served catalog.
+                        format: int64
+                        minimum: 1
+                        type: integer
                       ref:
                         description: |-
                           ref contains the resolved image digest-based reference.

helm/olmv1/base/catalogd/crd/experimental/olm.operatorframework.io_clustercatalogs.yaml

@@ -73,6 +73,7 @@ type ClusterCatalogReconciler struct {
 
 type storedCatalogData struct {
 	ref                reference.Canonical
+	catalogVersion     int64
 	lastUnpack         time.Time
 	lastSuccessfulPoll time.Time
 	observedGeneration int64
@@ -253,13 +254,61 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
 		return ctrl.Result{}, err
 	}
 
-	fsys, canonicalRef, unpackTime, err := r.ImagePuller.Pull(ctx, catalog.Name, catalog.Spec.Source.Image.Ref, r.ImageCache)
+	catalogPuller, ok := r.ImagePuller.(imageutil.CatalogPuller)
+	if !ok {
+		err := fmt.Errorf("catalog image puller does not provide publication metadata")
+		updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), err)
+		return ctrl.Result{}, err
+	}
+	fsys, canonicalRef, unpackTime, catalogVersion, err := catalogPuller.PullCatalog(ctx, catalog.Name, catalog.Spec.Source.Image.Ref, r.ImageCache)
 	if err != nil {
 		unpackErr := fmt.Errorf("source catalog content: %w", err)
 		updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), unpackErr)
 		return ctrl.Result{}, unpackErr
 	}
 
+	currentImage := (*ocv1.ResolvedImageSource)(nil)
+	if catalog.Status.ResolvedSource != nil {
+		currentImage = catalog.Status.ResolvedSource.Image
+	}
+	sameDigest := false
+	if currentImage != nil {
+		currentRef, err := reference.Parse(currentImage.Ref)
+		if err != nil {
+			statusErr := fmt.Errorf("invalid currently resolved catalog image reference %q: %w", currentImage.Ref, err)
+			updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), statusErr)
+			return ctrl.Result{}, statusErr
+		}
+		currentCanonicalRef, ok := currentRef.(reference.Canonical)
+		if !ok {
+			statusErr := fmt.Errorf("currently resolved catalog image reference %q is not digest-based", currentImage.Ref)
+			updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), statusErr)
+			return ctrl.Result{}, statusErr
+		}
+		sameDigest = canonicalRef.Digest() == currentCanonicalRef.Digest()
+	}
+	if currentImage != nil &&
+		!sameDigest &&
+		currentImage.CatalogVersion > 0 &&
+		catalogVersion <= currentImage.CatalogVersion {
+		rollbackErr := fmt.Errorf(
+			"rejecting catalog image %s with catalog version %d: currently served image %s has version %d; a different digest must have a greater catalog version",
+			canonicalRef,
+			catalogVersion,
+			currentImage.Ref,
+			currentImage.CatalogVersion,
+		)
+		updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), rollbackErr)
+		if !r.Storage.ContentExists(catalog.Name) {
+			updateStatusNotServingWithResolvedSource(&catalog.Status, catalog.GetGeneration())
+		}
+		return ctrl.Result{}, rollbackErr
+	}
+	if sameDigest && currentImage.CatalogVersion > catalogVersion {
+		// Preserve the accepted version for the same immutable image if older cache data lacks the metadata sidecar.
+		catalogVersion = currentImage.CatalogVersion
+	}
+
 	// TODO: We should check to see if the unpacked result has the same content
 	//   as the already unpacked content. If it does, we should skip this rest
 	//   of the unpacking steps.
@@ -271,12 +320,13 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
 	baseURL := r.Storage.BaseURL(catalog.Name)
 
 	updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), nil)
-	updateStatusServing(&catalog.Status, canonicalRef, unpackTime, baseURL, catalog.GetGeneration())
+	updateStatusServing(&catalog.Status, canonicalRef, catalogVersion, unpackTime, baseURL, catalog.GetGeneration())
 
 	lastSuccessfulPoll := time.Now()
 	r.storedCatalogsMu.Lock()
 	r.storedCatalogs[catalog.Name] = storedCatalogData{
 		ref:                canonicalRef,
+		catalogVersion:     catalogVersion,
 		lastUnpack:         unpackTime,
 		lastSuccessfulPoll: lastSuccessfulPoll,
 		observedGeneration: catalog.GetGeneration(),
@@ -295,7 +345,7 @@ func (r *ClusterCatalogReconciler) getCurrentState(catalog *ocv1.ClusterCatalog)
 	// Set expected status based on what we see in the stored catalog
 	clearUnknownConditions(expectedStatus)
 	if hasStoredCatalog && r.Storage.ContentExists(catalog.Name) {
-		updateStatusServing(expectedStatus, storedCatalog.ref, storedCatalog.lastUnpack, r.Storage.BaseURL(catalog.Name), storedCatalog.observedGeneration)
+		updateStatusServing(expectedStatus, storedCatalog.ref, storedCatalog.catalogVersion, storedCatalog.lastUnpack, r.Storage.BaseURL(catalog.Name), storedCatalog.observedGeneration)
 		updateStatusProgressing(expectedStatus, storedCatalog.observedGeneration, nil)
 	}
 
@@ -348,11 +398,12 @@ func updateStatusProgressing(status *ocv1.ClusterCatalogStatus, generation int64
 	meta.SetStatusCondition(&status.Conditions, progressingCond)
 }
 
-func updateStatusServing(status *ocv1.ClusterCatalogStatus, ref reference.Canonical, modTime time.Time, baseURL string, generation int64) {
+func updateStatusServing(status *ocv1.ClusterCatalogStatus, ref reference.Canonical, catalogVersion int64, modTime time.Time, baseURL string, generation int64) {
 	status.ResolvedSource = &ocv1.ResolvedCatalogSource{
 		Type: ocv1.SourceTypeImage,
 		Image: &ocv1.ResolvedImageSource{
-			Ref: ref.String(),
+			Ref:            ref.String(),
+			CatalogVersion: catalogVersion,
 		},
 	}
 	status.URLs = &ocv1.ClusterCatalogURLs{
@@ -398,6 +449,13 @@ func updateStatusProgressingUserSpecifiedUnavailable(status *ocv1.ClusterCatalog
 
 func updateStatusNotServing(status *ocv1.ClusterCatalogStatus, generation int64) {
 	status.ResolvedSource = nil
+	updateStatusNotServingWithResolvedSource(status, generation)
+}
+
+// updateStatusNotServingWithResolvedSource marks content unavailable while retaining the last accepted
+// source. In particular, this preserves a catalog publication version so a rejected rollback cannot
+// become acceptable on the next reconcile merely because served content is missing.
+func updateStatusNotServingWithResolvedSource(status *ocv1.ClusterCatalogStatus, generation int64) {
 	status.URLs = nil
 	status.LastUnpacked = nil
 	meta.SetStatusCondition(&status.Conditions, metav1.Condition{

helm/olmv1/base/catalogd/crd/standard/olm.operatorframework.io_clustercatalogs.yaml

@@ -1132,6 +1132,138 @@ func TestPollingReconcilerUnpack(t *testing.T) {
 	}
 }
 
+func TestCatalogVersionRollbackGuard(t *testing.T) {
+	oldRef := mustRef(t, "registry.example.com/catalog@sha256:a5d4f4467250074216eb1ba1c36e06a3ab797d81c431427fc2aca97ecaf4e9d8")
+	newRef := mustRef(t, "registry.example.com/catalog@sha256:f42337e7b85a46d83c94694638e2312e10ca16a03542399a65ba783c94a32b63")
+	sameDigestDifferentRepository := mustRef(t, "mirror.example.com/catalog@sha256:a5d4f4467250074216eb1ba1c36e06a3ab797d81c431427fc2aca97ecaf4e9d8")
+
+	newCatalog := func(current *ocv1.ResolvedImageSource) *ocv1.ClusterCatalog {
+		catalog := &ocv1.ClusterCatalog{
+			ObjectMeta: metav1.ObjectMeta{Name: "catalog", Finalizers: []string{fbcDeletionFinalizer}},
+			Spec: ocv1.ClusterCatalogSpec{Source: ocv1.CatalogSource{
+				Type:  ocv1.SourceTypeImage,
+				Image: &ocv1.ImageSource{Ref: "registry.example.com/catalog:latest"},
+			}},
+		}
+		if current != nil {
+			catalog.Status.ResolvedSource = &ocv1.ResolvedCatalogSource{Type: ocv1.SourceTypeImage, Image: current}
+		}
+		return catalog
+	}
+
+	for name, tc := range map[string]struct {
+		current        *ocv1.ResolvedImageSource
+		pulledRef      reference.Canonical
+		pulledVersion  int64
+		contentExists  bool
+		wantErr        string
+		wantRef        reference.Canonical
+		wantVersion    int64
+		wantNotServing bool
+		secondAttempt  bool
+	}{
+		"accepts first versioned catalog": {
+			pulledRef:     oldRef,
+			pulledVersion: 2,
+			contentExists: true,
+			wantRef:       oldRef,
+			wantVersion:   2,
+		},
+		"accepts higher version for a different digest": {
+			current:       &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:     newRef,
+			pulledVersion: 3,
+			contentExists: true,
+			wantRef:       newRef,
+			wantVersion:   3,
+		},
+		"rejects equal version for a different digest": {
+			current:       &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:     newRef,
+			pulledVersion: 2,
+			contentExists: true,
+			wantErr:       "different digest must have a greater catalog version",
+			wantRef:       oldRef,
+			wantVersion:   2,
+		},
+		"rejects lower version for a different digest": {
+			current:       &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:     newRef,
+			pulledVersion: 1,
+			contentExists: true,
+			wantErr:       "different digest must have a greater catalog version",
+			wantRef:       oldRef,
+			wantVersion:   2,
+		},
+		"rejects unversioned catalog after a versioned catalog": {
+			current:       &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:     newRef,
+			contentExists: true,
+			wantErr:       "catalog version 0",
+			wantRef:       oldRef,
+			wantVersion:   2,
+		},
+		"preserves version for the same digest from a different repository": {
+			current:       &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:     sameDigestDifferentRepository,
+			contentExists: true,
+			wantRef:       sameDigestDifferentRepository,
+			wantVersion:   2,
+		},
+		"rejected catalog remains rejected when content is absent": {
+			current:        &ocv1.ResolvedImageSource{Ref: oldRef.String(), CatalogVersion: 2},
+			pulledRef:      newRef,
+			pulledVersion:  1,
+			contentExists:  false,
+			wantErr:        "different digest must have a greater catalog version",
+			wantRef:        oldRef,
+			wantVersion:    2,
+			wantNotServing: true,
+			secondAttempt:  true,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			store := mockstorage.NewMockInstance(mockCtrl)
+			store.EXPECT().ContentExists("catalog").Return(tc.contentExists).AnyTimes()
+			store.EXPECT().BaseURL("catalog").Return("URL").AnyTimes()
+			if tc.wantErr == "" {
+				store.EXPECT().Store(gomock.Any(), "catalog", gomock.Any()).Return(nil)
+			}
+			reconciler := &ClusterCatalogReconciler{
+				ImagePuller:    &imageutil.FakePuller{ImageFS: fstest.MapFS{}, Ref: tc.pulledRef, CatalogVersion: tc.pulledVersion},
+				ImageCache:     &imageutil.FakeCache{},
+				Storage:        store,
+				storedCatalogs: map[string]storedCatalogData{},
+			}
+			require.NoError(t, reconciler.setupFinalizers())
+			catalog := newCatalog(tc.current)
+
+			_, err := reconciler.reconcile(context.Background(), catalog)
+			if tc.wantErr == "" {
+				require.NoError(t, err)
+			} else {
+				require.ErrorContains(t, err, tc.wantErr)
+			}
+			require.NotNil(t, catalog.Status.ResolvedSource)
+			require.NotNil(t, catalog.Status.ResolvedSource.Image)
+			assert.Equal(t, tc.wantRef.String(), catalog.Status.ResolvedSource.Image.Ref)
+			assert.Equal(t, tc.wantVersion, catalog.Status.ResolvedSource.Image.CatalogVersion)
+			if tc.wantNotServing {
+				assert.Nil(t, catalog.Status.URLs)
+				assert.Nil(t, catalog.Status.LastUnpacked)
+				assert.Equal(t, metav1.ConditionFalse, meta.FindStatusCondition(catalog.Status.Conditions, ocv1.TypeServing).Status)
+			}
+			if tc.secondAttempt {
+				_, err = reconciler.reconcile(context.Background(), catalog)
+				require.ErrorContains(t, err, tc.wantErr)
+				assert.Equal(t, tc.wantRef.String(), catalog.Status.ResolvedSource.Image.Ref)
+				assert.Equal(t, tc.wantVersion, catalog.Status.ResolvedSource.Image.CatalogVersion)
+			}
+		})
+	}
+}
+
 func mustRef(t *testing.T, ref string) reference.Canonical {
 	t.Helper()
 	p, err := reference.Parse(ref)