From 5bda4180cf4342164a92dc10d1251993624d6803 Mon Sep 17 00:00:00 2001 From: BryanFRD Date: Sun, 25 Oct 2026 09:49:44 +0100 Subject: [PATCH 1/2] fix(operator): refuse to overwrite a Secret the operator does not manage --- .../controller/ferrvaultsecret_controller.go | 5 ++ .../controller/ferrvaultsecret_guard_test.go | 63 +++++++++++++++++++ internal/controller/ferrvaultsecret_sync.go | 25 +++++++- internal/controller/reconcile_fixture_test.go | 2 +- 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/internal/controller/ferrvaultsecret_controller.go b/internal/controller/ferrvaultsecret_controller.go index 1a1ce3e..4bb5c52 100644 --- a/internal/controller/ferrvaultsecret_controller.go +++ b/internal/controller/ferrvaultsecret_controller.go @@ -2,12 +2,14 @@ package controller import ( "context" + "errors" "fmt" "sort" "sync" "time" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -195,6 +197,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()) } diff --git a/internal/controller/ferrvaultsecret_guard_test.go b/internal/controller/ferrvaultsecret_guard_test.go index 57afdbe..6c290cd 100644 --- a/internal/controller/ferrvaultsecret_guard_test.go +++ b/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) + } +} diff --git a/internal/controller/ferrvaultsecret_sync.go b/internal/controller/ferrvaultsecret_sync.go index 8149b77..a3c515f 100644 --- a/internal/controller/ferrvaultsecret_sync.go +++ b/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) diff --git a/internal/controller/reconcile_fixture_test.go b/internal/controller/reconcile_fixture_test.go index 5745b5c..f737e2b 100644 --- a/internal/controller/reconcile_fixture_test.go +++ b/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"}}, From 0379d6730d00b0846658afdb0d450ef525a092e3 Mon Sep 17 00:00:00 2001 From: BryanFRD Date: Sun, 25 Oct 2026 09:58:39 +0100 Subject: [PATCH 2/2] style(operator): drop stray blank line in imports --- internal/controller/ferrvaultsecret_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/controller/ferrvaultsecret_controller.go b/internal/controller/ferrvaultsecret_controller.go index 4bb5c52..8e1e646 100644 --- a/internal/controller/ferrvaultsecret_controller.go +++ b/internal/controller/ferrvaultsecret_controller.go @@ -9,7 +9,6 @@ import ( "time" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime"