sidechannel: Convert to use runtime directory to store socketspks-sidechannel-migrate-to-runtime-dir

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

8 files changed, 125 insertions, 6 deletions

diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 16021c4e5..125be64d6 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -371,15 +371,15 @@ func referenceTransactionHook(ctx context.Context, payload git.HooksPayload, hoo
 }
 
 func packObjectsHook(ctx context.Context, payload git.HooksPayload, hookClient gitalypb.HookServiceClient, args []string) error {
-	if err := handlePackObjectsWithSidechannel(ctx, hookClient, payload.Repo, args); err != nil {
+	if err := handlePackObjectsWithSidechannel(ctx, payload, hookClient, args); err != nil {
 		return hookError{returnCode: 1, err: fmt.Errorf("RPC failed: %w", err)}
 	}
 
 	return nil
 }
 
-func handlePackObjectsWithSidechannel(ctx context.Context, hookClient gitalypb.HookServiceClient, repo *gitalypb.Repository, args []string) error {
-	ctx, wt, err := hook.SetupSidechannel(ctx, func(c *net.UnixConn) error {
+func handlePackObjectsWithSidechannel(ctx context.Context, payload git.HooksPayload, hookClient gitalypb.HookServiceClient, args []string) error {
+	ctx, wt, err := hook.SetupSidechannel(ctx, payload, func(c *net.UnixConn) error {
 		return stream.ProxyPktLine(c, os.Stdin, os.Stdout, os.Stderr)
 	})
 	if err != nil {
@@ -389,7 +389,7 @@ func handlePackObjectsWithSidechannel(ctx context.Context, hookClient gitalypb.H
 
 	if _, err := hookClient.PackObjectsHookWithSidechannel(
 		ctx,
-		&gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo, Args: args},
+		&gitalypb.PackObjectsHookWithSidechannelRequest{Repository: payload.Repo, Args: args},
 	); err != nil {
 		return fmt.Errorf("call PackObjectsHookWithSidechannel: %w", err)
 	}
diff --git a/internal/git/hooks_payload.go b/internal/git/hooks_payload.go
index 7b6deb60c..d12bb0a81 100644
--- a/internal/git/hooks_payload.go
+++ b/internal/git/hooks_payload.go
@@ -58,6 +58,9 @@ type HooksPayload struct {
 
 	// Repo is the repository in which the hook is running.
 	Repo *gitalypb.Repository `json:"-"`
+
+	// RuntimeDir is the path to Gitaly's runtime directory.
+	RuntimeDir string `json:"runtime_dir"`
 	// InternalSocket is the path to Gitaly's internal socket.
 	InternalSocket string `json:"internal_socket"`
 	// InternalSocketToken is the token required to authenticate with
@@ -105,6 +108,7 @@ func NewHooksPayload(
 ) HooksPayload {
 	return HooksPayload{
 		Repo:                repo,
+		RuntimeDir:          cfg.RuntimeDir,
 		InternalSocket:      cfg.GitalyInternalSocketPath(),
 		InternalSocketToken: cfg.Auth.Token,
 		Transaction:         tx,
diff --git a/internal/git/hooks_payload_test.go b/internal/git/hooks_payload_test.go
index 1c40b5a4d..c1e97e380 100644
--- a/internal/git/hooks_payload_test.go
+++ b/internal/git/hooks_payload_test.go
@@ -40,6 +40,7 @@ func TestHooksPayload(t *testing.T) {
 
 		require.Equal(t, git.HooksPayload{
 			Repo:           repo,
+			RuntimeDir:     cfg.RuntimeDir,
 			InternalSocket: cfg.GitalyInternalSocketPath(),
 			RequestedHooks: git.PreReceiveHook,
 			FeatureFlags:   featureflag.Raw{"flag-key": "flag-value"},
@@ -55,6 +56,7 @@ func TestHooksPayload(t *testing.T) {
 
 		require.Equal(t, git.HooksPayload{
 			Repo:           repo,
+			RuntimeDir:     cfg.RuntimeDir,
 			InternalSocket: cfg.GitalyInternalSocketPath(),
 			Transaction:    &tx,
 			RequestedHooks: git.UpdateHook,
@@ -90,6 +92,7 @@ func TestHooksPayload(t *testing.T) {
 
 		require.Equal(t, git.HooksPayload{
 			Repo:                repo,
+			RuntimeDir:          cfg.RuntimeDir,
 			InternalSocket:      cfg.GitalyInternalSocketPath(),
 			InternalSocketToken: cfg.Auth.Token,
 			ReceiveHooksPayload: &git.ReceiveHooksPayload{
diff --git a/internal/gitaly/hook/sidechannel.go b/internal/gitaly/hook/sidechannel.go
index ca83a11f0..54b945c11 100644
--- a/internal/gitaly/hook/sidechannel.go
+++ b/internal/gitaly/hook/sidechannel.go
@@ -2,12 +2,16 @@ package hook
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io/fs"
 	"net"
 	"os"
 	"path"
+	"path/filepath"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	gitaly_metadata "gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
 	"google.golang.org/grpc/metadata"
 )
@@ -39,8 +43,28 @@ func GetSidechannel(ctx context.Context) (net.Conn, error) {
 // receives a connection. The address of the listener is stored in the
 // returned context, so that the caller can propagate it to a server. The
 // caller must Close the SidechannelWaiter to prevent resource leaks.
-func SetupSidechannel(ctx context.Context, callback func(*net.UnixConn) error) (_ context.Context, _ *SidechannelWaiter, err error) {
-	socketDir, err := os.MkdirTemp("", "gitaly")
+func SetupSidechannel(ctx context.Context, payload git.HooksPayload, callback func(*net.UnixConn) error) (_ context.Context, _ *SidechannelWaiter, err error) {
+	var sidechannelDir, sidechannelName string
+
+	// If there is a runtime directory we try to create a sidechannel directory in there that
+	// will hold all the temporary sidechannel subdirectories. Otherwise, we fall back to create
+	// the sidechannel directory in the system's temporary directory.
+	if payload.RuntimeDir != "" {
+		sidechannelDir := filepath.Join(payload.RuntimeDir, "chan.d")
+
+		// Note that we don't use `os.MkdirAll()` here: we don't want to accidentally create
+		// the full directory hierarchy, and the assumption is that the runtime directory
+		// must exist already.
+		if err := os.Mkdir(sidechannelDir, 0o700); err != nil && !errors.Is(err, fs.ErrExist) {
+			return nil, nil, err
+		}
+
+		sidechannelName = "*"
+	} else {
+		sidechannelName = "gitaly*"
+	}
+
+	socketDir, err := os.MkdirTemp(sidechannelDir, sidechannelName)
 	if err != nil {
 		return nil, nil, err
 	}
diff --git a/internal/gitaly/hook/sidechannel_test.go b/internal/gitaly/hook/sidechannel_test.go
index cffe5305a..0ac677df6 100644
--- a/internal/gitaly/hook/sidechannel_test.go
+++ b/internal/gitaly/hook/sidechannel_test.go
@@ -3,20 +3,42 @@ package hook
 import (
 	"io"
 	"net"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	grpc_metadata "google.golang.org/grpc/metadata"
 )
 
+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 TestSidechannel(t *testing.T) {
+	t.Parallel()
+	runTestsWithRuntimeDir(t, testSidechannelWithRuntimeDir)
+}
+
+func testSidechannelWithRuntimeDir(t *testing.T, runtimeDir string) {
 	ctx := testhelper.Context(t)
 
 	// Client side
 	ctxOut, wt, err := SetupSidechannel(
 		ctx,
+		git.HooksPayload{
+			RuntimeDir: runtimeDir,
+		},
 		func(c *net.UnixConn) error {
 			_, err := io.WriteString(c, "ping")
 			return err
@@ -41,11 +63,23 @@ func TestSidechannel(t *testing.T) {
 
 	// Client side
 	require.NoError(t, wt.Wait())
+
+	if runtimeDir != "" {
+		require.DirExists(t, filepath.Join(runtimeDir, "chan.d"))
+	}
 }
 
 func TestSidechannel_cleanup(t *testing.T) {
+	t.Parallel()
+	runTestsWithRuntimeDir(t, testSidechannelCleanupWithRuntimeDir)
+}
+
+func testSidechannelCleanupWithRuntimeDir(t *testing.T, runtimeDir string) {
 	_, wt, err := SetupSidechannel(
 		testhelper.Context(t),
+		git.HooksPayload{
+			RuntimeDir: runtimeDir,
+		},
 		func(c *net.UnixConn) error { return nil },
 	)
 	require.NoError(t, err)
@@ -54,6 +88,10 @@ func TestSidechannel_cleanup(t *testing.T) {
 	require.DirExists(t, wt.socketDir)
 	_ = wt.Close()
 	require.NoDirExists(t, wt.socketDir)
+
+	if runtimeDir != "" {
+		require.DirExists(t, filepath.Join(runtimeDir, "chan.d"))
+	}
 }
 
 func TestGetSidechannel(t *testing.T) {
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{