Skip to content

Commit 0dc0a28

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

2 files changed

Lines changed: 72 additions & 59 deletions

File tree

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

Lines changed: 47 additions & 30 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
7072
// Note: ApplyStripTransform never returns an error in practice
7173
_ = cache.ApplyStripAnnotationsTransform(&obj)
74+
sanitizedUnstructured(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.ApplyStripAnnotationsTransform(&unstr); err != nil {
123127
return nil, err
124128
}
129+
sanitizedUnstructured(ctx, &unstr)
125130

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

139-
func sanitize(obj *unstructured.Unstructured) {
140-
m := obj.Object
141-
142-
// Remove status (top-level)
143-
delete(m, "status")
144-
145-
// Remove common metadata fields set by the API server
146-
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-
}
143+
// sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below.
144+
// If any unallowed entries are removed, a warning will be logged.
145+
func sanitizedUnstructured(ctx context.Context, unstr *unstructured.Unstructured) {
146+
l := log.FromContext(ctx)
147+
obj := unstr.Object
148+
149+
// remove status
150+
if _, ok := obj["status"]; ok {
151+
l.Info("warning: extraneous status removed from manifest")
152+
delete(obj, "status")
153+
}
154+
155+
var allowedMetadata = []string{
156+
"annotations",
157+
"labels",
158+
"name",
159+
"namespace",
160+
}
161+
162+
var metadata map[string]any
163+
if metaRaw, ok := obj["metadata"]; ok {
164+
metadata, ok = metaRaw.(map[string]any)
165+
if !ok {
166+
return
164167
}
168+
} else {
169+
return
170+
}
171+
172+
metadataSanitized := map[string]any{}
173+
for _, key := range allowedMetadata {
174+
if val, ok := metadata[key]; ok {
175+
metadataSanitized[key] = val
176+
}
177+
}
178+
179+
if len(metadataSanitized) != len(metadata) {
180+
l.Info("warning: extraneous values removed from manifest metadata", "allowed metadata", allowedMetadata)
165181
}
182+
obj["metadata"] = metadataSanitized
166183
}
167184

168185
func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
@@ -220,7 +237,7 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
220237
return err
221238
}
222239

223-
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
240+
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels)
224241
if err != nil {
225242
return err
226243
}
@@ -266,7 +283,7 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro
266283

267284
func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
268285
// Generate desired revision
269-
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels, revisionAnnotations)
286+
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations)
270287
if err != nil {
271288
return false, "", err
272289
}

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

Lines changed: 25 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
},
@@ -149,7 +142,10 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
149142
},
150143
&appsv1.Deployment{
151144
ObjectMeta: metav1.ObjectMeta{
152-
Name: "test-deployment",
145+
Name: "test-deployment",
146+
Namespace: "test-ns",
147+
Labels: map[string]string{"my-label": "my-label-value"},
148+
Annotations: map[string]string{"my-annotation": "my-annotation-value"},
153149
// Fields to be sanitized
154150
Finalizers: []string{"test"},
155151
OwnerReferences: []metav1.OwnerReference{{Kind: "TestOwner"}},
@@ -160,11 +156,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
160156
ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "test-manager"}},
161157
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
162158
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-
},
168159
}, Status: appsv1.DeploymentStatus{
169160
Replicas: 3,
170161
},
@@ -184,7 +175,7 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
184175
},
185176
}
186177

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

190181
t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
@@ -216,9 +207,13 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
216207
"apiVersion": "apps/v1",
217208
"kind": "Deployment",
218209
"metadata": map[string]interface{}{
219-
"name": "test-deployment",
210+
"name": "test-deployment",
211+
"namespace": "test-ns",
212+
"labels": map[string]interface{}{
213+
"my-label": "my-label-value",
214+
},
220215
"annotations": map[string]interface{}{
221-
"do-not-delete-me": "please",
216+
"my-annotation": "my-annotation-value",
222217
},
223218
},
224219
"spec": map[string]interface{}{
@@ -259,7 +254,7 @@ func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
259254
ManifestProvider: r,
260255
}
261256

262-
_, err := b.GenerateRevision(bundleFS, ext, map[string]string{}, map[string]string{})
257+
_, err := b.GenerateRevision(t.Context(), bundleFS, ext, map[string]string{}, map[string]string{})
263258
require.NoError(t, err)
264259
}
265260

@@ -297,7 +292,7 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
297292
"other": "value",
298293
}
299294

300-
rev, err := b.GenerateRevision(fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
295+
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{
301296
"some": "value",
302297
}, revAnnotations)
303298
require.NoError(t, err)
@@ -325,7 +320,7 @@ func Test_SimpleRevisionGenerator_Failure(t *testing.T) {
325320
ManifestProvider: r,
326321
}
327322

328-
rev, err := b.GenerateRevision(fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{})
323+
rev, err := b.GenerateRevision(t.Context(), fstest.MapFS{}, &ocv1.ClusterExtension{}, map[string]string{}, map[string]string{})
329324
require.Nil(t, rev)
330325
t.Log("by checking rendering errors are propagated")
331326
require.Error(t, err)
@@ -402,7 +397,7 @@ func TestBoxcutter_Apply(t *testing.T) {
402397
{
403398
name: "first revision",
404399
mockBuilder: &mockBundleRevisionBuilder{
405-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
400+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
406401
return &ocv1.ClusterExtensionRevision{
407402
ObjectMeta: metav1.ObjectMeta{
408403
Annotations: revisionAnnotations,
@@ -450,7 +445,7 @@ func TestBoxcutter_Apply(t *testing.T) {
450445
{
451446
name: "no change, revision exists",
452447
mockBuilder: &mockBundleRevisionBuilder{
453-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
448+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
454449
return &ocv1.ClusterExtensionRevision{
455450
ObjectMeta: metav1.ObjectMeta{
456451
Annotations: revisionAnnotations,
@@ -496,7 +491,7 @@ func TestBoxcutter_Apply(t *testing.T) {
496491
{
497492
name: "new revision created when objects in new revision are different",
498493
mockBuilder: &mockBundleRevisionBuilder{
499-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
494+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
500495
return &ocv1.ClusterExtensionRevision{
501496
ObjectMeta: metav1.ObjectMeta{
502497
Annotations: revisionAnnotations,
@@ -557,7 +552,7 @@ func TestBoxcutter_Apply(t *testing.T) {
557552
{
558553
name: "error from GenerateRevision",
559554
mockBuilder: &mockBundleRevisionBuilder{
560-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
555+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
561556
return nil, errors.New("render boom")
562557
},
563558
},
@@ -573,7 +568,7 @@ func TestBoxcutter_Apply(t *testing.T) {
573568
{
574569
name: "keep at most 5 past revisions",
575570
mockBuilder: &mockBundleRevisionBuilder{
576-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
571+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
577572
return &ocv1.ClusterExtensionRevision{
578573
ObjectMeta: metav1.ObjectMeta{
579574
Annotations: revisionAnnotations,
@@ -675,7 +670,7 @@ func TestBoxcutter_Apply(t *testing.T) {
675670
{
676671
name: "keep active revisions when they are out of limit",
677672
mockBuilder: &mockBundleRevisionBuilder{
678-
makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
673+
makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
679674
return &ocv1.ClusterExtensionRevision{
680675
ObjectMeta: metav1.ObjectMeta{
681676
Annotations: revisionAnnotations,
@@ -933,14 +928,15 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
933928

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

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)
934+
func (m *mockBundleRevisionBuilder) GenerateRevision(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
935+
return m.makeRevisionFunc(ctx, bundleFS, ext, objectLabels, revisionAnnotations)
941936
}
942937

943938
func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
939+
ctx context.Context,
944940
helmRelease *release.Release, ext *ocv1.ClusterExtension,
945941
objectLabels map[string]string,
946942
) (*ocv1.ClusterExtensionRevision, error) {

0 commit comments

Comments
 (0)