From 6b694a6376a9754b26452189614d672b92c77b47 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Tue, 16 Jun 2026 12:31:31 +0530 Subject: [PATCH 1/6] Fix reconciliation blocked when managed labels are externally modified Signed-off-by: chiragkyal --- .../external_secrets/certificate.go | 4 +- .../external_secrets/certificate_test.go | 2 +- pkg/controller/external_secrets/configmap.go | 24 +- .../external_secrets/configmap_test.go | 304 ++++++++++++++++++ pkg/controller/external_secrets/controller.go | 43 ++- .../external_secrets/deployments.go | 4 +- .../external_secrets/networkpolicy.go | 12 +- .../external_secrets/networkpolicy_test.go | 4 +- pkg/controller/external_secrets/rbacs.go | 16 +- pkg/controller/external_secrets/rbacs_test.go | 8 +- pkg/controller/external_secrets/secret.go | 4 +- .../external_secrets/secret_test.go | 2 +- .../external_secrets/service_test.go | 4 +- .../external_secrets/serviceaccounts.go | 4 +- .../external_secrets/serviceaccounts_test.go | 2 +- pkg/controller/external_secrets/services.go | 4 +- pkg/controller/external_secrets/utils.go | 22 ++ pkg/controller/external_secrets/utils_test.go | 113 +++++++ .../external_secrets/validatingwebhook.go | 4 +- .../validatingwebhook_test.go | 2 +- 20 files changed, 525 insertions(+), 57 deletions(-) create mode 100644 pkg/controller/external_secrets/configmap_test.go create mode 100644 pkg/controller/external_secrets/utils_test.go diff --git a/pkg/controller/external_secrets/certificate.go b/pkg/controller/external_secrets/certificate.go index b972aa263..b087ab4a9 100644 --- a/pkg/controller/external_secrets/certificate.go +++ b/pkg/controller/external_secrets/certificate.go @@ -71,8 +71,8 @@ func (r *Reconciler) createOrApplyCertificate(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName) } if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s certificate resource", certificateName) + if err := r.createWithFallback(desired, resourceMetadata, certificateName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) } diff --git a/pkg/controller/external_secrets/certificate_test.go b/pkg/controller/external_secrets/certificate_test.go index ced01c305..090e1d273 100644 --- a/pkg/controller/external_secrets/certificate_test.go +++ b/pkg/controller/external_secrets/certificate_test.go @@ -268,7 +268,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = testIssuerName }, recon: false, - wantErr: fmt.Sprintf("failed to create %s/%s certificate resource: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient), }, { name: "successful webhook certificate creation", diff --git a/pkg/controller/external_secrets/configmap.go b/pkg/controller/external_secrets/configmap.go index b4d89654e..7cb694d0b 100644 --- a/pkg/controller/external_secrets/configmap.go +++ b/pkg/controller/external_secrets/configmap.go @@ -5,6 +5,7 @@ import ( "maps" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -57,9 +58,28 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern } if !exist { - // Create the ConfigMap + // NOTE: This ConfigMap cannot use the generic createWithFallback helper because + // the Data field is managed by CNO (not by this operator). A plain UpdateWithRetry + // with our empty-Data desired object would wipe the CA certificates CNO injected. + // Instead, on AlreadyExists we must fetch the real object first, preserve its Data, + // then update only labels/annotations. if err := r.Create(r.ctx, desiredConfigMap); err != nil { - return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) + if !apierrors.IsAlreadyExists(err) { + return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) + } + r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, restoring desired state", "name", configMapName) + actualConfigMap := &corev1.ConfigMap{} + if _, fetchErr := r.UncachedClient.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), actualConfigMap); fetchErr != nil { + return common.FromClientError(fetchErr, "failed to fetch existing %s trusted CA bundle ConfigMap for restoration", configMapName) + } + desiredConfigMap.Data = actualConfigMap.Data + desiredConfigMap.BinaryData = actualConfigMap.BinaryData + common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) + if updateErr := r.UncachedClient.UpdateWithRetry(r.ctx, desiredConfigMap); updateErr != nil { + return common.FromClientError(updateErr, "failed to restore %s trusted CA bundle ConfigMap to desired state", configMapName) + } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s restored to desired state", configMapName) + return nil } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s created", configMapName) return nil diff --git a/pkg/controller/external_secrets/configmap_test.go b/pkg/controller/external_secrets/configmap_test.go new file mode 100644 index 000000000..a51e9a1df --- /dev/null +++ b/pkg/controller/external_secrets/configmap_test.go @@ -0,0 +1,304 @@ +package external_secrets + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1" + "github.com/openshift/external-secrets-operator/pkg/controller/client/fakes" + "github.com/openshift/external-secrets-operator/pkg/controller/common" + "github.com/openshift/external-secrets-operator/pkg/controller/commontest" +) + +// testESCWithProxy returns an ExternalSecretsConfig with a proxy configured so that +// ensureTrustedCABundleConfigMap proceeds past the proxy-nil guard. +func testESCWithProxy() *operatorv1alpha1.ExternalSecretsConfig { + esc := commontest.TestExternalSecretsConfig() + esc.Spec.ApplicationConfig.Proxy = &operatorv1alpha1.ProxyConfig{ + HTTPProxy: "http://proxy.example.com:3128", + } + return esc +} + +func TestEnsureTrustedCABundleConfigMap(t *testing.T) { + cnoData := map[string]string{"ca-bundle.crt": "cert-data"} + + tests := []struct { + name string + resourceMetadata common.ResourceMetadata + preReq func(r *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) + wantErr string + wantCreate bool + wantUpdate bool + noProxy bool + }{ + { + name: "ConfigMap created when it does not exist", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, nil + }) + cached.CreateCalls(func(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + cm, ok := obj.(*corev1.ConfigMap) + if !ok { + t.Errorf("expected ConfigMap, got %T", obj) + } + if cm.Name != trustedCABundleConfigMapName { + t.Errorf("expected name %s, got %s", trustedCABundleConfigMapName, cm.Name) + } + if cm.Labels[trustedCABundleInjectLabel] != "true" { + t.Errorf("expected inject label to be 'true', got %q", cm.Labels[trustedCABundleInjectLabel]) + } + return nil + }) + }, + wantCreate: true, + }, + { + name: "ConfigMap exists in correct state, no update needed", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { + existing := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: trustedCABundleConfigMapName, + Namespace: commontest.TestExternalSecretsNamespace, + Labels: getTrustedCABundleLabels(controllerDefaultResourceLabels), + }, + Data: cnoData, + } + existing.DeepCopyInto(obj.(*corev1.ConfigMap)) + return true, nil + }) + }, + wantUpdate: false, + }, + { + name: "ConfigMap exists with wrong labels, update triggered", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { + existing := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: trustedCABundleConfigMapName, + Namespace: commontest.TestExternalSecretsNamespace, + Labels: map[string]string{"app": "something-else"}, + }, + Data: cnoData, + } + existing.DeepCopyInto(obj.(*corev1.ConfigMap)) + return true, nil + }) + cached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + cm, ok := obj.(*corev1.ConfigMap) + if !ok { + t.Errorf("expected ConfigMap, got %T", obj) + } + if cm.Labels[trustedCABundleInjectLabel] != "true" { + t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel]) + } + if cm.Data["ca-bundle.crt"] != "cert-data" { + t.Errorf("CNO-managed data should be preserved, got %v", cm.Data) + } + return nil + }) + }, + wantUpdate: true, + }, + { + // Regression test for: labels updating with app: external-secrets of cm + // external-secrets-trusted-ca-bundle will block the reconcile in the http_proxy env. + // + // When the managed label (app=external-secrets) is removed from the ConfigMap, + // the object falls out of the label-filtered cache. The cached Exists() returns + // false, Create() fails with AlreadyExists, and the controller must use the + // uncached client to fetch and restore the correct labels instead of returning + // an error that blocks reconciliation. + name: "Create returns AlreadyExists (label-filtered cache miss): uncached client restores labels", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + // Cached client doesn't see the ConfigMap because its label was changed. + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, nil + }) + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) + }) + // Uncached client fetches the real object from the API server. + uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { + existing := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: trustedCABundleConfigMapName, + Namespace: commontest.TestExternalSecretsNamespace, + ResourceVersion: "1000", + Labels: map[string]string{"app": "update-secrets"}, + }, + Data: cnoData, + } + existing.DeepCopyInto(obj.(*corev1.ConfigMap)) + return true, nil + }) + uncached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + cm, ok := obj.(*corev1.ConfigMap) + if !ok { + t.Errorf("expected ConfigMap, got %T", obj) + } + if cm.Labels["app"] != "external-secrets" { + t.Errorf("expected app=external-secrets label restored, got %q", cm.Labels["app"]) + } + if cm.Labels[trustedCABundleInjectLabel] != "true" { + t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel]) + } + if cm.Data["ca-bundle.crt"] != "cert-data" { + t.Errorf("CNO-managed data should be preserved, got %v", cm.Data) + } + return nil + }) + }, + wantUpdate: true, + }, + { + name: "proxy not configured: ConfigMap reconcile skipped", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + noProxy: true, + }, + { + name: "cached Exists check fails", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, commontest.ErrTestClient + }) + }, + wantErr: "failed to check external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource already exists: test client error", + }, + { + name: "Create fails with non-AlreadyExists error", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, nil + }) + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error", + }, + { + name: "uncached Exists fails after AlreadyExists from Create", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, nil + }) + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) + }) + uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, commontest.ErrTestClient + }) + }, + wantErr: "failed to fetch existing external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap for restoration: test client error", + }, + { + name: "uncached UpdateWithRetry fails after AlreadyExists from Create", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { + return false, nil + }) + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) + }) + uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { + existing := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: trustedCABundleConfigMapName, + Namespace: commontest.TestExternalSecretsNamespace, + ResourceVersion: "1000", + Labels: map[string]string{"app": "update-secrets"}, + }, + } + existing.DeepCopyInto(obj.(*corev1.ConfigMap)) + return true, nil + }) + uncached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: "failed to restore external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap to desired state: test client error", + }, + { + name: "cached UpdateWithRetry fails when ConfigMap has wrong labels", + resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { + existing := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: trustedCABundleConfigMapName, + Namespace: commontest.TestExternalSecretsNamespace, + Labels: nil, + }, + } + existing.DeepCopyInto(obj.(*corev1.ConfigMap)) + return true, nil + }) + cached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: "failed to update external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := testReconciler(t) + cached := &fakes.FakeCtrlClient{} + uncached := &fakes.FakeCtrlClient{} + if tt.preReq != nil { + tt.preReq(r, cached, uncached) + } + r.CtrlClient = cached + r.UncachedClient = uncached + + var esc *operatorv1alpha1.ExternalSecretsConfig + if tt.noProxy { + esc = commontest.TestExternalSecretsConfig() + } else { + esc = testESCWithProxy() + } + + err := r.ensureTrustedCABundleConfigMap(esc, tt.resourceMetadata) + + if tt.wantErr != "" { + if err == nil || err.Error() != tt.wantErr { + t.Errorf("ensureTrustedCABundleConfigMap() err = %v, wantErr = %v", err, tt.wantErr) + } + return + } + if err != nil { + t.Errorf("ensureTrustedCABundleConfigMap() unexpected error: %v", err) + } + + if tt.wantCreate && cached.CreateCallCount() == 0 { + t.Error("expected Create to be called, but it wasn't") + } + if tt.wantUpdate && cached.UpdateWithRetryCallCount() == 0 && uncached.UpdateWithRetryCallCount() == 0 { + t.Error("expected UpdateWithRetry to be called (on cached or uncached client), but it wasn't") + } + if !tt.wantUpdate && (cached.UpdateWithRetryCallCount() > 0 || uncached.UpdateWithRetryCallCount() > 0) { + t.Error("expected no UpdateWithRetry call, but one was made") + } + }) + } +} diff --git a/pkg/controller/external_secrets/controller.go b/pkg/controller/external_secrets/controller.go index 49035797c..0706bd009 100644 --- a/pkg/controller/external_secrets/controller.go +++ b/pkg/controller/external_secrets/controller.go @@ -40,6 +40,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -256,27 +257,35 @@ func checkAndRegisterCertificates(ctx context.Context, mgr ctrl.Manager, r *Reco func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { mapFunc := func(ctx context.Context, obj client.Object) []reconcile.Request { r.log.V(4).Info("received reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace()) - - objLabels := obj.GetLabels() - if objLabels != nil { - if objLabels[requestEnqueueLabelKey] == requestEnqueueLabelValue { - return []reconcile.Request{ - { - NamespacedName: types.NamespacedName{ - Name: common.ExternalSecretsConfigObjectName, - }, - }, - } - } + return []reconcile.Request{ + { + NamespacedName: types.NamespacedName{ + Name: common.ExternalSecretsConfigObjectName, + }, + }, } - r.log.V(4).Info("object not of interest, ignoring reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace()) - return []reconcile.Request{} } - // predicate function to ignore events for objects not managed by controller. - managedResources := predicate.NewPredicateFuncs(func(object client.Object) bool { + isManagedResource := func(object client.Object) bool { return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue - }) + } + + // On updates, check both old and new objects so that events where the managed + // label is removed externally still trigger reconciliation and label restoration. + managedResources := predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return isManagedResource(e.Object) + }, + UpdateFunc: func(e event.UpdateEvent) bool { + return isManagedResource(e.ObjectOld) || isManagedResource(e.ObjectNew) + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return isManagedResource(e.Object) + }, + GenericFunc: func(e event.GenericEvent) bool { + return isManagedResource(e.Object) + }, + } withIgnoreStatusUpdatePredicates := builder.WithPredicates(predicate.GenerationChangedPredicate{}, managedResources) managedResourcePredicate := builder.WithPredicates(managedResources) diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go index f6d52b930..e3af9a49a 100644 --- a/pkg/controller/external_secrets/deployments.go +++ b/pkg/controller/external_secrets/deployments.go @@ -88,8 +88,8 @@ func (r *Reconciler) createOrApplyDeploymentFromAsset(esc *operatorv1alpha1.Exte } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s updated", deploymentName) case !exist: - if err := r.Create(r.ctx, deployment); err != nil { - return common.FromClientError(err, "failed to create %s deployment resource", deploymentName) + if err := r.createWithFallback(deployment, resourceMetadata, deploymentName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s created", deploymentName) default: diff --git a/pkg/controller/external_secrets/networkpolicy.go b/pkg/controller/external_secrets/networkpolicy.go index 7f111a0b4..a8844ded3 100644 --- a/pkg/controller/external_secrets/networkpolicy.go +++ b/pkg/controller/external_secrets/networkpolicy.go @@ -144,8 +144,8 @@ func (r *Reconciler) createOrApplyCustomNetworkPolicy(esc *operatorv1alpha1.Exte } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName) case !exists: - if err := r.Create(r.ctx, networkPolicy); err != nil { - return common.FromClientError(err, "failed to create network policy %s", networkPolicyName) + if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName) default: @@ -183,8 +183,8 @@ func (r *Reconciler) createOrApplyNetworkPolicyFromAsset(esc *operatorv1alpha1.E } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName) case !exists: - if err := r.Create(r.ctx, networkPolicy); err != nil { - return common.FromClientError(err, "failed to create network policy %s", networkPolicyName) + if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName) default: @@ -288,8 +288,8 @@ func (r *Reconciler) reconcileProxyEgressPolicy(esc *operatorv1alpha1.ExternalSe } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s updated", npName) case !exists: - if err := r.Create(r.ctx, np); err != nil { - return common.FromClientError(err, "failed to create proxy egress network policy %s", npName) + if err := r.createWithFallback(np, resourceMetadata, npName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s created", npName) if recon { diff --git a/pkg/controller/external_secrets/networkpolicy_test.go b/pkg/controller/external_secrets/networkpolicy_test.go index 1f49620f9..ba77f1903 100644 --- a/pkg/controller/external_secrets/networkpolicy_test.go +++ b/pkg/controller/external_secrets/networkpolicy_test.go @@ -146,7 +146,7 @@ func TestCreateOrApplyStaticNetworkPolicies(t *testing.T) { return nil }) }, - wantErr: "failed to create network policy external-secrets/eso-sys-deny-all-traffic: test client error", + wantErr: "failed to create external-secrets/eso-sys-deny-all-traffic: test client error", }, { name: "network policy exists check fails", @@ -344,7 +344,7 @@ func TestCreateOrApplyCustomNetworkPolicies(t *testing.T) { }, } }, - wantErr: "failed to create network policy external-secrets/eso-user-test-fail-policy: test client error", + wantErr: "failed to create external-secrets/eso-user-test-fail-policy: test client error", }, { name: "custom network policy updated successfully", diff --git a/pkg/controller/external_secrets/rbacs.go b/pkg/controller/external_secrets/rbacs.go index b094b5689..543569364 100644 --- a/pkg/controller/external_secrets/rbacs.go +++ b/pkg/controller/external_secrets/rbacs.go @@ -125,8 +125,8 @@ func (r *Reconciler) createOrApplyClusterRole(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("clusterrole resource already exists and is in expected state", "name", clusterRoleName) } if !exist { - if err := r.Create(r.ctx, obj); err != nil { - return common.FromClientError(err, "failed to create %s clusterrole resource", clusterRoleName) + if err := r.createWithFallback(obj, resourceMetadata, clusterRoleName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s created", clusterRoleName) } @@ -171,8 +171,8 @@ func (r *Reconciler) createOrApplyClusterRoleBinding(esc *operatorv1alpha1.Exter r.log.V(4).Info("clusterrolebinding resource already exists and is in expected state", "name", clusterRoleBindingName) } if !exist { - if err := r.Create(r.ctx, obj); err != nil { - return common.FromClientError(err, "failed to create %s clusterrolebinding resource", clusterRoleBindingName) + if err := r.createWithFallback(obj, resourceMetadata, clusterRoleBindingName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrolebinding resource %s created", clusterRoleBindingName) } @@ -215,8 +215,8 @@ func (r *Reconciler) createOrApplyRole(esc *operatorv1alpha1.ExternalSecretsConf r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) } if !exist { - if err := r.Create(r.ctx, obj); err != nil { - return common.FromClientError(err, "failed to create %s role resource", roleName) + if err := r.createWithFallback(obj, resourceMetadata, roleName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) } @@ -258,8 +258,8 @@ func (r *Reconciler) createOrApplyRoleBinding(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName) } if !exist { - if err := r.Create(r.ctx, obj); err != nil { - return common.FromClientError(err, "failed to create %s rolebinding resource", roleBindingName) + if err := r.createWithFallback(obj, resourceMetadata, roleBindingName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName) } diff --git a/pkg/controller/external_secrets/rbacs_test.go b/pkg/controller/external_secrets/rbacs_test.go index 136e7f3f3..3e2f48d1f 100644 --- a/pkg/controller/external_secrets/rbacs_test.go +++ b/pkg/controller/external_secrets/rbacs_test.go @@ -124,7 +124,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets-cert-controller clusterrolebinding resource: test client error`, + wantErr: `failed to create external-secrets-cert-controller: test client error`, }, { name: "clusterrole reconciliation updating to desired state fails", @@ -164,7 +164,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets-cert-controller clusterrole resource: test client error`, + wantErr: `failed to create external-secrets-cert-controller: test client error`, }, { name: "role reconciliation updating to desired state fails", @@ -202,7 +202,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets/external-secrets-leaderelection role resource: test client error`, + wantErr: `failed to create external-secrets/external-secrets-leaderelection: test client error`, }, { name: "rolebindings reconciliation updating to desired state fails", @@ -240,7 +240,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets/external-secrets-leaderelection rolebinding resource: test client error`, + wantErr: `failed to create external-secrets/external-secrets-leaderelection: test client error`, }, { name: "clusterroles creation successful", diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go index 7bd24cc0c..6c8b334da 100644 --- a/pkg/controller/external_secrets/secret.go +++ b/pkg/controller/external_secrets/secret.go @@ -44,8 +44,8 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo } if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create %s secret resource", secretName) + if err := r.createWithFallback(desired, resourceMetadata, secretName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName) } diff --git a/pkg/controller/external_secrets/secret_test.go b/pkg/controller/external_secrets/secret_test.go index bfebdb6f6..913274fce 100644 --- a/pkg/controller/external_secrets/secret_test.go +++ b/pkg/controller/external_secrets/secret_test.go @@ -155,7 +155,7 @@ func TestCreateOrApplySecret(t *testing.T) { return nil }) }, - wantErr: fmt.Sprintf("failed to create %s/%s secret resource: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), }, { name: "successful secret creation", diff --git a/pkg/controller/external_secrets/service_test.go b/pkg/controller/external_secrets/service_test.go index cb5c0b430..34bc9e563 100644 --- a/pkg/controller/external_secrets/service_test.go +++ b/pkg/controller/external_secrets/service_test.go @@ -62,7 +62,7 @@ func TestCreateOrApplyServices(t *testing.T) { }, } }, - wantErr: `failed to create service external-secrets/bitwarden-sdk-server: test client error`, + wantErr: `failed to create external-secrets/bitwarden-sdk-server: test client error`, }, { @@ -107,7 +107,7 @@ func TestCreateOrApplyServices(t *testing.T) { return commontest.ErrTestClient }) }, - wantErr: `failed to create service external-secrets/external-secrets-webhook: test client error`, + wantErr: `failed to create external-secrets/external-secrets-webhook: test client error`, }, { name: "service with custom annotations applied successfully", diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go index f04067024..538ad0b6f 100644 --- a/pkg/controller/external_secrets/serviceaccounts.go +++ b/pkg/controller/external_secrets/serviceaccounts.go @@ -69,8 +69,8 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External } if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create serviceaccount %s", serviceAccountName) + if err := r.createWithFallback(desired, resourceMetadata, serviceAccountName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Created serviceaccount %s", serviceAccountName) } diff --git a/pkg/controller/external_secrets/serviceaccounts_test.go b/pkg/controller/external_secrets/serviceaccounts_test.go index 5b57466f6..cf961f181 100644 --- a/pkg/controller/external_secrets/serviceaccounts_test.go +++ b/pkg/controller/external_secrets/serviceaccounts_test.go @@ -119,7 +119,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return nil }) }, - wantErr: "failed to create serviceaccount external-secrets/external-secrets: test client error", + wantErr: "failed to create external-secrets/external-secrets: test client error", }, { name: "serviceaccount with custom annotations applied successfully", diff --git a/pkg/controller/external_secrets/services.go b/pkg/controller/external_secrets/services.go index 0dd93d000..ee24306da 100644 --- a/pkg/controller/external_secrets/services.go +++ b/pkg/controller/external_secrets/services.go @@ -74,8 +74,8 @@ func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.Externa } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s updated", serviceName) case !exists: - if err := r.Create(r.ctx, service); err != nil { - return common.FromClientError(err, "failed to create service %s", serviceName) + if err := r.createWithFallback(service, resourceMetadata, serviceName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s created", serviceName) default: diff --git a/pkg/controller/external_secrets/utils.go b/pkg/controller/external_secrets/utils.go index 51291276d..17ae9e9b9 100644 --- a/pkg/controller/external_secrets/utils.go +++ b/pkg/controller/external_secrets/utils.go @@ -6,6 +6,7 @@ import ( "net/url" "os" + apierrors "k8s.io/apimachinery/pkg/api/errors" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" @@ -203,3 +204,24 @@ func validateProxy(rawURL string) error { return nil } + +// createWithFallback attempts to create a resource and handles the AlreadyExists error that +// occurs when the resource exists on the API server but is invisible to the label-filtered +// informer cache (e.g. because the managed label app=external-secrets was externally removed). +// +// When Create returns AlreadyExists, this method bypasses the stale cache by using the +// uncached client to update the resource directly on the API server, restoring the managed +// labels and annotations to the desired state. +func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata common.ResourceMetadata, resourceName string) error { + if err := r.Create(r.ctx, desired); err != nil { + if !apierrors.IsAlreadyExists(err) { + return common.FromClientError(err, "failed to create %s", resourceName) + } + r.log.V(1).Info("resource exists on API server but absent from label-filtered cache (managed labels may have been externally modified), restoring desired state", "name", resourceName) + common.RemoveObsoleteAnnotations(desired, resourceMetadata) + if err := r.UncachedClient.UpdateWithRetry(r.ctx, desired); err != nil { + return common.FromClientError(err, "failed to restore %s to desired state", resourceName) + } + } + return nil +} diff --git a/pkg/controller/external_secrets/utils_test.go b/pkg/controller/external_secrets/utils_test.go new file mode 100644 index 000000000..f920e570b --- /dev/null +++ b/pkg/controller/external_secrets/utils_test.go @@ -0,0 +1,113 @@ +package external_secrets + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/openshift/external-secrets-operator/pkg/controller/client/fakes" + "github.com/openshift/external-secrets-operator/pkg/controller/common" + "github.com/openshift/external-secrets-operator/pkg/controller/commontest" +) + +func TestCreateWithFallback(t *testing.T) { + resourceMetadata := common.ResourceMetadata{ + Labels: controllerDefaultResourceLabels, + } + + tests := []struct { + name string + preReq func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) + wantErr string + wantUpdate bool + }{ + { + name: "resource created successfully", + preReq: func(cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return nil + }) + }, + }, + { + name: "create fails with non-AlreadyExists error", + preReq: func(cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: "failed to create test-ns/test-sa: test client error", + }, + { + name: "AlreadyExists triggers uncached update to restore labels", + preReq: func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "serviceaccounts"}, "test-sa") + }) + uncached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + if obj.GetLabels()["app"] != "external-secrets" { + t.Errorf("expected managed label app=external-secrets to be present, got %v", obj.GetLabels()) + } + return nil + }) + }, + wantUpdate: true, + }, + { + name: "AlreadyExists but uncached update fails", + preReq: func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "serviceaccounts"}, "test-sa") + }) + uncached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: "failed to restore test-ns/test-sa to desired state: test client error", + wantUpdate: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := testReconciler(t) + cached := &fakes.FakeCtrlClient{} + uncached := &fakes.FakeCtrlClient{} + if tt.preReq != nil { + tt.preReq(cached, uncached) + } + r.CtrlClient = cached + r.UncachedClient = uncached + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sa", + Namespace: "test-ns", + Labels: controllerDefaultResourceLabels, + }, + } + + err := r.createWithFallback(desired, resourceMetadata, "test-ns/test-sa") + + if tt.wantErr != "" { + if err == nil || err.Error() != tt.wantErr { + t.Errorf("createWithFallback() err = %v, wantErr = %v", err, tt.wantErr) + } + } else if err != nil { + t.Errorf("createWithFallback() unexpected error: %v", err) + } + + if tt.wantUpdate && uncached.UpdateWithRetryCallCount() == 0 { + t.Error("expected UncachedClient.UpdateWithRetry to be called, but it wasn't") + } + if !tt.wantUpdate && uncached.UpdateWithRetryCallCount() > 0 { + t.Error("expected no UncachedClient.UpdateWithRetry call, but one was made") + } + }) + } +} diff --git a/pkg/controller/external_secrets/validatingwebhook.go b/pkg/controller/external_secrets/validatingwebhook.go index 0c8fc0d03..fd2603465 100644 --- a/pkg/controller/external_secrets/validatingwebhook.go +++ b/pkg/controller/external_secrets/validatingwebhook.go @@ -39,8 +39,8 @@ func (r *Reconciler) createOrApplyValidatingWebhookConfiguration(esc *operatorv1 } if !exist { - if err := r.Create(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to create validatingWebhook resource %s", validatingWebhookName) + if err := r.createWithFallback(desired, resourceMetadata, validatingWebhookName); err != nil { + return err } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "validatingWebhook resource %s created", validatingWebhookName) } diff --git a/pkg/controller/external_secrets/validatingwebhook_test.go b/pkg/controller/external_secrets/validatingwebhook_test.go index 4617b0619..f192a7549 100644 --- a/pkg/controller/external_secrets/validatingwebhook_test.go +++ b/pkg/controller/external_secrets/validatingwebhook_test.go @@ -80,7 +80,7 @@ func TestCreateOrApplyValidatingWebhookConfiguration(t *testing.T) { return nil }) }, - wantErr: fmt.Sprintf("failed to create validatingWebhook resource %s: %s", testValidateWebhookConfigurationResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create %s: %s", testValidateWebhookConfigurationResourceName, commontest.ErrTestClient), }, { name: "validatingWebhookConfiguration creation successful", From 8c5fe487c6bf2414b495716589d5e88d445db871 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Tue, 16 Jun 2026 18:58:34 +0530 Subject: [PATCH 2/6] Address review comments Signed-off-by: chiragkyal --- .../external_secrets/certificate_test.go | 2 +- pkg/controller/external_secrets/configmap.go | 55 ++++++----- .../external_secrets/configmap_test.go | 98 +++++-------------- pkg/controller/external_secrets/controller.go | 28 +++++- .../external_secrets/networkpolicy_test.go | 4 +- pkg/controller/external_secrets/rbacs_test.go | 8 +- .../external_secrets/secret_test.go | 2 +- .../external_secrets/service_test.go | 4 +- .../external_secrets/serviceaccounts_test.go | 2 +- pkg/controller/external_secrets/test_utils.go | 8 +- pkg/controller/external_secrets/utils.go | 15 ++- pkg/controller/external_secrets/utils_test.go | 4 +- .../validatingwebhook_test.go | 2 +- 13 files changed, 117 insertions(+), 115 deletions(-) diff --git a/pkg/controller/external_secrets/certificate_test.go b/pkg/controller/external_secrets/certificate_test.go index 090e1d273..d20b5039e 100644 --- a/pkg/controller/external_secrets/certificate_test.go +++ b/pkg/controller/external_secrets/certificate_test.go @@ -268,7 +268,7 @@ func TestCreateOrApplyCertificates(t *testing.T) { esc.Spec.ControllerConfig.CertProvider.CertManager.IssuerRef.Name = testIssuerName }, recon: false, - wantErr: fmt.Sprintf("failed to create %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create Certificate %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateCertificateResourceName, commontest.ErrTestClient), }, { name: "successful webhook certificate creation", diff --git a/pkg/controller/external_secrets/configmap.go b/pkg/controller/external_secrets/configmap.go index 7cb694d0b..72630337c 100644 --- a/pkg/controller/external_secrets/configmap.go +++ b/pkg/controller/external_secrets/configmap.go @@ -1,15 +1,18 @@ package external_secrets import ( + "encoding/json" "fmt" "maps" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1" + operatorclient "github.com/openshift/external-secrets-operator/pkg/controller/client" "github.com/openshift/external-secrets-operator/pkg/controller/common" ) @@ -59,24 +62,17 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern if !exist { // NOTE: This ConfigMap cannot use the generic createWithFallback helper because - // the Data field is managed by CNO (not by this operator). A plain UpdateWithRetry - // with our empty-Data desired object would wipe the CA certificates CNO injected. - // Instead, on AlreadyExists we must fetch the real object first, preserve its Data, - // then update only labels/annotations. + // the Data/BinaryData fields are managed by CNO (not by this operator). On + // AlreadyExists we use a MergePatch that touches only metadata, leaving + // CNO-injected CA certificates untouched. if err := r.Create(r.ctx, desiredConfigMap); err != nil { if !apierrors.IsAlreadyExists(err) { return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) } - r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, restoring desired state", "name", configMapName) - actualConfigMap := &corev1.ConfigMap{} - if _, fetchErr := r.UncachedClient.Exists(r.ctx, client.ObjectKeyFromObject(desiredConfigMap), actualConfigMap); fetchErr != nil { - return common.FromClientError(fetchErr, "failed to fetch existing %s trusted CA bundle ConfigMap for restoration", configMapName) - } - desiredConfigMap.Data = actualConfigMap.Data - desiredConfigMap.BinaryData = actualConfigMap.BinaryData + r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName) common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) - if updateErr := r.UncachedClient.UpdateWithRetry(r.ctx, desiredConfigMap); updateErr != nil { - return common.FromClientError(updateErr, "failed to restore %s trusted CA bundle ConfigMap to desired state", configMapName) + if patchErr := r.patchConfigMapMetadata(desiredConfigMap, r.UncachedClient); patchErr != nil { + return common.FromClientError(patchErr, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName) } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s restored to desired state", configMapName) return nil @@ -85,17 +81,13 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern return nil } - // ConfigMap exists, ensure it has the correct labels and annotations. - // Do not update the data of the ConfigMap since it is managed by CNO. - // MergeFetchedAnnotations preserves external annotations (e.g., CNO's openshift.io/owning-component). + // ConfigMap exists in cache — ensure it has the correct labels and annotations. + // Use a metadata-only patch so CNO-managed Data/BinaryData are never touched. if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) { - r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, updating to desired state", "name", configMapName) - // Preserve data from existing ConfigMap (managed by CNO) - desiredConfigMap.Data = existingConfigMap.Data - desiredConfigMap.BinaryData = existingConfigMap.BinaryData + r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName) common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) - if err := r.UpdateWithRetry(r.ctx, desiredConfigMap); err != nil { - return common.FromClientError(err, "failed to update %s trusted CA bundle ConfigMap resource", configMapName) + if err := r.patchConfigMapMetadata(desiredConfigMap, r.CtrlClient); err != nil { + return common.FromClientError(err, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName) } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "trusted CA bundle ConfigMap resource %s reconciled back to desired state", configMapName) } else { @@ -105,6 +97,25 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern return nil } +// patchConfigMapMetadata sends a MergePatch that sets only labels and annotations +// on the ConfigMap, leaving all other fields (especially Data/BinaryData managed +// by CNO) untouched. The caller chooses the client: the cached client when the +// object is visible in the informer cache, or the uncached client when it has +// fallen out of the label-filtered cache. +func (r *Reconciler) patchConfigMapMetadata(desired *corev1.ConfigMap, patchClient operatorclient.CtrlClient) error { + patch := map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": desired.Labels, + "annotations": desired.Annotations, + }, + } + patchBytes, err := json.Marshal(patch) + if err != nil { + return fmt.Errorf("failed to marshal metadata patch: %w", err) + } + return patchClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes)) +} + // getTrustedCABundleLabels merges resource labels with the injection label. func getTrustedCABundleLabels(resourceLabels map[string]string) map[string]string { labels := make(map[string]string) diff --git a/pkg/controller/external_secrets/configmap_test.go b/pkg/controller/external_secrets/configmap_test.go index a51e9a1df..b4c7829ef 100644 --- a/pkg/controller/external_secrets/configmap_test.go +++ b/pkg/controller/external_secrets/configmap_test.go @@ -36,7 +36,7 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { preReq func(r *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) wantErr string wantCreate bool - wantUpdate bool + wantPatch bool noProxy bool }{ { @@ -79,10 +79,9 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { return true, nil }) }, - wantUpdate: false, }, { - name: "ConfigMap exists with wrong labels, update triggered", + name: "ConfigMap exists with wrong labels, patch triggered", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { @@ -97,21 +96,21 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { existing.DeepCopyInto(obj.(*corev1.ConfigMap)) return true, nil }) - cached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error { cm, ok := obj.(*corev1.ConfigMap) if !ok { t.Errorf("expected ConfigMap, got %T", obj) } if cm.Labels[trustedCABundleInjectLabel] != "true" { - t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel]) + t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel]) } - if cm.Data["ca-bundle.crt"] != "cert-data" { - t.Errorf("CNO-managed data should be preserved, got %v", cm.Data) + if patch.Type() != types.MergePatchType { + t.Errorf("expected MergePatch, got %s", patch.Type()) } return nil }) }, - wantUpdate: true, + wantPatch: true, }, { // Regression test for: labels updating with app: external-secrets of cm @@ -119,51 +118,36 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { // // When the managed label (app=external-secrets) is removed from the ConfigMap, // the object falls out of the label-filtered cache. The cached Exists() returns - // false, Create() fails with AlreadyExists, and the controller must use the - // uncached client to fetch and restore the correct labels instead of returning - // an error that blocks reconciliation. - name: "Create returns AlreadyExists (label-filtered cache miss): uncached client restores labels", + // false, Create() fails with AlreadyExists, and the controller must patch only + // the metadata (labels/annotations) via the uncached client, leaving + // CNO-managed Data/BinaryData untouched. + name: "Create returns AlreadyExists (label-filtered cache miss): uncached client patches metadata", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { - // Cached client doesn't see the ConfigMap because its label was changed. cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { return false, nil }) cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) }) - // Uncached client fetches the real object from the API server. - uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { - existing := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: trustedCABundleConfigMapName, - Namespace: commontest.TestExternalSecretsNamespace, - ResourceVersion: "1000", - Labels: map[string]string{"app": "update-secrets"}, - }, - Data: cnoData, - } - existing.DeepCopyInto(obj.(*corev1.ConfigMap)) - return true, nil - }) - uncached.UpdateWithRetryCalls(func(_ context.Context, obj client.Object, _ ...client.UpdateOption) error { + uncached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error { cm, ok := obj.(*corev1.ConfigMap) if !ok { t.Errorf("expected ConfigMap, got %T", obj) } if cm.Labels["app"] != "external-secrets" { - t.Errorf("expected app=external-secrets label restored, got %q", cm.Labels["app"]) + t.Errorf("expected app=external-secrets label in patch target, got %q", cm.Labels["app"]) } if cm.Labels[trustedCABundleInjectLabel] != "true" { - t.Errorf("expected inject label restored, got %q", cm.Labels[trustedCABundleInjectLabel]) + t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel]) } - if cm.Data["ca-bundle.crt"] != "cert-data" { - t.Errorf("CNO-managed data should be preserved, got %v", cm.Data) + if patch.Type() != types.MergePatchType { + t.Errorf("expected MergePatch, got %s", patch.Type()) } return nil }) }, - wantUpdate: true, + wantPatch: true, }, { name: "proxy not configured: ConfigMap reconcile skipped", @@ -194,23 +178,7 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error", }, { - name: "uncached Exists fails after AlreadyExists from Create", - resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), - preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { - cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { - return false, nil - }) - cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { - return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) - }) - uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { - return false, commontest.ErrTestClient - }) - }, - wantErr: "failed to fetch existing external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap for restoration: test client error", - }, - { - name: "uncached UpdateWithRetry fails after AlreadyExists from Create", + name: "uncached Patch fails after AlreadyExists from Create", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { @@ -219,26 +187,14 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) }) - uncached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { - existing := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: trustedCABundleConfigMapName, - Namespace: commontest.TestExternalSecretsNamespace, - ResourceVersion: "1000", - Labels: map[string]string{"app": "update-secrets"}, - }, - } - existing.DeepCopyInto(obj.(*corev1.ConfigMap)) - return true, nil - }) - uncached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + uncached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { return commontest.ErrTestClient }) }, - wantErr: "failed to restore external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap to desired state: test client error", + wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error", }, { - name: "cached UpdateWithRetry fails when ConfigMap has wrong labels", + name: "cached Patch fails when ConfigMap has wrong labels", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) { @@ -252,11 +208,11 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { existing.DeepCopyInto(obj.(*corev1.ConfigMap)) return true, nil }) - cached.UpdateWithRetryCalls(func(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { return commontest.ErrTestClient }) }, - wantErr: "failed to update external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error", + wantErr: "failed to patch external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap metadata: test client error", }, } @@ -293,11 +249,11 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { if tt.wantCreate && cached.CreateCallCount() == 0 { t.Error("expected Create to be called, but it wasn't") } - if tt.wantUpdate && cached.UpdateWithRetryCallCount() == 0 && uncached.UpdateWithRetryCallCount() == 0 { - t.Error("expected UpdateWithRetry to be called (on cached or uncached client), but it wasn't") + if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 { + t.Error("expected Patch to be called (on cached or uncached client), but it wasn't") } - if !tt.wantUpdate && (cached.UpdateWithRetryCallCount() > 0 || uncached.UpdateWithRetryCallCount() > 0) { - t.Error("expected no UpdateWithRetry call, but one was made") + if !tt.wantPatch && (cached.PatchCallCount() > 0 || uncached.PatchCallCount() > 0) { + t.Error("expected no Patch call, but one was made") } }) } diff --git a/pkg/controller/external_secrets/controller.go b/pkg/controller/external_secrets/controller.go index 0706bd009..49e9cba6d 100644 --- a/pkg/controller/external_secrets/controller.go +++ b/pkg/controller/external_secrets/controller.go @@ -270,20 +270,40 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue } + logIgnored := func(obj client.Object) { + r.log.V(4).Info("object not of interest, ignoring reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace()) + } + // On updates, check both old and new objects so that events where the managed // label is removed externally still trigger reconciliation and label restoration. managedResources := predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { - return isManagedResource(e.Object) + if !isManagedResource(e.Object) { + logIgnored(e.Object) + return false + } + return true }, UpdateFunc: func(e event.UpdateEvent) bool { - return isManagedResource(e.ObjectOld) || isManagedResource(e.ObjectNew) + if !isManagedResource(e.ObjectOld) && !isManagedResource(e.ObjectNew) { + logIgnored(e.ObjectNew) + return false + } + return true }, DeleteFunc: func(e event.DeleteEvent) bool { - return isManagedResource(e.Object) + if !isManagedResource(e.Object) { + logIgnored(e.Object) + return false + } + return true }, GenericFunc: func(e event.GenericEvent) bool { - return isManagedResource(e.Object) + if !isManagedResource(e.Object) { + logIgnored(e.Object) + return false + } + return true }, } diff --git a/pkg/controller/external_secrets/networkpolicy_test.go b/pkg/controller/external_secrets/networkpolicy_test.go index ba77f1903..ab5937679 100644 --- a/pkg/controller/external_secrets/networkpolicy_test.go +++ b/pkg/controller/external_secrets/networkpolicy_test.go @@ -146,7 +146,7 @@ func TestCreateOrApplyStaticNetworkPolicies(t *testing.T) { return nil }) }, - wantErr: "failed to create external-secrets/eso-sys-deny-all-traffic: test client error", + wantErr: "failed to create NetworkPolicy external-secrets/eso-sys-deny-all-traffic: test client error", }, { name: "network policy exists check fails", @@ -344,7 +344,7 @@ func TestCreateOrApplyCustomNetworkPolicies(t *testing.T) { }, } }, - wantErr: "failed to create external-secrets/eso-user-test-fail-policy: test client error", + wantErr: "failed to create NetworkPolicy external-secrets/eso-user-test-fail-policy: test client error", }, { name: "custom network policy updated successfully", diff --git a/pkg/controller/external_secrets/rbacs_test.go b/pkg/controller/external_secrets/rbacs_test.go index 3e2f48d1f..352b870b0 100644 --- a/pkg/controller/external_secrets/rbacs_test.go +++ b/pkg/controller/external_secrets/rbacs_test.go @@ -124,7 +124,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets-cert-controller: test client error`, + wantErr: `failed to create ClusterRoleBinding external-secrets-cert-controller: test client error`, }, { name: "clusterrole reconciliation updating to desired state fails", @@ -164,7 +164,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets-cert-controller: test client error`, + wantErr: `failed to create ClusterRole external-secrets-cert-controller: test client error`, }, { name: "role reconciliation updating to desired state fails", @@ -202,7 +202,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets/external-secrets-leaderelection: test client error`, + wantErr: `failed to create Role external-secrets/external-secrets-leaderelection: test client error`, }, { name: "rolebindings reconciliation updating to desired state fails", @@ -240,7 +240,7 @@ func TestCreateOrApplyRBACResource(t *testing.T) { return nil }) }, - wantErr: `failed to create external-secrets/external-secrets-leaderelection: test client error`, + wantErr: `failed to create RoleBinding external-secrets/external-secrets-leaderelection: test client error`, }, { name: "clusterroles creation successful", diff --git a/pkg/controller/external_secrets/secret_test.go b/pkg/controller/external_secrets/secret_test.go index 913274fce..a9db0b3d1 100644 --- a/pkg/controller/external_secrets/secret_test.go +++ b/pkg/controller/external_secrets/secret_test.go @@ -155,7 +155,7 @@ func TestCreateOrApplySecret(t *testing.T) { return nil }) }, - wantErr: fmt.Sprintf("failed to create %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create Secret %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), }, { name: "successful secret creation", diff --git a/pkg/controller/external_secrets/service_test.go b/pkg/controller/external_secrets/service_test.go index 34bc9e563..67f06b7c0 100644 --- a/pkg/controller/external_secrets/service_test.go +++ b/pkg/controller/external_secrets/service_test.go @@ -62,7 +62,7 @@ func TestCreateOrApplyServices(t *testing.T) { }, } }, - wantErr: `failed to create external-secrets/bitwarden-sdk-server: test client error`, + wantErr: `failed to create Service external-secrets/bitwarden-sdk-server: test client error`, }, { @@ -107,7 +107,7 @@ func TestCreateOrApplyServices(t *testing.T) { return commontest.ErrTestClient }) }, - wantErr: `failed to create external-secrets/external-secrets-webhook: test client error`, + wantErr: `failed to create Service external-secrets/external-secrets-webhook: test client error`, }, { name: "service with custom annotations applied successfully", diff --git a/pkg/controller/external_secrets/serviceaccounts_test.go b/pkg/controller/external_secrets/serviceaccounts_test.go index cf961f181..7eece199b 100644 --- a/pkg/controller/external_secrets/serviceaccounts_test.go +++ b/pkg/controller/external_secrets/serviceaccounts_test.go @@ -119,7 +119,7 @@ func TestCreateOrApplyServiceAccounts(t *testing.T) { return nil }) }, - wantErr: "failed to create external-secrets/external-secrets: test client error", + wantErr: "failed to create ServiceAccount external-secrets/external-secrets: test client error", }, { name: "serviceaccount with custom annotations applied successfully", diff --git a/pkg/controller/external_secrets/test_utils.go b/pkg/controller/external_secrets/test_utils.go index 77f82e6b9..87a40f77b 100644 --- a/pkg/controller/external_secrets/test_utils.go +++ b/pkg/controller/external_secrets/test_utils.go @@ -11,6 +11,8 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" "github.com/go-logr/logr/testr" @@ -36,8 +38,12 @@ func testResourceMetadata(esc *operatorv1alpha1.ExternalSecretsConfig) common.Re // testReconciler returns a sample Reconciler instance. func testReconciler(t *testing.T) *Reconciler { + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + utilruntime.Must(certmanagerv1.AddToScheme(scheme)) + return &Reconciler{ - Scheme: runtime.NewScheme(), + Scheme: scheme, ctx: context.Background(), eventRecorder: record.NewFakeRecorder(100), log: testr.New(t), diff --git a/pkg/controller/external_secrets/utils.go b/pkg/controller/external_secrets/utils.go index 17ae9e9b9..6763d191a 100644 --- a/pkg/controller/external_secrets/utils.go +++ b/pkg/controller/external_secrets/utils.go @@ -10,6 +10,7 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "go.uber.org/zap/zapcore" @@ -213,14 +214,22 @@ func validateProxy(rawURL string) error { // uncached client to update the resource directly on the API server, restoring the managed // labels and annotations to the desired state. func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata common.ResourceMetadata, resourceName string) error { + gvk, gvkErr := apiutil.GVKForObject(desired, r.Scheme) + kind := desired.GetObjectKind().GroupVersionKind().Kind + if gvkErr == nil { + kind = gvk.Kind + } + if err := r.Create(r.ctx, desired); err != nil { if !apierrors.IsAlreadyExists(err) { - return common.FromClientError(err, "failed to create %s", resourceName) + return common.FromClientError(err, "failed to create %s %s", kind, resourceName) } - r.log.V(1).Info("resource exists on API server but absent from label-filtered cache (managed labels may have been externally modified), restoring desired state", "name", resourceName) + + r.log.V(1).Info("resource exists on API server but absent from label-filtered cache, restoring desired state", + "kind", kind, "name", resourceName) common.RemoveObsoleteAnnotations(desired, resourceMetadata) if err := r.UncachedClient.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to restore %s to desired state", resourceName) + return common.FromClientError(err, "failed to restore %s %s to desired state", kind, resourceName) } } return nil diff --git a/pkg/controller/external_secrets/utils_test.go b/pkg/controller/external_secrets/utils_test.go index f920e570b..dbd37c166 100644 --- a/pkg/controller/external_secrets/utils_test.go +++ b/pkg/controller/external_secrets/utils_test.go @@ -41,7 +41,7 @@ func TestCreateWithFallback(t *testing.T) { return commontest.ErrTestClient }) }, - wantErr: "failed to create test-ns/test-sa: test client error", + wantErr: "failed to create ServiceAccount test-ns/test-sa: test client error", }, { name: "AlreadyExists triggers uncached update to restore labels", @@ -68,7 +68,7 @@ func TestCreateWithFallback(t *testing.T) { return commontest.ErrTestClient }) }, - wantErr: "failed to restore test-ns/test-sa to desired state: test client error", + wantErr: "failed to restore ServiceAccount test-ns/test-sa to desired state: test client error", wantUpdate: true, }, } diff --git a/pkg/controller/external_secrets/validatingwebhook_test.go b/pkg/controller/external_secrets/validatingwebhook_test.go index f192a7549..19eafaa18 100644 --- a/pkg/controller/external_secrets/validatingwebhook_test.go +++ b/pkg/controller/external_secrets/validatingwebhook_test.go @@ -80,7 +80,7 @@ func TestCreateOrApplyValidatingWebhookConfiguration(t *testing.T) { return nil }) }, - wantErr: fmt.Sprintf("failed to create %s: %s", testValidateWebhookConfigurationResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create ValidatingWebhookConfiguration %s: %s", testValidateWebhookConfigurationResourceName, commontest.ErrTestClient), }, { name: "validatingWebhookConfiguration creation successful", From 2654a788f55696b088d8eb9f30dfb91e3e586c4a Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Tue, 16 Jun 2026 19:02:52 +0530 Subject: [PATCH 3/6] Add e2e for the managed label modification Signed-off-by: chiragkyal --- test/e2e/e2e_test.go | 112 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 229b59038..4d17d71ea 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -1056,6 +1056,118 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }) + Context("Managed Label Restoration", Label("Platform:Generic"), func() { + const ( + managedLabelKey = "app" + managedLabelValue = "external-secrets" + ) + + It("should restore the app=external-secrets label on a ServiceAccount after external removal", func() { + saName := "external-secrets" + + By("Verifying ServiceAccount has the managed label initially") + Eventually(func(g Gomega) { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(sa.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the ServiceAccount") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(sa.Labels, managedLabelKey) + _, err = clientset.CoreV1().ServiceAccounts(operandNamespace).Update(ctx, sa, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(sa.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on ServiceAccount %s", saName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a Role after external removal", func() { + roleName := "external-secrets" + + By("Verifying Role has the managed label initially") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(role.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the Role") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(role.Labels, managedLabelKey) + _, err = clientset.RbacV1().Roles(operandNamespace).Update(ctx, role, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(role.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on Role %s", roleName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a Deployment after external removal", func() { + depName := "external-secrets" + + By("Verifying Deployment has the managed label initially") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dep.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the Deployment") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(dep.Labels, managedLabelKey) + _, err = clientset.AppsV1().Deployments(operandNamespace).Update(ctx, dep, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + dep, err := clientset.AppsV1().Deployments(operandNamespace).Get(ctx, depName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dep.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on Deployment %s", depName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should keep ExternalSecretsConfig in Ready state after label restoration", func() { + By("Verifying ExternalSecretsConfig is Ready and not Degraded") + Expect(utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, "cluster", 2*time.Minute)).To(Succeed(), + "ExternalSecretsConfig should remain Ready after label tampering and restoration") + }) + + It("should keep operand pods running after label restoration", func() { + By("Verifying operand pods are still ready") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed(), "operand pods should still be running after label restoration") + }) + }) + AfterAll(func() { By("Deleting the externalsecrets.openshift.operator.io/cluster CR") loader.DeleteFromFile(testassets.ReadFile, externalSecretsFile, "") From 5525eafa161a825610f2c49b729e96fe6868c8dc Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Wed, 17 Jun 2026 15:11:31 +0530 Subject: [PATCH 4/6] Address review comments Signed-off-by: chiragkyal --- .../external_secrets/certificate.go | 3 +- pkg/controller/external_secrets/configmap.go | 28 +------ .../external_secrets/configmap_test.go | 21 +++-- pkg/controller/external_secrets/controller.go | 38 +++------ .../external_secrets/deployments.go | 3 +- .../external_secrets/networkpolicy.go | 9 +-- pkg/controller/external_secrets/rbacs.go | 12 +-- pkg/controller/external_secrets/secret.go | 3 +- .../external_secrets/serviceaccounts.go | 3 +- pkg/controller/external_secrets/services.go | 3 +- pkg/controller/external_secrets/utils.go | 40 ++++++++- pkg/controller/external_secrets/utils_test.go | 81 ++++++++++++++++--- .../external_secrets/validatingwebhook.go | 3 +- test/e2e/e2e_test.go | 31 ++++--- 14 files changed, 158 insertions(+), 120 deletions(-) diff --git a/pkg/controller/external_secrets/certificate.go b/pkg/controller/external_secrets/certificate.go index b087ab4a9..d37ca4ed2 100644 --- a/pkg/controller/external_secrets/certificate.go +++ b/pkg/controller/external_secrets/certificate.go @@ -71,10 +71,9 @@ func (r *Reconciler) createOrApplyCertificate(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("certificate resource already exists and is in expected state", "name", certificateName) } if !exist { - if err := r.createWithFallback(desired, resourceMetadata, certificateName); err != nil { + if err := r.createWithFallback(desired, resourceMetadata, certificateName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "certificate resource %s created", certificateName) } return nil diff --git a/pkg/controller/external_secrets/configmap.go b/pkg/controller/external_secrets/configmap.go index 72630337c..73ab6f485 100644 --- a/pkg/controller/external_secrets/configmap.go +++ b/pkg/controller/external_secrets/configmap.go @@ -1,18 +1,15 @@ package external_secrets import ( - "encoding/json" "fmt" "maps" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1" - operatorclient "github.com/openshift/external-secrets-operator/pkg/controller/client" "github.com/openshift/external-secrets-operator/pkg/controller/common" ) @@ -70,8 +67,7 @@ func (r *Reconciler) ensureTrustedCABundleConfigMap(esc *operatorv1alpha1.Extern return common.FromClientError(err, "failed to create %s trusted CA bundle ConfigMap resource", configMapName) } r.log.V(1).Info("trusted CA bundle ConfigMap exists on API server but absent from label-filtered cache, patching metadata", "name", configMapName) - common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) - if patchErr := r.patchConfigMapMetadata(desiredConfigMap, r.UncachedClient); patchErr != nil { + if patchErr := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); patchErr != nil { return common.FromClientError(patchErr, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName) } 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 // Use a metadata-only patch so CNO-managed Data/BinaryData are never touched. if exist && common.ObjectMetadataModified(desiredConfigMap, existingConfigMap, &resourceMetadata) { r.log.V(1).Info("trusted CA bundle ConfigMap has been modified, patching metadata to desired state", "name", configMapName) - common.RemoveObsoleteAnnotations(desiredConfigMap, resourceMetadata) - if err := r.patchConfigMapMetadata(desiredConfigMap, r.CtrlClient); err != nil { + if err := r.patchResourceMetadata(desiredConfigMap, resourceMetadata); err != nil { return common.FromClientError(err, "failed to patch %s trusted CA bundle ConfigMap metadata", configMapName) } 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 return nil } -// patchConfigMapMetadata sends a MergePatch that sets only labels and annotations -// on the ConfigMap, leaving all other fields (especially Data/BinaryData managed -// by CNO) untouched. The caller chooses the client: the cached client when the -// object is visible in the informer cache, or the uncached client when it has -// fallen out of the label-filtered cache. -func (r *Reconciler) patchConfigMapMetadata(desired *corev1.ConfigMap, patchClient operatorclient.CtrlClient) error { - patch := map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": desired.Labels, - "annotations": desired.Annotations, - }, - } - patchBytes, err := json.Marshal(patch) - if err != nil { - return fmt.Errorf("failed to marshal metadata patch: %w", err) - } - return patchClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes)) -} - // getTrustedCABundleLabels merges resource labels with the injection label. func getTrustedCABundleLabels(resourceLabels map[string]string) map[string]string { labels := make(map[string]string) diff --git a/pkg/controller/external_secrets/configmap_test.go b/pkg/controller/external_secrets/configmap_test.go index b4c7829ef..00d528b2a 100644 --- a/pkg/controller/external_secrets/configmap_test.go +++ b/pkg/controller/external_secrets/configmap_test.go @@ -119,18 +119,17 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { // When the managed label (app=external-secrets) is removed from the ConfigMap, // the object falls out of the label-filtered cache. The cached Exists() returns // false, Create() fails with AlreadyExists, and the controller must patch only - // the metadata (labels/annotations) via the uncached client, leaving - // CNO-managed Data/BinaryData untouched. - name: "Create returns AlreadyExists (label-filtered cache miss): uncached client patches metadata", + // the metadata (labels/annotations), leaving CNO-managed Data/BinaryData untouched. + name: "Create returns AlreadyExists (label-filtered cache miss): patches metadata to restore labels", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), - preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { return false, nil }) cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) }) - uncached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error { + cached.PatchCalls(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error { cm, ok := obj.(*corev1.ConfigMap) if !ok { t.Errorf("expected ConfigMap, got %T", obj) @@ -178,16 +177,16 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { wantErr: "failed to create external-secrets/external-secrets-trusted-ca-bundle trusted CA bundle ConfigMap resource: test client error", }, { - name: "uncached Patch fails after AlreadyExists from Create", + name: "Patch fails after AlreadyExists from Create", resourceMetadata: testResourceMetadata(commontest.TestExternalSecretsConfig()), - preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { + preReq: func(_ *Reconciler, cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, _ client.Object) (bool, error) { return false, nil }) cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "configmaps"}, trustedCABundleConfigMapName) }) - uncached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + cached.PatchCalls(func(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { return commontest.ErrTestClient }) }, @@ -249,10 +248,10 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { if tt.wantCreate && cached.CreateCallCount() == 0 { t.Error("expected Create to be called, but it wasn't") } - if tt.wantPatch && cached.PatchCallCount() == 0 && uncached.PatchCallCount() == 0 { - t.Error("expected Patch to be called (on cached or uncached client), but it wasn't") + if tt.wantPatch && cached.PatchCallCount() == 0 { + t.Error("expected Patch to be called, but it wasn't") } - if !tt.wantPatch && (cached.PatchCallCount() > 0 || uncached.PatchCallCount() > 0) { + if !tt.wantPatch && cached.PatchCallCount() > 0 { t.Error("expected no Patch call, but one was made") } }) diff --git a/pkg/controller/external_secrets/controller.go b/pkg/controller/external_secrets/controller.go index 49e9cba6d..3be983d48 100644 --- a/pkg/controller/external_secrets/controller.go +++ b/pkg/controller/external_secrets/controller.go @@ -267,47 +267,33 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { } isManagedResource := func(object client.Object) bool { - return object.GetLabels() != nil && object.GetLabels()[requestEnqueueLabelKey] == requestEnqueueLabelValue - } - - logIgnored := func(obj client.Object) { - r.log.V(4).Info("object not of interest, ignoring reconcile event", "object", fmt.Sprintf("%T", obj), "name", obj.GetName(), "namespace", obj.GetNamespace()) + labels := object.GetLabels() + matches := labels != nil && labels[requestEnqueueLabelKey] == requestEnqueueLabelValue + r.log.V(4).Info("predicate evaluation", "object", fmt.Sprintf("%T", object), "name", object.GetName(), "namespace", object.GetNamespace(), "labels", labels, "matches", matches) + return matches } // On updates, check both old and new objects so that events where the managed // label is removed externally still trigger reconciliation and label restoration. managedResources := predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { - if !isManagedResource(e.Object) { - logIgnored(e.Object) - return false - } - return true + return isManagedResource(e.Object) }, UpdateFunc: func(e event.UpdateEvent) bool { - if !isManagedResource(e.ObjectOld) && !isManagedResource(e.ObjectNew) { - logIgnored(e.ObjectNew) - return false - } - return true + return isManagedResource(e.ObjectOld) || isManagedResource(e.ObjectNew) }, DeleteFunc: func(e event.DeleteEvent) bool { - if !isManagedResource(e.Object) { - logIgnored(e.Object) - return false - } - return true + return isManagedResource(e.Object) }, GenericFunc: func(e event.GenericEvent) bool { - if !isManagedResource(e.Object) { - logIgnored(e.Object) - return false - } - return true + return isManagedResource(e.Object) }, } - withIgnoreStatusUpdatePredicates := builder.WithPredicates(predicate.GenerationChangedPredicate{}, managedResources) + withIgnoreStatusUpdatePredicates := builder.WithPredicates( + predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{}), + managedResources, + ) managedResourcePredicate := builder.WithPredicates(managedResources) mgrBuilder := ctrl.NewControllerManagedBy(mgr). diff --git a/pkg/controller/external_secrets/deployments.go b/pkg/controller/external_secrets/deployments.go index e3af9a49a..a9ad9973b 100644 --- a/pkg/controller/external_secrets/deployments.go +++ b/pkg/controller/external_secrets/deployments.go @@ -88,10 +88,9 @@ func (r *Reconciler) createOrApplyDeploymentFromAsset(esc *operatorv1alpha1.Exte } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s updated", deploymentName) case !exist: - if err := r.createWithFallback(deployment, resourceMetadata, deploymentName); err != nil { + if err := r.createWithFallback(deployment, resourceMetadata, deploymentName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s created", deploymentName) default: r.log.V(4).Info("deployment resource already exists and is in expected state", "name", deploymentName) } diff --git a/pkg/controller/external_secrets/networkpolicy.go b/pkg/controller/external_secrets/networkpolicy.go index a8844ded3..53d8d1f76 100644 --- a/pkg/controller/external_secrets/networkpolicy.go +++ b/pkg/controller/external_secrets/networkpolicy.go @@ -144,10 +144,9 @@ func (r *Reconciler) createOrApplyCustomNetworkPolicy(esc *operatorv1alpha1.Exte } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName) case !exists: - if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil { + if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName) default: r.log.V(4).Info("NetworkPolicy already up-to-date", "name", networkPolicyName) } @@ -183,10 +182,9 @@ func (r *Reconciler) createOrApplyNetworkPolicyFromAsset(esc *operatorv1alpha1.E } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s updated", networkPolicyName) case !exists: - if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName); err != nil { + if err := r.createWithFallback(networkPolicy, resourceMetadata, networkPolicyName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "NetworkPolicy %s created", networkPolicyName) default: r.log.V(4).Info("NetworkPolicy already up-to-date", "name", networkPolicyName) } @@ -288,10 +286,9 @@ func (r *Reconciler) reconcileProxyEgressPolicy(esc *operatorv1alpha1.ExternalSe } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s updated", npName) case !exists: - if err := r.createWithFallback(np, resourceMetadata, npName); err != nil { + if err := r.createWithFallback(np, resourceMetadata, npName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "proxy egress NetworkPolicy %s created", npName) if recon { r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "proxy egress NetworkPolicy %s already exists", npName) } diff --git a/pkg/controller/external_secrets/rbacs.go b/pkg/controller/external_secrets/rbacs.go index 543569364..fabc7cba4 100644 --- a/pkg/controller/external_secrets/rbacs.go +++ b/pkg/controller/external_secrets/rbacs.go @@ -125,10 +125,9 @@ func (r *Reconciler) createOrApplyClusterRole(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("clusterrole resource already exists and is in expected state", "name", clusterRoleName) } if !exist { - if err := r.createWithFallback(obj, resourceMetadata, clusterRoleName); err != nil { + if err := r.createWithFallback(obj, resourceMetadata, clusterRoleName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrole resource %s created", clusterRoleName) } return nil @@ -171,10 +170,9 @@ func (r *Reconciler) createOrApplyClusterRoleBinding(esc *operatorv1alpha1.Exter r.log.V(4).Info("clusterrolebinding resource already exists and is in expected state", "name", clusterRoleBindingName) } if !exist { - if err := r.createWithFallback(obj, resourceMetadata, clusterRoleBindingName); err != nil { + if err := r.createWithFallback(obj, resourceMetadata, clusterRoleBindingName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "clusterrolebinding resource %s created", clusterRoleBindingName) } return nil @@ -215,10 +213,9 @@ func (r *Reconciler) createOrApplyRole(esc *operatorv1alpha1.ExternalSecretsConf r.log.V(4).Info("role resource already exists and is in expected state", "name", roleName) } if !exist { - if err := r.createWithFallback(obj, resourceMetadata, roleName); err != nil { + if err := r.createWithFallback(obj, resourceMetadata, roleName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "role resource %s created", roleName) } return nil @@ -258,10 +255,9 @@ func (r *Reconciler) createOrApplyRoleBinding(esc *operatorv1alpha1.ExternalSecr r.log.V(4).Info("rolebinding resource already exists and is in expected state", "name", roleBindingName) } if !exist { - if err := r.createWithFallback(obj, resourceMetadata, roleBindingName); err != nil { + if err := r.createWithFallback(obj, resourceMetadata, roleBindingName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "rolebinding resource %s created", roleBindingName) } return nil diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go index 6c8b334da..14b75fcd2 100644 --- a/pkg/controller/external_secrets/secret.go +++ b/pkg/controller/external_secrets/secret.go @@ -44,10 +44,9 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo } if !exist { - if err := r.createWithFallback(desired, resourceMetadata, secretName); err != nil { + if err := r.createWithFallback(desired, resourceMetadata, secretName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName) } return nil } diff --git a/pkg/controller/external_secrets/serviceaccounts.go b/pkg/controller/external_secrets/serviceaccounts.go index 538ad0b6f..0c9d99be1 100644 --- a/pkg/controller/external_secrets/serviceaccounts.go +++ b/pkg/controller/external_secrets/serviceaccounts.go @@ -69,10 +69,9 @@ func (r *Reconciler) createOrApplyServiceAccounts(esc *operatorv1alpha1.External } if !exist { - if err := r.createWithFallback(desired, resourceMetadata, serviceAccountName); err != nil { + if err := r.createWithFallback(desired, resourceMetadata, serviceAccountName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Created serviceaccount %s", serviceAccountName) } } diff --git a/pkg/controller/external_secrets/services.go b/pkg/controller/external_secrets/services.go index ee24306da..ab2329ae5 100644 --- a/pkg/controller/external_secrets/services.go +++ b/pkg/controller/external_secrets/services.go @@ -74,10 +74,9 @@ func (r *Reconciler) createOrApplyServiceFromAsset(esc *operatorv1alpha1.Externa } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s updated", serviceName) case !exists: - if err := r.createWithFallback(service, resourceMetadata, serviceName); err != nil { + if err := r.createWithFallback(service, resourceMetadata, serviceName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Service %s created", serviceName) default: r.log.V(4).Info("Service already up-to-date", "name", serviceName) } diff --git a/pkg/controller/external_secrets/utils.go b/pkg/controller/external_secrets/utils.go index 6763d191a..7fa7aba8b 100644 --- a/pkg/controller/external_secrets/utils.go +++ b/pkg/controller/external_secrets/utils.go @@ -2,11 +2,14 @@ package external_secrets import ( "context" + "encoding/json" "fmt" "net/url" "os" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" @@ -213,11 +216,20 @@ func validateProxy(rawURL string) error { // When Create returns AlreadyExists, this method bypasses the stale cache by using the // uncached client to update the resource directly on the API server, restoring the managed // labels and annotations to the desired state. -func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata common.ResourceMetadata, resourceName string) error { - gvk, gvkErr := apiutil.GVKForObject(desired, r.Scheme) +// +// It records a "created" event on success, or a "restored" event when the AlreadyExists +// fallback path is taken. +func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata common.ResourceMetadata, resourceName string, esc *operatorv1alpha1.ExternalSecretsConfig) error { kind := desired.GetObjectKind().GroupVersionKind().Kind - if gvkErr == nil { - kind = gvk.Kind + if kind == "" { + gvk, gvkErr := apiutil.GVKForObject(desired, r.Scheme) + if gvkErr != nil { + r.log.V(5).Info("could not determine GVK, falling back to Go type name", + "type", fmt.Sprintf("%T", desired), "err", gvkErr) + kind = fmt.Sprintf("%T", desired) + } else { + kind = gvk.Kind + } } if err := r.Create(r.ctx, desired); err != nil { @@ -231,6 +243,26 @@ func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata if err := r.UncachedClient.UpdateWithRetry(r.ctx, desired); err != nil { return common.FromClientError(err, "failed to restore %s %s to desired state", kind, resourceName) } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "%s resource %s restored to desired state", kind, resourceName) + return nil } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "%s resource %s created", kind, resourceName) return nil } + +// patchResourceMetadata sends a MergePatch that sets only labels and annotations on the +// object, leaving all other fields untouched. It also removes obsolete annotations before patching. +func (r *Reconciler) patchResourceMetadata(desired client.Object, resourceMetadata common.ResourceMetadata) error { + common.RemoveObsoleteAnnotations(desired, resourceMetadata) + patch := map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": desired.GetLabels(), + "annotations": desired.GetAnnotations(), + }, + } + patchBytes, err := json.Marshal(patch) + if err != nil { + return fmt.Errorf("failed to marshal metadata patch: %w", err) + } + return r.CtrlClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes)) +} diff --git a/pkg/controller/external_secrets/utils_test.go b/pkg/controller/external_secrets/utils_test.go index dbd37c166..a18760e15 100644 --- a/pkg/controller/external_secrets/utils_test.go +++ b/pkg/controller/external_secrets/utils_test.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" @@ -21,10 +22,11 @@ func TestCreateWithFallback(t *testing.T) { } tests := []struct { - name string - preReq func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) - wantErr string - wantUpdate bool + name string + preReq func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) + customDesired func() client.Object + wantErr string + wantUpdate bool }{ { name: "resource created successfully", @@ -43,6 +45,25 @@ func TestCreateWithFallback(t *testing.T) { }, wantErr: "failed to create ServiceAccount test-ns/test-sa: test client error", }, + { + name: "GVK resolved from object TypeMeta when pre-set, skipping scheme lookup", + preReq: func(cached *fakes.FakeCtrlClient, _ *fakes.FakeCtrlClient) { + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return commontest.ErrTestClient + }) + }, + customDesired: func() client.Object { + return &corev1.ServiceAccount{ + TypeMeta: metav1.TypeMeta{Kind: "ServiceAccount", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sa", + Namespace: "test-ns", + Labels: controllerDefaultResourceLabels, + }, + } + }, + wantErr: "failed to create ServiceAccount test-ns/test-sa: test client error", + }, { name: "AlreadyExists triggers uncached update to restore labels", preReq: func(cached *fakes.FakeCtrlClient, uncached *fakes.FakeCtrlClient) { @@ -84,15 +105,20 @@ func TestCreateWithFallback(t *testing.T) { r.CtrlClient = cached r.UncachedClient = uncached - desired := &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-sa", - Namespace: "test-ns", - Labels: controllerDefaultResourceLabels, - }, + var desired client.Object + if tt.customDesired != nil { + desired = tt.customDesired() + } else { + desired = &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sa", + Namespace: "test-ns", + Labels: controllerDefaultResourceLabels, + }, + } } - err := r.createWithFallback(desired, resourceMetadata, "test-ns/test-sa") + err := r.createWithFallback(desired, resourceMetadata, "test-ns/test-sa", commontest.TestExternalSecretsConfig()) if tt.wantErr != "" { if err == nil || err.Error() != tt.wantErr { @@ -111,3 +137,36 @@ func TestCreateWithFallback(t *testing.T) { }) } } + +// TestCreateWithFallback_GVKResolutionFallback verifies that when the scheme has no +// registered types, createWithFallback falls back to the Go type name in log and error messages. +func TestCreateWithFallback_GVKResolutionFallback(t *testing.T) { + resourceMetadata := common.ResourceMetadata{ + Labels: controllerDefaultResourceLabels, + } + + r := testReconciler(t) + // Use an empty scheme so apiutil.GVKForObject cannot resolve the type. + r.Scheme = runtime.NewScheme() + + cached := &fakes.FakeCtrlClient{} + cached.CreateCalls(func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return commontest.ErrTestClient + }) + r.CtrlClient = cached + r.UncachedClient = &fakes.FakeCtrlClient{} + + desired := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-sa", + Namespace: "test-ns", + Labels: controllerDefaultResourceLabels, + }, + } + + err := r.createWithFallback(desired, resourceMetadata, "test-ns/test-sa", commontest.TestExternalSecretsConfig()) + wantErr := "failed to create *v1.ServiceAccount test-ns/test-sa: test client error" + if err == nil || err.Error() != wantErr { + t.Errorf("createWithFallback() err = %v, wantErr = %v", err, wantErr) + } +} diff --git a/pkg/controller/external_secrets/validatingwebhook.go b/pkg/controller/external_secrets/validatingwebhook.go index fd2603465..190235fc8 100644 --- a/pkg/controller/external_secrets/validatingwebhook.go +++ b/pkg/controller/external_secrets/validatingwebhook.go @@ -39,10 +39,9 @@ func (r *Reconciler) createOrApplyValidatingWebhookConfiguration(esc *operatorv1 } if !exist { - if err := r.createWithFallback(desired, resourceMetadata, validatingWebhookName); err != nil { + if err := r.createWithFallback(desired, resourceMetadata, validatingWebhookName, esc); err != nil { return err } - r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "validatingWebhook resource %s created", validatingWebhookName) } } return nil diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 4d17d71ea..64740a6b7 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -1056,12 +1056,25 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }) - Context("Managed Label Restoration", Label("Platform:Generic"), func() { + Context("Managed Label Restoration", Label("Platform:AWS"), func() { const ( managedLabelKey = "app" managedLabelValue = "external-secrets" ) + AfterEach(func() { + By("Verifying ExternalSecretsConfig is Ready and not Degraded after label restoration") + Expect(utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, "cluster", 2*time.Minute)).To(Succeed(), + "ExternalSecretsConfig should remain Ready after label tampering and restoration") + + By("Verifying operand pods are still ready after label restoration") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed(), "operand pods should still be running after label restoration") + }) + It("should restore the app=external-secrets label on a ServiceAccount after external removal", func() { saName := "external-secrets" @@ -1093,7 +1106,7 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }) It("should restore the app=external-secrets label on a Role after external removal", func() { - roleName := "external-secrets" + roleName := "external-secrets-leaderelection" By("Verifying Role has the managed label initially") Eventually(func(g Gomega) { @@ -1152,20 +1165,6 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }, 2*time.Minute, 5*time.Second).Should(Succeed()) }) - It("should keep ExternalSecretsConfig in Ready state after label restoration", func() { - By("Verifying ExternalSecretsConfig is Ready and not Degraded") - Expect(utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, "cluster", 2*time.Minute)).To(Succeed(), - "ExternalSecretsConfig should remain Ready after label tampering and restoration") - }) - - It("should keep operand pods running after label restoration", func() { - By("Verifying operand pods are still ready") - Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ - operandCoreControllerPodPrefix, - operandCertControllerPodPrefix, - operandWebhookPodPrefix, - })).To(Succeed(), "operand pods should still be running after label restoration") - }) }) AfterAll(func() { From 14110aefcbc6120801b99eb82041f040d9db3b99 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Thu, 18 Jun 2026 01:32:43 +0530 Subject: [PATCH 5/6] Fix Secret metadata reconciliation to preserve cert-controller-managed TLS data Signed-off-by: chiragkyal --- pkg/controller/external_secrets/secret.go | 25 +++++++--- .../external_secrets/secret_test.go | 48 +++++++++++++++++-- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/pkg/controller/external_secrets/secret.go b/pkg/controller/external_secrets/secret.go index 14b75fcd2..4c2f6be41 100644 --- a/pkg/controller/external_secrets/secret.go +++ b/pkg/controller/external_secrets/secret.go @@ -4,6 +4,7 @@ import ( "fmt" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/client" operatorv1alpha1 "github.com/openshift/external-secrets-operator/api/v1alpha1" @@ -33,10 +34,9 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo } if exist && common.ObjectMetadataModified(desired, fetched, &resourceMetadata) { - r.log.V(1).Info("secret has been modified, updating to desired state", "name", secretName) - common.RemoveObsoleteAnnotations(desired, resourceMetadata) - if err := r.UpdateWithRetry(r.ctx, desired); err != nil { - return common.FromClientError(err, "failed to update %s secret resource", secretName) + r.log.V(1).Info("secret has been modified, patching metadata to desired state", "name", secretName) + if err := r.patchResourceMetadata(desired, resourceMetadata); err != nil { + return common.FromClientError(err, "failed to patch %s secret resource metadata", secretName) } r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s reconciled back to desired state", secretName) } else { @@ -44,9 +44,22 @@ func (r *Reconciler) createOrApplySecret(esc *operatorv1alpha1.ExternalSecretsCo } if !exist { - if err := r.createWithFallback(desired, resourceMetadata, secretName, esc); err != nil { - return err + // NOTE: This Secret cannot use the generic createWithFallback helper because + // its Data field is managed by the external-secrets cert-controller, which injects + // TLS content at runtime. On AlreadyExists we use a MergePatch that touches only + // metadata, leaving cert-controller-managed TLS certificates untouched. + if err := r.Create(r.ctx, desired); err != nil { + if !apierrors.IsAlreadyExists(err) { + return common.FromClientError(err, "failed to create %s secret resource", secretName) + } + r.log.V(1).Info("secret exists on API server but absent from label-filtered cache, patching metadata", "name", secretName) + if patchErr := r.patchResourceMetadata(desired, resourceMetadata); patchErr != nil { + return common.FromClientError(patchErr, "failed to patch %s secret resource metadata", secretName) + } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s restored to desired state", secretName) + return nil } + r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "secret resource %s created", secretName) } return nil } diff --git a/pkg/controller/external_secrets/secret_test.go b/pkg/controller/external_secrets/secret_test.go index a9db0b3d1..6a75d97b8 100644 --- a/pkg/controller/external_secrets/secret_test.go +++ b/pkg/controller/external_secrets/secret_test.go @@ -6,6 +6,8 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -87,7 +89,7 @@ func TestCreateOrApplySecret(t *testing.T) { wantErr: fmt.Sprintf("failed to check %s/%s secret resource already exists: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), }, { - name: "reconciliation of secret fails while restoring to expected state", + name: "reconciliation of secret fails while patching metadata to expected state", preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { m.GetCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) error { if o, ok := obj.(*corev1.Secret); ok { @@ -104,14 +106,52 @@ func TestCreateOrApplySecret(t *testing.T) { } return true, nil }) - m.UpdateWithRetryCalls(func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { if _, ok := obj.(*corev1.Secret); ok { return commontest.ErrTestClient } return nil }) }, - wantErr: fmt.Sprintf("failed to update %s/%s secret resource: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to patch %s/%s secret resource metadata: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), + }, + { + // Regression test: when the managed label is removed from the Secret, the object + // falls out of the label-filtered cache. cached Exists() returns false, Create() + // fails with AlreadyExists, and the controller must patch only the metadata, + // leaving cert-controller-injected TLS data untouched. + name: "Create returns AlreadyExists (label-filtered cache miss): patches metadata to restore labels", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "secrets"}, testValidateSecretResourceName) + }) + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*corev1.Secret); ok { + if obj.GetLabels()["app"] != "external-secrets" { + t.Errorf("expected app=external-secrets label in patch target, got %v", obj.GetLabels()) + } + } + return nil + }) + }, + }, + { + name: "Patch fails after AlreadyExists from Create", + preReq: func(r *Reconciler, m *fakes.FakeCtrlClient) { + m.ExistsCalls(func(ctx context.Context, ns types.NamespacedName, obj client.Object) (bool, error) { + return false, nil + }) + m.CreateCalls(func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + return apierrors.NewAlreadyExists(schema.GroupResource{Resource: "secrets"}, testValidateSecretResourceName) + }) + m.PatchCalls(func(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return commontest.ErrTestClient + }) + }, + wantErr: fmt.Sprintf("failed to patch %s/%s secret resource metadata: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), }, { name: "reconciliation of secret which already exists in expected state", @@ -155,7 +195,7 @@ func TestCreateOrApplySecret(t *testing.T) { return nil }) }, - wantErr: fmt.Sprintf("failed to create Secret %s/%s: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), + wantErr: fmt.Sprintf("failed to create %s/%s secret resource: %s", commontest.TestExternalSecretsNamespace, testValidateSecretResourceName, commontest.ErrTestClient), }, { name: "successful secret creation", From 1175fc1774f3b34fe21cef5a49f564fadacbd1fa Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Thu, 18 Jun 2026 17:22:28 +0530 Subject: [PATCH 6/6] use JSON Patch to fully replace labels/annotations on co-managed resources Signed-off-by: chiragkyal --- .../external_secrets/configmap_test.go | 8 +- pkg/controller/external_secrets/utils.go | 41 +- test/e2e/e2e_test.go | 560 ++++++++++++++++++ test/e2e/helpers_test.go | 30 + 4 files changed, 626 insertions(+), 13 deletions(-) diff --git a/pkg/controller/external_secrets/configmap_test.go b/pkg/controller/external_secrets/configmap_test.go index 00d528b2a..c728b0bda 100644 --- a/pkg/controller/external_secrets/configmap_test.go +++ b/pkg/controller/external_secrets/configmap_test.go @@ -104,8 +104,8 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { if cm.Labels[trustedCABundleInjectLabel] != "true" { t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel]) } - if patch.Type() != types.MergePatchType { - t.Errorf("expected MergePatch, got %s", patch.Type()) + if patch.Type() != types.JSONPatchType { + t.Errorf("expected JSONPatch, got %s", patch.Type()) } return nil }) @@ -140,8 +140,8 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) { if cm.Labels[trustedCABundleInjectLabel] != "true" { t.Errorf("expected inject label in patch target, got %q", cm.Labels[trustedCABundleInjectLabel]) } - if patch.Type() != types.MergePatchType { - t.Errorf("expected MergePatch, got %s", patch.Type()) + if patch.Type() != types.JSONPatchType { + t.Errorf("expected JSONPatch, got %s", patch.Type()) } return nil }) diff --git a/pkg/controller/external_secrets/utils.go b/pkg/controller/external_secrets/utils.go index 7fa7aba8b..bd566cf3d 100644 --- a/pkg/controller/external_secrets/utils.go +++ b/pkg/controller/external_secrets/utils.go @@ -250,19 +250,42 @@ func (r *Reconciler) createWithFallback(desired client.Object, resourceMetadata return nil } -// patchResourceMetadata sends a MergePatch that sets only labels and annotations on the -// object, leaving all other fields untouched. It also removes obsolete annotations before patching. +// jsonPatchOp is a single JSON Patch operation. +type jsonPatchOp struct { + Op string `json:"op"` + Path string `json:"path"` + Value interface{} `json:"value"` +} + +// patchResourceMetadata fully replaces the labels and annotations on an object using a +// JSON Patch, leaving all other fields untouched. This is safe for co-managed +// resources where this operator owns all metadata but data fields are owned by an external +// controller (e.g. CNO-managed ConfigMap data, cert-controller-managed TLS Secret data). +// +// JSON Patch "add" on an existing path replaces the entire value, so the resulting +// labels/annotations on the server exactly match desired. +// +// RemoveObsoleteAnnotations is called defensively to strip any deleted annotation keys +// from desired before building the patch, regardless of whether the caller already did so. func (r *Reconciler) patchResourceMetadata(desired client.Object, resourceMetadata common.ResourceMetadata) error { common.RemoveObsoleteAnnotations(desired, resourceMetadata) - patch := map[string]interface{}{ - "metadata": map[string]interface{}{ - "labels": desired.GetLabels(), - "annotations": desired.GetAnnotations(), - }, + + annotations := desired.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + labels := desired.GetLabels() + if labels == nil { + labels = map[string]string{} + } + + ops := []jsonPatchOp{ + {Op: "add", Path: "/metadata/labels", Value: labels}, + {Op: "add", Path: "/metadata/annotations", Value: annotations}, } - patchBytes, err := json.Marshal(patch) + patchBytes, err := json.Marshal(ops) if err != nil { return fmt.Errorf("failed to marshal metadata patch: %w", err) } - return r.CtrlClient.Patch(r.ctx, desired, client.RawPatch(types.MergePatchType, patchBytes)) + return r.CtrlClient.Patch(r.ctx, desired, client.RawPatch(types.JSONPatchType, patchBytes)) } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 64740a6b7..c29dba34a 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -1056,6 +1056,383 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }) + // Context("Managed Annotation Restoration") tests that when a managed annotation is + // externally removed from a resource, the operator detects the drift and restores it. + // Resources intentionally excluded: + // - Deployment: Kubernetes adds deployment.kubernetes.io/revision on every rollout, + // so annotation events are suppressed to avoid infinite reconcile loops. + Context("Managed Annotation Restoration", Ordered, Label("Platform:AWS"), func() { + const ( + restorationAnnotationKey = "example.com/restoration-test" + restorationAnnotationValue = "managed-value" + ) + + BeforeAll(func() { + By("Adding the restoration test annotation to ExternalSecretsConfig spec") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + cr := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, cr); err != nil { + return err + } + if cr.Spec.ControllerConfig.Annotations == nil { + cr.Spec.ControllerConfig.Annotations = make(map[string]string) + } + cr.Spec.ControllerConfig.Annotations[restorationAnnotationKey] = restorationAnnotationValue + return runtimeClient.Update(ctx, cr) + }) + Expect(err).NotTo(HaveOccurred(), "should add restoration annotation to ExternalSecretsConfig") + }) + + AfterAll(func() { + By("Removing the restoration test annotation from ExternalSecretsConfig spec") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + cr := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, cr); err != nil { + return err + } + delete(cr.Spec.ControllerConfig.Annotations, restorationAnnotationKey) + return runtimeClient.Update(ctx, cr) + }) + Expect(err).NotTo(HaveOccurred(), "should remove restoration annotation from ExternalSecretsConfig") + + By("Verifying ExternalSecretsConfig is Ready after annotation restoration tests") + Expect(utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, "cluster", 2*time.Minute)).To(Succeed()) + + By("Verifying operand pods are still ready after annotation restoration tests") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed()) + }) + + It("should restore a managed annotation on a ServiceAccount after external removal", func() { + saName := "external-secrets" + + By("Verifying ServiceAccount has the managed annotation initially") + Eventually(func(g Gomega) { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(sa.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the ServiceAccount") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(sa.Annotations, restorationAnnotationKey) + _, err = clientset.CoreV1().ServiceAccounts(operandNamespace).Update(ctx, sa, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + sa, err := clientset.CoreV1().ServiceAccounts(operandNamespace).Get(ctx, saName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(sa.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on ServiceAccount %s", saName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a Role after external removal", func() { + roleName := "external-secrets-leaderelection" + + By("Verifying Role has the managed annotation initially") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(role.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the Role") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(role.Annotations, restorationAnnotationKey) + _, err = clientset.RbacV1().Roles(operandNamespace).Update(ctx, role, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + role, err := clientset.RbacV1().Roles(operandNamespace).Get(ctx, roleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(role.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on Role %s", roleName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a RoleBinding after external removal", func() { + roleBindingName := "external-secrets-leaderelection" + + By("Verifying RoleBinding has the managed annotation initially") + Eventually(func(g Gomega) { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rb.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the RoleBinding") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(rb.Annotations, restorationAnnotationKey) + _, err = clientset.RbacV1().RoleBindings(operandNamespace).Update(ctx, rb, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rb.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on RoleBinding %s", roleBindingName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a ClusterRole after external removal", func() { + clusterRoleName := "external-secrets-controller" + + By("Verifying ClusterRole has the managed annotation initially") + Eventually(func(g Gomega) { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cr.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the ClusterRole") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(cr.Annotations, restorationAnnotationKey) + _, err = clientset.RbacV1().ClusterRoles().Update(ctx, cr, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cr.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on ClusterRole %s", clusterRoleName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a ClusterRoleBinding after external removal", func() { + clusterRoleBindingName := "external-secrets-controller" + + By("Verifying ClusterRoleBinding has the managed annotation initially") + Eventually(func(g Gomega) { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(crb.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the ClusterRoleBinding") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(crb.Annotations, restorationAnnotationKey) + _, err = clientset.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(crb.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on ClusterRoleBinding %s", clusterRoleBindingName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a Service after external removal", func() { + serviceName := "external-secrets-webhook" + + By("Verifying Service has the managed annotation initially") + Eventually(func(g Gomega) { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(svc.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the Service") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(svc.Annotations, restorationAnnotationKey) + _, err = clientset.CoreV1().Services(operandNamespace).Update(ctx, svc, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(svc.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on Service %s", serviceName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore a managed annotation on a NetworkPolicy after external removal", func() { + npName := "eso-sys-deny-all-traffic" + + By("Verifying NetworkPolicy has the managed annotation initially") + Eventually(func(g Gomega) { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(np.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Externally removing the managed annotation from the NetworkPolicy") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(np.Annotations, restorationAnnotationKey) + _, err = clientset.NetworkingV1().NetworkPolicies(operandNamespace).Update(ctx, np, metav1.UpdateOptions{}) + return err + })).To(Succeed()) + + By("Waiting for operator to restore the managed annotation") + Eventually(func(g Gomega) { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(np.Annotations).To(HaveKeyWithValue(restorationAnnotationKey, restorationAnnotationValue), + "operator should restore annotation on NetworkPolicy %s", npName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + }) + + // Context("Custom Labels") tests that labels added via esc.Spec.ControllerConfig.Labels + // are propagated to all managed resources, and that removing a label from the spec + // causes the operator to remove it from all resources. This exercises the full + // label lifecycle across every resource type, including the co-managed Secret whose + // metadata is patched via JSON Patch rather than a full update. + Context("Custom Labels", Label("Platform:AWS"), func() { + AfterEach(func() { + By("Verifying ExternalSecretsConfig is Ready and not Degraded after label lifecycle test") + Expect(utils.WaitForExternalSecretsConfigReady(ctx, dynamicClient, "cluster", 2*time.Minute)).To(Succeed(), + "ExternalSecretsConfig should remain Ready after custom label add/remove") + + By("Verifying operand pods are still ready after label lifecycle test") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed(), "operand pods should still be running after custom label lifecycle test") + }) + + It("should apply and remove custom labels to all managed resources", func() { + testLabels := map[string]string{ + "mycompany.io/env": "staging", + } + + existingCR := &operatorv1alpha1.ExternalSecretsConfig{} + Expect(runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, existingCR)).To(Succeed(), + "should get ExternalSecretsConfig to capture initial state") + originalLabels := maps.Clone(existingCR.Spec.ControllerConfig.Labels) + + defer func() { + By("Restoring original labels on ExternalSecretsConfig CR") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCR := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, currentCR); err != nil { + return err + } + currentCR.Spec.ControllerConfig.Labels = originalLabels + return runtimeClient.Update(ctx, currentCR) + }) + Expect(err).NotTo(HaveOccurred(), "should restore labels on ExternalSecretsConfig") + }() + + By("Updating ExternalSecretsConfig with custom labels") + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCR := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, currentCR); err != nil { + return err + } + merged := make(map[string]string) + if originalLabels != nil { + maps.Copy(merged, originalLabels) + } + maps.Copy(merged, testLabels) + currentCR.Spec.ControllerConfig.Labels = merged + return runtimeClient.Update(ctx, currentCR) + }) + Expect(err).NotTo(HaveOccurred(), "should update ExternalSecretsConfig with custom labels") + + By("Waiting for external-secrets operand pods to be ready") + Expect(utils.VerifyPodsReadyByPrefix(ctx, clientset, operandNamespace, []string{ + operandCoreControllerPodPrefix, + operandCertControllerPodPrefix, + operandWebhookPodPrefix, + })).To(Succeed()) + + for _, resourceType := range getResourceTypesToVerify() { + By(fmt.Sprintf("Verifying custom labels are applied to %s resources", resourceType.name)) + Eventually(func(g Gomega) { + objects, err := resourceType.listFunc(ctx, clientset, operandNamespace, g) + g.Expect(err).NotTo(HaveOccurred(), "should list %s", resourceType.name) + + for _, obj := range objects { + if !strings.HasPrefix(obj.GetName(), "external-secrets") { + continue + } + for k, v := range testLabels { + g.Expect(obj.GetLabels()).To(HaveKeyWithValue(k, v), + "%s %s should have label %s=%s", resourceType.name, obj.GetName(), k, v) + } + } + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + } + + By("Removing custom labels from ExternalSecretsConfig CR") + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentCR := &operatorv1alpha1.ExternalSecretsConfig{} + if err := runtimeClient.Get(ctx, client.ObjectKey{Name: "cluster"}, currentCR); err != nil { + return err + } + for k := range testLabels { + delete(currentCR.Spec.ControllerConfig.Labels, k) + } + return runtimeClient.Update(ctx, currentCR) + }) + Expect(err).NotTo(HaveOccurred(), "should remove custom labels from ExternalSecretsConfig") + + for _, resourceType := range getResourceTypesToVerify() { + By(fmt.Sprintf("Verifying custom labels are removed from %s resources", resourceType.name)) + Eventually(func(g Gomega) { + objects, err := resourceType.listFunc(ctx, clientset, operandNamespace, g) + g.Expect(err).NotTo(HaveOccurred(), "should list %s", resourceType.name) + + for _, obj := range objects { + if !strings.HasPrefix(obj.GetName(), "external-secrets") { + continue + } + for k := range testLabels { + g.Expect(obj.GetLabels()).NotTo(HaveKey(k), + "%s %s should NOT have label %s after removal", resourceType.name, obj.GetName(), k) + } + } + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + } + }) + }) + Context("Managed Label Restoration", Label("Platform:AWS"), func() { const ( managedLabelKey = "app" @@ -1165,6 +1542,189 @@ var _ = Describe("External Secrets Operator End-to-End test scenarios", Ordered, }, 2*time.Minute, 5*time.Second).Should(Succeed()) }) + // Secret uses patchResourceMetadata (JSON Patch) rather than UpdateWithRetry because + // its Data field is co-managed by the cert-controller. This test specifically exercises + // that the metadata-only patch path correctly restores the managed label. + It("should restore the app=external-secrets label on a Secret after external removal", func() { + secretName := "external-secrets-webhook" + + By("Verifying Secret has the managed label initially") + Eventually(func(g Gomega) { + secret, err := clientset.CoreV1().Secrets(operandNamespace).Get(ctx, secretName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(secret.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the Secret") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + secret, err := clientset.CoreV1().Secrets(operandNamespace).Get(ctx, secretName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(secret.Labels, managedLabelKey) + _, err = clientset.CoreV1().Secrets(operandNamespace).Update(ctx, secret, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + secret, err := clientset.CoreV1().Secrets(operandNamespace).Get(ctx, secretName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(secret.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on Secret %s", secretName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a Service after external removal", func() { + serviceName := "external-secrets-webhook" + + By("Verifying Service has the managed label initially") + Eventually(func(g Gomega) { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(svc.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the Service") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(svc.Labels, managedLabelKey) + _, err = clientset.CoreV1().Services(operandNamespace).Update(ctx, svc, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + svc, err := clientset.CoreV1().Services(operandNamespace).Get(ctx, serviceName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(svc.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on Service %s", serviceName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a RoleBinding after external removal", func() { + roleBindingName := "external-secrets-leaderelection" + + By("Verifying RoleBinding has the managed label initially") + Eventually(func(g Gomega) { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rb.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the RoleBinding") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(rb.Labels, managedLabelKey) + _, err = clientset.RbacV1().RoleBindings(operandNamespace).Update(ctx, rb, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + rb, err := clientset.RbacV1().RoleBindings(operandNamespace).Get(ctx, roleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rb.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on RoleBinding %s", roleBindingName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a ClusterRole after external removal", func() { + clusterRoleName := "external-secrets-controller" + + By("Verifying ClusterRole has the managed label initially") + Eventually(func(g Gomega) { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cr.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the ClusterRole") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(cr.Labels, managedLabelKey) + _, err = clientset.RbacV1().ClusterRoles().Update(ctx, cr, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + cr, err := clientset.RbacV1().ClusterRoles().Get(ctx, clusterRoleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(cr.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on ClusterRole %s", clusterRoleName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a ClusterRoleBinding after external removal", func() { + clusterRoleBindingName := "external-secrets-controller" + + By("Verifying ClusterRoleBinding has the managed label initially") + Eventually(func(g Gomega) { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(crb.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the ClusterRoleBinding") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(crb.Labels, managedLabelKey) + _, err = clientset.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + crb, err := clientset.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(crb.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on ClusterRoleBinding %s", clusterRoleBindingName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + + It("should restore the app=external-secrets label on a NetworkPolicy after external removal", func() { + npName := "eso-sys-deny-all-traffic" + + By("Verifying NetworkPolicy has the managed label initially") + Eventually(func(g Gomega) { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(np.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue)) + }, time.Minute, 5*time.Second).Should(Succeed()) + + By("Removing the managed label from the NetworkPolicy") + Expect(retry.RetryOnConflict(retry.DefaultRetry, func() error { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + if err != nil { + return err + } + delete(np.Labels, managedLabelKey) + _, err = clientset.NetworkingV1().NetworkPolicies(operandNamespace).Update(ctx, np, metav1.UpdateOptions{}) + return err + })).To(Succeed(), "should remove the managed label") + + By("Waiting for operator to restore the managed label") + Eventually(func(g Gomega) { + np, err := clientset.NetworkingV1().NetworkPolicies(operandNamespace).Get(ctx, npName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(np.Labels).To(HaveKeyWithValue(managedLabelKey, managedLabelValue), + "operator should restore app=external-secrets on NetworkPolicy %s", npName) + }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }) + }) AfterAll(func() { diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index b3a09d13a..dda844a91 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -155,6 +155,36 @@ func getResourceTypesToVerify() []resourceType { return objects, nil }, }, + { + // ClusterRoles are cluster-scoped; the namespace parameter is unused. + name: "ClusterRole", + listFunc: func(ctx context.Context, clientset *kubernetes.Clientset, _ string, g Gomega) ([]metav1.Object, error) { + clusterRoles, err := clientset.RbacV1().ClusterRoles().List(ctx, listOnlyManagedResources) + if err != nil { + return nil, err + } + objects := make([]metav1.Object, 0, len(clusterRoles.Items)) + for i := range clusterRoles.Items { + objects = append(objects, &clusterRoles.Items[i]) + } + return objects, nil + }, + }, + { + // ClusterRoleBindings are cluster-scoped; the namespace parameter is unused. + name: "ClusterRoleBinding", + listFunc: func(ctx context.Context, clientset *kubernetes.Clientset, _ string, g Gomega) ([]metav1.Object, error) { + crbs, err := clientset.RbacV1().ClusterRoleBindings().List(ctx, listOnlyManagedResources) + if err != nil { + return nil, err + } + objects := make([]metav1.Object, 0, len(crbs.Items)) + for i := range crbs.Items { + objects = append(objects, &crbs.Items[i]) + } + return objects, nil + }, + }, } }