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 }