refactor: delete duplication of applyOwnership logic (#6301)

pkg/controllers/externalsecret/externalsecret_controller.go

@@ -450,42 +450,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 
 	// mutationFunc is a function which can be applied to a secret to make it match the desired state.
 	mutationFunc := func(secret *v1.Secret) error {
-		// get information about the current owner of the secret
-		//  - we ignore the API version as it can change over time
-		//  - we ignore the UID for consistency with the SetControllerReference function
-		currentOwner := metav1.GetControllerOf(secret)
-		ownerIsESKind := false
-		ownerIsCurrentES := false
-		if currentOwner != nil {
-			currentOwnerGK := schema.FromAPIVersionAndKind(currentOwner.APIVersion, currentOwner.Kind).GroupKind()
-			ownerIsESKind = currentOwnerGK.String() == esv1.ExtSecretGroupKind
-			ownerIsCurrentES = ownerIsESKind && currentOwner.Name == externalSecret.Name
-		}
-
-		// if another ExternalSecret is the owner, we should return an error
-		// otherwise the controller will fight with itself to update the secret.
-		// note, this does not prevent other controllers from owning the secret.
-		if ownerIsESKind && !ownerIsCurrentES {
-			return fmt.Errorf("%w: %s", ErrSecretIsOwned, currentOwner.Name)
-		}
-
-		// if the CreationPolicy is Owner, we should set ourselves as the owner of the secret
-		if externalSecret.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
-			err = controllerutil.SetControllerReference(externalSecret, secret, r.Scheme)
-			if err != nil {
-				return fmt.Errorf("%w: %w", ErrSecretSetCtrlRef, err)
-			}
-		}
-
-		// if the creation policy is not Owner, we should remove ourselves as the owner
-		// this could happen if the creation policy was changed after the secret was created
-		if externalSecret.Spec.Target.CreationPolicy != esv1.CreatePolicyOwner && ownerIsCurrentES {
-			err = controllerutil.RemoveControllerReference(externalSecret, secret, r.Scheme)
-			if err != nil {
-				return fmt.Errorf("%w: %w", ErrSecretRemoveCtrlRef, err)
-			}
-		}
-
 		// initialize maps within the secret so it's safe to set values
 		if secret.Annotations == nil {
 			secret.Annotations = make(map[string]string)
@@ -526,14 +490,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ct
 			}
 		}
 
-		// we also use a label to keep track of the owner of the secret
-		// this lets us remove secrets that are no longer needed if the target secret name changes
-		if externalSecret.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
-			lblValue := esutils.ObjectHash(fmt.Sprintf("%v/%v", externalSecret.Namespace, externalSecret.Name))
-			secret.Labels[esv1.LabelOwner] = lblValue
-		} else {
-			// the label should not be set if the creation policy is not Owner
-			delete(secret.Labels, esv1.LabelOwner)
+		if err := r.applyOwnership(externalSecret, secret); err != nil {
+			return err
 		}
 
 		secret.Labels[esv1.LabelManaged] = esv1.LabelManagedValue
@@ -816,6 +774,60 @@ func (r *Reconciler) markAsFailed(msg string, err error, externalSecret *esv1.Ex
 	counter.Inc()
 }
 
+// applyOwnership handles ownership-related logic for target resources (Secrets and generic targets).
+// It detects ownership conflicts with other ExternalSecrets, sets or removes the controller reference
+// based on CreationPolicy, and manages the LabelOwner label.
+func (r *Reconciler) applyOwnership(es *esv1.ExternalSecret, target client.Object) error {
+	// get information about the current owner of the target
+	//  - we ignore the API version as it can change over time
+	//  - we ignore the UID for consistency with the SetControllerReference function
+	currentOwner := metav1.GetControllerOf(target)
+	ownerIsESKind := false
+	ownerIsCurrentES := false
+	if currentOwner != nil {
+		currentOwnerGK := schema.FromAPIVersionAndKind(currentOwner.APIVersion, currentOwner.Kind).GroupKind()
+		ownerIsESKind = currentOwnerGK.String() == esv1.ExtSecretGroupKind
+		ownerIsCurrentES = ownerIsESKind && currentOwner.Name == es.Name
+	}
+
+	// if another ExternalSecret is the owner, we should return an error
+	// otherwise the controller will fight with itself to update the target.
+	// note, this does not prevent other controllers from owning the target.
+	if ownerIsESKind && !ownerIsCurrentES {
+		return fmt.Errorf("%w: %s", ErrSecretIsOwned, currentOwner.Name)
+	}
+
+	// if the CreationPolicy is Owner, we should set ourselves as the owner of the target
+	if es.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
+		if err := controllerutil.SetControllerReference(es, target, r.Scheme); err != nil {
+			return fmt.Errorf("%w: %w", ErrSecretSetCtrlRef, err)
+		}
+	}
+
+	// if the creation policy is not Owner, we should remove ourselves as the owner
+	// this could happen if the creation policy was changed after the target was created
+	if es.Spec.Target.CreationPolicy != esv1.CreatePolicyOwner && ownerIsCurrentES {
+		if err := controllerutil.RemoveControllerReference(es, target, r.Scheme); err != nil {
+			return fmt.Errorf("%w: %w", ErrSecretRemoveCtrlRef, err)
+		}
+	}
+
+	// we also use a label to keep track of the owner of the target
+	// this lets us remove targets that are no longer needed if the target name changes
+	labels := target.GetLabels()
+	if labels == nil {
+		labels = make(map[string]string)
+	}
+	if es.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
+		labels[esv1.LabelOwner] = esutils.ObjectHash(fmt.Sprintf("%v/%v", es.Namespace, es.Name))
+	} else {
+		delete(labels, esv1.LabelOwner)
+	}
+	target.SetLabels(labels)
+
+	return nil
+}
+
 func (r *Reconciler) cleanupManagedSecrets(ctx context.Context, log logr.Logger, externalSecret *esv1.ExternalSecret) error {
 	// Only delete resources if DeletionPolicy is Delete
 	if externalSecret.Spec.Target.DeletionPolicy != esv1.DeletionPolicyDelete {

pkg/controllers/externalsecret/externalsecret_controller_manifest.go

@@ -24,15 +24,12 @@ import (
 	"github.com/go-logr/logr"
 	v1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	esv1 "github.com/external-secrets/external-secrets/apis/externalsecrets/v1"
 	"github.com/external-secrets/external-secrets/pkg/controllers/templating"
-	"github.com/external-secrets/external-secrets/runtime/esutils"
 	"github.com/external-secrets/external-secrets/runtime/template"
 )
 
@@ -246,60 +243,6 @@ func (r *Reconciler) applyTemplateToManifest(ctx context.Context, es *esv1.Exter
 	return result, nil
 }
 
-// applyOwnership manages the owner reference and owner label on the target manifest resource.
-func (r *Reconciler) applyOwnership(es *esv1.ExternalSecret, result *unstructured.Unstructured) error {
-	// get information about the current owner of the resource
-	//  - we ignore the API version as it can change over time
-	//  - we ignore the UID for consistency with the SetControllerReference function
-	currentOwner := metav1.GetControllerOf(result)
-	ownerIsESKind := false
-	ownerIsCurrentES := false
-	if currentOwner != nil {
-		currentOwnerGK := schema.FromAPIVersionAndKind(currentOwner.APIVersion, currentOwner.Kind).GroupKind()
-		ownerIsESKind = currentOwnerGK.String() == esv1.ExtSecretGroupKind
-		ownerIsCurrentES = ownerIsESKind && currentOwner.Name == es.Name
-	}
-
-	// if another ExternalSecret is the owner, we should return an error
-	// otherwise the controller will fight with itself to update the resource.
-	// note, this does not prevent other controllers from owning the resource.
-	if ownerIsESKind && !ownerIsCurrentES {
-		return fmt.Errorf("%w: %s", ErrSecretIsOwned, currentOwner.Name)
-	}
-
-	// if the CreationPolicy is Owner, we should set ourselves as the owner of the resource
-	if es.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
-		if err := controllerutil.SetControllerReference(es, result, r.Scheme); err != nil {
-			return fmt.Errorf("%w: %w", ErrSecretSetCtrlRef, err)
-		}
-	}
-
-	// if the creation policy is not Owner, we should remove ourselves as the owner
-	// this could happen if the creation policy was changed after the resource was created
-	if es.Spec.Target.CreationPolicy != esv1.CreatePolicyOwner && ownerIsCurrentES {
-		if err := controllerutil.RemoveControllerReference(es, result, r.Scheme); err != nil {
-			return fmt.Errorf("%w: %w", ErrSecretRemoveCtrlRef, err)
-		}
-	}
-
-	labels := result.GetLabels()
-	if labels == nil {
-		labels = make(map[string]string)
-	}
-
-	// we also use a label to keep track of the owner of the resource
-	// this lets us remove resources that are no longer needed if the target name changes
-	// the label should not be set if the creation policy is not Owner
-	if es.Spec.Target.CreationPolicy == esv1.CreatePolicyOwner {
-		labels[esv1.LabelOwner] = esutils.ObjectHash(fmt.Sprintf("%v/%v", es.Namespace, es.Name))
-	} else {
-		delete(labels, esv1.LabelOwner)
-	}
-	result.SetLabels(labels)
-
-	return nil
-}
-
 // createSimpleManifest creates a simple resource without templates (e.g., ConfigMap with data field).
 func (r *Reconciler) createSimpleManifest(obj *unstructured.Unstructured, dataMap map[string][]byte) *unstructured.Unstructured {
 	// For ConfigMaps and similar resources, put data in .data field