From 5fafe40b6e8a91e1c5a12394ff16b19a9c3e8b77 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Tue, 18 Nov 2025 18:37:15 +0000 Subject: [PATCH] fix: Use labels for bundle metadata and unify owner labels Fix bundle metadata storage to use labels instead of annotations, following Kubernetes best practices for queryable metadata. This enables kubectl queries like: kubectl get clusterextensionrevisions -l package-name=prometheus Also fixes inconsistent owner labels - now uses owner-name + owner-kind everywhere (previously ClusterExtensionRevision used just owner). Fixed issues: - Bundle metadata was in annotations (not queryable/indexed) - Owner labels were inconsistent across object types - Variable storeLbls was misleadingly named (actually annotations) Boxcutter runtime only - Helm unchanged except interface signature. Refs: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels Assisted-by: Cursor --- .../operator-controller/applier/boxcutter.go | 60 +++++---- .../applier/boxcutter_test.go | 117 +++++++++++------- internal/operator-controller/applier/helm.go | 7 +- .../operator-controller/applier/helm_test.go | 38 +++--- .../clusterextension_controller.go | 30 ++--- .../clusterextensionrevision_controller.go | 2 +- .../controllers/suite_test.go | 2 +- 7 files changed, 151 insertions(+), 105 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 1608c981e0..102586d0b5 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -35,11 +35,11 @@ const ( ) type ClusterExtensionRevisionGenerator interface { - GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) + GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) GenerateRevisionFromHelmRelease( ctx context.Context, helmRelease *release.Release, ext *ocv1.ClusterExtension, - objectLabels map[string]string, + objectLabels, revisionLabels map[string]string, ) (*ocv1.ClusterExtensionRevision, error) } @@ -51,7 +51,7 @@ type SimpleRevisionGenerator struct { func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( ctx context.Context, helmRelease *release.Release, ext *ocv1.ClusterExtension, - objectLabels map[string]string, + objectLabels, revisionLabels map[string]string, ) (*ocv1.ClusterExtensionRevision, error) { docs := splitManifestDocuments(helmRelease.Manifest) objs := make([]ocv1.ClusterExtensionRevisionObject, 0, len(docs)) @@ -75,12 +75,19 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( }) } - rev := r.buildClusterExtensionRevision(objs, ext, map[string]string{ - labels.BundleNameKey: helmRelease.Labels[labels.BundleNameKey], - labels.PackageNameKey: helmRelease.Labels[labels.PackageNameKey], - labels.BundleVersionKey: helmRelease.Labels[labels.BundleVersionKey], + // Merge provided revision labels with bundle metadata from Helm release + allRevisionLabels := make(map[string]string, len(revisionLabels)+4) + maps.Copy(allRevisionLabels, revisionLabels) + allRevisionLabels[labels.BundleNameKey] = helmRelease.Labels[labels.BundleNameKey] + allRevisionLabels[labels.PackageNameKey] = helmRelease.Labels[labels.PackageNameKey] + allRevisionLabels[labels.BundleVersionKey] = helmRelease.Labels[labels.BundleVersionKey] + + // Bundle reference goes to annotations (can be too long for labels) + revisionAnnotations := map[string]string{ labels.BundleReferenceKey: helmRelease.Labels[labels.BundleReferenceKey], - }) + } + + rev := r.buildClusterExtensionRevision(objs, ext, allRevisionLabels, revisionAnnotations) rev.Name = fmt.Sprintf("%s-1", ext.Name) rev.Spec.Revision = 1 return rev, nil @@ -89,7 +96,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease( func (r *SimpleRevisionGenerator) GenerateRevision( ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, - objectLabels, revisionAnnotations map[string]string, + objectLabels, revisionLabels, revisionAnnotations map[string]string, ) (*ocv1.ClusterExtensionRevision, error) { // extract plain manifests plain, err := r.ManifestProvider.Get(bundleFS, ext) @@ -97,7 +104,7 @@ func (r *SimpleRevisionGenerator) GenerateRevision( return nil, err } - // objectLabels + // Apply objectLabels to each managed object objs := make([]ocv1.ClusterExtensionRevisionObject, 0, len(plain)) for _, obj := range plain { existingLabels := obj.GetLabels() @@ -125,11 +132,14 @@ func (r *SimpleRevisionGenerator) GenerateRevision( }) } + if revisionLabels == nil { + revisionLabels = map[string]string{} + } if revisionAnnotations == nil { revisionAnnotations = map[string]string{} } - return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil + return r.buildClusterExtensionRevision(objs, ext, revisionLabels, revisionAnnotations), nil } // sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below. @@ -177,14 +187,20 @@ func sanitizedUnstructured(ctx context.Context, unstr *unstructured.Unstructured func (r *SimpleRevisionGenerator) buildClusterExtensionRevision( objects []ocv1.ClusterExtensionRevisionObject, ext *ocv1.ClusterExtension, - annotations map[string]string, + revisionLabels map[string]string, + revisionAnnotations map[string]string, ) *ocv1.ClusterExtensionRevision { + // Build labels: owner labels + provided revision labels (package, bundle, version, etc.) + // Use owner-name + owner-kind for consistency with managed objects + allLabels := make(map[string]string, len(revisionLabels)+2) + allLabels[labels.OwnerKindKey] = ocv1.ClusterExtensionKind + allLabels[labels.OwnerNameKey] = ext.Name + maps.Copy(allLabels, revisionLabels) + return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ - Annotations: annotations, - Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, - }, + Labels: allLabels, + Annotations: revisionAnnotations, }, Spec: ocv1.ClusterExtensionRevisionSpec{ // Explicitly set LifecycleState to Active. While the CRD has a default, @@ -240,7 +256,9 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste return err } - rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels) + // revisionLabels will be merged with bundle metadata from Helm release + revisionLabels := map[string]string{} + rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels, revisionLabels) if err != nil { return err } @@ -284,8 +302,8 @@ type Boxcutter struct { FieldOwner string } -func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) { - return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations) +func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (bool, string, error) { + return bc.apply(ctx, contentFS, ext, objectLabels, revisionLabels, revisionAnnotations) } func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object { @@ -309,9 +327,9 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) } -func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) { +func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (bool, string, error) { // Generate desired revision - desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations) + desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionLabels, revisionAnnotations) if err != nil { return false, "", err } diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index d80f83da6c..8983b30541 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -72,21 +72,23 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) objectLabels := map[string]string{ "my-label": "my-value", } + revisionLabels := map[string]string{} - rev, err := g.GenerateRevisionFromHelmRelease(t.Context(), helmRelease, ext, objectLabels) + rev, err := g.GenerateRevisionFromHelmRelease(t.Context(), helmRelease, ext, objectLabels, revisionLabels) require.NoError(t, err) assert.Equal(t, &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "test-123-1", + Labels: map[string]string{ + "olm.operatorframework.io/owner-kind": "ClusterExtension", + "olm.operatorframework.io/owner-name": "test-123", + "olm.operatorframework.io/package-name": "my-package", + "olm.operatorframework.io/bundle-name": "my-bundle", + "olm.operatorframework.io/bundle-version": "1.2.0", + }, Annotations: map[string]string{ - "olm.operatorframework.io/bundle-name": "my-bundle", "olm.operatorframework.io/bundle-reference": "bundle-ref", - "olm.operatorframework.io/bundle-version": "1.2.0", - "olm.operatorframework.io/package-name": "my-package", - }, - Labels: map[string]string{ - "olm.operatorframework.io/owner": "test-123", }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ @@ -175,12 +177,13 @@ 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(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{}, map[string]string{}) require.NoError(t, err) - t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension") + t.Log("by checking the owner labels are set correctly (owner-kind and owner-name)") require.Equal(t, map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: "test-extension", + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: "test-extension", }, rev.Labels) t.Log("by checking the revision number is 0") require.Equal(t, int64(0), rev.Spec.Revision) @@ -254,7 +257,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(), bundleFS, ext, map[string]string{}, map[string]string{}, map[string]string{}) require.NoError(t, err) } @@ -288,13 +291,17 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t ManifestProvider: r, } - revAnnotations := map[string]string{ + revisionLabels := map[string]string{ + "bundle": "label", + } + + revisionAnnotations := map[string]string{ "other": "value", } rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{ "some": "value", - }, revAnnotations) + }, revisionLabels, revisionAnnotations) require.NoError(t, err) t.Log("by checking the rendered objects contain the given object labels") for _, phase := range rev.Spec.Phases { @@ -305,8 +312,13 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t }, revObj.Object.GetLabels()) } } - t.Log("by checking the generated revision contain the given annotations") - require.Equal(t, revAnnotations, rev.Annotations) + t.Log("by checking the generated revision contains the given labels plus owner labels") + require.Contains(t, rev.Labels, "bundle") + require.Equal(t, "label", rev.Labels["bundle"]) + require.Equal(t, ocv1.ClusterExtensionKind, rev.Labels[labels.OwnerKindKey]) + require.Empty(t, rev.Labels[labels.OwnerNameKey]) // Empty because ext name is empty in test + t.Log("by checking the generated revision contains the given annotations") + require.Equal(t, revisionAnnotations, rev.Annotations) } func Test_SimpleRevisionGenerator_Failure(t *testing.T) { @@ -320,7 +332,7 @@ func Test_SimpleRevisionGenerator_Failure(t *testing.T) { ManifestProvider: r, } - rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{}) + rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{}, map[string]string{}) require.Nil(t, rev) t.Log("by checking rendering errors are propagated") require.Error(t, err) @@ -344,7 +356,8 @@ func TestBoxcutter_Apply(t *testing.T) { Name: "test-ext-1", UID: "rev-uid-1", Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ @@ -397,13 +410,14 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "first revision", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ - Annotations: revisionAnnotations, Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, }, + Annotations: revisionAnnotations, }, Spec: ocv1.ClusterExtensionRevisionSpec{ Phases: []ocv1.ClusterExtensionRevisionPhase{ @@ -445,7 +459,7 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "no change, revision exists", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Annotations: revisionAnnotations, @@ -491,7 +505,7 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "new revision created when objects in new revision are different", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Annotations: revisionAnnotations, @@ -549,7 +563,7 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "error from GenerateRevision", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return nil, errors.New("render boom") }, }, @@ -565,7 +579,7 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "keep at most 5 past revisions", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Annotations: revisionAnnotations, @@ -669,7 +683,7 @@ func TestBoxcutter_Apply(t *testing.T) { { name: "keep active revisions when they are out of limit", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Annotations: revisionAnnotations, @@ -787,15 +801,20 @@ func TestBoxcutter_Apply(t *testing.T) { }, }, { - name: "annotation-only update (same phases, different annotations)", + name: "label/annotation update (same phases, different metadata)", mockBuilder: &mockBundleRevisionBuilder{ - makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + // Merge provided revision labels with owner labels + allLabels := make(map[string]string, len(revisionLabels)+2) + allLabels[labels.OwnerKindKey] = ocv1.ClusterExtensionKind + allLabels[labels.OwnerNameKey] = ext.Name + for k, v := range revisionLabels { + allLabels[k] = v + } return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ + Labels: allLabels, Annotations: revisionAnnotations, - Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, - }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ Phases: []ocv1.ClusterExtensionRevisionPhase{ @@ -825,13 +844,12 @@ func TestBoxcutter_Apply(t *testing.T) { &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "test-ext-1", - Annotations: map[string]string{ + Labels: map[string]string{ + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, labels.BundleVersionKey: "1.0.0", labels.PackageNameKey: "test-package", }, - Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, - }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ Revision: 1, @@ -866,11 +884,12 @@ func TestBoxcutter_Apply(t *testing.T) { rev := revList.Items[0] assert.Equal(t, "test-ext-1", rev.Name) assert.Equal(t, int64(1), rev.Spec.Revision) - // Verify annotations were updated - assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey]) - assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey]) - // Verify owner label is still present - assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel]) + // Verify labels were updated (bundle metadata is now in labels, not annotations) + assert.Equal(t, "1.0.1", rev.Labels[labels.BundleVersionKey]) + assert.Equal(t, "test-package", rev.Labels[labels.PackageNameKey]) + // Verify owner labels are still present + assert.Equal(t, ext.Name, rev.Labels[labels.OwnerNameKey]) + assert.Equal(t, ocv1.ClusterExtensionKind, rev.Labels[labels.OwnerKindKey]) }, }, } @@ -895,15 +914,16 @@ func TestBoxcutter_Apply(t *testing.T) { testFS := fstest.MapFS{} // Execute + revisionLabels := map[string]string{} revisionAnnotations := map[string]string{} - if tc.name == "annotation-only update (same phases, different annotations)" { - // For annotation-only update test, pass NEW annotations - revisionAnnotations = map[string]string{ + if tc.name == "label/annotation update (same phases, different metadata)" { + // For label/annotation update test, pass NEW metadata + revisionLabels = map[string]string{ labels.BundleVersionKey: "1.0.1", labels.PackageNameKey: "test-package", } } - installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations) + installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionLabels, revisionAnnotations) // Assert if tc.expectedErr != "" { @@ -1045,23 +1065,24 @@ func TestBoxcutterStorageMigrator(t *testing.T) { // mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing. type mockBundleRevisionBuilder struct { - makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error) + makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) } -func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { - return m.makeRevisionFunc(ctx, bundleFS, ext, objectLabels, revisionAnnotations) +func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + return m.makeRevisionFunc(ctx, bundleFS, ext, objectLabels, revisionLabels, revisionAnnotations) } func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease( ctx context.Context, helmRelease *release.Release, ext *ocv1.ClusterExtension, - objectLabels map[string]string, + objectLabels, revisionLabels map[string]string, ) (*ocv1.ClusterExtensionRevision, error) { return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "test-revision", Labels: map[string]string{ - controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + labels.OwnerKindKey: ocv1.ClusterExtensionKind, + labels.OwnerNameKey: ext.Name, }, }, Spec: ocv1.ClusterExtensionRevisionSpec{}, diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 4e70268941..0879578dde 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -102,7 +102,12 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE return nil } -func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) (bool, string, error) { +// Apply applies Helm chart content. The revisionAnnotations parameter is ignored for Helm +// (only used by boxcutter runtime which uses ClusterExtensionRevision). +func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels, revisionAnnotations map[string]string) (bool, string, error) { + // revisionAnnotations is ignored - Helm doesn't use ClusterExtensionRevision + _ = revisionAnnotations + chrt, err := h.buildHelmChart(contentFS, ext) if err != nil { return false, "", err diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 8d07de9123..872821c1d7 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -229,7 +229,7 @@ func TestApply_Base(t *testing.T) { t.Run("fails converting content FS to helm chart", func(t *testing.T) { helmApplier := applier.Helm{} - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), os.DirFS("/"), testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), os.DirFS("/"), testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.False(t, installSucceeded) require.Empty(t, installStatus) @@ -242,7 +242,7 @@ func TestApply_Base(t *testing.T) { HelmChartProvider: DummyHelmChartProvider, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "getting action client") require.False(t, installSucceeded) @@ -256,7 +256,7 @@ func TestApply_Base(t *testing.T) { HelmChartProvider: DummyHelmChartProvider, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "getting current release") require.False(t, installSucceeded) @@ -275,7 +275,7 @@ func TestApply_Installation(t *testing.T) { HelmChartProvider: DummyHelmChartProvider, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "attempting to dry-run install chart") require.False(t, installSucceeded) @@ -295,7 +295,7 @@ func TestApply_Installation(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "install pre-flight check") require.False(t, installSucceeded) @@ -313,7 +313,7 @@ func TestApply_Installation(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "installing chart") require.False(t, installSucceeded) @@ -337,7 +337,7 @@ func TestApply_Installation(t *testing.T) { }, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.NoError(t, err) require.Empty(t, installStatus) require.True(t, installSucceeded) @@ -355,7 +355,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { HelmChartProvider: DummyHelmChartProvider, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "attempting to dry-run install chart") require.False(t, installSucceeded) @@ -380,7 +380,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "install pre-flight check") require.False(t, installSucceeded) @@ -409,7 +409,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { }, }, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "problem running preauthorization") require.False(t, installSucceeded) @@ -438,7 +438,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { }, }, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, errMissingRBAC) require.False(t, installSucceeded) @@ -473,7 +473,7 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { }, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, validCE, testObjectLabels, testStorageLabels, map[string]string{}) require.NoError(t, err) require.Empty(t, installStatus) require.True(t, installSucceeded) @@ -494,7 +494,7 @@ func TestApply_Upgrade(t *testing.T) { HelmChartProvider: DummyHelmChartProvider, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "attempting to dry-run upgrade chart") require.False(t, installSucceeded) @@ -518,7 +518,7 @@ func TestApply_Upgrade(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "upgrade pre-flight check") require.False(t, installSucceeded) @@ -541,7 +541,7 @@ func TestApply_Upgrade(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "upgrading chart") require.False(t, installSucceeded) @@ -565,7 +565,7 @@ func TestApply_Upgrade(t *testing.T) { HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.Error(t, err) require.ErrorContains(t, err, "reconciling charts") require.False(t, installSucceeded) @@ -589,7 +589,7 @@ func TestApply_Upgrade(t *testing.T) { }, } - installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + installSucceeded, installStatus, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.NoError(t, err) require.True(t, installSucceeded) require.Empty(t, installStatus) @@ -618,7 +618,7 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { }, } - _, _, _ = helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + _, _, _ = helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) }) t.Run("surfaces chart generation errors", func(t *testing.T) { @@ -640,7 +640,7 @@ func TestApply_RegistryV1ToChartConverterIntegration(t *testing.T) { }, } - _, _, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + _, _, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels, map[string]string{}) require.ErrorContains(t, err, "some error") }) } diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index e8f035193c..6efa40347c 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -82,9 +82,9 @@ type StorageMigrator interface { type Applier interface { // Apply applies the content in the provided fs.FS using the configuration of the provided ClusterExtension. - // It also takes in a map[string]string to be applied to all applied resources as labels and another - // map[string]string used to create a unique identifier for a stored reference to the resources created. - Apply(context.Context, fs.FS, *ocv1.ClusterExtension, map[string]string, map[string]string) (bool, string, error) + // It takes objectLabels to be applied to all managed resources, revisionLabels to be applied to the + // ClusterExtensionRevision for selection/querying, and revisionAnnotations for non-queryable metadata. + Apply(context.Context, fs.FS, *ocv1.ClusterExtension, map[string]string, map[string]string, map[string]string) (bool, string, error) } type RevisionStatesGetter interface { @@ -280,10 +280,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl return ctrl.Result{}, err } - storeLbls := map[string]string{ - labels.BundleNameKey: resolvedRevisionMetadata.Name, - labels.PackageNameKey: resolvedRevisionMetadata.Package, - labels.BundleVersionKey: resolvedRevisionMetadata.Version, + // Split bundle metadata into labels (for selection) and annotations (for reference data) + revisionLabels := map[string]string{ + labels.BundleNameKey: resolvedRevisionMetadata.Name, + labels.PackageNameKey: resolvedRevisionMetadata.Package, + labels.BundleVersionKey: resolvedRevisionMetadata.Version, + } + + revisionAnnotations := map[string]string{ labels.BundleReferenceKey: resolvedRevisionMetadata.Image, } @@ -297,7 +301,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl // to ensure exponential backoff can occur: // - Permission errors (it is not possible to watch changes to permissions. // The only way to eventually recover from permission errors is to keep retrying). - rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, storeLbls) + rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, revisionLabels, revisionAnnotations) // Set installed status if rolloutSucceeded { @@ -549,15 +553,13 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e continue } - // TODO: the setting of these annotations (happens in boxcutter applier when we pass in "revisionAnnotations") - // is fairly decoupled from this code where we get the annotations back out. We may want to co-locate - // the set/get logic a bit better to make it more maintainable and less likely to get out of sync. + // Read bundle metadata from labels (queryable fields) and annotations (reference data) rm := &RevisionMetadata{ - Package: rev.Annotations[labels.PackageNameKey], + Package: rev.Labels[labels.PackageNameKey], Image: rev.Annotations[labels.BundleReferenceKey], BundleMetadata: ocv1.BundleMetadata{ - Name: rev.Annotations[labels.BundleNameKey], - Version: rev.Annotations[labels.BundleVersionKey], + Name: rev.Labels[labels.BundleNameKey], + Version: rev.Labels[labels.BundleVersionKey], }, } diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 589ee47a76..a81faa897e 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -36,7 +36,7 @@ import ( ) const ( - ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner" + ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner-name" clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown" ) diff --git a/internal/operator-controller/controllers/suite_test.go b/internal/operator-controller/controllers/suite_test.go index 02d5382371..56bfd5ce2a 100644 --- a/internal/operator-controller/controllers/suite_test.go +++ b/internal/operator-controller/controllers/suite_test.go @@ -72,7 +72,7 @@ type MockApplier struct { err error } -func (m *MockApplier) Apply(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _ map[string]string, _ map[string]string) (bool, string, error) { +func (m *MockApplier) Apply(_ context.Context, _ fs.FS, _ *ocv1.ClusterExtension, _ map[string]string, _ map[string]string, _ map[string]string) (bool, string, error) { return m.installCompleted, m.installStatus, m.err }