Skip to content

Commit c95fc24

Browse files
dtfranzdtfranz
authored
🌱 Revision manifest sanitization (#2319)
* Removes read-only or otherwise ill-advised fields from the manifests applied by revisions. Assisted-by: Gemini+Claude Signed-off-by: Daniel Franz <dfranz@redhat.com> * Cleanup, add logging Signed-off-by: Daniel Franz <dfranz@redhat.com> --------- Signed-off-by: Daniel Franz <dfranz@redhat.com>
1 parent be43e58 commit c95fc24

File tree

2 files changed

+110
-28
lines changed

2 files changed

+110
-28
lines changed

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

Lines changed: 51 additions & 4 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-
obj.SetOwnerReferences(nil) // reset OwnerReferences for migration.
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,6 +126,7 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
122126
if err := cache.ApplyStripAnnotationsTransform(&unstr); err != nil {
123127
return nil, err
124128
}
129+
sanitizedUnstructured(ctx, &unstr)
125130

126131
objs = append(objs, ocv1.ClusterExtensionRevisionObject{
127132
Object: unstr,
@@ -135,6 +140,48 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
135140
return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
136141
}
137142

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
167+
}
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)
181+
}
182+
obj["metadata"] = metadataSanitized
183+
}
184+
138185
func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
139186
objects []ocv1.ClusterExtensionRevisionObject,
140187
ext *ocv1.ClusterExtension,
@@ -190,7 +237,7 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
190237
return err
191238
}
192239

193-
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
240+
rev, err := m.RevisionGenerator.GenerateRevisionFromHelmRelease(ctx, helmRelease, ext, objectLabels)
194241
if err != nil {
195242
return err
196243
}
@@ -236,7 +283,7 @@ func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) erro
236283

237284
func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
238285
// Generate desired revision
239-
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(contentFS, ext, objectLabels, revisionAnnotations)
286+
desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations)
240287
if err != nil {
241288
return false, "", err
242289
}

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

Lines changed: 59 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"fmt"
77
"io/fs"
8+
"strings"
89
"testing"
910
"testing/fstest"
1011

@@ -36,8 +37,25 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
3637
g := &applier.SimpleRevisionGenerator{}
3738

3839
helmRelease := &release.Release{
39-
Name: "test-123",
40-
Manifest: `{"apiVersion":"v1","kind":"ConfigMap"}` + "\n" + `{"apiVersion":"v1","kind":"Secret"}` + "\n",
40+
Name: "test-123",
41+
Manifest: strings.Join(strings.Fields(`
42+
{
43+
"apiVersion":"v1",
44+
"kind":"ConfigMap",
45+
"metadata":{
46+
"finalizers":["test"],
47+
"ownerReferences":[{"kind":"TestOwner"}],
48+
"creationTimestamp":{"time":"0"},
49+
"uid":"1a2b3c4d",
50+
"resourceVersion":"12345",
51+
"generation":123,
52+
"managedFields":[{"manager":"test-manager"}],
53+
"deletionTimestamp":{"time":"0"},
54+
"deletionGracePeriodSeconds":30,
55+
}, "status": {
56+
"replicas": 3,
57+
}
58+
}`), "") + "\n" + `{"apiVersion":"v1","kind":"Secret"}` + "\n",
4159
Labels: map[string]string{
4260
labels.BundleNameKey: "my-bundle",
4361
labels.PackageNameKey: "my-package",
@@ -56,7 +74,7 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
5674
"my-label": "my-value",
5775
}
5876

59-
rev, err := g.GenerateRevisionFromHelmRelease(helmRelease, ext, objectLabels)
77+
rev, err := g.GenerateRevisionFromHelmRelease(t.Context(), helmRelease, ext, objectLabels)
6078
require.NoError(t, err)
6179

6280
assert.Equal(t, &ocv1.ClusterExtensionRevision{
@@ -124,7 +142,22 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
124142
},
125143
&appsv1.Deployment{
126144
ObjectMeta: metav1.ObjectMeta{
127-
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"},
149+
// Fields to be sanitized
150+
Finalizers: []string{"test"},
151+
OwnerReferences: []metav1.OwnerReference{{Kind: "TestOwner"}},
152+
CreationTimestamp: metav1.Time{Time: metav1.Now().Time},
153+
UID: "1a2b3c4d",
154+
ResourceVersion: "12345",
155+
Generation: 123,
156+
ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "test-manager"}},
157+
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
158+
DeletionGracePeriodSeconds: func(i int64) *int64 { return &i }(30),
159+
}, Status: appsv1.DeploymentStatus{
160+
Replicas: 3,
128161
},
129162
},
130163
}, nil
@@ -142,15 +175,13 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
142175
},
143176
}
144177

145-
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{})
146179
require.NoError(t, err)
147180

148181
t.Log("by checking the olm.operatorframework.io/owner label is set to the name of the ClusterExtension")
149182
require.Equal(t, map[string]string{
150183
controllers.ClusterExtensionRevisionOwnerLabel: "test-extension",
151184
}, rev.Labels)
152-
t.Log("by checking there are no annotations")
153-
require.Empty(t, rev.Annotations)
154185
t.Log("by checking the revision number is 0")
155186
require.Equal(t, int64(0), rev.Spec.Revision)
156187
t.Log("by checking the rendered objects are present in the correct phases")
@@ -167,9 +198,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
167198
"name": "test-service",
168199
},
169200
"spec": map[string]interface{}{},
170-
"status": map[string]interface{}{
171-
"loadBalancer": map[string]interface{}{},
172-
},
173201
},
174202
},
175203
},
@@ -179,7 +207,14 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
179207
"apiVersion": "apps/v1",
180208
"kind": "Deployment",
181209
"metadata": map[string]interface{}{
182-
"name": "test-deployment",
210+
"name": "test-deployment",
211+
"namespace": "test-ns",
212+
"labels": map[string]interface{}{
213+
"my-label": "my-label-value",
214+
},
215+
"annotations": map[string]interface{}{
216+
"my-annotation": "my-annotation-value",
217+
},
183218
},
184219
"spec": map[string]interface{}{
185220
"selector": nil,
@@ -191,7 +226,6 @@ func Test_SimpleRevisionGenerator_GenerateRevision(t *testing.T) {
191226
},
192227
"strategy": map[string]interface{}{},
193228
},
194-
"status": map[string]interface{}{},
195229
},
196230
},
197231
},
@@ -220,7 +254,7 @@ func Test_SimpleRevisionGenerator_Renderer_Integration(t *testing.T) {
220254
ManifestProvider: r,
221255
}
222256

223-
_, 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{})
224258
require.NoError(t, err)
225259
}
226260

@@ -258,7 +292,7 @@ func Test_SimpleRevisionGenerator_AppliesObjectLabelsAndRevisionAnnotations(t *t
258292
"other": "value",
259293
}
260294

261-
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{
262296
"some": "value",
263297
}, revAnnotations)
264298
require.NoError(t, err)
@@ -286,7 +320,7 @@ func Test_SimpleRevisionGenerator_Failure(t *testing.T) {
286320
ManifestProvider: r,
287321
}
288322

289-
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{})
290324
require.Nil(t, rev)
291325
t.Log("by checking rendering errors are propagated")
292326
require.Error(t, err)
@@ -363,7 +397,7 @@ func TestBoxcutter_Apply(t *testing.T) {
363397
{
364398
name: "first revision",
365399
mockBuilder: &mockBundleRevisionBuilder{
366-
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) {
367401
return &ocv1.ClusterExtensionRevision{
368402
ObjectMeta: metav1.ObjectMeta{
369403
Annotations: revisionAnnotations,
@@ -411,7 +445,7 @@ func TestBoxcutter_Apply(t *testing.T) {
411445
{
412446
name: "no change, revision exists",
413447
mockBuilder: &mockBundleRevisionBuilder{
414-
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) {
415449
return &ocv1.ClusterExtensionRevision{
416450
ObjectMeta: metav1.ObjectMeta{
417451
Annotations: revisionAnnotations,
@@ -457,7 +491,7 @@ func TestBoxcutter_Apply(t *testing.T) {
457491
{
458492
name: "new revision created when objects in new revision are different",
459493
mockBuilder: &mockBundleRevisionBuilder{
460-
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) {
461495
return &ocv1.ClusterExtensionRevision{
462496
ObjectMeta: metav1.ObjectMeta{
463497
Annotations: revisionAnnotations,
@@ -518,7 +552,7 @@ func TestBoxcutter_Apply(t *testing.T) {
518552
{
519553
name: "error from GenerateRevision",
520554
mockBuilder: &mockBundleRevisionBuilder{
521-
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) {
522556
return nil, errors.New("render boom")
523557
},
524558
},
@@ -534,7 +568,7 @@ func TestBoxcutter_Apply(t *testing.T) {
534568
{
535569
name: "keep at most 5 past revisions",
536570
mockBuilder: &mockBundleRevisionBuilder{
537-
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) {
538572
return &ocv1.ClusterExtensionRevision{
539573
ObjectMeta: metav1.ObjectMeta{
540574
Annotations: revisionAnnotations,
@@ -636,7 +670,7 @@ func TestBoxcutter_Apply(t *testing.T) {
636670
{
637671
name: "keep active revisions when they are out of limit",
638672
mockBuilder: &mockBundleRevisionBuilder{
639-
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) {
640674
return &ocv1.ClusterExtensionRevision{
641675
ObjectMeta: metav1.ObjectMeta{
642676
Annotations: revisionAnnotations,
@@ -894,14 +928,15 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
894928

895929
// mockBundleRevisionBuilder is a mock implementation of the ClusterExtensionRevisionGenerator for testing.
896930
type mockBundleRevisionBuilder struct {
897-
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)
898932
}
899933

900-
func (m *mockBundleRevisionBuilder) GenerateRevision(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
901-
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)
902936
}
903937

904938
func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
939+
ctx context.Context,
905940
helmRelease *release.Release, ext *ocv1.ClusterExtension,
906941
objectLabels map[string]string,
907942
) (*ocv1.ClusterExtensionRevision, error) {

0 commit comments

Comments
 (0)