rubyserver: Ensure we sync objects if there is no Rugged gitconfigpks-rugged-inject-own-gitconfig

The Gitaly config exposes a `rugged_git_config_search_path` config that
allows the administrator to configure where Rugged should search for the
gitconfig it is supposed to use. While this option rather feels like an
edge case, it is in fact currently mandatory to set this configuration
to a gitconfig that sets `core.fsyncObjectFiles=true`. If there is no
such configuration, then the result is that Rugged will not flush newly
written objects to disk, which can easily lead to repository corruption.

Luckily, both CNG and Omnibus know to set up this config as expected.
But this brings its own problem with it: we have `core.fsyncObjectFiles`
set in a central location now by distributions, which also means that
Git as spawned by our Git command factory picks it up. But because that
option has been deprecated in Git v2.36.0 this means that all Git
commands would now print a warning. So we're between a rock and a hard
place now: we must make sure that the configuration is set in Rugged,
but it must not be present in the Git configuration.

Given that this is the only configuration that Rugged really needs, and
given that it must always be present or we otherwise corrupt repos, we
can fix this issue by simply writing our own Gitconfig file in case
`rugged_git_config_search_path` is unset. Like this, we can stop setting
the search path in distributions and eventually deprecate its use.

2 files changed, 124 insertions, 0 deletions

diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 125377514..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.
@@ -120,6 +121,10 @@ func (s *Server) Stop() {
 			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)
+		})
+	}
+}