Skip to content

Commit a809315

Browse files
fix: Preserve user changes to deployment pod templates
Fixes OLM reverting user changes like kubectl rollout restart. OLM no longer stores pod template metadata, allowing user changes o annotations, labels, and other fields to persist. Generate-by: Cursor/Claude
1 parent 10c2841 commit a809315

File tree

4 files changed

+516
-0
lines changed

4 files changed

+516
-0
lines changed

internal/operator-controller/applier/boxcutter.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"slices"
1313
"strings"
1414

15+
"github.com/go-logr/logr"
1516
"helm.sh/helm/v3/pkg/release"
1617
"helm.sh/helm/v3/pkg/storage/driver"
1718
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -153,8 +154,61 @@ func (r *SimpleRevisionGenerator) GenerateRevision(
153154
return r.buildClusterExtensionRevision(objs, ext, revisionAnnotations), nil
154155
}
155156

157+
// removePodTemplateMetadata conditionally removes empty pod template annotations from Deployments.
158+
// Empty annotations are removed to prevent OLM from claiming ownership via Server-Side Apply,
159+
// allowing users to add custom annotations. Non-empty (bundle-provided) annotations are preserved.
160+
// Labels are always preserved as they are required for selector matching and chart functionality.
161+
func removePodTemplateMetadata(unstr *unstructured.Unstructured, l logr.Logger) {
162+
if unstr.GetKind() != "Deployment" || unstr.GroupVersionKind().Group != "apps" {
163+
return
164+
}
165+
166+
obj := unstr.Object
167+
spec, ok := obj["spec"].(map[string]any)
168+
if !ok {
169+
return
170+
}
171+
172+
template, ok := spec["template"].(map[string]any)
173+
if !ok {
174+
return
175+
}
176+
177+
templateMeta, ok := template["metadata"].(map[string]any)
178+
if !ok {
179+
return
180+
}
181+
182+
// Remove pod template annotations only when the rendered manifest has no annotations.
183+
// This allows users to add custom annotations (like kubectl rollout restart) without
184+
// OLM overwriting them, while still preserving any chart-provided annotations when present.
185+
if annotationsVal, hasAnnotations := templateMeta["annotations"]; hasAnnotations {
186+
if annotationsMap, ok := annotationsVal.(map[string]any); ok {
187+
if len(annotationsMap) == 0 {
188+
delete(templateMeta, "annotations")
189+
l.V(1).Info("removed empty pod template annotations from Deployment to preserve user changes",
190+
"deployment", unstr.GetName())
191+
} else {
192+
// Non-empty annotations are preserved so bundle/chart-provided annotations
193+
// continue to be applied and maintained by OLM.
194+
l.V(2).Info("preserving non-empty pod template annotations on Deployment",
195+
"deployment", unstr.GetName(), "count", len(annotationsMap))
196+
}
197+
}
198+
}
199+
}
200+
156201
// sanitizedUnstructured takes an unstructured obj, removes status if present, and returns a sanitized copy containing only the allowed metadata entries set below.
157202
// If any unallowed entries are removed, a warning will be logged.
203+
//
204+
// For Deployment objects, this function conditionally removes empty pod template annotations.
205+
// Bundle-provided annotations are preserved to maintain operator functionality.
206+
// Empty annotations are removed to allow users to add custom annotations without OLM reverting them.
207+
// Examples of user annotations: "kubectl rollout restart", "kubectl annotate", custom monitoring annotations.
208+
// Labels are kept because: (1) deployment selector must match template labels, and
209+
// (2) chart-provided labels may be referenced by other resources.
210+
// This fixes the issue where user changes to pod template annotations would be undone by OLM.
211+
// See: https://github.com/operator-framework/operator-lifecycle-manager/issues/3392
158212
func sanitizedUnstructured(ctx context.Context, unstr *unstructured.Unstructured) {
159213
l := log.FromContext(ctx)
160214
obj := unstr.Object
@@ -193,6 +247,15 @@ func sanitizedUnstructured(ctx context.Context, unstr *unstructured.Unstructured
193247
l.Info("warning: extraneous values removed from manifest metadata", "allowed metadata", allowedMetadata)
194248
}
195249
obj["metadata"] = metadataSanitized
250+
251+
// For Deployment objects, conditionally remove empty pod template annotations (always keep labels).
252+
// Empty annotations are removed to prevent OLM from claiming ownership.
253+
// Non-empty (bundle-provided) annotations are preserved.
254+
// This allows users to add custom annotations (kubectl rollout restart, kubectl annotate, etc.)
255+
// without OLM overwriting them.
256+
// Labels are always preserved because the deployment selector must match template labels,
257+
// and chart-provided labels may be needed by other resources.
258+
removePodTemplateMetadata(unstr, l)
196259
}
197260

198261
func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(

internal/operator-controller/applier/boxcutter_test.go

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1884,3 +1884,144 @@ func (s *statusWriterMock) Apply(ctx context.Context, obj runtime.ApplyConfigura
18841884
// helps ensure tests fail if this assumption changes
18851885
return fmt.Errorf("unexpected call to StatusWriter.Apply() - this method is not expected to be used in these tests")
18861886
}
1887+
1888+
func Test_SimpleRevisionGenerator_PodTemplateAnnotationSanitization(t *testing.T) {
1889+
tests := []struct {
1890+
name string
1891+
podTemplateAnnotations map[string]string
1892+
expectAnnotationsInRevision bool
1893+
}{
1894+
{
1895+
name: "deployment with non-empty pod template annotations preserves them",
1896+
podTemplateAnnotations: map[string]string{
1897+
"kubectl.kubernetes.io/default-container": "main",
1898+
"prometheus.io/scrape": "true",
1899+
},
1900+
expectAnnotationsInRevision: true,
1901+
},
1902+
{
1903+
name: "deployment with empty pod template annotations removes them",
1904+
podTemplateAnnotations: map[string]string{},
1905+
expectAnnotationsInRevision: false,
1906+
},
1907+
{
1908+
name: "deployment with nil pod template annotations has none in revision",
1909+
podTemplateAnnotations: nil,
1910+
expectAnnotationsInRevision: false,
1911+
},
1912+
}
1913+
1914+
for _, tt := range tests {
1915+
t.Run(tt.name, func(t *testing.T) {
1916+
// Create a deployment with specified pod template annotations
1917+
deployment := &appsv1.Deployment{
1918+
ObjectMeta: metav1.ObjectMeta{
1919+
Name: "test-deployment",
1920+
},
1921+
Spec: appsv1.DeploymentSpec{
1922+
Replicas: ptr.To(int32(1)),
1923+
Selector: &metav1.LabelSelector{
1924+
MatchLabels: map[string]string{"app": "test"},
1925+
},
1926+
Template: corev1.PodTemplateSpec{
1927+
ObjectMeta: metav1.ObjectMeta{
1928+
Labels: map[string]string{"app": "test"},
1929+
Annotations: tt.podTemplateAnnotations,
1930+
},
1931+
Spec: corev1.PodSpec{
1932+
Containers: []corev1.Container{
1933+
{
1934+
Name: "main",
1935+
Image: "nginx:latest",
1936+
},
1937+
},
1938+
},
1939+
},
1940+
},
1941+
}
1942+
1943+
// Create revision generator with fake manifest provider
1944+
scheme := runtime.NewScheme()
1945+
require.NoError(t, k8scheme.AddToScheme(scheme))
1946+
require.NoError(t, ocv1.AddToScheme(scheme))
1947+
1948+
ext := &ocv1.ClusterExtension{
1949+
ObjectMeta: metav1.ObjectMeta{
1950+
Name: "test-extension",
1951+
},
1952+
}
1953+
1954+
manifestProvider := &FakeManifestProvider{
1955+
GetFn: func(_ fs.FS, _ *ocv1.ClusterExtension) ([]client.Object, error) {
1956+
return []client.Object{deployment}, nil
1957+
},
1958+
}
1959+
1960+
rg := &applier.SimpleRevisionGenerator{
1961+
Scheme: scheme,
1962+
ManifestProvider: manifestProvider,
1963+
}
1964+
1965+
// Create a valid bundle FS
1966+
bundleFS := bundlefs.Builder().
1967+
WithPackageName("test-package").
1968+
WithCSV(clusterserviceversion.Builder().WithName("test-csv").Build()).
1969+
Build()
1970+
1971+
// Generate revision
1972+
revision, err := rg.GenerateRevision(
1973+
context.Background(),
1974+
bundleFS,
1975+
ext,
1976+
map[string]string{"test": "label"},
1977+
nil,
1978+
)
1979+
require.NoError(t, err)
1980+
require.NotNil(t, revision)
1981+
1982+
// Find the deployment in the revision
1983+
var deploymentFound bool
1984+
for _, obj := range revision.Spec.Phases[0].Objects {
1985+
if obj.Object.GetKind() == "Deployment" {
1986+
deploymentFound = true
1987+
1988+
// Check pod template annotations
1989+
spec, ok := obj.Object.Object["spec"].(map[string]any)
1990+
require.True(t, ok, "deployment should have spec")
1991+
1992+
template, ok := spec["template"].(map[string]any)
1993+
require.True(t, ok, "deployment spec should have template")
1994+
1995+
templateMeta, ok := template["metadata"].(map[string]any)
1996+
require.True(t, ok, "pod template should have metadata")
1997+
1998+
annotations, hasAnnotations := templateMeta["annotations"]
1999+
if tt.expectAnnotationsInRevision {
2000+
assert.True(t, hasAnnotations, "expected annotations to be present in revision")
2001+
annotationsMap, ok := annotations.(map[string]any)
2002+
require.True(t, ok, "annotations should be a map")
2003+
2004+
for key, expectedValue := range tt.podTemplateAnnotations {
2005+
actualValue, exists := annotationsMap[key]
2006+
assert.True(t, exists, "expected annotation key %s to exist", key)
2007+
assert.Equal(t, expectedValue, actualValue, "annotation value mismatch for key %s", key)
2008+
}
2009+
assert.Len(t, annotationsMap, len(tt.podTemplateAnnotations), "annotation count mismatch")
2010+
} else {
2011+
assert.False(t, hasAnnotations, "expected annotations to be removed from revision")
2012+
}
2013+
2014+
// Labels should always be preserved
2015+
labels, hasLabels := templateMeta["labels"]
2016+
assert.True(t, hasLabels, "labels should always be preserved")
2017+
labelsMap, ok := labels.(map[string]any)
2018+
require.True(t, ok, "labels should be a map")
2019+
assert.Equal(t, "test", labelsMap["app"], "label app should be preserved")
2020+
2021+
break
2022+
}
2023+
}
2024+
assert.True(t, deploymentFound, "deployment should be found in revision")
2025+
})
2026+
}
2027+
}
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
Feature: Rollout Restart User Changes
2+
# Verifies that user-added pod template annotations persist after OLM reconciliation.
3+
# Fixes: https://github.com/operator-framework/operator-lifecycle-manager/issues/3392
4+
5+
Background:
6+
Given OLM is available
7+
And ClusterCatalog "test" serves bundles
8+
And ServiceAccount "olm-sa" with needed permissions is available in ${TEST_NAMESPACE}
9+
10+
@BoxcutterRuntime
11+
Scenario: User-initiated deployment changes persist after OLM reconciliation
12+
When ClusterExtension is applied
13+
"""
14+
apiVersion: olm.operatorframework.io/v1
15+
kind: ClusterExtension
16+
metadata:
17+
name: ${NAME}
18+
spec:
19+
namespace: ${TEST_NAMESPACE}
20+
serviceAccount:
21+
name: olm-sa
22+
source:
23+
sourceType: Catalog
24+
catalog:
25+
packageName: test
26+
selector:
27+
matchLabels:
28+
"olm.operatorframework.io/metadata.name": test-catalog
29+
"""
30+
Then ClusterExtension is rolled out
31+
And ClusterExtension is available
32+
And resource "deployment/test-operator" is installed
33+
And deployment "test-operator" is ready
34+
When user performs rollout restart on "deployment/test-operator"
35+
Then deployment "test-operator" rollout completes successfully
36+
And resource "deployment/test-operator" has restart annotation
37+
# Wait for OLM reconciliation cycle (runs every 10 seconds)
38+
And I wait for "30" seconds
39+
# Verify user changes persisted after OLM reconciliation
40+
Then deployment "test-operator" rollout is still successful
41+
And resource "deployment/test-operator" has restart annotation
42+
And deployment "test-operator" has expected number of ready replicas

0 commit comments

Comments
 (0)