From 8eabc7a0046db833a309b0fcb1e037c07269a048 Mon Sep 17 00:00:00 2001 From: John Cai Date: Mon, 24 Mar 2025 13:50:05 -0400 Subject: [PATCH] PreReceiveHook: Chunk stderr in response Although we currently chunk the responses in PreReceiveHook, we just send the stderr back in one whole response in the event of an error. This might cause a "received message larger than max" error on the client side if the stderr is large. Use the chunker abstraction to send the stderr back to the client. --- internal/gitaly/service/hook/pre_receive.go | 57 ++++++++++- .../gitaly/service/hook/pre_receive_test.go | 96 +++++++++++-------- 2 files changed, 111 insertions(+), 42 deletions(-) diff --git a/internal/gitaly/service/hook/pre_receive.go b/internal/gitaly/service/hook/pre_receive.go index 4062546dc2..6beeb4da46 100644 --- a/internal/gitaly/service/hook/pre_receive.go +++ b/internal/gitaly/service/hook/pre_receive.go @@ -7,10 +7,14 @@ import ( "os/exec" "sync" + "github.com/golang/protobuf/ptypes/wrappers" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" + "gitlab.com/gitlab-org/gitaly/v16/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/v16/internal/structerr" "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v16/streamio" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/wrapperspb" ) func (s *server) PreReceiveHook(stream gitalypb.HookService_PreReceiveHookServer) error { @@ -61,11 +65,56 @@ func validatePreReceiveHookRequest(ctx context.Context, locator storage.Locator, return locator.ValidateRepository(ctx, in.GetRepository()) } +type preReceiveHookErrorSender struct { + errorCode int32 + bytes *wrappers.BytesValue + send func(*wrappers.BytesValue, int32) error +} + +func (p *preReceiveHookErrorSender) Reset() { + if p.bytes != nil { + p.bytes.Value = p.bytes.Value[:0] + return + } + + p.bytes = &wrappers.BytesValue{} +} + +func (p *preReceiveHookErrorSender) Append(m proto.Message) { + p.bytes.Value = append(p.bytes.Value, m.(*wrappers.BytesValue).Value...) +} + +func (p *preReceiveHookErrorSender) Send() error { + return p.send(p.bytes, p.errorCode) +} + func preReceiveHookResponse(stream gitalypb.HookService_PreReceiveHookServer, code int32, stderr string) error { - if err := stream.Send(&gitalypb.PreReceiveHookResponse{ - ExitStatus: &gitalypb.ExitStatus{Value: code}, - Stderr: []byte(stderr), - }); err != nil { + if stderr == "" { + if err := stream.Send(&gitalypb.PreReceiveHookResponse{ + ExitStatus: &gitalypb.ExitStatus{Value: code}, + Stderr: []byte(""), + }); err != nil { + return structerr.NewInternal("sending response: %w", err) + } + + return nil + } + + chunker := chunk.New(&preReceiveHookErrorSender{ + errorCode: code, + send: func(bytes *wrappers.BytesValue, errorCode int32) error { + return stream.Send(&gitalypb.PreReceiveHookResponse{ + ExitStatus: &gitalypb.ExitStatus{Value: errorCode}, + Stderr: []byte(bytes.Value), + }) + }, + }) + + if err := chunker.Send(wrapperspb.Bytes([]byte(stderr))); err != nil { + return structerr.NewInternal("sending response: %w", err) + } + + if err := chunker.Flush(); err != nil { return structerr.NewInternal("sending response: %w", err) } diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go index b0d50c11a8..6dc0751063 100644 --- a/internal/gitaly/service/hook/pre_receive_test.go +++ b/internal/gitaly/service/hook/pre_receive_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -350,52 +351,71 @@ func TestPreReceiveHook_CustomHookErrors(t *testing.T) { ctx := testhelper.Context(t) repo, repoPath := gittest.CreateRepository(t, ctx, cfg) - customHookReturnCode := int32(128) - customHookReturnMsg := "custom hook error" + testCases := []struct { + desc string + returnCode int32 + returnMsg string + }{ + { + desc: "normal custom hook error", + returnCode: 128, + returnMsg: "custom hook error", + }, + { + desc: "large error message", + returnCode: 1, + returnMsg: strings.Repeat("a", (4*1024*1024)+1), + }, + } - //nolint:gitaly-linters - gittest.WriteCustomHook(t, repoPath, "pre-receive", []byte(fmt.Sprintf(`#!/usr/bin/env bash + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + //nolint:gitaly-linters + gittest.WriteCustomHook(t, repoPath, "pre-receive", []byte(fmt.Sprintf(`#!/usr/bin/env bash echo '%s' 1>&2 exit %d -`, customHookReturnMsg, customHookReturnCode))) +`, tc.returnMsg, tc.returnCode))) - client, conn := newHooksClient(t, cfg.SocketPath) - defer conn.Close() + client, conn := newHooksClient(t, cfg.SocketPath) + defer conn.Close() - hooksPayload, err := gitcmd.NewHooksPayload( - ctx, - cfg, - repo, - gittest.DefaultObjectHash, - nil, - &gitcmd.UserDetails{ - UserID: "key-123", - Username: "username", - Protocol: "web", - }, - gitcmd.PreReceiveHook, - featureflag.FromContext(ctx), - storage.ExtractTransactionID(ctx), - ).Env() - require.NoError(t, err) + hooksPayload, err := gitcmd.NewHooksPayload( + ctx, + cfg, + repo, + gittest.DefaultObjectHash, + nil, + &gitcmd.UserDetails{ + UserID: "key-123", + Username: "username", + Protocol: "web", + }, + gitcmd.PreReceiveHook, + featureflag.FromContext(ctx), + storage.ExtractTransactionID(ctx), + ).Env() + require.NoError(t, err) - stream, err := client.PreReceiveHook(ctx) - require.NoError(t, err) - require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{ - Repository: repo, - EnvironmentVariables: []string{ - hooksPayload, - }, - })) - require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{ - Stdin: []byte("changes\n"), - })) - require.NoError(t, stream.CloseSend()) + stream, err := client.PreReceiveHook(ctx) + require.NoError(t, err) + require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{ + Repository: repo, + EnvironmentVariables: []string{ + hooksPayload, + }, + })) + require.NoError(t, stream.Send(&gitalypb.PreReceiveHookRequest{ + Stdin: []byte("changes\n"), + })) + require.NoError(t, stream.CloseSend()) - _, stderr, status := receivePreReceive(t, stream, &bytes.Buffer{}) + _, stderr, status := receivePreReceive(t, stream, &bytes.Buffer{}) + + require.Equal(t, tc.returnCode, status) + assert.Equal(t, tc.returnMsg, text.ChompBytes(stderr), "hook stderr") + }) + } - require.Equal(t, customHookReturnCode, status) - assert.Equal(t, customHookReturnMsg, text.ChompBytes(stderr), "hook stderr") } func TestPreReceiveHook_Primary(t *testing.T) { -- GitLab