fix(operator): refuse to overwrite a Secret the operator does not manage

internal/controller/ferrvaultsecret_controller.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"sync"
@@ -195,6 +196,9 @@ func (r *FerrVaultSecretReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 
 	secret, _, err := r.ensureTargetSecret(ctx, &cr, transformed, newHash, !tokenTarget)
+	if errors.Is(err, errTargetConflict) {
+		return r.failReady(ctx, &cr, "TargetConflict", err.Error())
+	}
 	if err != nil {
 		return r.failReady(ctx, &cr, "SecretWriteFailed", err.Error())
 	}

internal/controller/ferrvaultsecret_guard_test.go

@@ -240,3 +240,66 @@ func TestOrdinaryTargetIsStillOwned(t *testing.T) {
 		t.Fatalf("ordinary target lost its controller reference: %+v", s.OwnerReferences)
 	}
 }
+
+func TestUnmanagedExistingTargetIsNotOverwritten(t *testing.T) {
+	existing := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "tls"},
+		Type:       corev1.SecretTypeTLS,
+		Data:       map[string][]byte{"tls.crt": []byte("crt"), "extra": []byte("keep")},
+	}
+	f := newGuardFixture(t, "tls", nil, partialVault{secrets: map[string]string{"A": "a"}}, existing)
+
+	f.reconcile(t)
+
+	s, _ := f.secret(t, "tls")
+	if string(s.Data["extra"]) != "keep" || string(s.Data["tls.crt"]) != "crt" || s.Data["A"] != nil {
+		t.Fatalf("unmanaged Secret was rewritten: %v", s.Data)
+	}
+	if s.Type != corev1.SecretTypeTLS || len(s.OwnerReferences) != 0 || s.Annotations[fvAnnotationManagedBy] != "" {
+		t.Fatalf("unmanaged Secret was adopted: type=%s owners=%v annotations=%v", s.Type, s.OwnerReferences, s.Annotations)
+	}
+	cond := f.ready(t)
+	if cond == nil || cond.Status != metav1.ConditionFalse || cond.Reason != "TargetConflict" {
+		t.Fatalf("want Ready=False TargetConflict, got %+v", cond)
+	}
+}
+
+func TestManagedExistingTargetIsStillUpdated(t *testing.T) {
+	existing := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace:   "default",
+			Name:        "runtime",
+			Annotations: map[string]string{fvAnnotationManagedBy: "ferrvault-operator"},
+		},
+		Data: map[string][]byte{"A": []byte("a-old")},
+	}
+	f := newGuardFixture(t, "runtime", nil, partialVault{secrets: map[string]string{"A": "a-new"}}, existing)
+
+	f.reconcile(t)
+
+	s, _ := f.secret(t, "runtime")
+	if secretValue(s, "A") != "a-new" {
+		t.Fatalf("managed Secret was not updated: %v", s.Data)
+	}
+	if cond := f.ready(t); cond == nil || cond.Reason != "Synced" {
+		t.Fatalf("want Synced, got %+v", cond)
+	}
+}
+
+func TestExistingTargetOwnedByTheCRIsStillUpdated(t *testing.T) {
+	controller := true
+	existing := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "runtime", OwnerReferences: []metav1.OwnerReference{{
+			APIVersion: fvv1alpha1.GroupVersion.String(), Kind: "FerrVaultSecret", Name: "sync", UID: guardOwnerUID, Controller: &controller,
+		}}},
+		Data: map[string][]byte{"A": []byte("a-old")},
+	}
+	f := newGuardFixture(t, "runtime", nil, partialVault{secrets: map[string]string{"A": "a-new"}}, existing)
+
+	f.reconcile(t)
+
+	s, _ := f.secret(t, "runtime")
+	if secretValue(s, "A") != "a-new" {
+		t.Fatalf("owned Secret was not updated: %v", s.Data)
+	}
+}

internal/controller/ferrvaultsecret_sync.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -18,6 +19,25 @@ import (
 	fvv1alpha1 "github.com/FerrLabs/FerrVault/api/ferrvault/v1alpha1"
 )
 
+const fvAnnotationManagedBy = "ferrvault.com/managed-by"
+
+var errTargetConflict = errors.New("target Secret exists and is not managed by this operator")
+
+func adoptable(secret *corev1.Secret, cr *fvv1alpha1.FerrVaultSecret, allowUnmanaged bool) bool {
+	if secret.ResourceVersion == "" || allowUnmanaged {
+		return true
+	}
+	if _, ok := secret.Annotations[fvAnnotationManagedBy]; ok {
+		return true
+	}
+	for _, ref := range secret.OwnerReferences {
+		if ref.UID == cr.UID {
+			return true
+		}
+	}
+	return false
+}
+
 func (r *FerrVaultSecretReconciler) loadToken(ctx context.Context, conn *fvv1alpha1.FerrVaultConnection) (string, error) {
 	broker := r.Broker
 	if broker == nil {
@@ -47,6 +67,9 @@ func (r *FerrVaultSecretReconciler) ensureTargetSecret(
 	}
 	var oldHash string
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, secret, func() error {
+		if !adoptable(secret, cr, !owned) {
+			return fmt.Errorf("%w: %s/%s", errTargetConflict, secret.Namespace, secret.Name)
+		}
 		if existing, ok := secret.Annotations[fvAnnotationContentHash]; ok {
 			oldHash = existing
 		}
@@ -61,7 +84,7 @@ func (r *FerrVaultSecretReconciler) ensureTargetSecret(
 		if secret.Annotations == nil {
 			secret.Annotations = map[string]string{}
 		}
-		secret.Annotations["ferrvault.com/managed-by"] = "ferrvault-operator"
+		secret.Annotations[fvAnnotationManagedBy] = "ferrvault-operator"
 		secret.Annotations[fvAnnotationContentHash] = newHash
 		secret.StringData = nil
 		secret.Data = secretBytes(data)

internal/controller/reconcile_fixture_test.go

@@ -69,7 +69,7 @@ func newReconcileFixture(t *testing.T, upstream map[string]string, funcs interce
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace:   "default",
 				Name:        "runtime",
-				Annotations: map[string]string{fvAnnotationContentHash: "content-before-the-change"},
+				Annotations: map[string]string{fvAnnotationContentHash: "content-before-the-change", fvAnnotationManagedBy: "ferrvault-operator"},
 			},
 		},
 		&appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "api"}},