diff options
author | Michael Leopard <mleopard@gitlab.com> | 2018-12-14 17:39:07 +0300 |
---|---|---|
committer | Michael Leopard <mleopard@gitlab.com> | 2018-12-14 17:39:07 +0300 |
commit | bf2d99c7e03aba8dbe19d418d20c16ca850cf569 (patch) | |
tree | d6780c221d3bf960727149a2d0a5c8b053734e2b | |
parent | 3c41caffc462091c833bd7fe63cb40719fca6395 (diff) | |
parent | 3d3e591a6caacfde7fbc58c7838af014d7e21f10 (diff) |
Merge branch 'master' into 1293-re-implement-findbranch-in-go
-rw-r--r-- | CHANGELOG.md | 16 | ||||
-rw-r--r-- | README.md | 39 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | doc/beginners_guide.md | 28 | ||||
-rw-r--r-- | internal/command/command.go | 23 | ||||
-rw-r--r-- | internal/command/command_test.go | 11 | ||||
-rw-r--r-- | internal/command/spawntoken.go | 2 | ||||
-rw-r--r-- | internal/middleware/metadatahandler/metadatahandler.go | 9 | ||||
-rw-r--r-- | internal/server/server.go | 4 | ||||
-rw-r--r-- | ruby/Gemfile.lock | 2 |
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 @@ -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. @@ -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) |