Skip to content

Commit 841760b

Browse files
chiragkyalchiragkyal
committed
use JSON Patch to fully replace labels/annotations on co-managed resources
Signed-off-by: chiragkyal <ckyal@redhat.com>
1 parent 14110ae commit 841760b

4 files changed

Lines changed: 664 additions & 51 deletions

File tree

‎pkg/controller/external_secrets/configmap_test.go‎

Lines changed: 42 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -104,50 +104,50 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
104104
if cm.Labels[trustedCABundleInjectLabel] != "true" {
105105
t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
106106
}
107-
if patch.Type() != types.MergePatchType {
108-
t.Errorf("expected MergePatch, got %s", patch.Type())
109-
}
110-
return nil
111-
})
112-
},
113-
wantPatch: true,
107+
if patch.Type() != types.JSONPatchType {
108+
t.Errorf("expected JSONPatch, got %s", patch.Type())
109+
}
110+
return nil
111+
})
114112
},
115-
{
116-
// Regression test for: labels updating with app: external-secrets of cm
117-
// external-secrets-trusted-ca-bundle will block the reconcile in the http_proxy env.
118-
//
119-
// When the managed label (app=external-secrets) is removed from the ConfigMap,
120-
// the object falls out of the label-filtered cache. The cached Exists() returns
121-
// false, Create() fails with AlreadyExists, and the controller must patch only
122-
// the metadata (labels/annotations), leaving CNO-managed Data/BinaryData untouched.
123-
name: "Create returns AlreadyExists (label-filtered cache miss): patches metadata to restore labels",
124-
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
125-
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
126-
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
127-
return false, nil
128-
})
129-
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
130-
return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
131-
})
132-
cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
133-
cm, ok := obj.(*corev1.ConfigMap)
134-
if !ok {
135-
t.Errorf("expected ConfigMap, got %T", obj)
136-
}
137-
if cm.Labels["app"] != "external-secrets" {
138-
t.Errorf("expected app=external-secrets label in patch target, got %q", cm.Labels["app"])
139-
}
140-
if cm.Labels[trustedCABundleInjectLabel] != "true" {
141-
t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
142-
}
143-
if patch.Type() != types.MergePatchType {
144-
t.Errorf("expected MergePatch, got %s", patch.Type())
145-
}
146-
return nil
147-
})
148-
},
149-
wantPatch: true,
113+
wantPatch: true,
114+
},
115+
{
116+
// Regression test for: labels updating with app: external-secrets of cm
117+
// external-secrets-trusted-ca-bundle will block the reconcile in the http_proxy env.
118+
//
119+
// When the managed label (app=external-secrets) is removed from the ConfigMap,
120+
// the object falls out of the label-filtered cache. The cached Exists() returns
121+
// false, Create() fails with AlreadyExists, and the controller must patch only
122+
// the metadata (labels/annotations), leaving CNO-managed Data/BinaryData untouched.
123+
name: "Create returns AlreadyExists (label-filtered cache miss): patches metadata to restore labels",
124+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
125+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
126+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
127+
return false, nil
128+
})
129+
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
130+
return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
131+
})
132+
cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
133+
cm, ok := obj.(*corev1.ConfigMap)
134+
if !ok {
135+
t.Errorf("expected ConfigMap, got %T", obj)
136+
}
137+
if cm.Labels["app"] != "external-secrets" {
138+
t.Errorf("expected app=external-secrets label in patch target, got %q", cm.Labels["app"])
139+
}
140+
if cm.Labels[trustedCABundleInjectLabel] != "true" {
141+
t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
142+
}
143+
if patch.Type() != types.JSONPatchType {
144+
t.Errorf("expected JSONPatch, got %s", patch.Type())
145+
}
146+
return nil
147+
})
150148
},
149+
wantPatch: true,
150+
},
151151
{
152152
name: "proxy not configured: ConfigMap reconcile skipped",
153153
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),

‎pkg/controller/external_secrets/utils.go‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -250,19 +250,42 @@ func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata
250250
return nil
251251
}
252252

253-
// patchResourceMetadata sends a MergePatch that sets only labels and annotations on the
254-
// object, leaving all other fields untouched. It also removes obsolete annotations before patching.
253+
// jsonPatchOp is a single JSON Patch operation.
254+
type jsonPatchOp struct {
255+
Op string `json:"op"`
256+
Path string `json:"path"`
257+
Value interface{} `json:"value"`
258+
}
259+
260+
// patchResourceMetadata fully replaces the labels and annotations on an object using a
261+
// JSON Patch, leaving all other fields untouched. This is safe for co-managed
262+
// resources where this operator owns all metadata but data fields are owned by an external
263+
// controller (e.g. CNO-managed ConfigMap data, cert-controller-managed TLS Secret data).
264+
//
265+
// JSON Patch "add" on an existing path replaces the entire value, so the resulting
266+
// labels/annotations on the server exactly match desired.
267+
//
268+
// RemoveObsoleteAnnotations is called defensively to strip any deleted annotation keys
269+
// from desired before building the patch, regardless of whether the caller already did so.
255270
func (r *Reconciler) patchResourceMetadata(desired client.Object, resourceMetadata common.ResourceMetadata) error {
256271
common.RemoveObsoleteAnnotations(desired, resourceMetadata)
257-
patch := map[string]interface{}{
258-
"metadata": map[string]interface{}{
259-
"labels": desired.GetLabels(),
260-
"annotations": desired.GetAnnotations(),
261-
},
272+
273+
annotations := desired.GetAnnotations()
274+
if annotations == nil {
275+
annotations = map[string]string{}
276+
}
277+
labels := desired.GetLabels()
278+
if labels == nil {
279+
labels = map[string]string{}
280+
}
281+
282+
ops := []jsonPatchOp{
283+
{Op: "add", Path: "/metadata/labels", Value: labels},
284+
{Op: "add", Path: "/metadata/annotations", Value: annotations},
262285
}
263-
patchBytes, err := json.Marshal(patch)
286+
patchBytes, err := json.Marshal(ops)
264287
if err != nil {
265288
return fmt.Errorf("failed to marshal metadata patch: %w", err)
266289
}
267-
return r.CtrlClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes))
290+
return r.CtrlClient.Patch(r.ctx, desired, client.RawPatch(types.JSONPatchType, patchBytes))
268291
}

0 commit comments

Comments
 (0)