diff options
-rw-r--r-- | internal/gitaly/service/ssh/server.go | 39 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_archive.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_archive_test.go | 25 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/upload_pack_test.go | 26 |
5 files changed, 72 insertions, 23 deletions
diff --git a/internal/gitaly/service/ssh/server.go b/internal/gitaly/service/ssh/server.go index 12bc965cd..95188c11e 100644 --- a/internal/gitaly/service/ssh/server.go +++ b/internal/gitaly/service/ssh/server.go @@ -7,6 +7,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) @@ -17,12 +18,12 @@ var ( type server struct { gitalypb.UnimplementedSSHServiceServer - locator storage.Locator - gitCmdFactory git.CommandFactory - txManager transaction.Manager - uploadPackRequestTimeout time.Duration - uploadArchiveRequestTimeout time.Duration - packfileNegotiationMetrics *prometheus.CounterVec + locator storage.Locator + gitCmdFactory git.CommandFactory + txManager transaction.Manager + uploadPackRequestTimeoutTickerFactory func() helper.Ticker + uploadArchiveRequestTimeoutTickerFactory func() helper.Ticker + packfileNegotiationMetrics *prometheus.CounterVec } // NewServer creates a new instance of a grpc SSHServer @@ -33,11 +34,15 @@ func NewServer( serverOpts ...ServerOpt, ) gitalypb.SSHServiceServer { s := &server{ - locator: locator, - gitCmdFactory: gitCmdFactory, - txManager: txManager, - uploadPackRequestTimeout: defaultUploadPackRequestTimeout, - uploadArchiveRequestTimeout: defaultUploadArchiveRequestTimeout, + locator: locator, + gitCmdFactory: gitCmdFactory, + txManager: txManager, + uploadPackRequestTimeoutTickerFactory: func() helper.Ticker { + return helper.NewTimerTicker(defaultUploadPackRequestTimeout) + }, + uploadArchiveRequestTimeoutTickerFactory: func() helper.Ticker { + return helper.NewTimerTicker(defaultUploadArchiveRequestTimeout) + }, packfileNegotiationMetrics: prometheus.NewCounterVec( prometheus.CounterOpts{}, []string{"git_negotiation_feature"}, @@ -54,17 +59,17 @@ func NewServer( // ServerOpt is a self referential option for server type ServerOpt func(s *server) -// WithUploadPackRequestTimeout sets the upload pack request timeout -func WithUploadPackRequestTimeout(d time.Duration) ServerOpt { +// WithUploadPackRequestTimeoutTickerFactory sets the upload pack request timeout ticker factory. +func WithUploadPackRequestTimeoutTickerFactory(factory func() helper.Ticker) ServerOpt { return func(s *server) { - s.uploadPackRequestTimeout = d + s.uploadPackRequestTimeoutTickerFactory = factory } } -// WithArchiveRequestTimeout sets the upload pack request timeout -func WithArchiveRequestTimeout(d time.Duration) ServerOpt { +// WithArchiveRequestTimeoutTickerFactory sets the upload pack request timeout ticker factory. +func WithArchiveRequestTimeoutTickerFactory(factory func() helper.Ticker) ServerOpt { return func(s *server) { - s.uploadArchiveRequestTimeout = d + s.uploadArchiveRequestTimeoutTickerFactory = factory } } diff --git a/internal/gitaly/service/ssh/upload_archive.go b/internal/gitaly/service/ssh/upload_archive.go index 8f087bb0b..610778943 100644 --- a/internal/gitaly/service/ssh/upload_archive.go +++ b/internal/gitaly/service/ssh/upload_archive.go @@ -10,7 +10,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/pktline" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service" - "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v15/streamio" @@ -62,7 +61,7 @@ func (s *server) sshUploadArchive(stream gitalypb.SSHService_SSHUploadArchiveSer return err } - timeoutTicker := helper.NewTimerTicker(s.uploadArchiveRequestTimeout) + timeoutTicker := s.uploadArchiveRequestTimeoutTickerFactory() // upload-archive expects a list of options terminated by a flush packet: // https://github.com/git/git/blob/v2.22.0/builtin/upload-archive.c#L38 diff --git a/internal/gitaly/service/ssh/upload_archive_test.go b/internal/gitaly/service/ssh/upload_archive_test.go index 87be3beaa..350a972c9 100644 --- a/internal/gitaly/service/ssh/upload_archive_test.go +++ b/internal/gitaly/service/ssh/upload_archive_test.go @@ -4,11 +4,11 @@ import ( "fmt" "os" "testing" - "time" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" @@ -23,7 +23,23 @@ func TestFailedUploadArchiveRequestDueToTimeout(t *testing.T) { cfg := testcfg.Build(t) - cfg.SocketPath = runSSHServerWithOptions(t, cfg, []ServerOpt{WithArchiveRequestTimeout(100 * time.Microsecond)}) + // Use a ticker channel so that we can observe that the ticker is being created. The channel + // is unbuffered on purpose so that we can assert that it is getting created exactly at the + // time we expect it to be. + tickerCh := make(chan *helper.ManualTicker) + + cfg.SocketPath = runSSHServerWithOptions(t, cfg, []ServerOpt{ + WithArchiveRequestTimeoutTickerFactory(func() helper.Ticker { + // Create a ticker that will immediately tick when getting reset so that the + // server-side can observe this as an emulated timeout. + ticker := helper.NewManualTicker() + ticker.ResetFunc = func() { + ticker.Tick() + } + tickerCh <- ticker + return ticker + }), + }) ctx := testhelper.Context(t) repo, _ := gittest.CreateRepository(t, ctx, cfg) @@ -36,6 +52,11 @@ func TestFailedUploadArchiveRequestDueToTimeout(t *testing.T) { // The first request is not limited by timeout, but also not under attacker control require.NoError(t, stream.Send(&gitalypb.SSHUploadArchiveRequest{Repository: repo})) + // We should now see that the ticker limiting the request is being created. We don't need to + // use the ticker, but this statement is only there in order to verify that the ticker is + // indeed getting created at the expected point in time. + <-tickerCh + // Because the client says nothing, the server would block. Because of // the timeout, it won't block forever, and return with a non-zero exit // code instead. diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go index 7542c3a2a..f08482d39 100644 --- a/internal/gitaly/service/ssh/upload_pack.go +++ b/internal/gitaly/service/ssh/upload_pack.go @@ -137,7 +137,7 @@ func (s *server) sshUploadPack(rpcContext context.Context, req sshUploadPackRequ return 0, err } - timeoutTicker := helper.NewTimerTicker(s.uploadPackRequestTimeout) + timeoutTicker := s.uploadPackRequestTimeoutTickerFactory() // upload-pack negotiation is terminated by either a flush, or the "done" // packet: https://github.com/git/git/blob/v2.20.0/Documentation/technical/pack-protocol.txt#L335 diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go index e6376e256..a40ddfb51 100644 --- a/internal/gitaly/service/ssh/upload_pack_test.go +++ b/internal/gitaly/service/ssh/upload_pack_test.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" @@ -97,10 +98,28 @@ func TestUploadPack_timeout(t *testing.T) { } func testUploadPackTimeout(t *testing.T, opts ...testcfg.Option) { + t.Parallel() + ctx := testhelper.Context(t) cfg := testcfg.Build(t, opts...) - cfg.SocketPath = runSSHServerWithOptions(t, cfg, []ServerOpt{WithUploadPackRequestTimeout(1)}) + // Use a ticker channel so that we can observe that the ticker is being created. The channel + // is unbuffered on purpose so that we can assert that it is getting created exactly at the + // time we expect it to be. + tickerCh := make(chan *helper.ManualTicker) + + cfg.SocketPath = runSSHServerWithOptions(t, cfg, []ServerOpt{ + WithUploadPackRequestTimeoutTickerFactory(func() helper.Ticker { + // Create a ticker that will immediately tick when getting reset so that the + // server-side can observe this as an emulated timeout. + ticker := helper.NewManualTicker() + ticker.ResetFunc = func() { + ticker.Tick() + } + tickerCh <- ticker + return ticker + }), + }) repo, repoPath := gittest.CreateRepository(t, testhelper.Context(t), cfg) gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main")) @@ -113,6 +132,11 @@ func testUploadPackTimeout(t *testing.T, opts ...testcfg.Option) { // The first request is not limited by timeout, but also not under attacker control require.NoError(t, stream.Send(&gitalypb.SSHUploadPackRequest{Repository: repo})) + // We should now see that the ticker limiting the request is being created. We don't need to + // use the ticker, but this statement is only there in order to verify that the ticker is + // indeed getting created at the expected point in time. + <-tickerCh + // Because the client says nothing, the server would block. Because of // the timeout, it won't block forever, and return with a non-zero exit // code instead. |