Add bundle olm.properties to revision annotations (#2471)

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com>

Author: perdasilva

internal/operator-controller/applier/boxcutter.go

@@ -33,6 +33,7 @@ import (
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
 	"github.com/operator-framework/operator-controller/internal/shared/util/cache"
 )
 
@@ -106,6 +107,27 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
 		return nil, err
 	}
 
+	if revisionAnnotations == nil {
+		revisionAnnotations = map[string]string{}
+	}
+
+	// add bundle properties of interest to revision annotations
+	bundleAnnotations, err := getBundleAnnotations(bundleFS)
+	if err != nil {
+		return nil, fmt.Errorf("error getting bundle annotations: %w", err)
+	}
+
+	// we don't care about all of the bundle and csv annotations as they can be quite confusing
+	// e.g. 'createdAt', 'capabilities', etc.
+	for _, key := range []string{
+		// used by other operators that care about the bundle properties (e.g. maxOpenShiftVersion)
+		source.PropertyOLMProperties,
+	} {
+		if value, ok := bundleAnnotations[key]; ok {
+			revisionAnnotations[key] = value
+		}
+	}
+
 	// objectLabels
 	objs := make([]ocv1.ClusterExtensionRevisionObject, 0, len(plain))
 	for _, obj := range plain {
@@ -137,11 +159,6 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
 			Object: unstr,
 		})
 	}
-
-	if revisionAnnotations == nil {
-		revisionAnnotations = map[string]string{}
-	}
-
 	return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
 }
 

internal/operator-controller/applier/boxcutter_test.go

@@ -36,6 +36,15 @@ import (
 	"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing/bundlefs"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util/testing/clusterserviceversion"
+)
+
+var (
+	dummyBundle = bundlefs.Builder().
+		WithPackageName("test-package").
+		WithCSV(clusterserviceversion.Builder().WithName("test-csv").Build()).
+		Build()
 )
 
 func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) {
@@ -195,7 +204,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
 		},
 	}
 
-	rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
+	rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{})
 	require.NoError(t, err)
 
 	t.Log("by checking the olm.operatorframework.io/owner-name label is set to the name of the ClusterExtension")
@@ -254,8 +263,88 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
 	}, rev.Spec.Phases)
 }
 
+func Test_SimpleRevisionGenerator_GenerateRevision_BundleAnnotations(t *testing.T) {
+	r := &FakeManifestProvider{
+		GetFn: func(_ fs.FS, _ *ocv1.ClusterExtension) ([]client.Object, error) {
+			return []client.Object{}, nil
+		},
+	}
+
+	b := applier.SimpleRevisionGenerator{
+		Scheme:           k8scheme.Scheme,
+		ManifestProvider: r,
+	}
+
+	ext := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-extension",
+		},
+		Spec: ocv1.ClusterExtensionSpec{
+			Namespace: "test-namespace",
+			ServiceAccount: ocv1.ServiceAccountReference{
+				Name: "test-sa",
+			},
+		},
+	}
+
+	t.Run("bundle properties are copied to the olm.properties annotation", func(t *testing.T) {
+		bundleFS := bundlefs.Builder().
+			WithPackageName("test-package").
+			WithBundleProperty("olm.bundle.property", "some-value").
+			WithBundleProperty("olm.another.bundle.property", "some-other-value").
+			WithCSV(clusterserviceversion.Builder().WithName("test-csv").Build()).
+			Build()
+
+		rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
+		require.NoError(t, err)
+
+		t.Log("by checking bundle properties are added to the revision annotations")
+		require.NotNil(t, rev.Annotations)
+		require.JSONEq(t, `[{"type":"olm.bundle.property","value":"some-value"},{"type":"olm.another.bundle.property","value":"some-other-value"}]`, rev.Annotations["olm.properties"])
+	})
+
+	t.Run("olm.properties should not be present if there are no bundle properties", func(t *testing.T) {
+		bundleFS := bundlefs.Builder().
+			WithPackageName("test-package").
+			WithCSV(clusterserviceversion.Builder().WithName("test-csv").Build()).
+			Build()
+
+		rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
+		require.NoError(t, err)
+
+		t.Log("by checking olm.properties is not present in the revision annotations")
+		_, ok := rev.Annotations["olm.properties"]
+		require.False(t, ok, "olm.properties should not be present in the revision annotations")
+	})
+
+	t.Run("csv annotations are not added to the revision annotations", func(t *testing.T) {
+		bundleFS := bundlefs.Builder().
+			WithPackageName("test-package").
+			WithBundleProperty("olm.bundle.property", "some-value").
+			WithCSV(clusterserviceversion.Builder().
+				WithName("test-csv").
+				WithAnnotations(map[string]string{
+					"some.csv.annotation": "some-other-value",
+				}).
+				Build()).
+			Build()
+
+		rev, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
+		require.NoError(t, err)
+
+		t.Log("by checking csv annotations are not added to the revision annotations")
+		_, ok := rev.Annotations["olm.csv.annotation"]
+		require.False(t, ok, "csv annotation should not be present in the revision annotations")
+	})
+
+	t.Run("errors getting bundle properties are surfaced", func(t *testing.T) {
+		_, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "metadata/annotations.yaml: file does not exist")
+	})
+}
+
 func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
-	bundleFS := fstest.MapFS{}
 	ext := &ocv1.ClusterExtension{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-extension",
@@ -264,7 +353,7 @@ func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
 	r := &FakeManifestProvider{
 		GetFn: func(b fs.FS, e *ocv1.ClusterExtension) ([]client.Object, error) {
 			t.Log("by checking renderer was called with the correct parameters")
-			require.Equal(t, bundleFS, b)
+			require.Equal(t, dummyBundle, b)
 			require.Equal(t, ext, e)
 			return nil, nil
 		},
@@ -274,7 +363,7 @@ func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
 		ManifestProvider: r,
 	}
 
-	_, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
+	_, err := b.GenerateRevision(t.Context(), dummyBundle, ext, map[string]string{}, map[string]string{})
 	require.NoError(t, err)
 }
 
@@ -312,7 +401,7 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
 		"other": "value",
 	}
 
-	rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{
+	rev, err := b.GenerateRevision(t.Context(), dummyBundle, &ocv1.ClusterExtension{
 		Spec: ocv1.ClusterExtensionSpec{
 			Namespace:      "test-namespace",
 			ServiceAccount: ocv1.ServiceAccountReference{Name: "test-sa"},
@@ -386,7 +475,7 @@ func Test_SimpleRevisionGenerator_PropagatesProgressDeadlineMinutes(t *testing.T
 				ext.Spec.ProgressDeadlineMinutes = *pd
 			}
 
-			rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, empty, empty)
+			rev, err := b.GenerateRevision(t.Context(), dummyBundle, ext, empty, empty)
 			require.NoError(t, err)
 			require.Equal(t, tc.want.progressDeadlineMinutes, rev.Spec.ProgressDeadlineMinutes)
 		})

internal/operator-controller/applier/provider.go

@@ -85,7 +85,6 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
 			opts = append(opts, render.WithTargetNamespaces(*watchNS))
 		}
 	}
-
 	return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
 }
 
@@ -102,7 +101,7 @@ func (r *RegistryV1HelmChartProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExten
 
 	chrt := &chart.Chart{Metadata: &chart.Metadata{}}
 	// The need to get the underlying bundle in order to extract its annotations
-	// will go away once with have a bundle interface that can surface the annotations independently of the
+	// will go away once we have a bundle interface that can surface the annotations independently of the
 	// underlying bundle format...
 	rv1, err := source.FromFS(bundleFS).GetBundle()
 	if err != nil {
@@ -149,3 +148,14 @@ func extensionConfigBytes(ext *ocv1.ClusterExtension) []byte {
 	}
 	return 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
+	// underlying bundle format...
+	rv1, err := source.FromFS(bundleFS).GetBundle()
+	if err != nil {
+		return nil, err
+	}
+	return rv1.CSV.GetAnnotations(), nil
+}

internal/operator-controller/rukpak/bundle/source/source.go

@@ -20,6 +20,10 @@ import (
 	registry "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/operator-registry"
 )
 
+const (
+	PropertyOLMProperties = "olm.properties"
+)
+
 type BundleSource interface {
 	GetBundle() (bundle.RegistryV1, error)
 }
@@ -173,6 +177,9 @@ func copyMetadataPropertiesToCSV(csv *v1alpha1.ClusterServiceVersion, fsys fs.FS
 	if err != nil {
 		return fmt.Errorf("failed to marshal registry+v1 properties to json: %w", err)
 	}
-	csv.Annotations["olm.properties"] = string(allPropertiesJSON)
+	if csv.Annotations == nil {
+		csv.Annotations = map[string]string{}
+	}
+	csv.Annotations[PropertyOLMProperties] = string(allPropertiesJSON)
 	return nil
 }

test/e2e/features/install.feature

@@ -390,3 +390,30 @@ Feature: Install ClusterExtension
       Revision has not rolled out for 1 minutes.
       """
     And ClusterExtension reports Progressing transition between 1 and 2 minutes since its creation
+
+  @BoxcutterRuntime
+  Scenario:  ClusterExtensionRevision is annotated with bundle properties
+    When ClusterExtension is applied
+      """
+      apiVersion: olm.operatorframework.io/v1
+      kind: ClusterExtension
+      metadata:
+        name: ${NAME}
+      spec:
+        namespace: ${TEST_NAMESPACE}
+        serviceAccount:
+          name: olm-sa
+        source:
+          sourceType: Catalog
+          catalog:
+            packageName: test
+            version: 1.2.0
+            selector:
+              matchLabels:
+                "olm.operatorframework.io/metadata.name": test-catalog
+      """
+    # The annotation key and value come from the bundle's metadata/properties.yaml file
+    Then ClusterExtensionRevision "${NAME}-1" contains annotation "olm.properties" with value
+      """
+      [{"type":"olm.test-property","value":"some-value"}]
+      """

test/e2e/steps/steps.go

@@ -73,6 +73,7 @@ func RegisterSteps(sc *godog.ScenarioContext) {
 	sc.Step(`^(?i)ClusterExtensionRevision "([^"]+)" reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+)$`, ClusterExtensionRevisionReportsConditionWithoutMsg)
 	sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since its creation$`, ClusterExtensionReportsConditionTransitionTime)
 	sc.Step(`^(?i)ClusterExtensionRevision "([^"]+)" is archived$`, ClusterExtensionRevisionIsArchived)
+	sc.Step(`^(?i)ClusterExtensionRevision "([^"]+)" contains annotation "([^"]+)" with value$`, ClusterExtensionRevisionHasAnnotationWithValue)
 
 	sc.Step(`^(?i)resource "([^"]+)" is installed$`, ResourceAvailable)
 	sc.Step(`^(?i)resource "([^"]+)" is available$`, ResourceAvailable)
@@ -485,6 +486,27 @@ func ClusterExtensionRevisionIsArchived(ctx context.Context, revisionName string
 	return waitForCondition(ctx, "clusterextensionrevision", substituteScenarioVars(revisionName, scenarioCtx(ctx)), "Progressing", "False", ptr.To("Archived"), nil)
 }
 
+func ClusterExtensionRevisionHasAnnotationWithValue(ctx context.Context, revisionName, annotationKey string, annotationValue *godog.DocString) error {
+	sc := scenarioCtx(ctx)
+	revisionName = substituteScenarioVars(strings.TrimSpace(revisionName), sc)
+	expectedValue := ""
+	if annotationValue != nil {
+		expectedValue = annotationValue.Content
+	}
+	waitFor(ctx, func() bool {
+		obj, err := getResource("clusterextensionrevision", revisionName, "")
+		if err != nil {
+			logger.V(1).Error(err, "failed to get clusterextensionrevision", "name", revisionName)
+			return false
+		}
+		if obj.GetAnnotations() == nil {
+			return false
+		}
+		return obj.GetAnnotations()[annotationKey] == expectedValue
+	})
+	return nil
+}
+
 func ResourceAvailable(ctx context.Context, resource string) error {
 	sc := scenarioCtx(ctx)
 	resource = substituteScenarioVars(resource, sc)

testdata/images/bundles/test-operator/v1.2.0/metadata/properties.yaml

@@ -0,0 +1,3 @@
+properties:
+  - type: olm.test-property
+    value: "some-value"