feat: refuse to let two Repositories manage one BorgBase repository

Adopting a repository that another Repository already manages leaves both
pushing their own spec to it. Each reconcile overwrites the other's settings and
the repository flaps between them, quietly: nothing is unhealthy, the values
just keep changing. Found on fennec, where an adopted Repository with no quota
cleared the 5GB limit its owner had set.

The newcomer is now held back with a RepositoryConflict condition naming the
incumbent, and its settings are left alone. Only a resource created earlier
counts as the incumbent, so the one already managing the repository keeps
working -- the same rule the healthchecks slug conflict uses, for the same
reason.

The check spans namespaces, because a BorgBase repository belongs to the account
rather than to any one namespace, and it matches on the repository a resource
has recorded or the one it was told to adopt, so a conflict is caught before the
newcomer's first edit rather than after it.

Sharing one repository between several backups is still perfectly fine: that is
one Repository with several ScheduledBackups pointing at it.

Author: USA-RedDragon

internal/controller/repository_controller.go

@@ -204,6 +204,25 @@ func (r *RepositoryReconciler) reconcile(
 			"%w: repository %s has format %q", borgbase.ErrNotRestic, remote.ID, remote.Format)
 	}
 
+	// Two Repository resources pointing at one BorgBase repository each push
+	// their own spec to it, so every reconcile overwrites the other's settings
+	// and the repository flaps between them. Hold the newcomer back rather than
+	// letting them fight.
+	if other, err := r.repositoryConflict(ctx, repo, remote.ID); err != nil {
+		return ctrl.Result{}, err
+	} else if other != "" {
+		r.setCondition(repo, metav1.Condition{
+			Type:   borgbasev1.RepositoryConditionReady,
+			Status: metav1.ConditionFalse,
+			Reason: "RepositoryConflict",
+			Message: fmt.Sprintf(
+				"BorgBase repository %s is already managed by Repository %s; "+
+					"point one of them elsewhere, or delete this one", remote.ID, other),
+		})
+		// Its settings are deliberately left alone: the incumbent owns them.
+		return ctrl.Result{RequeueAfter: r.interval(repo)}, nil
+	}
+
 	if remote, err = r.reconcileSettings(ctx, repo, remote, api); err != nil {
 		return ctrl.Result{}, err
 	}
@@ -325,6 +344,60 @@ func (r *RepositoryReconciler) resolveRepo(
 	return remote, nil
 }
 
+// repositoryConflict returns the Repository that already manages this BorgBase
+// repository, or "" when there is none.
+//
+// The check is cluster-wide, because a BorgBase repository belongs to the
+// account rather than to a namespace. Only a resource created earlier counts as
+// the incumbent, so the one already managing it keeps working and the newcomer
+// is the one held back -- the same rule the healthchecks slug conflict uses.
+func (r *RepositoryReconciler) repositoryConflict(
+	ctx context.Context, repo *borgbasev1.Repository, id string,
+) (string, error) {
+	if id == "" {
+		return "", nil
+	}
+
+	var list borgbasev1.RepositoryList
+	if err := r.List(ctx, &list); err != nil {
+		return "", fmt.Errorf("listing repositories: %w", err)
+	}
+
+	for i := range list.Items {
+		other := &list.Items[i]
+		if !other.DeletionTimestamp.IsZero() ||
+			(other.Namespace == repo.Namespace && other.Name == repo.Name) {
+			continue
+		}
+		if managedID(other) != id {
+			continue
+		}
+		if olderRepository(other, repo) {
+			return other.Namespace + "/" + other.Name, nil
+		}
+	}
+	return "", nil
+}
+
+// managedID is the BorgBase repository a resource manages: the one it has
+// recorded, or the one it was told to adopt before it has reconciled.
+func managedID(repo *borgbasev1.Repository) string {
+	if repo.Status.RepositoryID != "" {
+		return repo.Status.RepositoryID
+	}
+	return repo.Spec.ExistingRepositoryID
+}
+
+func olderRepository(a, b *borgbasev1.Repository) bool {
+	if !a.CreationTimestamp.Equal(&b.CreationTimestamp) {
+		return a.CreationTimestamp.Before(&b.CreationTimestamp)
+	}
+	if a.Namespace != b.Namespace {
+		return a.Namespace < b.Namespace
+	}
+	return a.Name < b.Name
+}
+
 // reconcileSettings brings the repository's mutable settings in line with the
 // spec, and returns the repository as it stands afterwards.
 //

internal/controller/repository_controller_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -783,3 +784,99 @@ func derefInt64(p *int64) any {
 	}
 	return *p
 }
+
+// Two Repository resources pointing at one BorgBase repository each push their
+// own spec to it, so they overwrite each other on every reconcile and the
+// repository flaps. The newcomer is held back instead.
+func TestConflictingRepositoriesDoNotFight(t *testing.T) {
+	incumbent := &borgbasev1.Repository{
+		Name: resticName, Namespace: testNS,
+		CreationTimestamp: metav1.NewTime(time.Now().Add(-time.Hour)),
+		Status:            borgbasev1.RepositoryStatus{RepositoryID: testRepoID, Initialized: true},
+	}
+	newcomer := &borgbasev1.Repository{
+		Name: "adopted", Namespace: testNS,
+		CreationTimestamp: metav1.NewTime(time.Now()),
+		Spec:              borgbasev1.RepositorySpec{ExistingRepositoryID: testRepoID},
+	}
+
+	api := newFakeAPI(&borgbase.Repo{
+		ID: testRepoID, Name: testNS, Format: borgbase.FormatRestic, Htpasswd: "t",
+		Quota: 100, QuotaEnabled: true,
+	})
+	r, _ := newHarness(t, api, incumbent, newcomer)
+
+	other, err := r.repositoryConflict(context.Background(), newcomer, testRepoID)
+	if err != nil {
+		t.Fatalf("repositoryConflict: %v", err)
+	}
+	if other != testNS+"/"+resticName {
+		t.Errorf("newcomer should defer to the incumbent, got %q", other)
+	}
+
+	// The incumbent carries on: it was there first.
+	other, err = r.repositoryConflict(context.Background(), incumbent, testRepoID)
+	if err != nil {
+		t.Fatalf("repositoryConflict: %v", err)
+	}
+	if other != "" {
+		t.Errorf("the incumbent must keep managing the repository, got conflict with %q", other)
+	}
+}
+
+// A repository nothing else claims is not a conflict, and neither is one whose
+// only other claimant is being deleted.
+func TestRepositoryConflictIgnoresUnrelatedAndDeleted(t *testing.T) {
+	deleting := &borgbasev1.Repository{
+		Name: "old", Namespace: testNS,
+		CreationTimestamp: metav1.NewTime(time.Now().Add(-time.Hour)),
+		DeletionTimestamp: ptr.To(metav1.Now()),
+		Finalizers:        []string{FinalizerName},
+		Status:            borgbasev1.RepositoryStatus{RepositoryID: testRepoID},
+	}
+	elsewhere := &borgbasev1.Repository{
+		Name: "other", Namespace: "somewhere-else",
+		CreationTimestamp: metav1.NewTime(time.Now().Add(-time.Hour)),
+		Status:            borgbasev1.RepositoryStatus{RepositoryID: "different"},
+	}
+	mine := &borgbasev1.Repository{
+		Name: resticName, Namespace: testNS,
+		CreationTimestamp: metav1.NewTime(time.Now()),
+		Status:            borgbasev1.RepositoryStatus{RepositoryID: testRepoID},
+	}
+
+	r, _ := newHarness(t, newFakeAPI(), deleting, elsewhere, mine)
+
+	other, err := r.repositoryConflict(context.Background(), mine, testRepoID)
+	if err != nil {
+		t.Fatalf("repositoryConflict: %v", err)
+	}
+	if other != "" {
+		t.Errorf("expected no conflict, got %q", other)
+	}
+}
+
+// The check spans namespaces, because a BorgBase repository belongs to the
+// account rather than to any one namespace.
+func TestRepositoryConflictIsClusterWide(t *testing.T) {
+	incumbent := &borgbasev1.Repository{
+		Name: resticName, Namespace: "team-a",
+		CreationTimestamp: metav1.NewTime(time.Now().Add(-time.Hour)),
+		Status:            borgbasev1.RepositoryStatus{RepositoryID: testRepoID},
+	}
+	newcomer := &borgbasev1.Repository{
+		Name: resticName, Namespace: "team-b",
+		CreationTimestamp: metav1.NewTime(time.Now()),
+		Spec:              borgbasev1.RepositorySpec{ExistingRepositoryID: testRepoID},
+	}
+
+	r, _ := newHarness(t, newFakeAPI(), incumbent, newcomer)
+
+	other, err := r.repositoryConflict(context.Background(), newcomer, testRepoID)
+	if err != nil {
+		t.Fatalf("repositoryConflict: %v", err)
+	}
+	if other != "team-a/"+resticName {
+		t.Errorf("expected the conflict to be found across namespaces, got %q", other)
+	}
+}