Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitaly.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPaul Okstad <pokstad@gitlab.com>2020-11-04 19:33:36 +0300
committerPaul Okstad <pokstad@gitlab.com>2020-11-04 19:33:36 +0300
commitfa974a4ab21aa6acc4c3a00456265248a4d70703 (patch)
tree398ac938da62adf1291e9f15779e80a0cc959b62
parente316a7047df16281e067bd7a0dcb90de602444cd (diff)
parenta77f28ee61f3238c10769ddbd6e428debadfc933 (diff)
Merge branch 'pks-featureset-refactoring' into 'master'
Quality of life improvements for feature sets See merge request gitlab-org/gitaly!2740
-rw-r--r--README.md2
-rw-r--r--REVIEWING.md16
-rw-r--r--cmd/gitaly-git2go/conflicts/conflicts.go (renamed from cmd/gitaly-git2go/conflicts.go)9
-rw-r--r--cmd/gitaly-git2go/conflicts/conflicts_test.go (renamed from cmd/gitaly-git2go/conflicts_test.go)17
-rw-r--r--cmd/gitaly-git2go/main.go4
-rw-r--r--cmd/gitaly-git2go/merge_test.go44
-rw-r--r--cmd/gitaly-git2go/revert_test.go17
-rw-r--r--cmd/gitaly-git2go/testhelper/testhelper.go48
-rw-r--r--cmd/gitaly-hooks/hooks.go42
-rw-r--r--cmd/gitaly-hooks/hooks_test.go88
-rw-r--r--internal/git/hooks.go76
-rw-r--r--internal/git/protocol.go12
-rw-r--r--internal/git/protocol_test.go10
-rw-r--r--internal/git/receivepack.go66
-rw-r--r--internal/gitaly/service/smarthttp/inforefs.go6
-rw-r--r--internal/gitaly/service/smarthttp/receive_pack.go14
-rw-r--r--internal/gitaly/service/smarthttp/upload_pack.go6
-rw-r--r--internal/gitaly/service/ssh/monitor_stdin_command.go4
-rw-r--r--internal/gitaly/service/ssh/receive_pack.go13
-rw-r--r--internal/gitaly/service/ssh/upload_pack.go7
-rw-r--r--internal/praefect/coordinator.go6
-rw-r--r--internal/praefect/router.go54
-rw-r--r--proto/README.md10
23 files changed, 251 insertions, 320 deletions
diff --git a/README.md b/README.md
index 0bb1031f2..544e298c7 100644
--- a/README.md
+++ b/README.md
@@ -58,7 +58,7 @@ Gitaly requires Go 1.13.9 or newer and Ruby 2.6. Run `make` to download
and compile Ruby dependencies, and to compile the Gitaly Go
executable.
-Gitaly uses `git`. Versions `2.26.x` and `2.27.x` are supported.
+Gitaly uses `git`. Versions `2.29.0` and newer are supported.
## Configuration
diff --git a/REVIEWING.md b/REVIEWING.md
index 0fe454bc6..9b23711a3 100644
--- a/REVIEWING.md
+++ b/REVIEWING.md
@@ -24,20 +24,20 @@ outage, because causing an outage is not the right thing to do.
### Tips for the Contributor
-As the contributor you have a dual role. You are driving the change in the MR, but you
+As the contributor you have a dual role. You are driving the change in the MR, but you
are also the **first reviewer** of the MR. Below you will find some tips for reviewing
your own MR. Doing this costs time, but it should also save time for the other
reviewers and thereby speed up the acceptance of your MR.
#### Use the GitLab MR page to put on your "reviewer hat"
-It is sometimes hard to switch from being the contributor, to being a reviewer
+It is sometimes hard to switch from being the contributor, to being a reviewer
of your own work. Looking at your work in the GitLab UI (the merge request page)
helps to get in the reviewer mindset.
#### Reconsider the title of your MR after you reviewed it
-When you are in the contributor mindset, you don't always know
+When you are in the contributor mindset, you don't always know
what you are doing, or why. You discover this as you go along. The title you wrote for
your MR when you first pushed it is often not the best title for what is really
going on.
@@ -64,7 +64,7 @@ It may feel funny to literally talk to yourself but it works. If
thoughts occur to you as you read your MR, use the comment function
and just write them down.
-You probably want to address some or most of your comments before you send your
+You probably want to address some or most of your comments before you send your
MR to another reviewer.
#### Your MR should pass your own review before it goes to a "real" reviewer
@@ -83,7 +83,7 @@ MR to another reviewer.
#### Use the "Start/Submit Review" feature of GitLab
-The "Start/Submit Review" lets you write comments on a MR that are initially
+The "Start/Submit Review" lets you write comments on a MR that are initially
only visible to you. Until you submit the review as a whole, you can still add,
change and remove comments.
@@ -105,7 +105,7 @@ You finished a review round and you are about to submit your review with the
"Submit Review" feature. Look at your comments. Do some of them point at a major
problem in the MR?
-For example:
+For example:
- the MR is solving the wrong problem
- the MR is making a backwards incompatible change
- the MR has a test that does not test the right thing
@@ -154,6 +154,10 @@ this" is a very important signal. Put it into words, write a comment.
Think out loud about why you don't understand the thing that gives you
this feeling.
+For example, if you do not understand what an RPC is supposed to do or
+why it exists, ask the reviewer to
+[add comments to the interface](proto/README.md#documentation).
+
Maybe now is the time to spend 15-30 minutes brushing up your
knowledge on this subject. Or to ask a colleague if they know more
about this. Or to bring in another reviewer who knows more about this.
diff --git a/cmd/gitaly-git2go/conflicts.go b/cmd/gitaly-git2go/conflicts/conflicts.go
index 85f3c5a9c..ac29e68cf 100644
--- a/cmd/gitaly-git2go/conflicts.go
+++ b/cmd/gitaly-git2go/conflicts/conflicts.go
@@ -1,6 +1,6 @@
// +build static,system_libgit2
-package main
+package conflicts
import (
"context"
@@ -17,11 +17,12 @@ import (
"google.golang.org/grpc/status"
)
-type conflictsSubcommand struct {
+// Subcommand contains params to performs conflicts calculation from main
+type Subcommand struct {
request string
}
-func (cmd *conflictsSubcommand) Flags() *flag.FlagSet {
+func (cmd *Subcommand) Flags() *flag.FlagSet {
flags := flag.NewFlagSet("conflicts", flag.ExitOnError)
flags.StringVar(&cmd.request, "request", "", "git2go.ConflictsCommand")
return flags
@@ -83,7 +84,7 @@ func conflictError(code codes.Code, message string) error {
}
// Run performs a merge and prints resulting conflicts to stdout.
-func (cmd *conflictsSubcommand) Run(context.Context, io.Reader, io.Writer) error {
+func (cmd *Subcommand) Run(context.Context, io.Reader, io.Writer) error {
request, err := git2go.ConflictsCommandFromSerialized(cmd.request)
if err != nil {
return err
diff --git a/cmd/gitaly-git2go/conflicts_test.go b/cmd/gitaly-git2go/conflicts/conflicts_test.go
index 4cef16c12..f1bf44ed0 100644
--- a/cmd/gitaly-git2go/conflicts_test.go
+++ b/cmd/gitaly-git2go/conflicts/conflicts_test.go
@@ -1,16 +1,25 @@
// +build static,system_libgit2
-package main
+package conflicts
import (
+ "os"
"testing"
"github.com/stretchr/testify/require"
+ cmdtesthelper "gitlab.com/gitlab-org/gitaly/cmd/gitaly-git2go/testhelper"
"gitlab.com/gitlab-org/gitaly/internal/git2go"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
)
+func TestMain(m *testing.M) {
+ defer testhelper.MustHaveNoChildProcess()
+ testhelper.Configure()
+ testhelper.ConfigureGitalyGit2Go()
+ os.Exit(m.Run())
+}
+
func TestConflicts(t *testing.T) {
testcases := []struct {
desc string
@@ -165,9 +174,9 @@ func TestConflicts(t *testing.T) {
_, repoPath, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
- base := buildCommit(t, repoPath, nil, tc.base)
- ours := buildCommit(t, repoPath, base, tc.ours)
- theirs := buildCommit(t, repoPath, base, tc.theirs)
+ base := cmdtesthelper.BuildCommit(t, repoPath, nil, tc.base)
+ ours := cmdtesthelper.BuildCommit(t, repoPath, base, tc.ours)
+ theirs := cmdtesthelper.BuildCommit(t, repoPath, base, tc.theirs)
t.Run(tc.desc, func(t *testing.T) {
ctx, cancel := testhelper.Context()
diff --git a/cmd/gitaly-git2go/main.go b/cmd/gitaly-git2go/main.go
index 68c9a8a0b..488300ba2 100644
--- a/cmd/gitaly-git2go/main.go
+++ b/cmd/gitaly-git2go/main.go
@@ -8,6 +8,8 @@ import (
"fmt"
"io"
"os"
+
+ "gitlab.com/gitlab-org/gitaly/cmd/gitaly-git2go/conflicts"
)
type subcmd interface {
@@ -16,8 +18,8 @@ type subcmd interface {
}
var subcommands = map[string]subcmd{
- "conflicts": &conflictsSubcommand{},
"commit": commitSubcommand{},
+ "conflicts": &conflicts.Subcommand{},
"merge": &mergeSubcommand{},
"revert": &revertSubcommand{},
"resolve": &resolveSubcommand{},
diff --git a/cmd/gitaly-git2go/merge_test.go b/cmd/gitaly-git2go/merge_test.go
index c201560a0..0a2b6bbab 100644
--- a/cmd/gitaly-git2go/merge_test.go
+++ b/cmd/gitaly-git2go/merge_test.go
@@ -9,6 +9,7 @@ import (
git "github.com/libgit2/git2go/v30"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+ cmdtesthelper "gitlab.com/gitlab-org/gitaly/cmd/gitaly-git2go/testhelper"
"gitlab.com/gitlab-org/gitaly/internal/git2go"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -82,43 +83,6 @@ func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) {
require.Contains(t, err.Error(), "merge: could not open repository")
}
-func buildCommit(t testing.TB, repoPath string, parent *git.Oid, fileContents map[string]string) *git.Oid {
- repo, err := git.OpenRepository(repoPath)
- require.NoError(t, err)
- defer repo.Free()
-
- odb, err := repo.Odb()
- require.NoError(t, err)
-
- treeBuilder, err := repo.TreeBuilder()
- require.NoError(t, err)
-
- for file, contents := range fileContents {
- oid, err := odb.Write([]byte(contents), git.ObjectBlob)
- require.NoError(t, err)
- treeBuilder.Insert(file, oid, git.FilemodeBlob)
- }
-
- tree, err := treeBuilder.Write()
- require.NoError(t, err)
-
- committer := git.Signature{
- Name: "Foo",
- Email: "foo@example.com",
- When: time.Date(2020, 1, 1, 1, 1, 1, 1, time.FixedZone("UTC+2", 2*60*60)),
- }
-
- var commit *git.Oid
- if parent != nil {
- commit, err = repo.CreateCommitFromIds("", &committer, &committer, "Message", tree, parent)
- } else {
- commit, err = repo.CreateCommitFromIds("", &committer, &committer, "Message", tree)
- }
- require.NoError(t, err)
-
- return commit
-}
-
func TestMergeTrees(t *testing.T) {
testcases := []struct {
desc string
@@ -210,9 +174,9 @@ func TestMergeTrees(t *testing.T) {
_, repoPath, cleanup := testhelper.NewTestRepo(t)
defer cleanup()
- base := buildCommit(t, repoPath, nil, tc.base)
- ours := buildCommit(t, repoPath, base, tc.ours)
- theirs := buildCommit(t, repoPath, base, tc.theirs)
+ base := cmdtesthelper.BuildCommit(t, repoPath, nil, tc.base)
+ ours := cmdtesthelper.BuildCommit(t, repoPath, base, tc.ours)
+ theirs := cmdtesthelper.BuildCommit(t, repoPath, base, tc.theirs)
authorDate := time.Date(2020, 7, 30, 7, 45, 50, 0, time.FixedZone("UTC+2", +2*60*60))
diff --git a/cmd/gitaly-git2go/revert_test.go b/cmd/gitaly-git2go/revert_test.go
index 6c62b9f93..8e6036baa 100644
--- a/cmd/gitaly-git2go/revert_test.go
+++ b/cmd/gitaly-git2go/revert_test.go
@@ -9,6 +9,7 @@ import (
git "github.com/libgit2/git2go/v30"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+ cmdtesthelper "gitlab.com/gitlab-org/gitaly/cmd/gitaly-git2go/testhelper"
"gitlab.com/gitlab-org/gitaly/internal/git2go"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -81,15 +82,15 @@ func TestRevert_trees(t *testing.T) {
{
desc: "trivial revert succeeds",
setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
- baseOid := buildCommit(t, repoPath, nil, map[string]string{
+ baseOid := cmdtesthelper.BuildCommit(t, repoPath, nil, map[string]string{
"a": "apple",
"b": "banana",
})
- revertOid := buildCommit(t, repoPath, baseOid, map[string]string{
+ revertOid := cmdtesthelper.BuildCommit(t, repoPath, baseOid, map[string]string{
"a": "apple",
"b": "pineapple",
})
- oursOid := buildCommit(t, repoPath, revertOid, map[string]string{
+ oursOid := cmdtesthelper.BuildCommit(t, repoPath, revertOid, map[string]string{
"a": "apple",
"b": "pineapple",
"c": "carrot",
@@ -109,13 +110,13 @@ func TestRevert_trees(t *testing.T) {
{
desc: "conflicting revert fails",
setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
- baseOid := buildCommit(t, repoPath, nil, map[string]string{
+ baseOid := cmdtesthelper.BuildCommit(t, repoPath, nil, map[string]string{
"a": "apple",
})
- revertOid := buildCommit(t, repoPath, baseOid, map[string]string{
+ revertOid := cmdtesthelper.BuildCommit(t, repoPath, baseOid, map[string]string{
"a": "pineapple",
})
- oursOid := buildCommit(t, repoPath, revertOid, map[string]string{
+ oursOid := cmdtesthelper.BuildCommit(t, repoPath, revertOid, map[string]string{
"a": "carrot",
})
@@ -126,7 +127,7 @@ func TestRevert_trees(t *testing.T) {
{
desc: "nonexistent ours fails",
setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
- revertOid := buildCommit(t, repoPath, nil, map[string]string{
+ revertOid := cmdtesthelper.BuildCommit(t, repoPath, nil, map[string]string{
"a": "apple",
})
@@ -137,7 +138,7 @@ func TestRevert_trees(t *testing.T) {
{
desc: "nonexistent revert fails",
setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
- oursOid := buildCommit(t, repoPath, nil, map[string]string{
+ oursOid := cmdtesthelper.BuildCommit(t, repoPath, nil, map[string]string{
"a": "apple",
})
diff --git a/cmd/gitaly-git2go/testhelper/testhelper.go b/cmd/gitaly-git2go/testhelper/testhelper.go
new file mode 100644
index 000000000..3a7a24b0d
--- /dev/null
+++ b/cmd/gitaly-git2go/testhelper/testhelper.go
@@ -0,0 +1,48 @@
+// +build static,system_libgit2
+
+package testhelper
+
+import (
+ "testing"
+ "time"
+
+ git "github.com/libgit2/git2go/v30"
+ "github.com/stretchr/testify/require"
+)
+
+func BuildCommit(t testing.TB, repoPath string, parent *git.Oid, fileContents map[string]string) *git.Oid {
+ repo, err := git.OpenRepository(repoPath)
+ require.NoError(t, err)
+ defer repo.Free()
+
+ odb, err := repo.Odb()
+ require.NoError(t, err)
+
+ treeBuilder, err := repo.TreeBuilder()
+ require.NoError(t, err)
+
+ for file, contents := range fileContents {
+ oid, err := odb.Write([]byte(contents), git.ObjectBlob)
+ require.NoError(t, err)
+ treeBuilder.Insert(file, oid, git.FilemodeBlob)
+ }
+
+ tree, err := treeBuilder.Write()
+ require.NoError(t, err)
+
+ committer := git.Signature{
+ Name: "Foo",
+ Email: "foo@example.com",
+ When: time.Date(2020, 1, 1, 1, 1, 1, 1, time.FixedZone("UTC+2", 2*60*60)),
+ }
+
+ var commit *git.Oid
+ if parent != nil {
+ commit, err = repo.CreateCommitFromIds("", &committer, &committer, "Message", tree, parent)
+ } else {
+ commit, err = repo.CreateCommitFromIds("", &committer, &committer, "Message", tree)
+ }
+ require.NoError(t, err)
+
+ return commit
+}
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 3c44b80ba..c4cbf3bef 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -27,32 +27,6 @@ import (
"google.golang.org/grpc"
)
-// fixupReftxHook fixes up the subcommand in case case the wrong hook was executed instead of the
-// reference-transaction hook. This bug was introduced together with the reference-transaction hook
-// in Git v2.28.0. As pre-receive and post-receive don't have any arguments and update has the fully
-// qualified reference as first argument, none should ever be invoked with the verbs that the
-// reference-transaction hook receives.
-func fixupReftxHook(args []string) []string {
- if len(args) != 3 {
- return args
- }
-
- name := args[1]
- if name != "pre-receive" && name != "post-receive" && name != "update" {
- return args
- }
-
- arg := args[2]
- if arg != "prepared" && arg != "committed" && arg != "aborted" {
- return args
- }
-
- return append(
- []string{args[0], "reference-transaction"},
- args[2:]...,
- )
-}
-
func main() {
var logger = gitalylog.NewHookLogger()
@@ -60,17 +34,15 @@ func main() {
logger.Fatalf("requires hook name. args: %v", os.Args)
}
- fixedArgs := fixupReftxHook(os.Args)
-
- subCmd := fixedArgs[1]
+ subCmd := os.Args[1]
if subCmd == "check" {
logrus.SetLevel(logrus.ErrorLevel)
- if len(fixedArgs) != 3 {
+ if len(os.Args) != 3 {
logger.Fatal(errors.New("no configuration file path provided invoke with: gitaly-hooks check <config_path>"))
}
- configPath := fixedArgs[2]
+ configPath := os.Args[2]
fmt.Print("Checking GitLab API access: ")
info, err := check(configPath)
@@ -113,7 +85,7 @@ func main() {
switch subCmd {
case "update":
- args := fixedArgs[2:]
+ args := os.Args[2:]
if len(args) != 3 {
logger.Fatalf("hook %q expects exactly three arguments", subCmd)
}
@@ -205,12 +177,12 @@ func main() {
os.Exit(0)
}
- if len(fixedArgs) != 3 {
+ if len(os.Args) != 3 {
logger.Fatalf("hook %q is missing required arguments", subCmd)
}
var state gitalypb.ReferenceTransactionHookRequest_State
- switch fixedArgs[2] {
+ switch os.Args[2] {
case "prepared":
state = gitalypb.ReferenceTransactionHookRequest_PREPARED
case "committed":
@@ -218,7 +190,7 @@ func main() {
case "aborted":
state = gitalypb.ReferenceTransactionHookRequest_ABORTED
default:
- logger.Fatalf("hook %q has invalid state %s", subCmd, fixedArgs[2])
+ logger.Fatalf("hook %q has invalid state %s", subCmd, os.Args[2])
}
referenceTransactionHookStream, err := hookClient.ReferenceTransactionHook(ctx)
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index 91f73073a..f2c570708 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -50,94 +50,6 @@ func testMain(m *testing.M) int {
return m.Run()
}
-func TestReftxHookFixup(t *testing.T) {
- testcases := []struct {
- desc string
- args []string
- fixed bool
- }{
- {
- desc: "empty arguments",
- args: []string{},
- fixed: false,
- },
- {
- desc: "single argument",
- args: []string{"gitaly-hooks"},
- fixed: false,
- },
- {
- desc: "check",
- args: []string{"gitaly-hooks", "check"},
- fixed: false,
- },
- {
- desc: "check with arguments",
- args: []string{"gitaly-hooks", "check", "foo", "bar"},
- fixed: false,
- },
- {
- desc: "pre-receive hook without arguments",
- args: []string{"gitaly-hooks", "pre-receive"},
- fixed: false,
- },
- {
- desc: "post-receive hook without arguments",
- args: []string{"gitaly-hooks", "post-receive"},
- fixed: false,
- },
- {
- desc: "update hook without arguments",
- args: []string{"gitaly-hooks", "update"},
- fixed: false,
- },
- {
- desc: "update with typical arguments",
- args: []string{"gitaly-hooks", "update", "refs/heads/master", "foo", "bar"},
- fixed: false,
- },
- {
- desc: "update with prepared argument",
- args: []string{"gitaly-hooks", "update", "prepared", "foo"},
- fixed: false,
- },
- {
- desc: "wrongly called update hook",
- args: []string{"gitaly-hooks", "update", "prepared"},
- fixed: true,
- },
- {
- desc: "wrongly called pre-receive hook",
- args: []string{"gitaly-hooks", "pre-receive", "committed"},
- fixed: true,
- },
- {
- desc: "wrongly called post-receive hook",
- args: []string{"gitaly-hooks", "post-receive", "aborted"},
- fixed: true,
- },
- {
- desc: "unrelated hook",
- args: []string{"gitaly-hooks", "pre-commit", "prepared"},
- fixed: false,
- },
- }
-
- for _, tc := range testcases {
- t.Run(tc.desc, func(t *testing.T) {
- actual := fixupReftxHook(tc.args)
-
- expected := make([]string, len(tc.args))
- copy(expected, tc.args)
- if tc.fixed {
- expected[1] = "reference-transaction"
- }
-
- require.Equal(t, actual, expected)
- })
- }
-}
-
func TestHooksPrePostWithSymlinkedStoragePath(t *testing.T) {
defer func(cfg config.Cfg) {
config.Config = cfg
diff --git a/internal/git/hooks.go b/internal/git/hooks.go
index faf98281a..0629d052d 100644
--- a/internal/git/hooks.go
+++ b/internal/git/hooks.go
@@ -2,13 +2,19 @@ package git
import (
"context"
+ "errors"
"fmt"
+ "github.com/golang/protobuf/jsonpb"
"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
+ "gitlab.com/gitlab-org/gitaly/internal/gitlabshell"
"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
+ "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
)
+var jsonpbMarshaller = &jsonpb.Marshaler{}
+
// WithRefTxHook returns an option that populates the safe command with the
// environment variables necessary to properly execute a reference hook for
// repository changes that may possibly update references
@@ -58,3 +64,73 @@ func RequireRefHook(ctx context.Context) context.Context {
func IsRefHookRequired(ctx context.Context) bool {
return ctx.Value(refHookRequired{}) != nil
}
+
+// ReceivePackRequest abstracts away the different requests that end up
+// spawning git-receive-pack.
+type ReceivePackRequest interface {
+ GetGlId() string
+ GetGlUsername() string
+ GetGlRepository() string
+ GetRepository() *gitalypb.Repository
+}
+
+// WithReceivePackHooks returns an option that populates the safe command with the environment
+// variables necessary to properly execute the pre-receive, update and post-receive hooks for
+// git-receive-pack(1).
+func WithReceivePackHooks(ctx context.Context, req ReceivePackRequest, protocol string) CmdOpt {
+ return func(cc *cmdCfg) error {
+ env, err := receivePackHookEnv(ctx, req, protocol)
+ if err != nil {
+ return fmt.Errorf("receive-pack hook envvars: %w", err)
+ }
+
+ cc.env = append(cc.env, env...)
+ return nil
+ }
+}
+
+func receivePackHookEnv(ctx context.Context, req ReceivePackRequest, protocol string) ([]string, error) {
+ gitlabshellEnv, err := gitlabshell.Env()
+ if err != nil {
+ return nil, err
+ }
+
+ env, err := refHookEnv(ctx, req.GetRepository(), config.Config)
+ if err != nil {
+ return nil, err
+ }
+
+ env = append(env,
+ fmt.Sprintf("GL_ID=%s", req.GetGlId()),
+ fmt.Sprintf("GL_USERNAME=%s", req.GetGlUsername()),
+ fmt.Sprintf("GL_REPOSITORY=%s", req.GetGlRepository()),
+ fmt.Sprintf("GL_PROJECT_PATH=%s", req.GetRepository().GetGlProjectPath()),
+ fmt.Sprintf("GL_PROTOCOL=%s", protocol),
+ fmt.Sprintf("%s=true", featureflag.ReferenceTransactionHookEnvVar),
+ )
+ env = append(env, gitlabshellEnv...)
+
+ transaction, err := metadata.TransactionFromContext(ctx)
+ if err == nil {
+ praefect, err := metadata.PraefectFromContext(ctx)
+ if err != nil {
+ return nil, err
+ }
+
+ praefectEnv, err := praefect.Env()
+ if err != nil {
+ return nil, err
+ }
+
+ transactionEnv, err := transaction.Env()
+ if err != nil {
+ return nil, err
+ }
+
+ env = append(env, praefectEnv, transactionEnv)
+ } else if !errors.Is(err, metadata.ErrTransactionNotFound) {
+ return nil, err
+ }
+
+ return env, nil
+}
diff --git a/internal/git/protocol.go b/internal/git/protocol.go
index 6b0541027..a562db783 100644
--- a/internal/git/protocol.go
+++ b/internal/git/protocol.go
@@ -34,10 +34,18 @@ type RequestWithGitProtocol interface {
GetGitProtocol() string
}
-// AddGitProtocolEnv checks whether the request has Git protocol v2
+// WithGitProtocol checks whether the request has Git protocol v2
// and sets this in the environment.
-func AddGitProtocolEnv(ctx context.Context, req RequestWithGitProtocol, env []string) []string {
+func WithGitProtocol(ctx context.Context, req RequestWithGitProtocol) CmdOpt {
+ return func(cc *cmdCfg) error {
+ cc.env = append(cc.env, gitProtocolEnv(ctx, req)...)
+ return nil
+ }
+}
+
+func gitProtocolEnv(ctx context.Context, req RequestWithGitProtocol) []string {
var protocol string
+ var env []string
switch gp := req.GetGitProtocol(); gp {
case ProtocolV2:
diff --git a/internal/git/protocol_test.go b/internal/git/protocol_test.go
index f5ec602db..3a4b5cd36 100644
--- a/internal/git/protocol_test.go
+++ b/internal/git/protocol_test.go
@@ -15,9 +15,7 @@ func (f fakeProtocolMessage) GetGitProtocol() string {
return f.protocol
}
-func TestAddGitProtocolEnv(t *testing.T) {
- env := []string{"fake=value"}
-
+func TestGitProtocolEnv(t *testing.T) {
for _, tt := range []struct {
desc string
msg fakeProtocolMessage
@@ -25,19 +23,19 @@ func TestAddGitProtocolEnv(t *testing.T) {
}{
{
desc: "no V2 request",
- env: env,
+ env: nil,
},
{
desc: "V2 request",
msg: fakeProtocolMessage{protocol: "version=2"},
- env: append(env, "GIT_PROTOCOL=version=2"),
+ env: []string{"GIT_PROTOCOL=version=2"},
},
} {
t.Run(tt.desc, func(t *testing.T) {
ctx, cancel := testhelper.Context()
defer cancel()
- actual := AddGitProtocolEnv(ctx, tt.msg, env)
+ actual := gitProtocolEnv(ctx, tt.msg)
require.Equal(t, tt.env, actual)
})
}
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index 9b55235f1..46a5a45e6 100644
--- a/internal/git/receivepack.go
+++ b/internal/git/receivepack.go
@@ -1,77 +1,11 @@
package git
import (
- "context"
- "errors"
"fmt"
- "github.com/golang/protobuf/jsonpb"
"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
- "gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
- "gitlab.com/gitlab-org/gitaly/internal/gitlabshell"
- "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
- "gitlab.com/gitlab-org/gitaly/internal/praefect/metadata"
- "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
)
-// ReceivePackRequest abstracts away the different requests that end up
-// spawning git-receive-pack.
-type ReceivePackRequest interface {
- GetGlId() string
- GetGlUsername() string
- GetGlRepository() string
- GetRepository() *gitalypb.Repository
-}
-
-var jsonpbMarshaller = &jsonpb.Marshaler{}
-
-// ReceivePackHookEnv is information we pass down to the Git hooks during
-// git-receive-pack.
-func ReceivePackHookEnv(ctx context.Context, req ReceivePackRequest) ([]string, error) {
- gitlabshellEnv, err := gitlabshell.Env()
- if err != nil {
- return nil, err
- }
-
- env, err := refHookEnv(ctx, req.GetRepository(), config.Config)
- if err != nil {
- return nil, err
- }
-
- env = append(env,
- fmt.Sprintf("GL_ID=%s", req.GetGlId()),
- fmt.Sprintf("GL_USERNAME=%s", req.GetGlUsername()),
- fmt.Sprintf("GL_REPOSITORY=%s", req.GetGlRepository()),
- fmt.Sprintf("GL_PROJECT_PATH=%s", req.GetRepository().GetGlProjectPath()),
- fmt.Sprintf("%s=true", featureflag.ReferenceTransactionHookEnvVar),
- )
- env = append(env, gitlabshellEnv...)
-
- transaction, err := metadata.TransactionFromContext(ctx)
- if err == nil {
- praefect, err := metadata.PraefectFromContext(ctx)
- if err != nil {
- return nil, err
- }
-
- praefectEnv, err := praefect.Env()
- if err != nil {
- return nil, err
- }
-
- transactionEnv, err := transaction.Env()
- if err != nil {
- return nil, err
- }
-
- env = append(env, praefectEnv, transactionEnv)
- } else if !errors.Is(err, metadata.ErrTransactionNotFound) {
- return nil, err
- }
-
- return env, nil
-}
-
// ReceivePackConfig contains config options we want to enforce when
// receiving a push with git-receive-pack.
func ReceivePackConfig() []Option {
diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go
index 3dc6112bb..dd4b78a24 100644
--- a/internal/gitaly/service/smarthttp/inforefs.go
+++ b/internal/gitaly/service/smarthttp/inforefs.go
@@ -42,8 +42,6 @@ func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitaly
"service": service,
}).Debug("handleInfoRefs")
- env := git.AddGitProtocolEnv(ctx, req, []string{})
-
repoPath, err := s.locator.GetRepoPath(req.Repository)
if err != nil {
return err
@@ -62,11 +60,11 @@ func (s *server) handleInfoRefs(ctx context.Context, service string, req *gitaly
globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
}
- cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, env, globalOpts, git.SubCmd{
+ cmd, err := git.SafeBareCmd(ctx, git.CmdStream{}, nil, globalOpts, git.SubCmd{
Name: service,
Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}, git.Flag{Name: "--advertise-refs"}},
Args: []string{repoPath},
- })
+ }, git.WithGitProtocol(ctx, req))
if err != nil {
if _, ok := status.FromError(err); ok {
diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index 756e40bf3..31e0128ba 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -3,7 +3,6 @@ package smarthttp
import (
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
log "github.com/sirupsen/logrus"
- "gitlab.com/gitlab-org/gitaly/internal/command"
"gitlab.com/gitlab-org/gitaly/internal/git"
"gitlab.com/gitlab-org/gitaly/internal/helper"
"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -38,30 +37,21 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
return stream.Send(&gitalypb.PostReceivePackResponse{Data: p})
})
- hookEnv, err := git.ReceivePackHookEnv(ctx, req)
- if err != nil {
- return err
- }
- env := append(hookEnv, "GL_PROTOCOL=http")
-
repoPath, err := s.locator.GetRepoPath(req.Repository)
if err != nil {
return err
}
- env = git.AddGitProtocolEnv(ctx, req, env)
- env = append(env, command.GitEnv...)
-
globalOpts := git.ReceivePackConfig()
for _, o := range req.GitConfigOptions {
globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
}
- cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, env, globalOpts, git.SubCmd{
+ cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, nil, globalOpts, git.SubCmd{
Name: "receive-pack",
Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}},
Args: []string{repoPath},
- })
+ }, git.WithReceivePackHooks(ctx, req, "http"), git.WithGitProtocol(ctx, req))
if err != nil {
return status.Errorf(codes.Unavailable, "PostReceivePack: %v", err)
diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go
index d99544cf6..ccab8d2b7 100644
--- a/internal/gitaly/service/smarthttp/upload_pack.go
+++ b/internal/gitaly/service/smarthttp/upload_pack.go
@@ -65,8 +65,6 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
stdout := inspect.NewWriter(stdoutWriter, inspect.LogPackInfoStatistic(ctx))
defer stdout.Close()
- env := git.AddGitProtocolEnv(ctx, req, command.GitEnv)
-
repoPath, err := s.locator.GetRepoPath(req.Repository)
if err != nil {
return err
@@ -79,11 +77,11 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
}
- cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, env, globalOpts, git.SubCmd{
+ cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout}, nil, globalOpts, git.SubCmd{
Name: "upload-pack",
Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}},
Args: []string{repoPath},
- })
+ }, git.WithGitProtocol(ctx, req))
if err != nil {
return status.Errorf(codes.Unavailable, "PostUploadPack: cmd: %v", err)
diff --git a/internal/gitaly/service/ssh/monitor_stdin_command.go b/internal/gitaly/service/ssh/monitor_stdin_command.go
index c0c2c11fb..658602a70 100644
--- a/internal/gitaly/service/ssh/monitor_stdin_command.go
+++ b/internal/gitaly/service/ssh/monitor_stdin_command.go
@@ -10,13 +10,13 @@ import (
"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
)
-func monitorStdinCommand(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, env []string, globals []git.Option, sc git.SubCmd) (*command.Command, *pktline.ReadMonitor, error) {
+func monitorStdinCommand(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, env []string, globals []git.Option, sc git.SubCmd, opts ...git.CmdOpt) (*command.Command, *pktline.ReadMonitor, error) {
stdinPipe, monitor, err := pktline.NewReadMonitor(ctx, stdin)
if err != nil {
return nil, nil, fmt.Errorf("create monitor: %v", err)
}
- cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdinPipe, Out: stdout, Err: stderr}, env, globals, sc)
+ cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdinPipe, Out: stdout, Err: stderr}, env, globals, sc, opts...)
stdinPipe.Close() // this now belongs to cmd
if err != nil {
return nil, nil, fmt.Errorf("start cmd: %v", err)
diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index 94f3dfb9d..4d62adaf0 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -55,29 +55,20 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
return stream.Send(&gitalypb.SSHReceivePackResponse{Stderr: p})
})
- hookEnv, err := git.ReceivePackHookEnv(ctx, req)
- if err != nil {
- return err
- }
- env := append(hookEnv, "GL_PROTOCOL=ssh")
-
repoPath, err := s.locator.GetRepoPath(req.Repository)
if err != nil {
return err
}
- env = git.AddGitProtocolEnv(ctx, req, env)
- env = append(env, command.GitEnv...)
-
globalOpts := git.ReceivePackConfig()
for _, o := range req.GitConfigOptions {
globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
}
- cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout, Err: stderr}, env, globalOpts, git.SubCmd{
+ cmd, err := git.SafeBareCmd(ctx, git.CmdStream{In: stdin, Out: stdout, Err: stderr}, nil, globalOpts, git.SubCmd{
Name: "receive-pack",
Args: []string{repoPath},
- })
+ }, git.WithReceivePackHooks(ctx, req, "ssh"), git.WithGitProtocol(ctx, req))
if err != nil {
return fmt.Errorf("start cmd: %v", err)
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index e48352a84..f85b27751 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -71,8 +71,6 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
return stream.Send(&gitalypb.SSHUploadPackResponse{Stderr: p})
})
- env := git.AddGitProtocolEnv(ctx, req, command.GitEnv)
-
repoPath, err := s.locator.GetRepoPath(req.Repository)
if err != nil {
return err
@@ -105,10 +103,11 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
stats.UpdateMetrics(s.packfileNegotiationMetrics)
}()
- cmd, monitor, err := monitorStdinCommand(ctx, stdin, stdout, stderr, env, globalOpts, git.SubCmd{
+ cmd, monitor, err := monitorStdinCommand(ctx, stdin, stdout, stderr, nil, globalOpts, git.SubCmd{
Name: "upload-pack",
Args: []string{repoPath},
- })
+ }, git.WithGitProtocol(ctx, req))
+
if err != nil {
return err
}
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 9767a6e0a..10db79826 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -206,7 +206,7 @@ func shouldUseTransaction(ctx context.Context, method string) bool {
return ok
}
-func (c *Coordinator) registerTransaction(ctx context.Context, primary Node, secondaries []Node) (*transactions.Transaction, transactions.CancelFunc, error) {
+func (c *Coordinator) registerTransaction(ctx context.Context, primary RouterNode, secondaries []RouterNode) (*transactions.Transaction, transactions.CancelFunc, error) {
var voters []transactions.Voter
var threshold uint
@@ -316,7 +316,7 @@ func (c *Coordinator) mutatorStreamParameters(ctx context.Context, call grpcCall
call.targetRepo,
route.Primary.Storage,
nil,
- append(nodesToStorages(route.Secondaries), route.ReplicationTargets...),
+ append(routerNodesToStorages(route.Secondaries), route.ReplicationTargets...),
change,
params,
))
@@ -586,7 +586,7 @@ func (c *Coordinator) createTransactionFinalizer(
}
}
-func nodesToStorages(nodes []Node) []string {
+func routerNodesToStorages(nodes []RouterNode) []string {
storages := make([]string, len(nodes))
for i, n := range nodes {
storages[i] = n.Storage
diff --git a/internal/praefect/router.go b/internal/praefect/router.go
index 6b84d67fa..ad27675cd 100644
--- a/internal/praefect/router.go
+++ b/internal/praefect/router.go
@@ -6,22 +6,31 @@ import (
"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
"gitlab.com/gitlab-org/gitaly/internal/praefect/nodes"
+ "google.golang.org/grpc"
)
+// RouterNode represents a Node the router in a routing decision.
+type RouterNode struct {
+ // Storage is storage of the node.
+ Storage string
+ // Connection is the connection to the node.
+ Connection *grpc.ClientConn
+}
+
// StorageMutatorRoute describes how to route a storage scoped mutator call.
type StorageMutatorRoute struct {
// Primary is the primary node of the routing decision.
- Primary Node
+ Primary RouterNode
// Secondaries are the secondary nodes of the routing decision.
- Secondaries []Node
+ Secondaries []RouterNode
}
// StorageMutatorRoute describes how to route a repository scoped mutator call.
type RepositoryMutatorRoute struct {
// Primary is the primary node of the transaction.
- Primary Node
+ Primary RouterNode
// Secondaries are the secondary participating in a transaction.
- Secondaries []Node
+ Secondaries []RouterNode
// ReplicationTargets are additional nodes that do not participate in a transaction
// but need the changes replicated.
ReplicationTargets []string
@@ -30,12 +39,12 @@ type RepositoryMutatorRoute struct {
// Router decides which nodes to direct accessor and mutator RPCs to.
type Router interface {
// RouteStorageAccessor returns the node which should serve the storage accessor request.
- RouteStorageAccessor(ctx context.Context, virtualStorage string) (Node, error)
+ RouteStorageAccessor(ctx context.Context, virtualStorage string) (RouterNode, error)
// RouteStorageAccessor returns the primary and secondaries that should handle the storage
// mutator request.
RouteStorageMutator(ctx context.Context, virtualStorage string) (StorageMutatorRoute, error)
// RouteRepositoryAccessor returns the node that should serve the repository accessor request.
- RouteRepositoryAccessor(ctx context.Context, virtualStorage, relativePath string) (Node, error)
+ RouteRepositoryAccessor(ctx context.Context, virtualStorage, relativePath string) (RouterNode, error)
// RouteRepositoryMutatorTransaction returns the primary and secondaries that should handle the repository mutator request.
// Additionally, it returns nodes which should have the change replicated to.
RouteRepositoryMutator(ctx context.Context, virtualStorage, relativePath string) (RepositoryMutatorRoute, error)
@@ -46,10 +55,17 @@ type nodeManagerRouter struct {
rs datastore.RepositoryStore
}
-func toNodes(nodes []nodes.Node) []Node {
- out := make([]Node, len(nodes))
+func toRouterNode(node nodes.Node) RouterNode {
+ return RouterNode{
+ Storage: node.GetStorage(),
+ Connection: node.GetConnection(),
+ }
+}
+
+func toRouterNodes(nodes []nodes.Node) []RouterNode {
+ out := make([]RouterNode, len(nodes))
for i := range nodes {
- out[i] = toNode(nodes[i])
+ out[i] = toRouterNode(nodes[i])
}
return out
}
@@ -59,22 +75,22 @@ func NewNodeManagerRouter(mgr nodes.Manager, rs datastore.RepositoryStore) Route
return &nodeManagerRouter{mgr: mgr, rs: rs}
}
-func (r *nodeManagerRouter) RouteRepositoryAccessor(ctx context.Context, virtualStorage, relativePath string) (Node, error) {
+func (r *nodeManagerRouter) RouteRepositoryAccessor(ctx context.Context, virtualStorage, relativePath string) (RouterNode, error) {
node, err := r.mgr.GetSyncedNode(ctx, virtualStorage, relativePath)
if err != nil {
- return Node{}, fmt.Errorf("get synced node: %w", err)
+ return RouterNode{}, fmt.Errorf("get synced node: %w", err)
}
- return toNode(node), nil
+ return toRouterNode(node), nil
}
-func (r *nodeManagerRouter) RouteStorageAccessor(ctx context.Context, virtualStorage string) (Node, error) {
+func (r *nodeManagerRouter) RouteStorageAccessor(ctx context.Context, virtualStorage string) (RouterNode, error) {
shard, err := r.mgr.GetShard(virtualStorage)
if err != nil {
- return Node{}, err
+ return RouterNode{}, err
}
- return toNode(shard.Primary), nil
+ return toRouterNode(shard.Primary), nil
}
func (r *nodeManagerRouter) RouteStorageMutator(ctx context.Context, virtualStorage string) (StorageMutatorRoute, error) {
@@ -84,8 +100,8 @@ func (r *nodeManagerRouter) RouteStorageMutator(ctx context.Context, virtualStor
}
return StorageMutatorRoute{
- Primary: toNode(shard.Primary),
- Secondaries: toNodes(shard.GetHealthySecondaries()),
+ Primary: toRouterNode(shard.Primary),
+ Secondaries: toRouterNodes(shard.GetHealthySecondaries()),
}, nil
}
@@ -122,8 +138,8 @@ func (r *nodeManagerRouter) RouteRepositoryMutator(ctx context.Context, virtualS
}
return RepositoryMutatorRoute{
- Primary: toNode(shard.Primary),
- Secondaries: toNodes(participatingSecondaries),
+ Primary: toRouterNode(shard.Primary),
+ Secondaries: toRouterNodes(participatingSecondaries),
ReplicationTargets: replicationTargets,
}, nil
}
diff --git a/proto/README.md b/proto/README.md
index 99fef6b6f..4216c18a4 100644
--- a/proto/README.md
+++ b/proto/README.md
@@ -273,6 +273,16 @@ you forget to add a `go_package` option, you may receive an error similar to:
`blob.proto is missing the go_package option`
+### Documentation
+
+New or updated RPCs and message types should be accompanied by comment strings.
+Good comment strings will explain why the RPC exists and how it behaves. Good
+message type comments will explain what the message is communicating. Each updated
+message field should have a comment.
+
+Refer to official protobuf documentation for
+[how to add comments](https://developers.google.com/protocol-buffers/docs/proto#adding_comments).
+
## Contributing
The CI at https://gitlab.com/gitlab-org/gitaly-proto regenerates the