From 15d948b98255c579d512fbd07761f8d409de355d Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 27 Mar 2025 15:34:21 -0400 Subject: [PATCH 1/7] middleware: Add client context handler to inject correlaction id The ClientContext is propagated all the way back into Rails. Currently the correlation_id does not get propagated all the way back into the internal API. Add a handler that injects the correlation_id into the client context bin. Later on down the line, this client context gets wrapped up and sent to hooks, which then sends it along to the internal api calls. --- .../clientcontexthandler.go | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 internal/grpc/middleware/clientcontexthandler/clientcontexthandler.go diff --git a/internal/grpc/middleware/clientcontexthandler/clientcontexthandler.go b/internal/grpc/middleware/clientcontexthandler/clientcontexthandler.go new file mode 100644 index 0000000000..f5ba91b527 --- /dev/null +++ b/internal/grpc/middleware/clientcontexthandler/clientcontexthandler.go @@ -0,0 +1,66 @@ +package clientcontexthandler + +import ( + "context" + "encoding/base64" + "encoding/json" + + grpcmw "github.com/grpc-ecosystem/go-grpc-middleware" + "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/metadata" + "gitlab.com/gitlab-org/labkit/correlation" + "google.golang.org/grpc" +) + +func addCorrelationIDToClientContext(ctx context.Context) (context.Context, error) { + gitalyClientContextEncoded := metadata.GetValue(ctx, metadata.ClientContextMetadataKey) + gitalyClientContextDecoded, err := base64.StdEncoding.DecodeString(gitalyClientContextEncoded) + if err != nil { + return nil, err + } + + // Create a map to store the decoded JSON + gitalyClientContext := make(map[string]interface{}) + + if string(gitalyClientContextDecoded) != "" { + // Unmarshal the JSON string into the map + if err = json.Unmarshal(gitalyClientContextDecoded, &gitalyClientContext); err != nil { + return nil, err + } + } + + gitalyClientContext["correlation_id"] = correlation.ExtractFromContext(ctx) + + jsonBytes, err := json.Marshal(gitalyClientContext) + if err != nil { + return nil, err + } + + base64String := base64.StdEncoding.EncodeToString(jsonBytes) + + return metadata.ReplaceInIncomingContext(ctx, metadata.ClientContextMetadataKey, base64String), nil +} + +// UnaryInterceptor returns a Unary Interceptor that initializes and injects a log.CustomFields object into the context +func UnaryInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + ctx, err := addCorrelationIDToClientContext(ctx) + if err != nil { + return nil, err + } + + res, err := handler(ctx, req) + + return res, err +} + +// StreamInterceptor returns a Stream Interceptor that initializes and injects a log.CustomFields object into the context +func StreamInterceptor(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { + ctx, err := addCorrelationIDToClientContext(stream.Context()) + if err != nil { + return err + } + + wrapped := grpcmw.WrapServerStream(stream) + wrapped.WrappedContext = ctx + + return handler(srv, wrapped) +} -- GitLab From 7040b8676dd8393cb1496b640198d3deb77bd0a0 Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 3 Apr 2025 21:00:19 -0400 Subject: [PATCH 2/7] metadata: Add a method to replace a grpc metadata field Sometimes it is useful to be able to not just append a value to the metadata field, but to replace it altogether. --- internal/grpc/metadata/metadata.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/grpc/metadata/metadata.go b/internal/grpc/metadata/metadata.go index ab9499c1ef..22446a7e18 100644 --- a/internal/grpc/metadata/metadata.go +++ b/internal/grpc/metadata/metadata.go @@ -7,6 +7,7 @@ import ( ) // ClientContextMetadataKey is the key used by rails to propagate client context back to internal APIs +// The contents is a base64 encoded json string. const ClientContextMetadataKey = "gitaly-client-context-bin" // IncomingToOutgoing creates an outgoing context out of an incoming context with the same storage metadata @@ -49,3 +50,15 @@ func AppendToIncomingContext(ctx context.Context, key, value string) context.Con md.Append(key, value) return metadata.NewIncomingContext(ctx, md) } + +// ReplaceInIncomingContext sets a key/value pair in the incoming context +func ReplaceInIncomingContext(ctx context.Context, key, value string) context.Context { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + md = metadata.New(nil) + } + + md.Set(key, value) + + return metadata.NewIncomingContext(ctx, md) +} -- GitLab From 47f3e48427079ed1a8e4c15d5b303a8e79be14ce Mon Sep 17 00:00:00 2001 From: John Cai Date: Thu, 27 Mar 2025 15:40:57 -0400 Subject: [PATCH 3/7] server: Wire up client context handler --- internal/gitaly/server/server.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/gitaly/server/server.go b/internal/gitaly/server/server.go index 3c28d64f06..be9dfa92d8 100644 --- a/internal/gitaly/server/server.go +++ b/internal/gitaly/server/server.go @@ -14,6 +14,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/grpcstats" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/listenmux" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/cache" + "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/clientcontexthandler" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/customfieldshandler" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/featureflag" "gitlab.com/gitlab-org/gitaly/v16/internal/grpc/middleware/loghandler" @@ -105,6 +106,7 @@ func (s *GitalyServerFactory) New(external, secure bool, opts ...Option) (*grpc. streamServerInterceptors := []grpc.StreamServerInterceptor{ grpccorrelation.StreamServerCorrelationInterceptor(), // Must be above the metadata handler + clientcontexthandler.StreamInterceptor, requestinfohandler.StreamInterceptor, grpcprometheus.StreamServerInterceptor, customfieldshandler.StreamInterceptor, @@ -119,6 +121,7 @@ func (s *GitalyServerFactory) New(external, secure bool, opts ...Option) (*grpc. } unaryServerInterceptors := []grpc.UnaryServerInterceptor{ grpccorrelation.UnaryServerCorrelationInterceptor(), // Must be above the metadata handler + clientcontexthandler.UnaryInterceptor, requestinfohandler.UnaryInterceptor, grpcprometheus.UnaryServerInterceptor, customfieldshandler.UnaryInterceptor, -- GitLab From 6e5a1d898992a9da958dfaaa14a10ee71d8618bd Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 28 Mar 2025 11:21:27 -0400 Subject: [PATCH 4/7] gitlab: Allow creating a test server with custom handlers Introduce a function that allows creating a test server by injecting handlers. This is useful to test the data that the handlers receive. --- internal/gitlab/test_server.go | 35 ++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/internal/gitlab/test_server.go b/internal/gitlab/test_server.go index 81845ccf0e..74e41b3227 100644 --- a/internal/gitlab/test_server.go +++ b/internal/gitlab/test_server.go @@ -54,19 +54,42 @@ type TestServerOptions struct { GlRepository string ClientCertificate *testhelper.Certificate ServerCertificate *testhelper.Certificate + HandlePreReceive internalAPIHandler + HandleAllowed internalAPIHandler + HandlePostReceive internalAPIHandler + HandleLFS internalAPIHandler + HandleCheck internalAPIHandler } +type internalAPIHandler func(testing.TB, TestServerOptions) func(http.ResponseWriter, *http.Request) + // NewTestServer returns a mock gitlab server that responds to the hook api endpoints func NewTestServer(tb testing.TB, options TestServerOptions) (url string, cleanup func()) { tb.Helper() + if options.HandlePreReceive == nil { + options.HandlePreReceive = handlePreReceive + } + if options.HandleAllowed == nil { + options.HandleAllowed = handleAllowed + } + if options.HandlePostReceive == nil { + options.HandlePostReceive = handlePostReceive + } + if options.HandleLFS == nil { + options.HandleLFS = handleLfs + } + if options.HandleCheck == nil { + options.HandleCheck = handleCheck + } + mux := http.NewServeMux() prefix := strings.TrimRight(options.RelativeURLRoot, "/") + "/api/v4/internal" - mux.Handle(prefix+"/allowed", http.HandlerFunc(handleAllowed(tb, options))) - mux.Handle(prefix+"/pre_receive", http.HandlerFunc(handlePreReceive(tb, options))) - mux.Handle(prefix+"/post_receive", http.HandlerFunc(handlePostReceive(options))) - mux.Handle(prefix+"/check", http.HandlerFunc(handleCheck(tb, options))) - mux.Handle(prefix+"/lfs", http.HandlerFunc(handleLfs(tb, options))) + mux.Handle(prefix+"/allowed", http.HandlerFunc(options.HandleAllowed(tb, options))) + mux.Handle(prefix+"/pre_receive", http.HandlerFunc(options.HandlePreReceive(tb, options))) + mux.Handle(prefix+"/post_receive", http.HandlerFunc(options.HandlePostReceive(tb, options))) + mux.Handle(prefix+"/check", http.HandlerFunc(options.HandleCheck(tb, options))) + mux.Handle(prefix+"/lfs", http.HandlerFunc(options.HandleLFS(tb, options))) var tlsCfg *tls.Config if options.ClientCertificate != nil { @@ -376,7 +399,7 @@ func handlePreReceive(tb testing.TB, options TestServerOptions) func(w http.Resp } } -func handlePostReceive(options TestServerOptions) func(w http.ResponseWriter, r *http.Request) { +func handlePostReceive(tb testing.TB, options TestServerOptions) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { if err := r.ParseForm(); err != nil { http.Error(w, "couldn't parse form", http.StatusBadRequest) -- GitLab From 3bd942d9e7f03cf556d890613e8303f2c0118259 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 28 Mar 2025 11:21:33 -0400 Subject: [PATCH 5/7] smarthttp: Add test to ensure correlation_id is passed to the API Add a test that checks whether or not a correlation_id is detected in the client context that the GitLab API receives in the request body. --- .../service/smarthttp/receive_pack_test.go | 102 +++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 0a7824394f..d37ec6218f 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -3,9 +3,12 @@ package smarthttp import ( "bytes" "context" + "encoding/base64" + "encoding/json" "errors" "fmt" "io" + "net/http" "path/filepath" "strings" "testing" @@ -18,6 +21,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v16/internal/git/pktline" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config/prometheus" gitalyhook "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage/storagemgr" @@ -34,6 +38,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v16/streamio" "google.golang.org/grpc" + grpcmetadata "google.golang.org/grpc/metadata" ) func TestPostReceivePack_successful(t *testing.T) { @@ -121,6 +126,9 @@ func TestPostReceivePack_successful(t *testing.T) { } client := newSmartHTTPClient(t, server.Address(), cfg.Auth.Token) + + correlationID := "correlation123" + ctx = grpcmetadata.AppendToOutgoingContext(ctx, "X-GitLab-Correlation-ID", correlationID) stream, err := client.PostReceivePack(ctx) require.NoError(t, err) @@ -195,6 +203,14 @@ func TestPostReceivePack_successful(t *testing.T) { transactionID = 3 } + expectedGitalyClientContext := map[string]string{ + "correlation_id": correlationID, + } + + marshalled, err := json.Marshal(expectedGitalyClientContext) + require.NoError(t, err) + expectedGitalyClientContextEncoded := base64.StdEncoding.EncodeToString(marshalled) + require.Equal(t, gitcmd.HooksPayload{ ObjectFormat: gittest.DefaultObjectHash.Format, RuntimeDir: cfg.RuntimeDir, @@ -205,8 +221,9 @@ func TestPostReceivePack_successful(t *testing.T) { Username: "user", Protocol: "http", }, - RequestedHooks: expectedHooks, - TransactionID: transactionID, + RequestedHooks: expectedHooks, + TransactionID: transactionID, + GitalyClientContext: []byte(expectedGitalyClientContextEncoded), }, payload) require.Equal(t, 1, preReceiveCount) @@ -941,6 +958,87 @@ func TestPostReceivePack_notAllowed(t *testing.T) { require.Equal(t, 1, refTransactionServer.called) } +func handleAllowed(tb testing.TB, options gitlab.TestServerOptions) func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + params := struct { + ClientContext []byte `json:"gitaly_client_context_bin"` + }{} + + decoder := json.NewDecoder(r.Body) + defer r.Body.Close() + + err := decoder.Decode(¶ms) + require.NoError(tb, err) + + gitalyClientContextDecoded, err := base64.StdEncoding.DecodeString(string(params.ClientContext)) + require.NoError(tb, err) + + var clientCtx map[string]string + + require.NoError(tb, json.Unmarshal(gitalyClientContextDecoded, &clientCtx)) + _, ok := clientCtx["correlation_id"] + require.True(tb, ok, "correlation id exists in client context") + } +} + +func TestPostReceivePack_clientContext(t *testing.T) { + t.Parallel() + + const ( + secretToken = "secret token" + glRepository = "some_repo" + glID = "key-123" + ) + + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + + var cleanup func() + cfg.Gitlab.URL, cleanup = gitlab.NewTestServer( + t, + gitlab.TestServerOptions{ + HandleAllowed: handleAllowed, + }, + ) + defer cleanup() + + gitlabClient, err := gitlab.NewHTTPClient( + testhelper.NewLogger(t), + cfg.Gitlab, + config.TLS{}, + prometheus.Config{}, + ) + + require.NoError(t, err) + gitalyServer := startSmartHTTPServerWithOptions(t, cfg, nil, []testserver.GitalyServerOpt{testserver.WithGitLabClient( + gitlabClient, + )}) + + cfg.SocketPath = gitalyServer.Address() + cfg.GitlabShell.Dir = testhelper.TempDir(t) + cfg.Auth.Token = "abc123" + cfg.Gitlab.SecretFile = gitlab.WriteShellSecretFile(t, cfg.GitlabShell.Dir, secretToken) + + testcfg.BuildGitalyHooks(t, cfg) + + repo, repoPath := gittest.CreateRepository(t, ctx, cfg) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch)) + push := setupSimplePush(t, ctx, cfg, repoPath, git.DefaultRef) + + gittest.WriteCheckNewObjectExistsHook(t, repoPath) + + client := newSmartHTTPClient(t, cfg.SocketPath, cfg.Auth.Token) + + stream, err := client.PostReceivePack(ctx) + require.NoError(t, err) + + push.perform(t, stream, &gitalypb.PostReceivePackRequest{ + Repository: repo, + GlId: glID, + GlRepository: glRepository, + }) +} + type refUpdate struct { ref git.ReferenceName from, to git.ObjectID -- GitLab From 2e68adbc00f1fc08f6d4c90bc9c46d3632858117 Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 4 Apr 2025 11:55:31 -0400 Subject: [PATCH 6/7] ssh: Add correlation id if it exists in the context In tests, we want to be able to confirm if a correlation id made it all the way to the gitaly client context in the hooks payload. The current helper however, fires off a push command which does not include the correlation id if it has been passed in through the ctx. This change appends an env var to include the correlation id in the push command. --- internal/gitaly/service/ssh/receive_pack_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index 5f2a309f28..e6c5b86128 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -34,6 +34,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/transaction/txinfo" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v16/streamio" + "gitlab.com/gitlab-org/labkit/correlation" "google.golang.org/protobuf/encoding/protojson" ) @@ -880,6 +881,10 @@ func sshPushCommand(t *testing.T, ctx context.Context, cfg config.Cfg, repo repo fmt.Sprintf("GIT_SSH_COMMAND=%s receive-pack", cfg.BinaryPath("gitaly-ssh")), ) + if correlationID := correlation.ExtractFromContext(ctx); correlationID != "" { + cmd.Env = append(cmd.Env, fmt.Sprintf("CORRELATION_ID=%s", correlationID)) + } + return cmd } -- GitLab From 92833ca69f84c654d2cbe52e8adc6b4b2ad4c85e Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 4 Apr 2025 11:57:18 -0400 Subject: [PATCH 7/7] ssh: Adjust test to expect gitaly client context is correctly set Set the correlation id explicitly and test that the gitaly client context includes this value. --- .../gitaly/service/ssh/receive_pack_test.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index e6c5b86128..acb86af50b 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -3,6 +3,8 @@ package ssh import ( "bytes" "context" + "encoding/base64" + "encoding/json" "fmt" "io" "os" @@ -204,6 +206,8 @@ func TestReceivePack_success(t *testing.T) { ctx = featureflag.ContextWithFeatureFlag(ctx, featureFlag, true) } + correlationID := "correlation123" + ctx = correlation.ContextWithCorrelation(ctx, correlationID) lHead, rHead, err := setupRepoAndPush(t, ctx, cfg, &gitalypb.SSHReceivePackRequest{ Repository: remoteRepo, GlId: "123", @@ -266,6 +270,14 @@ func TestReceivePack_success(t *testing.T) { transactionID = 6 } + expectedGitalyClientContext := map[string]string{ + "correlation_id": correlationID, + } + + marshalled, err := json.Marshal(expectedGitalyClientContext) + require.NoError(t, err) + expectedGitalyClientContextEncoded := base64.StdEncoding.EncodeToString(marshalled) + require.Equal(t, gitcmd.HooksPayload{ ObjectFormat: gittest.DefaultObjectHash.Format, RuntimeDir: cfg.RuntimeDir, @@ -276,8 +288,9 @@ func TestReceivePack_success(t *testing.T) { Username: "user", Protocol: "ssh", }, - RequestedHooks: expectedHooks, - TransactionID: transactionID, + RequestedHooks: expectedHooks, + TransactionID: transactionID, + GitalyClientContext: []byte(expectedGitalyClientContextEncoded), }, payload) require.Equal(t, 1, preReceiveCount) -- GitLab