Skip to content

Commit dbd066c

Browse files
camilamacedo86camilamacedo86
committed
(fix):ClusterExtensionRevision had inconsistent owner labeling and confusing code.
1 parent 6ef62de commit dbd066c

5 files changed

Lines changed: 38 additions & 28 deletions

File tree

‎internal/operator-controller/applier/boxcutter.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,12 +179,16 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
179179
ext *ocv1.ClusterExtension,
180180
annotations map[string]string,
181181
) *ocv1.ClusterExtensionRevision {
182+
// Build owner labels - use owner-name + owner-kind for consistency with managed objects
183+
revisionLabels := map[string]string{
184+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
185+
labels.OwnerNameKey: ext.Name,
186+
}
187+
182188
return &ocv1.ClusterExtensionRevision{
183189
ObjectMeta: metav1.ObjectMeta{
190+
Labels: revisionLabels,
184191
Annotations: annotations,
185-
Labels: map[string]string{
186-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
187-
},
188192
},
189193
Spec: ocv1.ClusterExtensionRevisionSpec{
190194
// Explicitly set LifecycleState to Active. While the CRD has a default,

‎internal/operator-controller/applier/boxcutter_test.go‎

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
8686
"olm.operatorframework.io/package-name": "my-package",
8787
},
8888
Labels: map[string]string{
89-
"olm.operatorframework.io/owner": "test-123",
89+
"olm.operatorframework.io/owner-kind": "ClusterExtension",
90+
"olm.operatorframework.io/owner-name": "test-123",
9091
},
9192
},
9293
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -178,9 +179,10 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
178179
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
179180
require.NoError(t, err)
180181

181-
t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
182+
t.Log("by checking the owner labels are set correctly (owner-kind and owner-name)")
182183
require.Equal(t, map[string]string{
183-
controllers.ClusterExtensionRevisionOwnerLabel: "test-extension",
184+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
185+
labels.OwnerNameKey: "test-extension",
184186
}, rev.Labels)
185187
t.Log("by checking the revision number is 0")
186188
require.Equal(t, int64(0), rev.Spec.Revision)
@@ -288,13 +290,13 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
288290
ManifestProvider: r,
289291
}
290292

291-
revAnnotations := map[string]string{
293+
revisionAnnotations := map[string]string{
292294
"other": "value",
293295
}
294296

295297
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
296298
"some": "value",
297-
}, revAnnotations)
299+
}, revisionAnnotations)
298300
require.NoError(t, err)
299301
t.Log("by checking the rendered objects contain the given object labels")
300302
for _, phase := range rev.Spec.Phases {
@@ -305,8 +307,8 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
305307
}, revObj.Object.GetLabels())
306308
}
307309
}
308-
t.Log("by checking the generated revision contain the given annotations")
309-
require.Equal(t, revAnnotations, rev.Annotations)
310+
t.Log("by checking the generated revision contains the given annotations")
311+
require.Equal(t, revisionAnnotations, rev.Annotations)
310312
}
311313

312314
func Test_SimpleRevisionGenerator_Failure(t *testing.T) {
@@ -344,7 +346,8 @@ func TestBoxcutter_Apply(t *testing.T) {
344346
Name: "test-ext-1",
345347
UID: "rev-uid-1",
346348
Labels: map[string]string{
347-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
349+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
350+
labels.OwnerNameKey: ext.Name,
348351
},
349352
},
350353
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -400,10 +403,11 @@ func TestBoxcutter_Apply(t *testing.T) {
400403
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
401404
return &ocv1.ClusterExtensionRevision{
402405
ObjectMeta: metav1.ObjectMeta{
403-
Annotations: revisionAnnotations,
404406
Labels: map[string]string{
405-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
407+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
408+
labels.OwnerNameKey: ext.Name,
406409
},
410+
Annotations: revisionAnnotations,
407411
},
408412
Spec: ocv1.ClusterExtensionRevisionSpec{
409413
Phases: []ocv1.ClusterExtensionRevisionPhase{
@@ -787,14 +791,15 @@ func TestBoxcutter_Apply(t *testing.T) {
787791
},
788792
},
789793
{
790-
name: "annotation-only update (same phases, different annotations)",
794+
name: "annotation update (same phases, different metadata)",
791795
mockBuilder: &mockBundleRevisionBuilder{
792796
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
793797
return &ocv1.ClusterExtensionRevision{
794798
ObjectMeta: metav1.ObjectMeta{
795799
Annotations: revisionAnnotations,
796800
Labels: map[string]string{
797-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
801+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
802+
labels.OwnerNameKey: ext.Name,
798803
},
799804
},
800805
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -830,7 +835,8 @@ func TestBoxcutter_Apply(t *testing.T) {
830835
labels.PackageNameKey: "test-package",
831836
},
832837
Labels: map[string]string{
833-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
838+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
839+
labels.OwnerNameKey: ext.Name,
834840
},
835841
},
836842
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -870,7 +876,8 @@ func TestBoxcutter_Apply(t *testing.T) {
870876
assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey])
871877
assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey])
872878
// Verify owner label is still present
873-
assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel])
879+
assert.Equal(t, ext.Name, rev.Labels[labels.OwnerNameKey])
880+
assert.Equal(t, ocv1.ClusterExtensionKind, rev.Labels[labels.OwnerKindKey])
874881
},
875882
},
876883
}
@@ -896,8 +903,8 @@ func TestBoxcutter_Apply(t *testing.T) {
896903

897904
// Execute
898905
revisionAnnotations := map[string]string{}
899-
if tc.name == "annotation-only update (same phases, different annotations)" {
900-
// For annotation-only update test, pass NEW annotations
906+
if tc.name == "annotation update (same phases, different metadata)" {
907+
// For annotation update test, pass NEW metadata in annotations
901908
revisionAnnotations = map[string]string{
902909
labels.BundleVersionKey: "1.0.1",
903910
labels.PackageNameKey: "test-package",
@@ -1045,7 +1052,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
10451052

10461053
// mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing.
10471054
type mockBundleRevisionBuilder struct {
1048-
makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error)
1055+
makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error)
10491056
}
10501057

10511058
func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
@@ -1061,7 +1068,8 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
10611068
ObjectMeta: metav1.ObjectMeta{
10621069
Name: "test-revision",
10631070
Labels: map[string]string{
1064-
controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
1071+
labels.OwnerKindKey: ocv1.ClusterExtensionKind,
1072+
labels.OwnerNameKey: ext.Name,
10651073
},
10661074
},
10671075
Spec: ocv1.ClusterExtensionRevisionSpec{},

‎internal/operator-controller/applier/helm.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
102102
return nil
103103
}
104104

105-
func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) (bool, string, error) {
105+
func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, storageLabels map[string]string) (bool, string, error) {
106106
chrt, err := h.buildHelmChart(contentFS, ext)
107107
if err != nil {
108108
return false, "", err

‎internal/operator-controller/controllers/clusterextension_controller.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
280280
return ctrl.Result{}, err
281281
}
282282

283-
storeLbls := map[string]string{
283+
revisionAnnotations := map[string]string{
284284
labels.BundleNameKey: resolvedRevisionMetadata.Name,
285285
labels.PackageNameKey: resolvedRevisionMetadata.Package,
286286
labels.BundleVersionKey: resolvedRevisionMetadata.Version,
@@ -297,7 +297,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
297297
// to ensure exponential backoff can occur:
298298
// - Permission errors (it is not possible to watch changes to permissions.
299299
// The only way to eventually recover from permission errors is to keep retrying).
300-
rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, storeLbls)
300+
rolloutSucceeded, rolloutStatus, err := r.Applier.Apply(ctx, imageFS, ext, objLbls, revisionAnnotations)
301301

302302
// Set installed status
303303
if rolloutSucceeded {
@@ -549,9 +549,7 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
549549
continue
550550
}
551551

552-
// TODO: the setting of these annotations (happens in boxcutter applier when we pass in "revisionAnnotations")
553-
// is fairly decoupled from this code where we get the annotations back out. We may want to co-locate
554-
// the set/get logic a bit better to make it more maintainable and less likely to get out of sync.
552+
// Read bundle metadata from annotations (informational, not queried)
555553
rm := &RevisionMetadata{
556554
Package: rev.Annotations[labels.PackageNameKey],
557555
Image: rev.Annotations[labels.BundleReferenceKey],

‎internal/operator-controller/controllers/clusterextensionrevision_controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import (
3636
)
3737

3838
const (
39-
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner"
39+
ClusterExtensionRevisionOwnerLabel = "olm.operatorframework.io/owner-name"
4040
clusterExtensionRevisionTeardownFinalizer = "olm.operatorframework.io/teardown"
4141
)
4242

0 commit comments

Comments
 (0)