Skip to content

Commit 5525eaf

Browse files
committed
Address review comments
Signed-off-by: chiragkyal <ckyal@redhat.com>
1 parent 2654a78 commit 5525eaf

14 files changed

Lines changed: 158 additions & 120 deletions

pkg/controller/external_secrets/certificate.go

Lines changed: 1 addition & 2 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.createWithFallback(desired, resourceMetadata, certificateName); err != nil {
74+
if err := r.createWithFallback(desired, resourceMetadata, certificateName, esc); err != nil {
7575
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/configmap.go

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,15 @@
11
package external_secrets
22

33
import (
4-
"encoding/json"
54
"fmt"
65
"maps"
76

87
corev1 "k8s.io/api/core/v1"
98
apierrors "k8s.io/apimachinery/pkg/api/errors"
109
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11-
"k8s.io/apimachinery/pkg/types"
1210
"sigs.k8s.io/controller-runtime/pkg/client"
1311

1412
operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1"
15-
operatorclient "github.com/openshift/external-secrets-operator/pkg/controller/client"
1613
"github.com/openshift/external-secrets-operator/pkg/controller/common"
1714
)
1815

@@ -70,8 +67,7 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
7067
return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName)
7168
}
7269
r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName)
73-
common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata)
74-
if patchErr := r.patchConfigMapMetadata(desiredConfigMap, r.UncachedClient); patchErr != nil {
70+
if patchErr := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); patchErr != nil {
7571
return common.FromClientError(patchErr, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
7672
}
7773
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s restored to desired state", configMapName)
@@ -85,8 +81,7 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
8581
// Use a metadata-only patch so CNO-managed Data/BinaryData are never touched.
8682
if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) {
8783
r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName)
88-
common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata)
89-
if err := r.patchConfigMapMetadata(desiredConfigMap, r.CtrlClient); err != nil {
84+
if err := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); err != nil {
9085
return common.FromClientError(err, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName)
9186
}
9287
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s reconciled back to desired state", configMapName)
@@ -97,25 +92,6 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern
9792
return nil
9893
}
9994

100-
// patchConfigMapMetadata sends a MergePatch that sets only labels and annotations
101-
// on the ConfigMap, leaving all other fields (especially Data/BinaryData managed
102-
// by CNO) untouched. The caller chooses the client: the cached client when the
103-
// object is visible in the informer cache, or the uncached client when it has
104-
// fallen out of the label-filtered cache.
105-
func (r *Reconciler) patchConfigMapMetadata(desired *corev1.ConfigMap, patchClient operatorclient.CtrlClient) error {
106-
patch := map[string]interface{}{
107-
"metadata": map[string]interface{}{
108-
"labels": desired.Labels,
109-
"annotations": desired.Annotations,
110-
},
111-
}
112-
patchBytes, err := json.Marshal(patch)
113-
if err != nil {
114-
return fmt.Errorf("failed to marshal metadata patch: %w", err)
115-
}
116-
return patchClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes))
117-
}
118-
11995
// getTrustedCABundleLabels merges resource labels with the injection label.
12096
func getTrustedCABundleLabels(resourceLabels map[string]string) map[string]string {
12197
labels := make(map[string]string)

pkg/controller/external_secrets/configmap_test.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -119,18 +119,17 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
119119
// When the managed label (app=external-secrets) is removed from the ConfigMap,
120120
// the object falls out of the label-filtered cache. The cached Exists() returns
121121
// false, Create() fails with AlreadyExists, and the controller must patch only
122-
// the metadata (labels/annotations) via the uncached client, leaving
123-
// CNO-managed Data/BinaryData untouched.
124-
name: "Create returns AlreadyExists (label-filtered cache miss): uncached client patches metadata",
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",
125124
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
126-
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) {
125+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
127126
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
128127
return false, nil
129128
})
130129
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
131130
return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
132131
})
133-
uncached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
132+
cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error {
134133
cm, ok := obj.(*corev1.ConfigMap)
135134
if !ok {
136135
t.Errorf("expected ConfigMap, got %T", obj)
@@ -178,16 +177,16 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
178177
wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error",
179178
},
180179
{
181-
name: "uncached Patch fails after AlreadyExists from Create",
180+
name: "Patch fails after AlreadyExists from Create",
182181
resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()),
183-
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) {
182+
preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) {
184183
cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) {
185184
return false, nil
186185
})
187186
cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error {
188187
return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName)
189188
})
190-
uncached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
189+
cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error {
191190
return commontest.ErrTestClient
192191
})
193192
},
@@ -249,10 +248,10 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
249248
if tt.wantCreate && cached.CreateCallCount() == 0 {
250249
t.Error("expected Create to be called, but it wasn't")
251250
}
252-
if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 {
253-
t.Error("expected Patch to be called (on cached or uncached client), but it wasn't")
251+
if tt.wantPatch && cached.PatchCallCount() == 0 {
252+
t.Error("expected Patch to be called, but it wasn't")
254253
}
255-
if !tt.wantPatch && (cached.PatchCallCount() > 0 || uncached.PatchCallCount() > 0) {
254+
if !tt.wantPatch && cached.PatchCallCount() > 0 {
256255
t.Error("expected no Patch call, but one was made")
257256
}
258257
})

pkg/controller/external_secrets/controller.go

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -267,47 +267,33 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
267267
}
268268

269269
isManagedResource := func(object client.Object) bool {
270-
return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue
271-
}
272-
273-
logIgnored := func(obj client.Object) {
274-
r.log.V(4).Info("object not of interest, ignoring reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace())
270+
labels := object.GetLabels()
271+
matches := labels != nil && labels[requestEnqueueLabelKey] == requestEnqueueLabelValue
272+
r.log.V(4).Info("predicate evaluation", "object", fmt.Sprintf("%T", object), "name", object.GetName(), "namespace", object.GetNamespace(), "labels", labels, "matches", matches)
273+
return matches
275274
}
276275

277276
// On updates, check both old and new objects so that events where the managed
278277
// label is removed externally still trigger reconciliation and label restoration.
279278
managedResources := predicate.Funcs{
280279
CreateFunc: func(e event.CreateEvent) bool {
281-
if !isManagedResource(e.Object) {
282-
logIgnored(e.Object)
283-
return false
284-
}
285-
return true
280+
return isManagedResource(e.Object)
286281
},
287282
UpdateFunc: func(e event.UpdateEvent) bool {
288-
if !isManagedResource(e.ObjectOld) && !isManagedResource(e.ObjectNew) {
289-
logIgnored(e.ObjectNew)
290-
return false
291-
}
292-
return true
283+
return isManagedResource(e.ObjectOld) || isManagedResource(e.ObjectNew)
293284
},
294285
DeleteFunc: func(e event.DeleteEvent) bool {
295-
if !isManagedResource(e.Object) {
296-
logIgnored(e.Object)
297-
return false
298-
}
299-
return true
286+
return isManagedResource(e.Object)
300287
},
301288
GenericFunc: func(e event.GenericEvent) bool {
302-
if !isManagedResource(e.Object) {
303-
logIgnored(e.Object)
304-
return false
305-
}
306-
return true
289+
return isManagedResource(e.Object)
307290
},
308291
}
309292

310-
withIgnoreStatusUpdatePredicates := builder.WithPredicates(predicate.GenerationChangedPredicate{}, managedResources)
293+
withIgnoreStatusUpdatePredicates := builder.WithPredicates(
294+
predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}),
295+
managedResources,
296+
)
311297
managedResourcePredicate := builder.WithPredicates(managedResources)
312298

313299
mgrBuilder := ctrl.NewControllerManagedBy(mgr).

pkg/controller/external_secrets/deployments.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,9 @@ func (r *Reconciler) createOrApplyDeploymentFromAsset(esc *operatorv1alpha1.Exte
8888
}
8989
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s updated", deploymentName)
9090
case !exist:
91-
if err := r.createWithFallback(deployment, resourceMetadata, deploymentName); err != nil {
91+
if err := r.createWithFallback(deployment, resourceMetadata, deploymentName, esc); err != nil {
9292
return err
9393
}
94-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s created", deploymentName)
9594
default:
9695
r.log.V(4).Info("deployment resource already exists and is in expected state", "name", deploymentName)
9796
}

pkg/controller/external_secrets/networkpolicy.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,9 @@ func (r *Reconciler) createOrApplyCustomNetworkPolicy(esc *operatorv1alpha1.Exte
144144
}
145145
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName)
146146
case !exists:
147-
if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil {
147+
if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName, esc); err != nil {
148148
return err
149149
}
150-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName)
151150
default:
152151
r.log.V(4).Info("NetworkPolicy already up-to-date", "name", networkPolicyName)
153152
}
@@ -183,10 +182,9 @@ func (r *Reconciler) createOrApplyNetworkPolicyFromAsset(esc *operatorv1alpha1.E
183182
}
184183
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName)
185184
case !exists:
186-
if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil {
185+
if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName, esc); err != nil {
187186
return err
188187
}
189-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName)
190188
default:
191189
r.log.V(4).Info("NetworkPolicy already up-to-date", "name", networkPolicyName)
192190
}
@@ -288,10 +286,9 @@ func (r *Reconciler) reconcileProxyEgressPolicy(esc *operatorv1alpha1.ExternalSe
288286
}
289287
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s updated", npName)
290288
case !exists:
291-
if err := r.createWithFallback(np, resourceMetadata, npName); err != nil {
289+
if err := r.createWithFallback(np, resourceMetadata, npName, esc); err != nil {
292290
return err
293291
}
294-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s created", npName)
295292
if recon {
296293
r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "proxy egress NetworkPolicy %s already exists", npName)
297294
}

pkg/controller/external_secrets/rbacs.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,9 @@ func (r *Reconciler) createOrApplyClusterRole(esc *operatorv1alpha1.ExternalSecr
125125
r.log.V(4).Info("clusterrole resource already exists and is in expected state", "name", clusterRoleName)
126126
}
127127
if !exist {
128-
if err := r.createWithFallback(obj, resourceMetadata, clusterRoleName); err != nil {
128+
if err := r.createWithFallback(obj, resourceMetadata, clusterRoleName, esc); err != nil {
129129
return err
130130
}
131-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s created", clusterRoleName)
132131
}
133132

134133
return nil
@@ -171,10 +170,9 @@ func (r *Reconciler) createOrApplyClusterRoleBinding(esc *operatorv1alpha1.Exter
171170
r.log.V(4).Info("clusterrolebinding resource already exists and is in expected state", "name", clusterRoleBindingName)
172171
}
173172
if !exist {
174-
if err := r.createWithFallback(obj, resourceMetadata, clusterRoleBindingName); err != nil {
173+
if err := r.createWithFallback(obj, resourceMetadata, clusterRoleBindingName, esc); err != nil {
175174
return err
176175
}
177-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrolebinding resource %s created", clusterRoleBindingName)
178176
}
179177

180178
return nil
@@ -215,10 +213,9 @@ func (r *Reconciler) createOrApplyRole(esc *operatorv1alpha1.ExternalSecretsConf
215213
r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName)
216214
}
217215
if !exist {
218-
if err := r.createWithFallback(obj, resourceMetadata, roleName); err != nil {
216+
if err := r.createWithFallback(obj, resourceMetadata, roleName, esc); err != nil {
219217
return err
220218
}
221-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName)
222219
}
223220

224221
return nil
@@ -258,10 +255,9 @@ func (r *Reconciler) createOrApplyRoleBinding(esc *operatorv1alpha1.ExternalSecr
258255
r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName)
259256
}
260257
if !exist {
261-
if err := r.createWithFallback(obj, resourceMetadata, roleBindingName); err != nil {
258+
if err := r.createWithFallback(obj, resourceMetadata, roleBindingName, esc); err != nil {
262259
return err
263260
}
264-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName)
265261
}
266262

267263
return nil

pkg/controller/external_secrets/secret.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,9 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo
4444
}
4545

4646
if !exist {
47-
if err := r.createWithFallback(desired, resourceMetadata, secretName); err != nil {
47+
if err := r.createWithFallback(desired, resourceMetadata, secretName, esc); err != nil {
4848
return err
4949
}
50-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName)
5150
}
5251
return nil
5352
}

pkg/controller/external_secrets/serviceaccounts.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,9 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External
6969
}
7070

7171
if !exist {
72-
if err := r.createWithFallback(desired, resourceMetadata, serviceAccountName); err != nil {
72+
if err := r.createWithFallback(desired, resourceMetadata, serviceAccountName, esc); err != nil {
7373
return err
7474
}
75-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Created serviceaccount %s", serviceAccountName)
7675
}
7776
}
7877

pkg/controller/external_secrets/services.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,9 @@ func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.Externa
7474
}
7575
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s updated", serviceName)
7676
case !exists:
77-
if err := r.createWithFallback(service, resourceMetadata, serviceName); err != nil {
77+
if err := r.createWithFallback(service, resourceMetadata, serviceName, esc); err != nil {
7878
return err
7979
}
80-
r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s created", serviceName)
8180
default:
8281
r.log.V(4).Info("Service already up-to-date", "name", serviceName)
8382
}

0 commit comments

Comments
 (0)