ref: cleanup condition handling for objects (#2829)

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
+	"github.com/prometheus/client_golang/prometheus"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -189,10 +190,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	}
 	if !shouldReconcile(externalSecret) {
 		log.V(1).Info("stopping reconciling", "rv", getResourceVersion(externalSecret))
-		return ctrl.Result{
-			RequeueAfter: 0,
-			Requeue:      false,
-		}, nil
+		return ctrl.Result{}, nil
 	}
 
 	// patch status when done processing
@@ -215,11 +213,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 	dataMap, err := r.getProviderSecretData(ctx, &externalSecret)
 	if err != nil {
-		log.Error(err, errGetSecretData)
-		r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
-		conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, errGetSecretData)
-		SetExternalSecretCondition(&externalSecret, *conditionSynced)
-		syncCallsError.With(resourceLabels).Inc()
+		r.markAsFailed(log, errGetSecretData, err, &externalSecret, syncCallsError.With(resourceLabels))
 		return ctrl.Result{}, err
 	}
 
@@ -232,20 +226,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			// this is also implemented in the es validation webhook
 			if externalSecret.Spec.Target.CreationPolicy != esv1beta1.CreatePolicyOwner {
 				err := fmt.Errorf(errInvalidCreatePolicy, externalSecret.Spec.Target.CreationPolicy)
-				log.Error(err, errDeleteSecret)
-				r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
-				conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, errDeleteSecret)
-				SetExternalSecretCondition(&externalSecret, *conditionSynced)
-				syncCallsError.With(resourceLabels).Inc()
+				r.markAsFailed(log, errDeleteSecret, err, &externalSecret, syncCallsError.With(resourceLabels))
 				return ctrl.Result{}, err
 			}
+
 			err = r.Delete(ctx, secret)
 			if err != nil && !apierrors.IsNotFound(err) {
-				log.Error(err, errDeleteSecret)
-				r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
-				conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, errDeleteSecret)
-				SetExternalSecretCondition(&externalSecret, *conditionSynced)
-				syncCallsError.With(resourceLabels).Inc()
+				r.markAsFailed(log, errDeleteSecret, err, &externalSecret, syncCallsError.With(resourceLabels))
 			}
 
 			conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, esv1beta1.ConditionReasonSecretDeleted, "secret deleted due to DeletionPolicy")
@@ -318,40 +305,44 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			delErr := deleteOrphanedSecrets(ctx, r.Client, &externalSecret)
 			if delErr != nil {
 				msg := fmt.Sprintf("failed to clean up orphaned secrets: %v", delErr)
-				log.Error(delErr, msg)
-				r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, delErr.Error())
-				conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, msg)
-				SetExternalSecretCondition(&externalSecret, *conditionSynced)
-				syncCallsError.With(resourceLabels).Inc()
+				r.markAsFailed(log, msg, delErr, &externalSecret, syncCallsError.With(resourceLabels))
 				return ctrl.Result{}, delErr
 			}
 		}
 	}
 
 	if err != nil {
-		log.Error(err, errUpdateSecret)
-		r.recorder.Event(&externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
-		conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, errUpdateSecret)
-		SetExternalSecretCondition(&externalSecret, *conditionSynced)
-		syncCallsError.With(resourceLabels).Inc()
+		r.markAsFailed(log, errUpdateSecret, err, &externalSecret, syncCallsError.With(resourceLabels))
 		return ctrl.Result{}, err
 	}
 
-	r.recorder.Event(&externalSecret, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret")
+	r.markAsDone(&externalSecret, start, log)
+
+	return ctrl.Result{
+		RequeueAfter: refreshInt,
+	}, nil
+}
+
+func (r *Reconciler) markAsDone(externalSecret *esv1beta1.ExternalSecret, start time.Time, log logr.Logger) {
+	r.recorder.Event(externalSecret, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret")
 	conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionTrue, esv1beta1.ConditionReasonSecretSynced, "Secret was synced")
 	currCond := GetExternalSecretCondition(externalSecret.Status, esv1beta1.ExternalSecretReady)
-	SetExternalSecretCondition(&externalSecret, *conditionSynced)
+	SetExternalSecretCondition(externalSecret, *conditionSynced)
 	externalSecret.Status.RefreshTime = metav1.NewTime(start)
-	externalSecret.Status.SyncedResourceVersion = getResourceVersion(externalSecret)
+	externalSecret.Status.SyncedResourceVersion = getResourceVersion(*externalSecret)
 	if currCond == nil || currCond.Status != conditionSynced.Status {
 		log.Info("reconciled secret") // Log once if on success in any verbosity
 	} else {
 		log.V(1).Info("reconciled secret") // Log all reconciliation cycles if higher verbosity applied
 	}
+}
 
-	return ctrl.Result{
-		RequeueAfter: refreshInt,
-	}, nil
+func (r *Reconciler) markAsFailed(log logr.Logger, msg string, err error, externalSecret *esv1beta1.ExternalSecret, counter prometheus.Counter) {
+	log.Error(err, msg)
+	r.recorder.Event(externalSecret, v1.EventTypeWarning, esv1beta1.ReasonUpdateFailed, err.Error())
+	conditionSynced := NewExternalSecretCondition(esv1beta1.ExternalSecretReady, v1.ConditionFalse, esv1beta1.ConditionReasonSecretSyncedError, msg)
+	SetExternalSecretCondition(externalSecret, *conditionSynced)
+	counter.Inc()
 }
 
 func deleteOrphanedSecrets(ctx context.Context, cl client.Client, externalSecret *esv1beta1.ExternalSecret) error {

pkg/controllers/pushsecret/pushsecret_controller.go

@@ -62,6 +62,14 @@ type Reconciler struct {
 	ControllerClass string
 }
 
+func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
+	r.recorder = mgr.GetEventRecorderFor("pushsecret")
+
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&esapi.PushSecret{}).
+		Complete(r)
+}
+
 func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := r.Log.WithValues("pushsecret", req.NamespacedName)
 
@@ -72,15 +80,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	defer func() { pushSecretReconcileDuration.With(resourceLabels).Set(float64(time.Since(start))) }()
 
 	var ps esapi.PushSecret
-	err := r.Get(ctx, req.NamespacedName, &ps)
 	mgr := secretstore.NewManager(r.Client, r.ControllerClass, false)
 	defer mgr.Close(ctx)
-	if apierrors.IsNotFound(err) {
-		return ctrl.Result{}, nil
-	} else if err != nil {
+
+	if err := r.Get(ctx, req.NamespacedName, &ps); err != nil {
+		if apierrors.IsNotFound(err) {
+			return ctrl.Result{}, nil
+		}
+
 		msg := "unable to get PushSecret"
 		r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, msg)
 		log.Error(err, msg)
+
 		return ctrl.Result{}, fmt.Errorf("get resource: %w", err)
 	}
 
@@ -91,8 +102,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 	p := client.MergeFrom(ps.DeepCopy())
 	defer func() {
-		err := r.Client.Status().Patch(ctx, &ps, p)
-		if err != nil {
+		if err := r.Client.Status().Patch(ctx, &ps, p); err != nil {
 			log.Error(err, errPatchStatus)
 		}
 	}()
@@ -102,10 +112,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		if ps.ObjectMeta.DeletionTimestamp.IsZero() {
 			if !controllerutil.ContainsFinalizer(&ps, pushSecretFinalizer) {
 				controllerutil.AddFinalizer(&ps, pushSecretFinalizer)
-				err := r.Client.Update(ctx, &ps, &client.UpdateOptions{})
-				if err != nil {
+				if err := r.Client.Update(ctx, &ps, &client.UpdateOptions{}); err != nil {
 					return ctrl.Result{}, fmt.Errorf("could not update finalizers: %w", err)
 				}
+
 				return ctrl.Result{}, nil
 			}
 		} else {
@@ -114,17 +124,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 				badState, err := r.DeleteSecretFromProviders(ctx, &ps, esapi.SyncedPushSecretsMap{}, mgr)
 				if err != nil {
 					msg := fmt.Sprintf("Failed to Delete Secrets from Provider: %v", err)
-					cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, msg)
-					ps = SetPushSecretCondition(ps, *cond)
-					r.SetSyncedSecrets(&ps, badState)
-					r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, msg)
+					r.markAsFailed(msg, &ps, badState)
+
 					return ctrl.Result{}, err
 				}
+
 				controllerutil.RemoveFinalizer(&ps, pushSecretFinalizer)
-				err = r.Client.Update(ctx, &ps, &client.UpdateOptions{})
-				if err != nil {
+				if err := r.Client.Update(ctx, &ps, &client.UpdateOptions{}); err != nil {
 					return ctrl.Result{}, fmt.Errorf("could not update finalizers: %w", err)
 				}
+
 				return ctrl.Result{}, nil
 			}
 		}
@@ -134,16 +143,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 	secret, err := r.GetSecret(ctx, ps)
 	if err != nil {
-		cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, errFailedGetSecret)
-		ps = SetPushSecretCondition(ps, *cond)
-		r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, errFailedGetSecret)
+		r.markAsFailed(errFailedGetSecret, &ps, nil)
+
 		return ctrl.Result{}, err
 	}
 	secretStores, err := r.GetSecretStores(ctx, ps)
 	if err != nil {
-		cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, err.Error())
-		ps = SetPushSecretCondition(ps, *cond)
-		r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, err.Error())
+		r.markAsFailed(err.Error(), &ps, nil)
+
 		return ctrl.Result{}, err
 	}
 	syncedSecrets, err := r.PushSecretToProviders(ctx, secretStores, ps, secret, mgr)
@@ -153,12 +160,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			return ctrl.Result{Requeue: true}, nil
 		}
 
-		msg := fmt.Sprintf(errFailedSetSecret, err)
-		cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, msg)
-		ps = SetPushSecretCondition(ps, *cond)
 		totalSecrets := mergeSecretState(syncedSecrets, ps.Status.SyncedPushSecrets)
-		r.SetSyncedSecrets(&ps, totalSecrets)
-		r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, msg)
+		msg := fmt.Sprintf(errFailedSetSecret, err)
+		r.markAsFailed(msg, &ps, totalSecrets)
+
 		return ctrl.Result{}, err
 	}
 	switch ps.Spec.DeletionPolicy {
@@ -166,23 +171,36 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		badSyncState, err := r.DeleteSecretFromProviders(ctx, &ps, syncedSecrets, mgr)
 		if err != nil {
 			msg := fmt.Sprintf("Failed to Delete Secrets from Provider: %v", err)
-			cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, msg)
-			ps = SetPushSecretCondition(ps, *cond)
-			r.SetSyncedSecrets(&ps, badSyncState)
-			r.recorder.Event(&ps, v1.EventTypeWarning, esapi.ReasonErrored, msg)
+			r.markAsFailed(msg, &ps, badSyncState)
 			return ctrl.Result{}, err
 		}
 	case esapi.PushSecretDeletionPolicyNone:
 	default:
 	}
-	msg := "PushSecret synced successfully"
-	cond := NewPushSecretCondition(esapi.PushSecretReady, v1.ConditionTrue, esapi.ReasonSynced, msg)
-	ps = SetPushSecretCondition(ps, *cond)
-	r.SetSyncedSecrets(&ps, syncedSecrets)
-	r.recorder.Event(&ps, v1.EventTypeNormal, esapi.ReasonSynced, msg)
+
+	r.markAsDone(&ps, syncedSecrets)
+
 	return ctrl.Result{RequeueAfter: refreshInt}, nil
 }
-func (r *Reconciler) SetSyncedSecrets(ps *esapi.PushSecret, status esapi.SyncedPushSecretsMap) {
+
+func (r *Reconciler) markAsFailed(msg string, ps *esapi.PushSecret, badSyncState esapi.SyncedPushSecretsMap) {
+	cond := newPushSecretCondition(esapi.PushSecretReady, v1.ConditionFalse, esapi.ReasonErrored, msg)
+	setPushSecretCondition(ps, *cond)
+	if badSyncState != nil {
+		r.setSyncedSecrets(ps, badSyncState)
+	}
+	r.recorder.Event(ps, v1.EventTypeWarning, esapi.ReasonErrored, msg)
+}
+
+func (r *Reconciler) markAsDone(ps *esapi.PushSecret, syncedSecrets esapi.SyncedPushSecretsMap) {
+	msg := "PushSecret synced successfully"
+	cond := newPushSecretCondition(esapi.PushSecretReady, v1.ConditionTrue, esapi.ReasonSynced, msg)
+	setPushSecretCondition(ps, *cond)
+	r.setSyncedSecrets(ps, syncedSecrets)
+	r.recorder.Event(ps, v1.EventTypeNormal, esapi.ReasonSynced, msg)
+}
+
+func (r *Reconciler) setSyncedSecrets(ps *esapi.PushSecret, status esapi.SyncedPushSecretsMap) {
 	ps.Status.SyncedPushSecrets = status
 }
 
@@ -276,6 +294,7 @@ func (r *Reconciler) PushSecretToProviders(ctx context.Context, stores map[esapi
 	}
 	return out, nil
 }
+
 func (r *Reconciler) GetSecret(ctx context.Context, ps esapi.PushSecret) (*v1.Secret, error) {
 	secretName := types.NamespacedName{Name: ps.Spec.Selector.Secret.Name, Namespace: ps.Namespace}
 	secret := &v1.Secret{}
@@ -355,15 +374,8 @@ func (r *Reconciler) getSecretStoreFromName(ctx context.Context, refStore esapi.
 	}
 	return &store, nil
 }
-func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
-	r.recorder = mgr.GetEventRecorderFor("pushsecret")
-
-	return ctrl.NewControllerManagedBy(mgr).
-		For(&esapi.PushSecret{}).
-		Complete(r)
-}
 
-func NewPushSecretCondition(condType esapi.PushSecretConditionType, status v1.ConditionStatus, reason, message string) *esapi.PushSecretStatusCondition {
+func newPushSecretCondition(condType esapi.PushSecretConditionType, status v1.ConditionStatus, reason, message string) *esapi.PushSecretStatusCondition {
 	return &esapi.PushSecretStatusCondition{
 		Type:               condType,
 		Status:             status,
@@ -373,12 +385,11 @@ func NewPushSecretCondition(condType esapi.PushSecretConditionType, status v1.Co
 	}
 }
 
-func SetPushSecretCondition(gs esapi.PushSecret, condition esapi.PushSecretStatusCondition) esapi.PushSecret {
-	status := gs.Status
-	currentCond := GetPushSecretCondition(status, condition.Type)
+func setPushSecretCondition(ps *esapi.PushSecret, condition esapi.PushSecretStatusCondition) {
+	currentCond := getPushSecretCondition(ps.Status, condition.Type)
 	if currentCond != nil && currentCond.Status == condition.Status &&
 		currentCond.Reason == condition.Reason && currentCond.Message == condition.Message {
-		return gs
+		return
 	}
 
 	// Do not update lastTransitionTime if the status of the condition doesn't change.
@@ -386,9 +397,7 @@ func SetPushSecretCondition(gs esapi.PushSecret, condition esapi.PushSecretStatu
 		condition.LastTransitionTime = currentCond.LastTransitionTime
 	}
 
-	status.Conditions = append(filterOutCondition(status.Conditions, condition.Type), condition)
-	gs.Status = status
-	return gs
+	ps.Status.Conditions = append(filterOutCondition(ps.Status.Conditions, condition.Type), condition)
 }
 
 // filterOutCondition returns an empty set of conditions with the provided type.
@@ -403,8 +412,8 @@ func filterOutCondition(conditions []esapi.PushSecretStatusCondition, condType e
 	return newConditions
 }
 
-// GetSecretStoreCondition returns the condition with the provided type.
-func GetPushSecretCondition(status esapi.PushSecretStatus, condType esapi.PushSecretConditionType) *esapi.PushSecretStatusCondition {
+// getPushSecretCondition returns the condition with the provided type.
+func getPushSecretCondition(status esapi.PushSecretStatus, condType esapi.PushSecretConditionType) *esapi.PushSecretStatusCondition {
 	for i := range status.Conditions {
 		c := status.Conditions[i]
 		if c.Type == condType {

pkg/controllers/pushsecret/pushsecret_controller_test.go

@@ -73,16 +73,9 @@ type testTweaks func(*testCase)
 
 var _ = Describe("ExternalSecret controller", func() {
 	const (
-		PushSecretName             = "test-es"
-		PushSecretFQDN             = "externalsecrets.external-secrets.io/test-es"
-		PushSecretStore            = "test-store"
-		SecretName                 = "test-secret"
-		PushSecretTargetSecretName = "test-secret"
-		FakeManager                = "fake.manager"
-		expectedSecretVal          = "SOMEVALUE was templated"
-		targetPropObj              = "{{ .targetProperty | toString | upper }} was templated"
-		FooValue                   = "map-foo-value"
-		BarValue                   = "map-bar-value"
+		PushSecretName  = "test-es"
+		PushSecretStore = "test-store"
+		SecretName      = "test-secret"
 	)
 
 	var PushSecretNamespace string