Merge branch 'master' into 1293-re-implement-findbranch-in-go

10 files changed, 108 insertions, 28 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ad3c2ffd..288c6d3c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,21 @@
 # Gitaly changelog
 
+## v1.8.0
+
+#### Other
+- Log correlation_id field in structured logging output
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/995
+- Add explicit null byte check in internal/command.New
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/997
+- README cleanup
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/996
+
+## v1.7.1
+
+#### Other
+- Log correlation_id field in structured logging output
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/995
+
 ## v1.7.0
 
 #### Added
diff --git a/README.md b/README.md
index 72dbc7c91..2a69e2663 100644
--- a/README.md
+++ b/README.md
@@ -3,12 +3,8 @@
 [![Pipeline status](https://gitlab.com/gitlab-org/gitaly/badges/master/pipeline.svg)](https://gitlab.com/gitlab-org/gitaly/commits/master) [![coverage report](https://gitlab.com/gitlab-org/gitaly/badges/master/coverage.svg)](https://codecov.io/gl/gitlab-org/gitaly)
 
 **Quick Links**:
-  [**Migration Board**](https://gitlab.com/gitlab-org/gitaly/boards/331341?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Migration) |
   [**Roadmap**](https://gitlab.com/groups/gitlab-org/-/roadmap?label_name%5B%5D=Gitaly&scope=all&sort=start_date_asc&state=opened) |
-  [Open Conversations](https://gitlab.com/gitlab-org/gitaly/issues?label_name%5B%5D=Conversation) |
-  [Unassigned Conversations](https://gitlab.com/gitlab-org/gitaly/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Conversation&label_name[]=To%20Do&assignee_id=0) |
-  [Migrations](https://gitlab.com/gitlab-org/gitaly/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Conversation&label_name[]=Migration) |
-  [Want to Contribute?](https://gitlab.com/gitlab-org/gitaly/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Accepting%20Merge%20Requests) |
+  [Want to Contribute?](https://gitlab.com/gitlab-org/gitaly/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Accepting%20merge%20requests) |
   [GitLab Gitaly Issues](https://gitlab.com/groups/gitlab-org/issues?scope=all&state=opened&utf8=%E2%9C%93&label_name%5B%5D=Gitaly) |
   [GitLab Gitaly Merge Requests](https://gitlab.com/groups/gitlab-org/merge_requests?label_name%5B%5D=Gitaly) |
   [gitlab.com dashboard](https://performance.gitlab.net/dashboard/db/gitaly) |
@@ -31,21 +27,23 @@ This will be achieved by focusing on two areas (in this order):
 
 ## Current Status
 
-As of GitLab 11.1, most application code accesses Git repositories
-through Gitaly instead of direct disk access. We are close to removing
-all Git disk access from gitlab-rails (the Gitaly
-[v1.1](https://gitlab.com/gitlab-org/gitaly/milestones/55) milestone).
-We are even closer to fully supporting the subset of Git operations
-needed by gitlab.com (the
-[v1.0](https://gitlab.com/gitlab-org/gitaly/milestones/54) milestone).
-When these two milestones are closed the migration project will be
-complete.
+As of GitLab 11.5, almost all application code accesses Git repositories
+through Gitaly instead of direct disk access. GitLab.com production no
+longer uses direct disk access to touch Git repositories; the [NFS
+mounts have been
+removed](https://about.gitlab.com/2018/09/12/the-road-to-gitaly-1-0/).
 
-[The roadmap is available here](https://gitlab.com/groups/gitlab-org/-/roadmap?label_name%5B%5D=Gitaly&scope=all&sort=start_date_asc&state=opened).
+The last feature that remains to be migrated is the [ElasticSearch
+indexer](https://gitlab.com/gitlab-org/gitaly/issues/760). Once that is
+done we can conclude the migration project by [removing the Git
+repository storage paths from gitlab-rails's
+configuration](https://gitlab.com/gitlab-org/gitaly/issues/1282).
 
-The migration process is [documented](/doc/MIGRATION_PROCESS.md).
+In the meantime we are building features according to our
+[roadmap](https://gitlab.com/groups/gitlab-org/-/roadmap?label_name%5B%5D=Gitaly&scope=all&sort=start_date_asc&state=opened).
 
-If you're interested in seeing how well Gitaly is performing on GitLab.com, we have dashboards!
+If you're interested in seeing how well Gitaly is performing on
+GitLab.com, we have dashboards!
 
 ##### Overall
 
@@ -55,12 +53,11 @@ If you're interested in seeing how well Gitaly is performing on GitLab.com, we h
 
  [![image](https://gitlab.com/gitlab-org/gitaly/uploads/5b3825e01c48975c2a64e01ae37b4a3d/image.png)](http://monitor.gitlab.net/dashboard/db/gitaly-features?orgId=1&var-job=gitaly-production&from=now-7d&to=now)
 
-## Migrations
-
-The progress of Gitaly's endpoint migrations is tracked via the [**Migration Board**](https://gitlab.com/gitlab-org/gitaly/boards/331341?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Migration)
-
 ## Installation
 
+Most users won't install Gitaly on its own. It is already included in
+[your GitLab installation](https://about.gitlab.com/install/).
+
 Gitaly requires Go 1.10 or newer and Ruby 2.4. Run `make` to download
 and compile Ruby dependencies, and to compile the Gitaly Go
 executable.
diff --git a/VERSION b/VERSION
index bd8bf882d..27f9cd322 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-1.7.0
+1.8.0
diff --git a/doc/beginners_guide.md b/doc/beginners_guide.md
index dc8fcc390..755dfaad4 100644
--- a/doc/beginners_guide.md
+++ b/doc/beginners_guide.md
@@ -9,7 +9,6 @@ Before you can develop on Gitaly, it's required to have a
 it to be working by starting the required servers and visiting GitLab on
 `http://localhost:3000`.
 
-
 #### Gitaly Proto
 
 GitLab will want to read and manipulate Git data, to do this it needs to talk
@@ -80,6 +79,33 @@ some tools break. In this case run commands in your global GOPATH instead.
 
 ### Development
 
+#### General advice
+
+##### Editing code and seeing what happens
+
+If you're used to Ruby on Rails development you may be used to a "edit
+code and reload" cycle where you keep editing and reloading until you
+have your change working. This is not a good workflow for Gitaly
+development.
+
+At some point you will know which Gitaly RPC you need to work on. This
+is where you probably want to stop using `localhost:3000` and zoom in on
+the RPC instead.
+
+To experiment with changing an RPC you should use the Gitaly service
+tests. The RPC you want to work on will have tests somewhere in
+`internal/services/...`. Find the tests for your RPC. Next, before you
+edit any code, make sure the tests pass when you run them:
+`go test ./internal/service/foobar -count 1 -run MyRPC`. In this
+command, `MyRPC` is a regex that will match functions like
+`TestMyRPCSuccess` and `TestMyRPCValidationFailure`. Once you have found
+your tests and your test command, you can start tweaking the
+implementation or adding test cases and re-running the tests. The cycle
+is "edit code, run tests".
+
+This is many times faster than "edit gitaly, reinstall Gitaly into GDK,
+restart, reload localhost:3000".
+
 #### Process
 
 In general there are a couple of stages to go through, in order:
diff --git a/internal/command/command.go b/internal/command/command.go
index 38813f988..9272de8bf 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -127,8 +128,9 @@ func WaitAllDone() {
 	wg.Wait()
 }
 
-type spawnTimeoutError error
+type spawnTimeoutError struct{ error }
 type contextWithoutDonePanic string
+type nullInArgvError struct{ error }
 
 // New creates a Command from an exec.Cmd. On success, the Command
 // contains a running subprocess. When ctx is canceled the embedded
@@ -141,6 +143,10 @@ func New(ctx context.Context, cmd *exec.Cmd, stdin io.Reader, stdout, stderr io.
 		panic(contextWithoutDonePanic("command spawned with context without Done() channel"))
 	}
 
+	if err := checkNullArgv(cmd); err != nil {
+		return nil, err
+	}
+
 	putToken, err := getSpawnToken(ctx)
 	if err != nil {
 		return nil, err
@@ -309,3 +315,18 @@ func (c *Command) logProcessComplete(ctx context.Context, exitCode int) {
 
 	entry.Debug("spawn complete")
 }
+
+// Command arguments will be passed to the exec syscall as
+// null-terminated C strings. That means the arguments themselves may not
+// contain a null byte. The go stdlib checks for null bytes but it
+// returns a cryptic error. This function returns a more explicit error.
+func checkNullArgv(cmd *exec.Cmd) error {
+	for _, arg := range cmd.Args {
+		if strings.IndexByte(arg, 0) > -1 {
+			// Use %q so that the null byte gets printed as \x00
+			return nullInArgvError{fmt.Errorf("detected null byte in command argument %q", arg)}
+		}
+	}
+
+	return nil
+}
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 7bff115a9..feaf52711 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -176,3 +176,14 @@ func TestNewCommandWithSetupStdin(t *testing.T) {
 
 	require.NoError(t, cmd.Wait())
 }
+
+func TestNewCommandNullInArg(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	_, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"), nil, nil, nil)
+	require.Error(t, err)
+
+	_, ok := err.(nullInArgvError)
+	require.True(t, ok, "expected %+v to be nullInArgvError", err)
+}
diff --git a/internal/command/spawntoken.go b/internal/command/spawntoken.go
index 1428beee6..fdadbc679 100644
--- a/internal/command/spawntoken.go
+++ b/internal/command/spawntoken.go
@@ -68,7 +68,7 @@ func getSpawnToken(ctx context.Context) (putToken func(), err error) {
 		logTime(ctx, start, "spawn token timeout")
 		spawnTimeoutCount.Inc()
 
-		return nil, spawnTimeoutError(fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout))
+		return nil, spawnTimeoutError{fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout)}
 	case <-ctx.Done():
 		return nil, ctx.Err()
 	}
diff --git a/internal/middleware/metadatahandler/metadatahandler.go b/internal/middleware/metadatahandler/metadatahandler.go
index 06b67b945..5091e6654 100644
--- a/internal/middleware/metadatahandler/metadatahandler.go
+++ b/internal/middleware/metadatahandler/metadatahandler.go
@@ -5,6 +5,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/labkit/correlation"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/metadata"
@@ -41,6 +42,8 @@ const ClientNameKey = "grpc.meta.client_name"
 // AuthVersionKey is the key used in ctx_tags to store the auth version
 const AuthVersionKey = "grpc.meta.auth_version"
 
+const correlationIDKey = "correlation_id"
+
 // Unknown client and feature. Matches the prometheus grpc unknown value
 const unknownValue = "unknown"
 
@@ -88,6 +91,12 @@ func addMetadataTags(ctx context.Context) metadataTags {
 		tags.Set(AuthVersionKey, authInfo.Version)
 	}
 
+	// This is a stop-gap approach to logging correlation_ids
+	correlationID := correlation.ExtractFromContext(ctx)
+	if correlationID != "" {
+		tags.Set(correlationIDKey, correlationID)
+	}
+
 	return metaTags
 }
 
diff --git a/internal/server/server.go b/internal/server/server.go
index a29405c0d..cf5630bad 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -74,6 +74,7 @@ func createNewServer(rubyServer *rubyserver.Server, secure bool) *grpc.Server {
 	opts := []grpc.ServerOption{
 		grpc.StreamInterceptor(grpc_middleware.ChainStreamServer(
 			grpc_ctxtags.StreamServerInterceptor(ctxTagOpts...),
+			grpccorrelation.StreamServerCorrelationInterceptor(), // Must be above the metadata handler
 			metadatahandler.StreamInterceptor,
 			grpc_prometheus.StreamServerInterceptor,
 			grpc_logrus.StreamServerInterceptor(logrusEntry),
@@ -81,13 +82,13 @@ func createNewServer(rubyServer *rubyserver.Server, secure bool) *grpc.Server {
 			cancelhandler.Stream, // Should be below LogHandler
 			lh.StreamInterceptor(),
 			auth.StreamServerInterceptor(),
-			grpccorrelation.StreamServerCorrelationInterceptor(),
 			// Panic handler should remain last so that application panics will be
 			// converted to errors and logged
 			panichandler.StreamPanicHandler,
 		)),
 		grpc.UnaryInterceptor(grpc_middleware.ChainUnaryServer(
 			grpc_ctxtags.UnaryServerInterceptor(ctxTagOpts...),
+			grpccorrelation.UnaryServerCorrelationInterceptor(), // Must be above the metadata handler
 			metadatahandler.UnaryInterceptor,
 			grpc_prometheus.UnaryServerInterceptor,
 			grpc_logrus.UnaryServerInterceptor(logrusEntry),
@@ -95,7 +96,6 @@ func createNewServer(rubyServer *rubyserver.Server, secure bool) *grpc.Server {
 			cancelhandler.Unary, // Should be below LogHandler
 			lh.UnaryInterceptor(),
 			auth.UnaryServerInterceptor(),
-			grpccorrelation.UnaryServerCorrelationInterceptor(),
 			// Panic handler should remain last so that application panics will be
 			// converted to errors and logged
 			panichandler.UnaryPanicHandler,
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index 73dab9a33..59f8ad69f 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -82,7 +82,7 @@ GEM
     nokogumbo (1.5.0)
       nokogiri
     parallel (1.12.1)
-    parser (2.5.1.2)
+    parser (2.5.3.0)
       ast (~> 2.4.0)
     posix-spawn (0.3.13)
     powerpack (0.1.2)