Publish the secret updated events only when they are updated (#3293)

Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>
Co-authored-by: Gustavo Fernandes de Carvalho <gusfcarvalho@gmail.com>

apis/externalsecrets/v1beta1/externalsecret_types.go

@@ -426,13 +426,11 @@ const (
 	// ConditionReasonSecretDeleted indicates that the secret has been deleted.
 	ConditionReasonSecretDeleted = "SecretDeleted"
 
-	ReasonInvalidStoreRef      = "InvalidStoreRef"
-	ReasonUnavailableStore     = "UnavailableStore"
-	ReasonProviderClientConfig = "InvalidProviderClientConfig"
-	ReasonUpdateFailed         = "UpdateFailed"
-	ReasonDeprecated           = "ParameterDeprecated"
-	ReasonUpdated              = "Updated"
-	ReasonDeleted              = "Deleted"
+	ReasonUpdateFailed = "UpdateFailed"
+	ReasonDeprecated   = "ParameterDeprecated"
+	ReasonCreated      = "Created"
+	ReasonUpdated      = "Updated"
+	ReasonDeleted      = "Deleted"
 )
 
 type ExternalSecretStatus struct {

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -279,7 +279,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 	switch externalSecret.Spec.Target.CreationPolicy { //nolint:exhaustive
 	case esv1beta1.CreatePolicyMerge:
-		err = patchSecret(ctx, r.Client, r.Scheme, secret, mutationFunc, externalSecret.Name)
+		err = r.patchSecret(ctx, secret, mutationFunc, &externalSecret)
 		if err == nil {
 			externalSecret.Status.Binding = v1.LocalObjectReference{Name: secret.Name}
 		}
@@ -288,7 +288,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		err = nil
 	default:
 		var created bool
-		created, err = createOrUpdate(ctx, r.Client, secret, mutationFunc, externalSecret.Name)
+		created, err = r.createOrUpdateSecret(ctx, secret, mutationFunc, &externalSecret)
 		if err == nil {
 			externalSecret.Status.Binding = v1.LocalObjectReference{Name: secret.Name}
 		}
@@ -316,7 +316,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 }
 
 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)
@@ -364,51 +363,52 @@ func deleteOrphanedSecrets(ctx context.Context, cl client.Client, externalSecret
 	return nil
 }
 
-func createOrUpdate(ctx context.Context, c client.Client, obj client.Object, f func() error, fieldOwner string) (bool, error) {
-	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
-	key := client.ObjectKeyFromObject(obj)
-	if err := c.Get(ctx, key, obj); err != nil {
+func (r *Reconciler) createOrUpdateSecret(ctx context.Context, secret *v1.Secret, mutationFunc func() error, es *esv1beta1.ExternalSecret) (bool, error) {
+	fqdn := fmt.Sprintf(fieldOwnerTemplate, es.Name)
+	key := client.ObjectKeyFromObject(secret)
+	if err := r.Client.Get(ctx, key, secret); err != nil {
 		if !apierrors.IsNotFound(err) {
 			return false, err
 		}
-		if err := f(); err != nil {
+		if err := mutationFunc(); err != nil {
 			return false, err
 		}
 		// Setting Field Owner even for CreationPolicy==Create
-		if err := c.Create(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+		if err := r.Client.Create(ctx, secret, client.FieldOwner(fqdn)); err != nil {
 			return false, err
 		}
+		r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonCreated, "Created Secret")
 		return true, nil
 	}
 
-	existing := obj.DeepCopyObject()
-	if err := f(); err != nil {
+	existing := secret.DeepCopyObject()
+	if err := mutationFunc(); err != nil {
 		return false, err
 	}
 
-	if equality.Semantic.DeepEqual(existing, obj) {
+	if equality.Semantic.DeepEqual(existing, secret) {
 		return false, nil
 	}
 
-	if err := c.Update(ctx, obj, client.FieldOwner(fqdn)); err != nil {
+	if err := r.Client.Update(ctx, secret, client.FieldOwner(fqdn)); err != nil {
 		return false, err
 	}
+	r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret")
 	return false, nil
 }
 
-func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, secret *v1.Secret, mutationFunc func() error, fieldOwner string) error {
-	fqdn := fmt.Sprintf(fieldOwnerTemplate, fieldOwner)
-	err := c.Get(ctx, client.ObjectKeyFromObject(secret), secret.DeepCopy())
+func (r *Reconciler) patchSecret(ctx context.Context, secret *v1.Secret, mutationFunc func() error, es *esv1beta1.ExternalSecret) error {
+	fqdn := fmt.Sprintf(fieldOwnerTemplate, es.Name)
+	err := r.Client.Get(ctx, client.ObjectKeyFromObject(secret), secret.DeepCopy())
 	if apierrors.IsNotFound(err) {
 		return fmt.Errorf(errPolicyMergeNotFound, secret.Name)
 	}
 	if err != nil {
 		return fmt.Errorf(errPolicyMergeGetSecret, secret.Name, err)
 	}
-	existing := secret.DeepCopyObject()
 
-	err = mutationFunc()
-	if err != nil {
+	existing := secret.DeepCopyObject()
+	if err = mutationFunc(); err != nil {
 		return fmt.Errorf(errPolicyMergeMutate, secret.Name, err)
 	}
 
@@ -417,7 +417,7 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	// https://github.com/kubernetes-sigs/controller-runtime/issues/1517
 	// https://github.com/kubernetes/kubernetes/issues/80609
 	// we need to manually set it before doing a Patch() as it depends on the GVK
-	gvks, unversioned, err := scheme.ObjectKinds(secret)
+	gvks, unversioned, err := r.Scheme.ObjectKinds(secret)
 	if err != nil {
 		return err
 	}
@@ -432,10 +432,10 @@ func patchSecret(ctx context.Context, c client.Client, scheme *runtime.Scheme, s
 	secret.ObjectMeta.ManagedFields = nil
 	// we're not able to resolve conflicts so we force ownership
 	// see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#using-server-side-apply-in-a-controller
-	err = c.Patch(ctx, secret, client.Apply, client.FieldOwner(fqdn), client.ForceOwnership)
-	if err != nil {
+	if err := r.Client.Patch(ctx, secret, client.Apply, client.FieldOwner(fqdn), client.ForceOwnership); err != nil {
 		return fmt.Errorf(errPolicyMergePatch, secret.Name, err)
 	}
+	r.recorder.Event(es, v1.EventTypeNormal, esv1beta1.ReasonUpdated, "Updated Secret")
 	return nil
 }