Merge branch 'pks-rugged-inject-own-gitconfig' into 'master'

rubyserver: Ensure we sync objects if there is no Rugged gitconfig

Closes #4232

See merge request gitlab-org/gitaly!4560

2 files changed, 125 insertions, 1 deletions

diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 8db9c500d..bc14bd2c1 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -100,6 +100,7 @@ type Server struct {
 	workers       []*worker
 	clientConnMu  sync.Mutex
 	clientConn    *grpc.ClientConn
+	gitconfigDir  string
 }
 
 // New returns a new instance of the server.
@@ -117,8 +118,12 @@ func (s *Server) Stop() {
 		}
 
 		for _, w := range s.workers {
-			w.stopMonitor()
 			w.Process.Stop()
+			w.stopMonitor()
+		}
+
+		if s.gitconfigDir != "" {
+			_ = os.RemoveAll(s.gitconfigDir)
 		}
 	}
 }
@@ -136,6 +141,43 @@ func (s *Server) start() error {
 	}
 
 	cfg := s.cfg
+
+	// Both Omnibus and CNG set up the gitconfig in a non-default location. This means that they
+	// need to tell us where to find it, which is done via the Rugged config search path. In
+	// fact though, this configuration really only contains a single entry that is of importance
+	// to us in the context of Rugged, which is `core.fsyncObjectFiles`: if not set, then we may
+	// fail to persist objects correctly and thus corrupt the repository. We don't care about
+	// anything else nowadays anymore because most of the functionality was stripped out of the
+	// sidecar.
+	//
+	// Because we only care about a single option, and because that option is in fact mandatory
+	// or we may end up with corrupted data, we want to get rid of this configuration. Rugged
+	// doesn't give us any way to force-enable fsyncing though except if we write it to a file.
+	// Consequentially, we'll have to inject our own gitconfig into Rugged that enables this
+	// config. And that's exactly what the following block does: if we detect that the distro
+	// isn't telling us where to find the Rugged configuration, we write our own config. This is
+	// required so that we can phase out support of the gitconfig in these distributions.
+	//
+	// This is transitory until either the sidecar goes away or the upstream pull request is
+	// released (https://github.com/libgit2/rugged/pull/918).
+	if cfg.Ruby.RuggedGitConfigSearchPath == "" {
+		gitconfigDir := filepath.Join(cfg.RuntimeDir, "ruby-gitconfig")
+		if err := os.Mkdir(gitconfigDir, 0o777); err != nil {
+			return fmt.Errorf("creating gitconfig dir: %w", err)
+		}
+
+		// This file must be called `gitconfig` given that we pretend it's the system-level
+		// Git configuration. Otherwise, Rugged wouldn't find it.
+		if err := os.WriteFile(filepath.Join(gitconfigDir, "gitconfig"), []byte(
+			"[core]\n\tfsyncObjectFiles = true\n",
+		), 0o666); err != nil {
+			return fmt.Errorf("writing gitconfig: %w", err)
+		}
+
+		cfg.Ruby.RuggedGitConfigSearchPath = gitconfigDir
+		s.gitconfigDir = gitconfigDir
+	}
+
 	env, err := setupEnv(cfg, s.gitCmdFactory)
 	if err != nil {
 		return fmt.Errorf("setting up sidecar environment: %w", err)
diff --git a/internal/gitaly/rubyserver/rubyserver_test.go b/internal/gitaly/rubyserver/rubyserver_test.go
index a98d0eeb4..9e24585b8 100644
--- a/internal/gitaly/rubyserver/rubyserver_test.go
+++ b/internal/gitaly/rubyserver/rubyserver_test.go
@@ -3,11 +3,14 @@ package rubyserver
 import (
 	"context"
 	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"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/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/auth"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config/log"
@@ -141,3 +144,82 @@ func TestSetupEnv(t *testing.T) {
 		"GIT_CONFIG_COUNT=1",
 	})
 }
+
+func TestServer_gitconfig(t *testing.T) {
+	for _, tc := range []struct {
+		desc              string
+		setup             func(t *testing.T) (config.Cfg, string)
+		expectedGitconfig string
+	}{
+		{
+			desc: "writes a default gitconfig",
+			setup: func(t *testing.T) (config.Cfg, string) {
+				cfg := testcfg.Build(t)
+				expectedPath := filepath.Join(cfg.RuntimeDir, "ruby-gitconfig", "gitconfig")
+				return cfg, expectedPath
+			},
+			// If no search path is provided, we should create a placeholder file that
+			// contains all settings important to Rugged.
+			expectedGitconfig: "[core]\n\tfsyncObjectFiles = true\n",
+		},
+		{
+			desc: "uses injected gitconfig",
+			setup: func(t *testing.T) (config.Cfg, string) {
+				gitconfigDir := testhelper.TempDir(t)
+				expectedPath := filepath.Join(gitconfigDir, "gitconfig")
+				require.NoError(t, os.WriteFile(expectedPath, []byte("garbage"), 0o666))
+
+				cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{
+					Ruby: config.Ruby{
+						RuggedGitConfigSearchPath: gitconfigDir,
+					},
+				}))
+
+				return cfg, expectedPath
+			},
+			// The gitconfig shouldn't have been rewritten, so it should still contain
+			// garbage after the server starts.
+			expectedGitconfig: "garbage",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx := testhelper.Context(t)
+			cfg, expectedPath := tc.setup(t)
+
+			gitCmdFactory := gittest.NewCommandFactory(t, cfg)
+			locator := config.NewLocator(cfg)
+
+			rubyServer := New(cfg, gitCmdFactory)
+			require.NoError(t, rubyServer.Start())
+			defer rubyServer.Stop()
+
+			// Verify that the expected gitconfig exists and has expected contents.
+			gitconfigContents := testhelper.MustReadFile(t, expectedPath)
+			require.Equal(t, tc.expectedGitconfig, string(gitconfigContents))
+
+			// Write a gitconfig that is invalidly formatted. Like this we can assert
+			// whether the Ruby server tries to read it or not because it should in fact
+			// fail.
+			require.NoError(t, os.WriteFile(expectedPath, []byte(
+				`[`,
+			), 0o666))
+
+			repo, _ := gittest.InitRepo(t, cfg, cfg.Storages[0])
+
+			// We now do any random RPC request that hits the Ruby server...
+			client, err := rubyServer.WikiServiceClient(ctx)
+			require.NoError(t, err)
+
+			ctx, err = SetHeaders(ctx, locator, repo)
+			require.NoError(t, err)
+
+			stream, err := client.WikiListPages(ctx, &gitalypb.WikiListPagesRequest{Repository: repo})
+			require.NoError(t, err)
+
+			// ... and expect it to fail with an error parsing the configuration. This
+			// demonstrates the config was injected successfully.
+			_, err = stream.Recv()
+			testhelper.RequireGrpcError(t, fmt.Errorf("Rugged::ConfigError: failed to parse config file: missing ']' in section header (in %s:1)", expectedPath), err)
+		})
+	}
+}