fix: propagate objectMeta and ownerReferences to target resources (#6132)

Co-authored-by: Gergely Bräutigam <gergely.brautigam@sap.com>

pkg/controllers/externalsecret/externalsecret_controller_manifest.go

@@ -24,12 +24,15 @@ 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"
 )
 
@@ -199,10 +202,13 @@ func (r *Reconciler) applyTemplateToManifest(ctx context.Context, es *esv1.Exter
 		annotations = make(map[string]string)
 	}
 
+	srcLabels, srcAnnotations := es.ObjectMeta.Labels, es.ObjectMeta.Annotations
 	if es.Spec.Target.Template != nil {
-		maps.Copy(labels, es.Spec.Target.Template.Metadata.Labels)
-		maps.Copy(annotations, es.Spec.Target.Template.Metadata.Annotations)
+		srcLabels = es.Spec.Target.Template.Metadata.Labels
+		srcAnnotations = es.Spec.Target.Template.Metadata.Annotations
 	}
+	maps.Copy(labels, srcLabels)
+	maps.Copy(annotations, srcAnnotations)
 
 	labels[esv1.LabelManaged] = esv1.LabelManagedValue
 
@@ -233,9 +239,67 @@ func (r *Reconciler) applyTemplateToManifest(ctx context.Context, es *esv1.Exter
 	ann[esv1.AnnotationDataHash] = hash
 	result.SetAnnotations(ann)
 
+	if err := r.applyOwnership(es, result); err != nil {
+		return nil, err
+	}
+
 	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

pkg/controllers/externalsecret/externalsecret_controller_manifest_test.go

@@ -344,6 +344,7 @@ func TestApplyTemplateToManifest_SimpleConfigMap(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{
@@ -395,6 +396,7 @@ func TestApplyTemplateToManifest_WithMetadata(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{
@@ -447,6 +449,282 @@ func TestApplyTemplateToManifest_WithMetadata(t *testing.T) {
 	assert.Equal(t, "This is a test", annotations["description"])
 }
 
+func TestApplyTemplateToManifest_AppliesOwnershipWhenCreationPolicyOwner(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+	r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-es",
+			Namespace: "default",
+			UID:       "abc-123",
+		},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name:           "test-configmap",
+				CreationPolicy: esv1.CreatePolicyOwner,
+				Manifest: &esv1.ManifestReference{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+			},
+		},
+	}
+
+	result, err := r.applyTemplateToManifest(context.Background(), es, map[string][]byte{"key": []byte("val")}, nil)
+
+	require.NoError(t, err)
+	owners := result.GetOwnerReferences()
+	require.Len(t, owners, 1)
+	assert.Equal(t, "test-es", owners[0].Name)
+	assert.True(t, *owners[0].Controller)
+	labels := result.GetLabels()
+	assert.Equal(t, esutils.ObjectHash("default/test-es"), labels[esv1.LabelOwner])
+}
+
+func TestApplyOwnership(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	isController := true
+
+	tests := []struct {
+		name           string
+		creationPolicy esv1.ExternalSecretCreationPolicy
+		existing       *unstructured.Unstructured
+		expectedErr    error
+		validate       func(t *testing.T, result *unstructured.Unstructured)
+	}{
+		{
+			name:           "removes LabelOwner when policy is not Owner",
+			creationPolicy: esv1.CreatePolicyOrphan,
+			existing: func() *unstructured.Unstructured {
+				u := &unstructured.Unstructured{}
+				u.SetLabels(map[string]string{
+					esv1.LabelOwner: esutils.ObjectHash("default/test-es"),
+				})
+				return u
+			}(),
+			validate: func(t *testing.T, result *unstructured.Unstructured) {
+				assert.Empty(t, result.GetLabels()[esv1.LabelOwner])
+			},
+		},
+		{
+			name:           "removes owner reference when policy changes from Owner to Orphan",
+			creationPolicy: esv1.CreatePolicyOrphan,
+			existing: func() *unstructured.Unstructured {
+				u := &unstructured.Unstructured{}
+				u.SetGroupVersionKind(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"})
+				u.SetOwnerReferences([]metav1.OwnerReference{
+					{
+						APIVersion: esv1.SchemeGroupVersion.String(),
+						Kind:       esv1.ExtSecretKind,
+						Name:       "test-es",
+						UID:        "abc-123",
+						Controller: &isController,
+					},
+				})
+				return u
+			}(),
+			validate: func(t *testing.T, result *unstructured.Unstructured) {
+				assert.Empty(t, result.GetOwnerReferences())
+			},
+		},
+		{
+			name:           "returns ErrSecretIsOwned when owned by a different ExternalSecret",
+			creationPolicy: esv1.CreatePolicyOwner,
+			existing: func() *unstructured.Unstructured {
+				u := &unstructured.Unstructured{}
+				u.SetOwnerReferences([]metav1.OwnerReference{
+					{
+						APIVersion: esv1.SchemeGroupVersion.String(),
+						Kind:       esv1.ExtSecretKind,
+						Name:       "other-es",
+						UID:        "xyz-999",
+						Controller: &isController,
+					},
+				})
+				return u
+			}(),
+			expectedErr: ErrSecretIsOwned,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+			r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+			es := &esv1.ExternalSecret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-es",
+					Namespace: "default",
+					UID:       "abc-123",
+				},
+				Spec: esv1.ExternalSecretSpec{
+					Target: esv1.ExternalSecretTarget{
+						Name:           "test-configmap",
+						CreationPolicy: tt.creationPolicy,
+						Manifest: &esv1.ManifestReference{
+							APIVersion: "v1",
+							Kind:       "ConfigMap",
+						},
+					},
+				},
+			}
+
+			err := r.applyOwnership(es, tt.existing)
+
+			if tt.expectedErr != nil {
+				require.ErrorIs(t, err, tt.expectedErr)
+				return
+			}
+			require.NoError(t, err)
+			if tt.validate != nil {
+				tt.validate(t, tt.existing)
+			}
+		})
+	}
+}
+
+func TestApplyTemplateToManifest_NoOwnerRefWhenCreationPolicyOrphan(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+	r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-es",
+			Namespace: "default",
+			UID:       "abc-123",
+		},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name:           "test-configmap",
+				CreationPolicy: esv1.CreatePolicyOrphan,
+				Manifest: &esv1.ManifestReference{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+			},
+		},
+	}
+
+	result, err := r.applyTemplateToManifest(context.Background(), es, map[string][]byte{"key": []byte("val")}, nil)
+
+	require.NoError(t, err)
+	assert.Empty(t, result.GetOwnerReferences())
+}
+
+func TestApplyTemplateToManifest_PropagatesESLabelsAndAnnotations(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+	r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-es",
+			Namespace: "default",
+			Labels: map[string]string{
+				"app.kubernetes.io/instance": "my-argocd-app",
+				"custom-label":               "custom-value",
+			},
+			Annotations: map[string]string{
+				"argocd.argoproj.io/tracking-id": "my-argocd-app:external-secrets.io/ExternalSecret:default/test-es",
+			},
+		},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name: "test-configmap",
+				Manifest: &esv1.ManifestReference{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+			},
+		},
+	}
+
+	result, err := r.applyTemplateToManifest(context.Background(), es, map[string][]byte{"key": []byte("val")}, nil)
+
+	require.NoError(t, err)
+	labels := result.GetLabels()
+	assert.Equal(t, "my-argocd-app", labels["app.kubernetes.io/instance"])
+	assert.Equal(t, "custom-value", labels["custom-label"])
+	assert.Equal(t, esv1.LabelManagedValue, labels[esv1.LabelManaged])
+
+	annotations := result.GetAnnotations()
+	assert.Equal(t, "my-argocd-app:external-secrets.io/ExternalSecret:default/test-es", annotations["argocd.argoproj.io/tracking-id"])
+}
+
+func TestApplyTemplateToManifest_TemplateMetadataWinsOverESLabels(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+	r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-es",
+			Namespace: "default",
+			Labels: map[string]string{
+				"app.kubernetes.io/instance": "my-argocd-app",
+			},
+		},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name: "test-configmap",
+				Manifest: &esv1.ManifestReference{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+				Template: &esv1.ExternalSecretTemplate{
+					EngineVersion: esv1.TemplateEngineV2,
+					Metadata: esv1.ExternalSecretTemplateMetadata{
+						Labels: map[string]string{
+							"app": "explicit-template-label",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	result, err := r.applyTemplateToManifest(context.Background(), es, map[string][]byte{"key": []byte("val")}, nil)
+
+	require.NoError(t, err)
+	labels := result.GetLabels()
+	assert.Equal(t, "explicit-template-label", labels["app"])
+	assert.Empty(t, labels["app.kubernetes.io/instance"])
+	assert.Equal(t, esv1.LabelManagedValue, labels[esv1.LabelManaged])
+}
+
+func TestApplyTemplateToManifest_NoESLabels(t *testing.T) {
+	_ = esv1.AddToScheme(scheme.Scheme)
+	fakeClient := fakeclient.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+	r := &Reconciler{Client: fakeClient, Scheme: scheme.Scheme}
+
+	es := &esv1.ExternalSecret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-es",
+			Namespace: "default",
+		},
+		Spec: esv1.ExternalSecretSpec{
+			Target: esv1.ExternalSecretTarget{
+				Name: "test-configmap",
+				Manifest: &esv1.ManifestReference{
+					APIVersion: "v1",
+					Kind:       "ConfigMap",
+				},
+			},
+		},
+	}
+
+	result, err := r.applyTemplateToManifest(context.Background(), es, map[string][]byte{"key": []byte("val")}, nil)
+
+	require.NoError(t, err)
+	labels := result.GetLabels()
+	assert.Equal(t, esv1.LabelManagedValue, labels[esv1.LabelManaged])
+	assert.Len(t, labels, 1)
+}
+
 func TestGetGenericResource(t *testing.T) {
 	// Setup
 	_ = esv1.AddToScheme(scheme.Scheme)
@@ -471,6 +749,7 @@ func TestGetGenericResource(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{
@@ -512,6 +791,7 @@ func TestGetGenericResource_NotFound(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{
@@ -552,6 +832,7 @@ func TestApplyTemplateToManifest_LiteralWithDeployment(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{
@@ -633,6 +914,7 @@ func TestApplyTemplateToManifest_MergeBehavior(t *testing.T) {
 
 	r := &Reconciler{
 		Client: fakeClient,
+		Scheme: scheme.Scheme,
 	}
 
 	es := &esv1.ExternalSecret{