Disable implicit pool creation on link behind a feature flagsmh-no-implicit-pool-creation

This commit adds a feature flag to disable the implicit pool creation
when joining a repository to a non-existent object pool. It looks like
this behavior is not needed given Rails should create the object pool
prior to considering it ready for joining. Disabling this behavior makes
it easier to handle pools in Praefect as Praefect needs to create metadata
records for all of the repositories. This implicit behavior might create
a repository without Praefect's knowledge. Special handling would be
required for this particular RPC only, so let's see if we can remove the
behavior rather than implement it in Praefect.

4 files changed, 40 insertions, 7 deletions

diff --git a/internal/gitaly/service/objectpool/link.go b/internal/gitaly/service/objectpool/link.go
index 3e378d40b..cb57a1ecf 100644
--- a/internal/gitaly/service/objectpool/link.go
+++ b/internal/gitaly/service/objectpool/link.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -21,11 +22,17 @@ func (s *server) LinkRepositoryToObjectPool(ctx context.Context, req *gitalypb.L
 
 	repo := s.localrepo(req.GetRepository())
 
-	if err := pool.Init(ctx); err != nil {
-		return nil, helper.ErrInternal(err)
+	if featureflag.LinkRepositoryToObjectPoolNotFound.IsDisabled(ctx) {
+		if err := pool.Init(ctx); err != nil {
+			return nil, helper.ErrInternal(err)
+		}
 	}
 
 	if err := pool.Link(ctx, repo); err != nil {
+		if featureflag.LinkRepositoryToObjectPoolNotFound.IsEnabled(ctx) {
+			return nil, helper.ErrInternal(err)
+		}
+
 		return nil, helper.ErrInternal(helper.SanitizeError(err))
 	}
 
diff --git a/internal/gitaly/service/objectpool/link_test.go b/internal/gitaly/service/objectpool/link_test.go
index 49d9176a0..bd516a521 100644
--- a/internal/gitaly/service/objectpool/link_test.go
+++ b/internal/gitaly/service/objectpool/link_test.go
@@ -1,6 +1,7 @@
 package objectpool
 
 import (
+	"context"
 	"os"
 	"path/filepath"
 	"testing"
@@ -11,6 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -18,7 +20,10 @@ import (
 )
 
 func TestLink(t *testing.T) {
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.LinkRepositoryToObjectPoolNotFound).Run(t, testLink)
+}
+
+func testLink(t *testing.T, ctx context.Context) {
 	cfg, repo, _, _, client := setup(ctx, t, testserver.WithDisablePraefect())
 
 	localRepo := localrepo.NewTestRepo(t, cfg, repo)
@@ -84,7 +89,10 @@ func TestLink(t *testing.T) {
 }
 
 func TestLinkIdempotent(t *testing.T) {
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.LinkRepositoryToObjectPoolNotFound).Run(t, testLinkIdempotent)
+}
+
+func testLinkIdempotent(t *testing.T, ctx context.Context) {
 	cfg, repoProto, _, _, client := setup(ctx, t)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
@@ -133,7 +141,10 @@ func TestLinkNoClobber(t *testing.T) {
 }
 
 func TestLinkNoPool(t *testing.T) {
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.LinkRepositoryToObjectPoolNotFound).Run(t, testLinkNoPool)
+}
+
+func testLinkNoPool(t *testing.T, ctx context.Context) {
 	cfg, repo, _, locator, client := setup(ctx, t)
 
 	pool := initObjectPool(t, cfg, cfg.Storages[0])
@@ -154,6 +165,12 @@ func TestLinkNoPool(t *testing.T) {
 	}
 
 	_, err = client.LinkRepositoryToObjectPool(ctx, request)
+	if featureflag.LinkRepositoryToObjectPoolNotFound.IsEnabled(ctx) {
+		testhelper.RequireGrpcCode(t, err, codes.NotFound)
+		require.Error(t, err, "GetRepoPath: not a git repository:")
+		return
+	}
+
 	require.NoError(t, err)
 
 	pool = rewrittenObjectPool(ctx, t, cfg, pool)
diff --git a/internal/metadata/featureflag/ff_link_repository_to_object_pool_not_found.go b/internal/metadata/featureflag/ff_link_repository_to_object_pool_not_found.go
new file mode 100644
index 000000000..274ee788c
--- /dev/null
+++ b/internal/metadata/featureflag/ff_link_repository_to_object_pool_not_found.go
@@ -0,0 +1,5 @@
+package featureflag
+
+// LinkRepositoryToObjectPoolNotFound disables the implicit pool creation when linking a repository
+// to an object pool that does not exist.
+var LinkRepositoryToObjectPoolNotFound = NewFeatureFlag("link_repository_to_object_pool_not_found", false)
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 6ddb85128..7a9915a4a 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -26,6 +26,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
@@ -50,9 +51,12 @@ func TestMain(m *testing.M) {
 }
 
 func TestReplMgr_ProcessBacklog(t *testing.T) {
-	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.LinkRepositoryToObjectPoolNotFound).Run(t, testReplMgr_ProcessBacklog)
+}
 
-	ctx := testhelper.Context(t)
+//nolint:revive,stylecheck
+func testReplMgr_ProcessBacklog(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	primaryCfg, testRepoProto, testRepoPath := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))
 	testRepo := localrepo.NewTestRepo(t, primaryCfg, testRepoProto)