ESO-396: incorporated review suggestions

Signed-off-by: Bharath B <bhb@redhat.com>

Author: bharath-b-rh

pkg/controller/external_secrets/controller.go

@@ -484,10 +484,7 @@ func (r *Reconciler) reconcileDeploymentFailureResult(
 	errUpdate := r.updateStatusConditionsOnFailure(esc, degradedCond, readyCond, isFatal, reconcileErr)
 
 	if isFatal {
-		if errUpdate != nil {
-			return ctrl.Result{}, errUpdate
-		}
-		return ctrl.Result{}, reconcileErr
+		return ctrl.Result{}, errUpdate
 	}
 	if isUserConfig {
 		return userConfigurationFailureResult(reconcileErr, errUpdate)

pkg/controller/external_secrets/controller_test.go

@@ -261,12 +261,14 @@ func TestReconcileDeploymentFailureResult(t *testing.T) {
 		),
 	)
 	irrecoverableErr := common.NewIrrecoverableError(errors.New("forbidden"), "permission denied")
+	statusUpdateErr := errors.New("status update failed")
 	retryErr := common.NewRetryRequiredError(errors.New("timeout"), "temporary failure")
 	proxyErr := common.NewUserConfigurationError(errors.New("invalid proxy URL configured"), "proxy configuration validation failed")
 
 	tests := []struct {
 		name            string
 		reconcileErr    error
+		statusUpdateErr error
 		wantRequeue     time.Duration
 		wantReturnError error
 	}{
@@ -276,9 +278,14 @@ func TestReconcileDeploymentFailureResult(t *testing.T) {
 			wantRequeue:  common.DefaultRequeueTime,
 		},
 		{
-			name:            "irrecoverable error does not requeue",
+			name:         "irrecoverable error does not requeue",
+			reconcileErr: irrecoverableErr,
+		},
+		{
+			name:            "irrecoverable error returns status update failure",
 			reconcileErr:    irrecoverableErr,
-			wantReturnError: irrecoverableErr,
+			statusUpdateErr: statusUpdateErr,
+			wantReturnError: statusUpdateErr,
 		},
 		{
 			name:         "proxy configuration error waits for CR update",
@@ -303,7 +310,7 @@ func TestReconcileDeploymentFailureResult(t *testing.T) {
 				return nil
 			})
 			mock.StatusUpdateCalls(func(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error {
-				return nil
+				return tt.statusUpdateErr
 			})
 			r.CtrlClient = mock
 
@@ -314,7 +321,10 @@ func TestReconcileDeploymentFailureResult(t *testing.T) {
 				1,
 			)
 			if tt.wantReturnError != nil {
-				if err == nil || err.Error() != tt.wantReturnError.Error() {
+				if err == nil {
+					t.Fatalf("reconcileDeploymentFailureResult() err = nil, want %v", tt.wantReturnError)
+				}
+				if err.Error() != tt.wantReturnError.Error() && !strings.Contains(err.Error(), tt.wantReturnError.Error()) {
 					t.Fatalf("reconcileDeploymentFailureResult() err = %v, want %v", err, tt.wantReturnError)
 				}
 			} else if err != nil {

pkg/controller/external_secrets/deployments.go

@@ -70,6 +70,9 @@ func (r *Reconciler) createOrApplyDeploymentFromAsset(esc *operatorv1alpha1.Exte
 	externalSecretsConfigCreateRecon bool,
 ) error {
 	deployment, trustedCAErr := r.getDeploymentObject(assetName, esc, resourceMetadata)
+	// trustedCABundle may fail in getDeploymentObject (e.g. missing ConfigMap) while still
+	// returning a deployment with the stale user-ca-bundle mount removed. Apply that spec
+	// first, then return the error so status becomes Degraded and the reconcile requeues.
 	if deployment == nil {
 		return trustedCAErr
 	}
@@ -80,29 +83,35 @@ func (r *Reconciler) createOrApplyDeploymentFromAsset(esc *operatorv1alpha1.Exte
 	if err != nil {
 		return common.FromClientError(err, "failed to check %s deployment resource already exists", deploymentName)
 	}
-	if exist && externalSecretsConfigCreateRecon {
-		r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s deployment resource already exists", deploymentName)
-	}
-	switch {
-	case exist && common.HasObjectChanged(deployment, fetched, &resourceMetadata):
-		r.log.V(1).Info("deployment has been modified, updating to desired state", "name", deploymentName)
-		common.RemoveObsoleteAnnotations(deployment, resourceMetadata)
-		if err := r.UpdateWithRetry(r.ctx, deployment); err != nil {
-			return common.FromClientError(err, "failed to update %s deployment resource", deploymentName)
-		}
-		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s updated", deploymentName)
-	case !exist:
+
+	if !exist {
 		if err := r.Create(r.ctx, deployment); err != nil {
 			return common.FromClientError(err, "failed to create %s deployment resource", deploymentName)
 		}
 		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s created", deploymentName)
-	default:
+		return trustedCAErr
+	}
+
+	if externalSecretsConfigCreateRecon {
+		r.eventRecorder.Eventf(esc, corev1.EventTypeWarning, "ResourceAlreadyExists", "%s deployment resource already exists", deploymentName)
+	}
+
+	if !common.HasObjectChanged(deployment, fetched, &resourceMetadata) {
 		r.log.V(4).Info("deployment resource already exists and is in expected state", "name", deploymentName)
+		return trustedCAErr
+	}
+
+	r.log.V(1).Info("deployment has been modified, updating to desired state", "name", deploymentName)
+	common.RemoveObsoleteAnnotations(deployment, resourceMetadata)
+	if err := r.UpdateWithRetry(r.ctx, deployment); err != nil {
+		return common.FromClientError(err, "failed to update %s deployment resource", deploymentName)
 	}
+	r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "deployment resource %s updated", deploymentName)
 
 	if trustedCAErr != nil {
 		return trustedCAErr
 	}
+
 	return nil
 }
 
@@ -233,7 +242,7 @@ func (r *Reconciler) updateResourceRequirement(deployment *appsv1.Deployment, es
 	switch {
 	case esc.Spec.ApplicationConfig.Resources != nil:
 		esc.Spec.ApplicationConfig.Resources.DeepCopyInto(&rscReqs)
-	case r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil:
+	case r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Resources != nil:
 		r.esm.Spec.GlobalConfig.Resources.DeepCopyInto(&rscReqs)
 	default:
 		return nil
@@ -265,7 +274,7 @@ func (r *Reconciler) updateNodeSelector(deployment *appsv1.Deployment, esc *oper
 
 	if esc.Spec.ApplicationConfig.NodeSelector != nil {
 		nodeSelector = esc.Spec.ApplicationConfig.NodeSelector
-	} else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil {
+	} else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.NodeSelector != nil {
 		nodeSelector = r.esm.Spec.GlobalConfig.NodeSelector
 	}
 
@@ -287,7 +296,7 @@ func (r *Reconciler) updateAffinityRules(deployment *appsv1.Deployment, esc *ope
 
 	if esc.Spec.ApplicationConfig.Affinity != nil {
 		affinity = esc.Spec.ApplicationConfig.Affinity
-	} else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil {
+	} else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Affinity != nil {
 		affinity = r.esm.Spec.GlobalConfig.Affinity
 	}
 
@@ -309,7 +318,7 @@ func (r *Reconciler) updatePodTolerations(deployment *appsv1.Deployment, esc *op
 
 	if esc.Spec.ApplicationConfig.Tolerations != nil {
 		tolerations = esc.Spec.ApplicationConfig.Tolerations
-	} else if r.esm != nil && r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil {
+	} else if r.esm.Spec.GlobalConfig != nil && r.esm.Spec.GlobalConfig.Tolerations != nil {
 		tolerations = r.esm.Spec.GlobalConfig.Tolerations
 	}
 

pkg/controller/external_secrets/deployments_test.go

@@ -1999,7 +1999,7 @@ func TestApplyUserDeploymentConfigsWithOverrideEnv(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			r := &Reconciler{}
+			r := testReconciler(t)
 			initEnv := []corev1.EnvVar{{Name: "INIT_ONLY_VAR", Value: "init-value"}}
 			deployment := &appsv1.Deployment{
 				Spec: appsv1.DeploymentSpec{
@@ -2395,3 +2395,81 @@ func TestApplyUserCABundleConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateOrApplyDeploymentFromAssetReturnsTrustedCAError(t *testing.T) {
+	esc := commontest.TestExternalSecretsConfig()
+	esc.Spec.ControllerConfig.TrustedCABundle = &v1alpha1.ConfigMapKeyReference{
+		Name: "user-ca-bundle",
+		Key:  UserCABundleKeyPath,
+	}
+	resourceMetadata := testResourceMetadata(esc)
+
+	t.Setenv("RELATED_IMAGE_EXTERNAL_SECRETS", commontest.TestExternalSecretsImageName)
+	t.Setenv("RELATED_IMAGE_BITWARDEN_SDK_SERVER", commontest.TestBitwardenImageName)
+
+	setupMissingUserCAClients := func() (*fakes.FakeCtrlClient, *fakes.FakeCtrlClient) {
+		cached := &fakes.FakeCtrlClient{}
+		uncached := &fakes.FakeCtrlClient{}
+		notFound := func(_ context.Context, ns types.NamespacedName, _ client.Object) error {
+			return apierrors.NewNotFound(corev1.Resource("configmaps"), ns.Name)
+		}
+		cached.GetCalls(notFound)
+		uncached.GetCalls(notFound)
+		return cached, uncached
+	}
+
+	newReconciler := func(cached, uncached *fakes.FakeCtrlClient) *Reconciler {
+		r := testReconciler(t)
+		r.CtrlClient = cached
+		r.UncachedClient = uncached
+		return r
+	}
+
+	t.Run("returns TrustedCABundleError when deployment is unchanged", func(t *testing.T) {
+		t.Parallel()
+
+		cached, uncached := setupMissingUserCAClients()
+		r := newReconciler(cached, uncached)
+
+		desired, trustedCAErr := r.getDeploymentObject(controllerDeploymentAssetName, esc, resourceMetadata)
+		if trustedCAErr == nil {
+			t.Fatal("expected trustedCAErr from missing ConfigMap")
+		}
+		if !common.IsUserConfigurationNotFound(trustedCAErr) {
+			t.Fatalf("expected NotFound user configuration error, got %v", trustedCAErr)
+		}
+
+		cached.ExistsCalls(func(_ context.Context, _ types.NamespacedName, obj client.Object) (bool, error) {
+			desired.DeepCopyInto(obj.(*appsv1.Deployment))
+			return true, nil
+		})
+
+		err := r.createOrApplyDeploymentFromAsset(esc, controllerDeploymentAssetName, resourceMetadata, false)
+		if err == nil {
+			t.Fatal("createOrApplyDeploymentFromAsset() error = nil, want TrustedCABundleError")
+		}
+		if !common.IsUserConfigurationNotFound(err) {
+			t.Fatalf("expected NotFound user configuration error, got %v", err)
+		}
+	})
+
+	t.Run("returns TrustedCABundleError after create", func(t *testing.T) {
+		t.Parallel()
+
+		cached, uncached := setupMissingUserCAClients()
+		r := newReconciler(cached, uncached)
+
+		cached.ExistsCalls(doesNotExist())
+		cached.CreateCalls(func(context.Context, client.Object, ...client.CreateOption) error {
+			return nil
+		})
+
+		err := r.createOrApplyDeploymentFromAsset(esc, controllerDeploymentAssetName, resourceMetadata, false)
+		if err == nil {
+			t.Fatal("createOrApplyDeploymentFromAsset() error = nil, want TrustedCABundleError")
+		}
+		if !common.IsUserConfigurationNotFound(err) {
+			t.Fatalf("expected NotFound user configuration error, got %v", err)
+		}
+	})
+}

pkg/controller/external_secrets/install_external_secrets.go

@@ -160,21 +160,25 @@ func (r *Reconciler) createOrApplyNamespace(esc *operatorv1alpha1.ExternalSecret
 		return fmt.Errorf("failed to check if namespace %s exists: %w", namespaceName, err)
 	}
 
-	switch {
-	case !exists:
+	if !exists {
 		r.log.V(4).Info("Creating namespace", "name", namespaceName)
 		if err := r.Create(r.ctx, desired); err != nil {
 			return fmt.Errorf("failed to create namespace %s: %w", namespaceName, err)
 		}
 		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Namespace %s created", namespaceName)
-	case r.resourceMetadataNeedsUpdate(fetched, resourceMetadata):
-		if err := r.UpdateWithRetry(r.ctx, fetched); err != nil {
-			return fmt.Errorf("failed to update namespace %s: %w", namespaceName, err)
-		}
-		r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Namespace %s updated", namespaceName)
-	default:
+		return nil
+	}
+
+	if !r.resourceMetadataNeedsUpdate(fetched, resourceMetadata) {
 		r.log.V(4).Info("Namespace already exists in desired state", "name", namespaceName)
+		return nil
+	}
+
+	r.log.V(1).Info("Namespace has been modified, updating to desired state", "name", namespaceName)
+	if err := r.UpdateWithRetry(r.ctx, fetched); err != nil {
+		return fmt.Errorf("failed to update namespace %s: %w", namespaceName, err)
 	}
+	r.eventRecorder.Eventf(esc, corev1.EventTypeNormal, "Reconciled", "Namespace %s updated", namespaceName)
 
 	return nil
 }
@@ -258,7 +262,7 @@ func (r *Reconciler) getResourceMetadata(esc *operatorv1alpha1.ExternalSecretsCo
 // and defaultLabels have the highest priority. Labels matching DisallowedLabelMatcher are skipped.
 func (r *Reconciler) getResourceLabels(esc *operatorv1alpha1.ExternalSecretsConfig, resourceMetadata *common.ResourceMetadata) {
 	resourceMetadata.Labels = make(map[string]string)
-	if r.esm != nil && common.IsESMSpecEmpty(r.esm) && r.esm.Spec.GlobalConfig != nil {
+	if common.IsESMSpecEmpty(r.esm) && r.esm.Spec.GlobalConfig != nil {
 		for k, v := range r.esm.Spec.GlobalConfig.Labels {
 			if disallowedLabelMatcher.MatchString(k) {
 				r.log.V(1).Info("skip adding unallowed label configured in externalsecretsmanagers.operator.openshift.io", "label", k, "value", v)