Merge branch 'pks-remotes-voting' into 'master'

remote: Vote when adding and removing remotes

See merge request gitlab-org/gitaly!3508

11 files changed, 177 insertions, 6 deletions

diff --git a/changelogs/unreleased/pks-remotes-voting.yml b/changelogs/unreleased/pks-remotes-voting.yml
new file mode 100644
index 000000000..fffe31aed
--- /dev/null
+++ b/changelogs/unreleased/pks-remotes-voting.yml
@@ -0,0 +1,5 @@
+---
+title: 'remote: Vote when adding and removing remotes'
+merge_request: 3508
+author:
+type: added
diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go
index 09871c334..94ee16f6c 100644
--- a/internal/gitaly/service/remote/fetch_internal_remote_test.go
+++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go
@@ -187,6 +187,7 @@ func TestSuccessfulFetchInternalRemote(t *testing.T) {
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 		gitalypb.RegisterHookServiceServer(srv, hook.NewServer(deps.GetCfg(), deps.GetHookManager(), deps.GetGitCmdFactory()))
 	}, testserver.WithHookManager(hookManager), testserver.WithDisablePraefect())
diff --git a/internal/gitaly/service/remote/remotes.go b/internal/gitaly/service/remote/remotes.go
index c99438bf7..95f078e6e 100644
--- a/internal/gitaly/service/remote/remotes.go
+++ b/internal/gitaly/service/remote/remotes.go
@@ -4,11 +4,17 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
+	"gitlab.com/gitlab-org/gitaly/internal/transaction/txinfo"
+	"gitlab.com/gitlab-org/gitaly/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -30,7 +36,20 @@ func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest)
 		return nil, err
 	}
 
-	return client.AddRemote(clientCtx, req)
+	if err := s.voteOnRemote(ctx, req.GetRepository(), req.GetName()); err != nil {
+		return nil, helper.ErrInternalf("preimage vote on remote: %v", err)
+	}
+
+	response, err := client.AddRemote(clientCtx, req)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := s.voteOnRemote(ctx, req.GetRepository(), req.GetName()); err != nil {
+		return nil, helper.ErrInternalf("postimage vote on remote: %v", err)
+	}
+
+	return response, nil
 }
 
 func validateAddRemoteRequest(req *gitalypb.AddRemoteRequest) error {
@@ -60,10 +79,18 @@ func (s *server) RemoveRemote(ctx context.Context, req *gitalypb.RemoveRemoteReq
 		return &gitalypb.RemoveRemoteResponse{Result: false}, nil
 	}
 
+	if err := s.voteOnRemote(ctx, req.GetRepository(), req.GetName()); err != nil {
+		return nil, helper.ErrInternalf("preimage vote on remote: %v", err)
+	}
+
 	if err := remote.Remove(ctx, req.Name); err != nil {
 		return nil, err
 	}
 
+	if err := s.voteOnRemote(ctx, req.GetRepository(), req.GetName()); err != nil {
+		return nil, helper.ErrInternalf("postimage vote on remote: %v", err)
+	}
+
 	return &gitalypb.RemoveRemoteResponse{Result: true}, nil
 }
 
@@ -110,3 +137,37 @@ func validateRemoveRemoteRequest(req *gitalypb.RemoveRemoteRequest) error {
 
 	return nil
 }
+
+func (s *server) voteOnRemote(ctx context.Context, repo *gitalypb.Repository, remoteName string) error {
+	if featureflag.IsDisabled(ctx, featureflag.TxRemote) {
+		return nil
+	}
+
+	return transaction.RunOnContext(ctx, func(tx txinfo.Transaction, server txinfo.PraefectServer) error {
+		localrepo := s.localrepo(repo)
+
+		configEntries, err := localrepo.Config().GetRegexp(ctx, "remote\\."+remoteName+"\\.", git.ConfigGetRegexpOpts{})
+		if err != nil {
+			return fmt.Errorf("get remote configuration: %w", err)
+		}
+
+		hash := voting.NewVoteHash()
+		for _, configEntry := range configEntries {
+			config := fmt.Sprintf("%s\t%s\n", configEntry.Key, configEntry.Value)
+			if _, err := io.WriteString(hash, config); err != nil {
+				return fmt.Errorf("hash remote config entry: %w", err)
+			}
+		}
+
+		vote, err := hash.Vote()
+		if err != nil {
+			return fmt.Errorf("compute remote config vote: %w", err)
+		}
+
+		if err := s.txManager.Vote(ctx, tx, server, vote); err != nil {
+			return fmt.Errorf("vote: %w", err)
+		}
+
+		return nil
+	})
+}
diff --git a/internal/gitaly/service/remote/remotes_test.go b/internal/gitaly/service/remote/remotes_test.go
index c1e7ae7b4..3ba800e5e 100644
--- a/internal/gitaly/service/remote/remotes_test.go
+++ b/internal/gitaly/service/remote/remotes_test.go
@@ -2,6 +2,7 @@ package remote
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"io"
 	"net/http"
@@ -12,7 +13,14 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/transaction"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper/testserver"
+	"gitlab.com/gitlab-org/gitaly/internal/transaction/txinfo"
+	"gitlab.com/gitlab-org/gitaly/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 )
@@ -94,6 +102,47 @@ func testSuccessfulAddRemote(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.S
 	}
 }
 
+func testAddRemoteTransactional(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.TxRemote,
+	}).Run(t, func(t *testing.T, ctx context.Context) {
+		var votes []voting.Vote
+		txManager := transaction.MockManager{
+			VoteFn: func(_ context.Context, _ txinfo.Transaction, _ txinfo.PraefectServer, vote voting.Vote) error {
+				votes = append(votes, vote)
+				return nil
+			},
+		}
+
+		_, repo, repoPath, client := setupRemoteServiceWithRuby(t, cfg, rubySrv, testserver.WithTransactionManager(&txManager))
+
+		ctx, err := (&txinfo.PraefectServer{SocketPath: "i-dont-care"}).Inject(ctx)
+		require.NoError(t, err)
+		ctx, err = txinfo.InjectTransaction(ctx, 1, "node", true)
+		require.NoError(t, err)
+		ctx = helper.IncomingToOutgoing(ctx)
+
+		preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
+
+		_, err = client.AddRemote(ctx, &gitalypb.AddRemoteRequest{
+			Repository: repo,
+			Name:       "origin",
+			Url:        "foo/bar",
+		})
+		require.NoError(t, err)
+
+		if featureflag.IsEnabled(ctx, featureflag.TxRemote) {
+			preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
+			require.Equal(t, []voting.Vote{
+				voting.VoteFromData([]byte(preimageVote)),
+				voting.VoteFromData([]byte("remote.origin.url\tfoo/bar\n")),
+			}, votes)
+		} else {
+			require.Len(t, votes, 0)
+		}
+	})
+}
+
 func TestFailedAddRemoteDueToValidation(t *testing.T) {
 	_, repo, _, client := setupRemoteService(t)
 
@@ -189,6 +238,46 @@ func TestFailedRemoveRemoteDueToValidation(t *testing.T) {
 	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
 }
 
+func TestRemoveRemoteTransactional(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.TxRemote,
+	}).Run(t, func(t *testing.T, ctx context.Context) {
+		var votes []voting.Vote
+		txManager := transaction.MockManager{
+			VoteFn: func(_ context.Context, _ txinfo.Transaction, _ txinfo.PraefectServer, vote voting.Vote) error {
+				votes = append(votes, vote)
+				return nil
+			},
+		}
+
+		cfg, repo, repoPath, client := setupRemoteService(t, testserver.WithTransactionManager(&txManager))
+
+		ctx, err := (&txinfo.PraefectServer{SocketPath: "i-dont-care"}).Inject(ctx)
+		require.NoError(t, err)
+		ctx, err = txinfo.InjectTransaction(ctx, 1, "node", true)
+		require.NoError(t, err)
+		ctx = helper.IncomingToOutgoing(ctx)
+
+		preimageURL := text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "remote", "get-url", "origin"))
+
+		_, err = client.RemoveRemote(ctx, &gitalypb.RemoveRemoteRequest{
+			Repository: repo,
+			Name:       "origin",
+		})
+		require.NoError(t, err)
+
+		if featureflag.IsEnabled(ctx, featureflag.TxRemote) {
+			preimageVote := fmt.Sprintf("remote.origin.url\t%s\n", preimageURL)
+			require.Equal(t, []voting.Vote{
+				voting.VoteFromData([]byte(preimageVote)),
+				voting.VoteFromData([]byte{}),
+			}, votes)
+		} else {
+			require.Len(t, votes, 0)
+		}
+	})
+}
+
 func TestFindRemoteRepository(t *testing.T) {
 	_, repo, _, client := setupRemoteService(t)
 
diff --git a/internal/gitaly/service/remote/server.go b/internal/gitaly/service/remote/server.go
index b8b0ffae4..6ae0fb04b 100644
--- a/internal/gitaly/service/remote/server.go
+++ b/internal/gitaly/service/remote/server.go
@@ -8,6 +8,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
@@ -18,6 +19,7 @@ type server struct {
 	locator       storage.Locator
 	gitCmdFactory git.CommandFactory
 	catfileCache  catfile.Cache
+	txManager     transaction.Manager
 
 	conns *client.Pool
 }
@@ -29,6 +31,7 @@ func NewServer(
 	locator storage.Locator,
 	gitCmdFactory git.CommandFactory,
 	catfileCache catfile.Cache,
+	txManager transaction.Manager,
 ) gitalypb.RemoteServiceServer {
 	return &server{
 		cfg:           cfg,
@@ -36,6 +39,7 @@ func NewServer(
 		locator:       locator,
 		gitCmdFactory: gitCmdFactory,
 		catfileCache:  catfileCache,
+		txManager:     txManager,
 		conns: client.NewPoolWithOptions(
 			client.WithDialer(client.HealthCheckDialer(client.DialContext)),
 			client.WithDialOptions(client.FailOnNonTempDialError()...),
diff --git a/internal/gitaly/service/remote/testhelper_test.go b/internal/gitaly/service/remote/testhelper_test.go
index 416c8ad0f..942a18b23 100644
--- a/internal/gitaly/service/remote/testhelper_test.go
+++ b/internal/gitaly/service/remote/testhelper_test.go
@@ -44,6 +44,7 @@ func TestWithRubySidecar(t *testing.T) {
 		testSuccessfulUpdateRemoteMirrorRequestWithKeepDivergentRefs,
 		testFailedUpdateRemoteMirrorRequestDueToValidation,
 		testSuccessfulAddRemote,
+		testAddRemoteTransactional,
 		testUpdateRemoteMirror,
 	}
 
@@ -54,7 +55,7 @@ func TestWithRubySidecar(t *testing.T) {
 	}
 }
 
-func setupRemoteServiceWithRuby(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) (config.Cfg, *gitalypb.Repository, string, gitalypb.RemoteServiceClient) {
+func setupRemoteServiceWithRuby(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server, opts ...testserver.GitalyServerOpt) (config.Cfg, *gitalypb.Repository, string, gitalypb.RemoteServiceClient) {
 	t.Helper()
 
 	repo, repoPath, cleanup := gittest.CloneRepoAtStorage(t, cfg, cfg.Storages[0], t.Name())
@@ -67,8 +68,9 @@ func setupRemoteServiceWithRuby(t *testing.T, cfg config.Cfg, rubySrv *rubyserve
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
-	})
+	}, opts...)
 	cfg.SocketPath = addr
 
 	client, conn := newRemoteClient(t, addr)
@@ -77,11 +79,11 @@ func setupRemoteServiceWithRuby(t *testing.T, cfg config.Cfg, rubySrv *rubyserve
 	return cfg, repo, repoPath, client
 }
 
-func setupRemoteService(t *testing.T) (config.Cfg, *gitalypb.Repository, string, gitalypb.RemoteServiceClient) {
+func setupRemoteService(t *testing.T, opts ...testserver.GitalyServerOpt) (config.Cfg, *gitalypb.Repository, string, gitalypb.RemoteServiceClient) {
 	t.Helper()
 
 	cfg := testcfg.Build(t)
-	return setupRemoteServiceWithRuby(t, cfg, nil)
+	return setupRemoteServiceWithRuby(t, cfg, nil, opts...)
 }
 
 func newRemoteClient(t *testing.T, serverSocketPath string) (gitalypb.RemoteServiceClient, *grpc.ClientConn) {
diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go
index 6266d60ef..705ea747f 100644
--- a/internal/gitaly/service/remote/update_remote_mirror_test.go
+++ b/internal/gitaly/service/remote/update_remote_mirror_test.go
@@ -474,6 +474,7 @@ func testSuccessfulUpdateRemoteMirrorRequestFeatured(t *testing.T, ctx context.C
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 	})
 
@@ -580,6 +581,7 @@ func testSuccessfulUpdateRemoteMirrorRequestWithWildcardsFeatured(t *testing.T,
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 	})
 
@@ -670,6 +672,7 @@ func testSuccessfulUpdateRemoteMirrorRequestWithKeepDivergentRefsFeatured(t *tes
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 	})
 
@@ -762,6 +765,7 @@ func testFailedUpdateRemoteMirrorRequestDueToValidationFeatured(t *testing.T, ct
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 	})
 
diff --git a/internal/gitaly/service/repository/fork_test.go b/internal/gitaly/service/repository/fork_test.go
index fd4a3f66b..76eca1e42 100644
--- a/internal/gitaly/service/repository/fork_test.go
+++ b/internal/gitaly/service/repository/fork_test.go
@@ -226,7 +226,7 @@ func runSecureServer(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) s
 
 	gitalypb.RegisterRepositoryServiceServer(server, NewServer(cfg, rubySrv, locator, txManager, gitCmdFactory, catfileCache))
 	gitalypb.RegisterHookServiceServer(server, hookservice.NewServer(cfg, hookManager, gitCmdFactory))
-	gitalypb.RegisterRemoteServiceServer(server, remote.NewServer(cfg, rubySrv, locator, gitCmdFactory, catfileCache))
+	gitalypb.RegisterRemoteServiceServer(server, remote.NewServer(cfg, rubySrv, locator, gitCmdFactory, catfileCache, txManager))
 	gitalypb.RegisterSSHServiceServer(server, ssh.NewServer(cfg, locator, gitCmdFactory, txManager))
 	gitalypb.RegisterRefServiceServer(server, ref.NewServer(cfg, locator, gitCmdFactory, txManager, catfileCache))
 	gitalypb.RegisterCommitServiceServer(server, commit.NewServer(cfg, locator, gitCmdFactory, nil, catfileCache))
diff --git a/internal/gitaly/service/repository/testhelper_test.go b/internal/gitaly/service/repository/testhelper_test.go
index 693f0b083..b6be1dfbc 100644
--- a/internal/gitaly/service/repository/testhelper_test.go
+++ b/internal/gitaly/service/repository/testhelper_test.go
@@ -136,6 +136,7 @@ func runRepositoryServerWithConfig(t testing.TB, cfg config.Cfg, rubySrv *rubyse
 			deps.GetLocator(),
 			deps.GetGitCmdFactory(),
 			deps.GetCatfileCache(),
+			deps.GetTxManager(),
 		))
 		gitalypb.RegisterSSHServiceServer(srv, ssh.NewServer(
 			cfg,
diff --git a/internal/gitaly/service/setup/register.go b/internal/gitaly/service/setup/register.go
index 8b0bfe969..88adac4ff 100644
--- a/internal/gitaly/service/setup/register.go
+++ b/internal/gitaly/service/setup/register.go
@@ -129,6 +129,7 @@ func RegisterAll(srv *grpc.Server, deps *service.Dependencies) {
 		deps.GetLocator(),
 		deps.GetGitCmdFactory(),
 		deps.GetCatfileCache(),
+		deps.GetTxManager(),
 	))
 	gitalypb.RegisterServerServiceServer(srv, server.NewServer(deps.GetGitCmdFactory(), deps.GetCfg().Storages))
 	gitalypb.RegisterObjectPoolServiceServer(srv, objectpool.NewServer(
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 7c6d615cb..548f33a68 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -25,6 +25,8 @@ var (
 	FetchInternalRemoteErrors = FeatureFlag{Name: "fetch_internal_remote_errors", OnByDefault: false}
 	// TxConfig enables transactional voting for SetConfig and DeleteConfig RPCs.
 	TxConfig = FeatureFlag{Name: "tx_config", OnByDefault: false}
+	// TxRemote enables transactional voting for AddRemote and DeleteRemote.
+	TxRemote = FeatureFlag{Name: "tx_remote", OnByDefault: false}
 )
 
 // All includes all feature flags.
@@ -37,4 +39,5 @@ var All = []FeatureFlag{
 	GoUpdateRemoteMirror,
 	FetchInternalRemoteErrors,
 	TxConfig,
+	TxRemote,
 }