From 4fef8d1aa7cb10f295e07acff8f29fb924cf9c06 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 22 Mar 2022 12:30:05 +0100 Subject: sidechannel: Convert to use runtime directory to store sockets The sidechannel code is used to create sidechannels that circumvent gRPC for efficiency's sake. This socket is currently created in the system's temporary directory as returned by `os.MkdirTemp()`. This has the very real downside that it easily leaks created files and directories in case we have a logic bug anywhere. We have recently introduced a new runtime directory that can help us in this situation: the runtime directory is created once at a central place and will be cleaned up when shutting down Gitaly. Consequentially, even if we leaked and sidechannel files, we'd still remove them on shutdown. And even if we didn't: the runtime directory is designed so that we can check whether it's used because it has the current process's PID as part of its component. So if a runtime directory exists whose PID doesn't refer to any existing process it's safe to remove. While we don't have any such logic yet, it can easily be added at a later point and have all code which started to use the runtime directory benefit at the same time. Migrate the sidechannel code to create sockets in a subdirectory within the runtime directory called "sidechannel.d" if the runtime directory is set via the hooks payload. Changelog: changed --- internal/gitaly/service/hook/pack_objects_test.go | 48 ++++++++++++++++++++++ .../gitaly/service/smarthttp/receive_pack_test.go | 1 + internal/gitaly/service/ssh/receive_pack_test.go | 1 + 3 files changed, 50 insertions(+) (limited to 'internal/gitaly/service') diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go index 54deef710..e2ffb15b8 100644 --- a/internal/gitaly/service/hook/pack_objects_test.go +++ b/internal/gitaly/service/hook/pack_objects_test.go @@ -11,6 +11,7 @@ import ( "github.com/sirupsen/logrus" "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/v14/internal/git" "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v14/internal/git/pktline" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" @@ -23,6 +24,18 @@ import ( "google.golang.org/grpc/codes" ) +func runTestsWithRuntimeDir(t *testing.T, testFunc func(*testing.T, string)) { + t.Helper() + + t.Run("no runtime dir", func(t *testing.T) { + testFunc(t, "") + }) + + t.Run("with runtime dir", func(t *testing.T) { + testFunc(t, testhelper.TempDir(t)) + }) +} + func cfgWithCache(t *testing.T) config.Cfg { cfg := testcfg.Build(t) cfg.PackObjectsCache.Enabled = true @@ -59,6 +72,11 @@ func TestParsePackObjectsArgs(t *testing.T) { } func TestServer_PackObjectsHook_separateContext(t *testing.T) { + t.Parallel() + runTestsWithRuntimeDir(t, testServerPackObjectsHookSeparateContextWithRuntimeDir) +} + +func testServerPackObjectsHookSeparateContextWithRuntimeDir(t *testing.T, runtimeDir string) { cfg := cfgWithCache(t) cfg.SocketPath = runHooksServer(t, cfg, nil) @@ -85,6 +103,9 @@ func TestServer_PackObjectsHook_separateContext(t *testing.T) { ctx1, wt1, err := hookPkg.SetupSidechannel( ctx1, + git.HooksPayload{ + RuntimeDir: runtimeDir, + }, func(c *net.UnixConn) error { defer close(start2) <-start1 @@ -121,6 +142,9 @@ func TestServer_PackObjectsHook_separateContext(t *testing.T) { var stdout2 []byte ctx2, wt2, err := hookPkg.SetupSidechannel( ctx2, + git.HooksPayload{ + RuntimeDir: runtimeDir, + }, func(c *net.UnixConn) error { <-start2 if _, err := io.WriteString(c, stdin); err != nil { @@ -161,6 +185,11 @@ func TestServer_PackObjectsHook_separateContext(t *testing.T) { } func TestServer_PackObjectsHook_usesCache(t *testing.T) { + t.Parallel() + runTestsWithRuntimeDir(t, testServerPackObjectsHookUsesCache) +} + +func testServerPackObjectsHookUsesCache(t *testing.T, runtimeDir string) { cfg := cfgWithCache(t) tlc := &streamcache.TestLoggingCache{} @@ -178,6 +207,9 @@ func TestServer_PackObjectsHook_usesCache(t *testing.T) { var stdout []byte ctx, wt, err := hookPkg.SetupSidechannel( ctx, + git.HooksPayload{ + RuntimeDir: runtimeDir, + }, func(c *net.UnixConn) error { if _, err := io.WriteString(c, "3dd08961455abf80ef9115f4afdc1c6f968b503c\n--not\n\n"); err != nil { return err @@ -236,7 +268,10 @@ func TestServer_PackObjectsHook_usesCache(t *testing.T) { func TestServer_PackObjectsHookWithSidechannel(t *testing.T) { t.Parallel() + runTestsWithRuntimeDir(t, testServerPackObjectsHookWithSidechannelWithRuntimeDir) +} +func testServerPackObjectsHookWithSidechannelWithRuntimeDir(t *testing.T, runtimeDir string) { testCases := []struct { desc string stdin string @@ -268,6 +303,9 @@ func TestServer_PackObjectsHookWithSidechannel(t *testing.T) { var packets []string ctx, wt, err := hookPkg.SetupSidechannel( ctx, + git.HooksPayload{ + RuntimeDir: runtimeDir, + }, func(c *net.UnixConn) error { if _, err := io.WriteString(c, tc.stdin); err != nil { return err @@ -351,6 +389,8 @@ func TestServer_PackObjectsHookWithSidechannel(t *testing.T) { } func TestServer_PackObjectsHookWithSidechannel_invalidArgument(t *testing.T) { + t.Parallel() + cfg := testcfg.Build(t) cfg.SocketPath = runHooksServer(t, cfg, nil) ctx := testhelper.Context(t) @@ -389,11 +429,19 @@ func TestServer_PackObjectsHookWithSidechannel_invalidArgument(t *testing.T) { } func TestServer_PackObjectsHookWithSidechannel_Canceled(t *testing.T) { + t.Parallel() + runTestsWithRuntimeDir(t, testServerPackObjectsHookWithSidechannelCanceledWithRuntimeDir) +} + +func testServerPackObjectsHookWithSidechannelCanceledWithRuntimeDir(t *testing.T, runtimeDir string) { cfg := cfgWithCache(t) ctx := testhelper.Context(t) ctx, wt, err := hookPkg.SetupSidechannel( ctx, + git.HooksPayload{ + RuntimeDir: runtimeDir, + }, func(c *net.UnixConn) error { // Simulate a client that successfully initiates a request, but hangs up // before fully consuming the response. diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index 122defe7e..7eb070b4d 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -101,6 +101,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) { payload.Transaction = nil require.Equal(t, git.HooksPayload{ + RuntimeDir: cfg.RuntimeDir, InternalSocket: cfg.GitalyInternalSocketPath(), InternalSocketToken: cfg.Auth.Token, ReceiveHooksPayload: &git.ReceiveHooksPayload{ diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index cda2fa4aa..90797458d 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -158,6 +158,7 @@ func TestReceivePackPushSuccess(t *testing.T) { } require.Equal(t, git.HooksPayload{ + RuntimeDir: cfg.RuntimeDir, InternalSocket: cfg.GitalyInternalSocketPath(), InternalSocketToken: cfg.Auth.Token, ReceiveHooksPayload: &git.ReceiveHooksPayload{ -- cgit v1.2.3