Skip to content

Commit 35d2ed9

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #153 from chiragkyal/fix-recon
ESO-237: Fix reconciliation blocked when managed labels are externally modified
2 parents 6fcdb4a + 1175fc1 commit 35d2ed9

23 files changed

Lines changed: 1373 additions & 87 deletions

‎pkg/controller/external_secrets/certificate.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,9 @@ func (r *Reconciler) createOrApplyCertificate(esc *operatorv1alpha1.ExternalSecr
7171
r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName)
7272
}
7373
if !exist {
74-
if err := r.Create(r.ctx, desired); err != nil {
75-
return common.FromClientError(err, "failed to create %s certificate resource", certificateName)
74+
if err := r.createWithFallback(desired, resourceMetadata, certificateName, esc); err != nil {
75+
return err
7676
}
77-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName)
7877
}
7978

8079
return nil

‎pkg/controller/external_secrets/certificate_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ func TestCreateOrApplyCertificates(t *testing.T) {
268268
esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = testIssuerName
269269
},
270270
recon: false,
271-
wantErr: fmt.Sprintf("failed to create %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient),
271+
wantErr: fmt.Sprintf("failed to create Certificate %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient),
272272
},
273273
{
274274
name: "successful webhook certificate creation",

‎pkg/controller/external_secrets/configmap.go‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"maps"
66

77
corev1 "k8s.io/api/core/v1"
8+
apierrors "k8s.io/apimachinery/pkg/api/errors"
89
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
910
"sigs.k8s.io/controller-runtime/pkg/client"
1011

@@ -57,25 +58,31 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
5758
}
5859

5960
if !exist {
60-
// Create the ConfigMap
61+
// NOTE: This ConfigMap cannot use the generic createWithFallback helper because
62+
// the Data/BinaryData fields are managed by CNO (not by this operator). On
63+
// AlreadyExists we use a MergePatch that touches only metadata, leaving
64+
// CNO-injected CA certificates untouched.
6165
if err := r.Create(r.ctx, desiredConfigMap); err != nil {
62-
return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName)
66+
if !apierrors.IsAlreadyExists(err) {
67+
return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName)
68+
}
69+
r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName)
70+
if patchErr := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); patchErr != nil {
71+
return common.FromClientError(patchErr, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
72+
}
73+
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s restored to desired state", configMapName)
74+
return nil
6375
}
6476
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s created", configMapName)
6577
return nil
6678
}
6779

68-
// ConfigMap exists, ensure it has the correct labels and annotations.
69-
// Do not update the data of the ConfigMap since it is managed by CNO.
70-
// MergeFetchedAnnotations preserves external annotations (e.g., CNO's openshift.io/owning-component).
80+
// ConfigMap exists in cache — ensure it has the correct labels and annotations.
81+
// Use a metadata-only patch so CNO-managed Data/BinaryData are never touched.
7182
if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) {
72-
r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName)
73-
// Preserve data from existing ConfigMap (managed by CNO)
74-
desiredConfigMap.Data = existingConfigMap.Data
75-
desiredConfigMap.BinaryData = existingConfigMap.BinaryData
76-
common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata)
77-
if err := r.UpdateWithRetry(r.ctx, desiredConfigMap); err != nil {
78-
return common.FromClientError(err, "failed to update %s trusted CA bundle ConfigMap resource", configMapName)
83+
r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName)
84+
if err := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); err != nil {
85+
return common.FromClientError(err, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
7986
}
8087
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s reconciled back to desired state", configMapName)
8188
} else {

‎pkg/controller/external_secrets/configmap_test.go‎

Lines changed: 259 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,259 @@
1+
package external_secrets
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
corev1 "k8s.io/api/core/v1"
8+
apierrors "k8s.io/apimachinery/pkg/api/errors"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
"k8s.io/apimachinery/pkg/runtime/schema"
11+
"k8s.io/apimachinery/pkg/types"
12+
"sigs.k8s.io/controller-runtime/pkg/client"
13+
14+
operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
15+
"github.com/openshift/external-secrets-operator/pkg/controller/client/fakes"
16+
"github.com/openshift/external-secrets-operator/pkg/controller/common"
17+
"github.com/openshift/external-secrets-operator/pkg/controller/commontest"
18+
)
19+
20+
// testESCWithProxy returns an ExternalSecretsConfig with a proxy configured so that
21+
// ensureTrustedCABundleConfigMap proceeds past the proxy-nil guard.
22+
func testESCWithProxy() *operatorv1alpha1.ExternalSecretsConfig {
23+
esc := commontest.TestExternalSecretsConfig()
24+
esc.Spec.ApplicationConfig.Proxy = &operatorv1alpha1.ProxyConfig{
25+
HTTPProxy: "http://proxy.example.com:3128",
26+
}
27+
return esc
28+
}
29+
30+
func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
31+
cnoData := map[string]string{"ca-bundle.crt": "cert-data"}
32+
33+
tests := []struct {
34+
name string
35+
resourceMetadata common.ResourceMetadata
36+
preReq func(r *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient)
37+
wantErr string
38+
wantCreate bool
39+
wantPatch bool
40+
noProxy bool
41+
}{
42+
{
43+
name: "ConfigMap created when it does not exist",
44+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
45+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
46+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
47+
return false, nil
48+
})
49+
cached.CreateCalls(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error {
50+
cm, ok := obj.(*corev1.ConfigMap)
51+
if !ok {
52+
t.Errorf("expected ConfigMap, got %T", obj)
53+
}
54+
if cm.Name != trustedCABundleConfigMapName {
55+
t.Errorf("expected name %s, got %s", trustedCABundleConfigMapName, cm.Name)
56+
}
57+
if cm.Labels[trustedCABundleInjectLabel] != "true" {
58+
t.Errorf("expected inject label to be 'true', got %q", cm.Labels[trustedCABundleInjectLabel])
59+
}
60+
return nil
61+
})
62+
},
63+
wantCreate: true,
64+
},
65+
{
66+
name: "ConfigMap exists in correct state, no update needed",
67+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
68+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
69+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
70+
existing := &corev1.ConfigMap{
71+
ObjectMeta: metav1.ObjectMeta{
72+
Name: trustedCABundleConfigMapName,
73+
Namespace: commontest.TestExternalSecretsNamespace,
74+
Labels: getTrustedCABundleLabels(controllerDefaultResourceLabels),
75+
},
76+
Data: cnoData,
77+
}
78+
existing.DeepCopyInto(obj.(*corev1.ConfigMap))
79+
return true, nil
80+
})
81+
},
82+
},
83+
{
84+
name: "ConfigMap exists with wrong labels, patch triggered",
85+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
86+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
87+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
88+
existing := &corev1.ConfigMap{
89+
ObjectMeta: metav1.ObjectMeta{
90+
Name: trustedCABundleConfigMapName,
91+
Namespace: commontest.TestExternalSecretsNamespace,
92+
Labels: map[string]string{"app": "something-else"},
93+
},
94+
Data: cnoData,
95+
}
96+
existing.DeepCopyInto(obj.(*corev1.ConfigMap))
97+
return true, nil
98+
})
99+
cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
100+
cm, ok := obj.(*corev1.ConfigMap)
101+
if !ok {
102+
t.Errorf("expected ConfigMap, got %T", obj)
103+
}
104+
if cm.Labels[trustedCABundleInjectLabel] != "true" {
105+
t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel])
106+
}
107+
if patch.Type() != types.JSONPatchType {
108+
t.Errorf("expected JSONPatch, got %s", patch.Type())
109+
}
110+
return nil
111+
})
112+
},
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+
})
148+
},
149+
wantPatch: true,
150+
},
151+
{
152+
name: "proxy not configured: ConfigMap reconcile skipped",
153+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
154+
noProxy: true,
155+
},
156+
{
157+
name: "cached Exists check fails",
158+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
159+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
160+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
161+
return false, commontest.ErrTestClient
162+
})
163+
},
164+
wantErr: "failed to check external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource already exists: test client error",
165+
},
166+
{
167+
name: "Create fails with non-AlreadyExists error",
168+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
169+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
170+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
171+
return false, nil
172+
})
173+
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
174+
return commontest.ErrTestClient
175+
})
176+
},
177+
wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error",
178+
},
179+
{
180+
name: "Patch fails after AlreadyExists from Create",
181+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
182+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
183+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
184+
return false, nil
185+
})
186+
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
187+
return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
188+
})
189+
cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
190+
return commontest.ErrTestClient
191+
})
192+
},
193+
wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error",
194+
},
195+
{
196+
name: "cached Patch fails when ConfigMap has wrong labels",
197+
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
198+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
199+
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
200+
existing := &corev1.ConfigMap{
201+
ObjectMeta: metav1.ObjectMeta{
202+
Name: trustedCABundleConfigMapName,
203+
Namespace: commontest.TestExternalSecretsNamespace,
204+
Labels: nil,
205+
},
206+
}
207+
existing.DeepCopyInto(obj.(*corev1.ConfigMap))
208+
return true, nil
209+
})
210+
cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
211+
return commontest.ErrTestClient
212+
})
213+
},
214+
wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error",
215+
},
216+
}
217+
218+
for _, tt := range tests {
219+
t.Run(tt.name, func(t *testing.T) {
220+
r := testReconciler(t)
221+
cached := &fakes.FakeCtrlClient{}
222+
uncached := &fakes.FakeCtrlClient{}
223+
if tt.preReq != nil {
224+
tt.preReq(r, cached, uncached)
225+
}
226+
r.CtrlClient = cached
227+
r.UncachedClient = uncached
228+
229+
var esc *operatorv1alpha1.ExternalSecretsConfig
230+
if tt.noProxy {
231+
esc = commontest.TestExternalSecretsConfig()
232+
} else {
233+
esc = testESCWithProxy()
234+
}
235+
236+
err := r.ensureTrustedCABundleConfigMap(esc, tt.resourceMetadata)
237+
238+
if tt.wantErr != "" {
239+
if err == nil || err.Error() != tt.wantErr {
240+
t.Errorf("ensureTrustedCABundleConfigMap() err = %v, wantErr = %v", err, tt.wantErr)
241+
}
242+
return
243+
}
244+
if err != nil {
245+
t.Errorf("ensureTrustedCABundleConfigMap() unexpected error: %v", err)
246+
}
247+
248+
if tt.wantCreate && cached.CreateCallCount() == 0 {
249+
t.Error("expected Create to be called, but it wasn't")
250+
}
251+
if tt.wantPatch && cached.PatchCallCount() == 0 {
252+
t.Error("expected Patch to be called, but it wasn't")
253+
}
254+
if !tt.wantPatch && cached.PatchCallCount() > 0 {
255+
t.Error("expected no Patch call, but one was made")
256+
}
257+
})
258+
}
259+
}

0 commit comments

Comments
 (0)