Skip to content

Commit e237a5d

Browse files
dtfranzdtfranz
committed
Cleanup, add logging
Signed-off-by: Daniel Franz <dfranz@redhat.com>
1 parent c4fb19f commit e237a5d

2 files changed

Lines changed: 64 additions & 55 deletions

File tree

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

Lines changed: 49 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"sigs.k8s.io/controller-runtime/pkg/client"
2121
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
2222
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
23+
"sigs.k8s.io/controller-runtime/pkg/log"
2324
"sigs.k8s.io/yaml"
2425

2526
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
@@ -35,8 +36,9 @@ const (
3536
)
3637

3738
type ClusterExtensionRevisionGenerator interface {
38-
GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error)
39+
GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error)
3940
GenerateRevisionFromHelmRelease(
41+
ctx context.Context,
4042
helmRelease *release.Release, ext *ocv1.ClusterExtension,
4143
objectLabels map[string]string,
4244
) (*ocv1.ClusterExtensionRevision, error)
@@ -48,6 +50,7 @@ type SimpleRevisionGenerator struct {
4850
}
4951

5052
func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
53+
ctx context.Context,
5154
helmRelease *release.Release, ext *ocv1.ClusterExtension,
5255
objectLabels map[string]string,
5356
) (*ocv1.ClusterExtensionRevision, error) {
@@ -64,11 +67,11 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
6467
maps.Copy(labels, existingLabels)
6568
maps.Copy(labels, objectLabels)
6669
obj.SetLabels(labels)
67-
sanitize(&obj)
6870

6971
// Memory optimization: strip large annotations and managed fields
7072
// Note: ApplyStripTransform never returns an error in practice
7173
_ = cache.ApplyStripTransform(&obj)
74+
stripMetadata(ctx, &obj)
7275

7376
objs = append(objs, ocv1.ClusterExtensionRevisionObject{
7477
Object: obj,
@@ -88,6 +91,7 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
8891
}
8992

9093
func (r *SimpleRevisionGenerator) GenerateRevision(
94+
ctx context.Context,
9195
bundleFS fs.FS, ext *ocv1.ClusterExtension,
9296
objectLabels, revisionAnnotations map[string]string,
9397
) (*ocv1.ClusterExtensionRevision, error) {
@@ -122,8 +126,8 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
122126
if err := cache.ApplyStripTransform(&unstr); err != nil {
123127
return nil, err
124128
}
129+
stripMetadata(ctx, &unstr)
125130

126-
sanitize(&unstr)
127131
objs = append(objs, ocv1.ClusterExtensionRevisionObject{
128132
Object: unstr,
129133
})
@@ -136,32 +140,51 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
136140
return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
137141
}
138142

139-
func sanitize(obj *unstructured.Unstructured) {
143+
var restrictedMetadata = []string{
144+
"finalizers",
145+
"ownerReferences",
146+
"creationTimestamp",
147+
"uid",
148+
"resourceVersion",
149+
"generation",
150+
"managedFields",
151+
"deletionTimestamp",
152+
"deletionGracePeriodSeconds",
153+
}
154+
155+
// stripMetadata takes an unstructured obj and removes any of the above listed fields in-place, if they are present.
156+
// If any of the above fields are found and removed, a warning will be logged.
157+
func stripMetadata(ctx context.Context, obj *unstructured.Unstructured) {
158+
l := log.FromContext(ctx)
140159
m := obj.Object
160+
strippedFields := []string{}
141161

142-
// Remove status (top-level)
143-
delete(m, "status")
162+
// remove status
163+
if status, ok := m["status"].(map[string]any); ok && len(status) > 0 {
164+
strippedFields = append(strippedFields, "status")
165+
delete(m, "status")
166+
}
144167

145-
// Remove common metadata fields set by the API server
168+
var metadata map[string]any
146169
if metaRaw, ok := m["metadata"]; ok {
147-
if meta, ok := metaRaw.(map[string]any); ok {
148-
delete(meta, "finalizers")
149-
delete(meta, "ownerReferences")
150-
delete(meta, "creationTimestamp")
151-
delete(meta, "uid")
152-
delete(meta, "resourceVersion")
153-
delete(meta, "generation")
154-
delete(meta, "managedFields")
155-
delete(meta, "deletionTimestamp")
156-
delete(meta, "deletionGracePeriodSeconds")
157-
158-
// remove kubectl last-applied annotation
159-
if annRaw, ok := meta["annotations"]; ok {
160-
if ann, ok := annRaw.(map[string]any); ok {
161-
delete(ann, "kubectl.kubernetes.io/last-applied-configuration")
162-
}
163-
}
170+
metadata, ok = metaRaw.(map[string]any)
171+
if !ok {
172+
return
164173
}
174+
} else {
175+
return
176+
}
177+
178+
// remove common metadata fields set by the API server
179+
for _, key := range restrictedMetadata {
180+
if _, ok := metadata[key]; ok {
181+
strippedFields = append(strippedFields, key)
182+
delete(metadata, key)
183+
}
184+
}
185+
186+
if len(strippedFields) > 0 {
187+
l.Info("warning: removed restricted metadata fields from manifest", "keys", strippedFields)
165188
}
166189
}
167190

@@ -220,7 +243,7 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
220243
return err
221244
}
222245

223-
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
246+
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels)
224247
if err != nil {
225248
return err
226249
}
@@ -266,7 +289,7 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro
266289

267290
func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
268291
// Generate desired revision
269-
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels, revisionAnnotations)
292+
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations)
270293
if err != nil {
271294
return false, "", err
272295
}

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

Lines changed: 15 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
5252
"managedFields":[{"manager":"test-manager"}],
5353
"deletionTimestamp":{"time":"0"},
5454
"deletionGracePeriodSeconds":30,
55-
"annotations":{
56-
"do-not-delete-me":"please",
57-
"kubectl.kubernetes.io/last-applied-configuration":"delete me"
58-
},
5955
}, "status": {
6056
"replicas": 3,
6157
}
@@ -78,7 +74,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
7874
"my-label": "my-value",
7975
}
8076

81-
rev, err := g.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
77+
rev, err := g.GenerateRevisionFromHelmRelease(context.Background(), helmRelease, ext, objectLabels)
8278
require.NoError(t, err)
8379

8480
assert.Equal(t, &ocv1.ClusterExtensionRevision{
@@ -106,9 +102,6 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
106102
"apiVersion": "v1",
107103
"kind": "ConfigMap",
108104
"metadata": map[string]interface{}{
109-
"annotations": map[string]interface{}{
110-
"do-not-delete-me": "please",
111-
},
112105
"labels": map[string]interface{}{
113106
"my-label": "my-value",
114107
},
@@ -160,11 +153,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
160153
ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "test-manager"}},
161154
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
162155
DeletionGracePeriodSeconds: func(i int64) *int64 { return &i }(30),
163-
Annotations: map[string]string{
164-
// User-set annotations should not be touched
165-
"do-not-delete-me": "please",
166-
"kubectl.kubernetes.io/last-applied-configuration": "delete me",
167-
},
168156
}, Status: appsv1.DeploymentStatus{
169157
Replicas: 3,
170158
},
@@ -184,7 +172,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
184172
},
185173
}
186174

187-
rev, err := b.GenerateRevision(fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
175+
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, ext, map[string]string{}, map[string]string{})
188176
require.NoError(t, err)
189177

190178
t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
@@ -217,9 +205,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
217205
"kind": "Deployment",
218206
"metadata": map[string]interface{}{
219207
"name": "test-deployment",
220-
"annotations": map[string]interface{}{
221-
"do-not-delete-me": "please",
222-
},
223208
},
224209
"spec": map[string]interface{}{
225210
"selector": nil,
@@ -259,7 +244,7 @@ func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
259244
ManifestProvider: r,
260245
}
261246

262-
_, err := b.GenerateRevision(bundleFS, ext, map[string]string{}, map[string]string{})
247+
_, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
263248
require.NoError(t, err)
264249
}
265250

@@ -297,7 +282,7 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
297282
"other": "value",
298283
}
299284

300-
rev, err := b.GenerateRevision(fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
285+
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
301286
"some": "value",
302287
}, revAnnotations)
303288
require.NoError(t, err)
@@ -325,7 +310,7 @@ func Test_SimpleRevisionGenerator_Failure(t *testing.T) {
325310
ManifestProvider: r,
326311
}
327312

328-
rev, err := b.GenerateRevision(fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{})
313+
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{})
329314
require.Nil(t, rev)
330315
t.Log("by checking rendering errors are propagated")
331316
require.Error(t, err)
@@ -402,7 +387,7 @@ func TestBoxcutter_Apply(t *testing.T) {
402387
{
403388
name: "first revision",
404389
mockBuilder: &mockBundleRevisionBuilder{
405-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
390+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
406391
return &ocv1.ClusterExtensionRevision{
407392
ObjectMeta: metav1.ObjectMeta{
408393
Annotations: revisionAnnotations,
@@ -450,7 +435,7 @@ func TestBoxcutter_Apply(t *testing.T) {
450435
{
451436
name: "no change, revision exists",
452437
mockBuilder: &mockBundleRevisionBuilder{
453-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
438+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
454439
return &ocv1.ClusterExtensionRevision{
455440
ObjectMeta: metav1.ObjectMeta{
456441
Annotations: revisionAnnotations,
@@ -496,7 +481,7 @@ func TestBoxcutter_Apply(t *testing.T) {
496481
{
497482
name: "new revision created when objects in new revision are different",
498483
mockBuilder: &mockBundleRevisionBuilder{
499-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
484+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
500485
return &ocv1.ClusterExtensionRevision{
501486
ObjectMeta: metav1.ObjectMeta{
502487
Annotations: revisionAnnotations,
@@ -557,7 +542,7 @@ func TestBoxcutter_Apply(t *testing.T) {
557542
{
558543
name: "error from GenerateRevision",
559544
mockBuilder: &mockBundleRevisionBuilder{
560-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
545+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
561546
return nil, errors.New("render boom")
562547
},
563548
},
@@ -573,7 +558,7 @@ func TestBoxcutter_Apply(t *testing.T) {
573558
{
574559
name: "keep at most 5 past revisions",
575560
mockBuilder: &mockBundleRevisionBuilder{
576-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
561+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
577562
return &ocv1.ClusterExtensionRevision{
578563
ObjectMeta: metav1.ObjectMeta{
579564
Annotations: revisionAnnotations,
@@ -675,7 +660,7 @@ func TestBoxcutter_Apply(t *testing.T) {
675660
{
676661
name: "keep active revisions when they are out of limit",
677662
mockBuilder: &mockBundleRevisionBuilder{
678-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
663+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
679664
return &ocv1.ClusterExtensionRevision{
680665
ObjectMeta: metav1.ObjectMeta{
681666
Annotations: revisionAnnotations,
@@ -933,14 +918,15 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
933918

934919
// mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing.
935920
type mockBundleRevisionBuilder struct {
936-
makeRevisionFunc func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error)
921+
makeRevisionFunc func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error)
937922
}
938923

939-
func (m *mockBundleRevisionBuilder) GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
940-
return m.makeRevisionFunc(bundleFS, ext, objectLabels, revisionAnnotations)
924+
func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
925+
return m.makeRevisionFunc(ctx, bundleFS, ext, objectLabels, revisionAnnotations)
941926
}
942927

943928
func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
929+
ctx context.Context,
944930
helmRelease *release.Release, ext *ocv1.ClusterExtension,
945931
objectLabels map[string]string,
946932
) (*ocv1.ClusterExtensionRevision, error) {

0 commit comments

Comments
 (0)