git: Track supported execution environments in a single place

The knowledge about which execution environments exist is distributed
across multiple different locations: first we need to set them up, and
second we need to make use them depending on whether they have been
configured and enabled. This is really fragile and ultimately hard to
maintain.

We now have the logic to set up the different environments in self
contained structures which all have the same interface. It is thus
trivial to move this logic into a single place, which is a new array of
all supported execution environment constructors. The Git command
factory can thus be stripped of all knowledge around which different
execution environments exists and only needs to loop through all
constructors -- if at least one of them returns a valid environment,
then we're good to go.

We need to maintain two properties though:

    - We prioritize the order in which we consult the different
      environments. If bundled Git is configured, then it should be
      preferred over distributed Git. And if distributed Git is
      configured, it should be preferred over the fallback Git
      environment which was resolved via PATH.

    - Environments must be feature-flaggable. So even if an environment
      with a higher priority is available, it may still be disabled
      which would thus cause us to pick a lower-priority backend.

These properties are maintained by encoding the priority of backends via
the order in which environment constructors are inside their array. The
Git command factory thus constructs all environments in the order they
are listed in and will then loop through them. The first backend that is
enabled (that is, all its feature flags are turned on) will be picked.

Like this, it is trivial to add new execution environments as they are
all tracked in a single central location.

3 files changed, 103 insertions, 62 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 3c3935d06..a8a9bdbf9 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -43,11 +43,6 @@ var globalOptions = []GlobalOption{
 	ConfigPair{Key: "core.useReplaceRefs", Value: "false"},
 }
 
-type executionEnvironments struct {
-	externalGit ExecutionEnvironment
-	bundledGit  ExecutionEnvironment
-}
-
 // CommandFactory is designed to create and run git commands in a protected and fully managed manner.
 type CommandFactory interface {
 	// New creates a new command for the repo repository.
@@ -106,7 +101,7 @@ type cachedGitVersion struct {
 type ExecCommandFactory struct {
 	locator               storage.Locator
 	cfg                   config.Cfg
-	execEnvs              executionEnvironments
+	execEnvs              []ExecutionEnvironment
 	cgroupsManager        cgroups.Manager
 	invalidCommandsMetric *prometheus.CounterVec
 	hookDirs              hookDirectories
@@ -169,53 +164,38 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_
 
 // setupGitExecutionEnvironments assembles a Git execution environment that can be used to run Git
 // commands. It warns if no path was specified in the configuration.
-func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactoryConfig) (executionEnvironments, func(), error) {
+func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactoryConfig) ([]ExecutionEnvironment, func(), error) {
 	if factoryCfg.gitBinaryPath != "" {
-		return executionEnvironments{
-			externalGit: ExecutionEnvironment{
-				BinaryPath: factoryCfg.gitBinaryPath,
-			},
+		return []ExecutionEnvironment{
+			{BinaryPath: factoryCfg.gitBinaryPath},
 		}, func() {}, nil
 	}
 
-	var execEnvs executionEnvironments
-	var cleanups []func()
+	var execEnvs []ExecutionEnvironment
+	for _, constructor := range ExecutionEnvironmentConstructors {
+		execEnv, err := constructor.Construct(cfg)
+		if err != nil {
+			// In case the environment has not been configured by the user we simply
+			// skip it.
+			if errors.Is(err, ErrNotConfigured) {
+				continue
+			}
 
-	if execEnv, err := (DistributedGitEnvironmentConstructor{}).Construct(cfg); err != nil {
-		if !errors.Is(err, ErrNotConfigured) {
-			return executionEnvironments{}, nil, fmt.Errorf("constructing distributed Git environment: %w", err)
+			// But if it has been configured and we fail to set it up then it signifies
+			// a real error.
+			return nil, nil, fmt.Errorf("constructing Git environment: %w", err)
 		}
-	} else {
-		execEnvs.externalGit = execEnv
-		cleanups = append(cleanups, execEnv.Cleanup)
-	}
 
-	if execEnv, err := (BundledGitEnvironmentConstructor{}).Construct(cfg); err != nil {
-		if !errors.Is(err, ErrNotConfigured) {
-			return executionEnvironments{}, nil, fmt.Errorf("constructing bundled Git environment: %w", err)
-		}
-	} else {
-		execEnvs.bundledGit = execEnv
-		cleanups = append(cleanups, execEnv.Cleanup)
+		execEnvs = append(execEnvs, execEnv)
 	}
 
-	// If neither an external Git distribution nor a bundled Git was configured we need to
-	// fall back to resolve the Git executable via PATH. We explicitly don't want to do this
-	// in case either one of those has been configured though: we have no clue where system Git
-	// comes from and what its version is, so it's the worst of all choices.
-	if execEnvs.externalGit.BinaryPath == "" && execEnvs.bundledGit.BinaryPath == "" {
-		execEnv, err := (FallbackGitEnvironmentConstructor{}).Construct(cfg)
-		if err != nil {
-			return executionEnvironments{}, nil, fmt.Errorf("constructing fallback Git environment: %w", err)
-		}
-
-		execEnvs.externalGit = execEnv
-		cleanups = append(cleanups, execEnv.Cleanup)
+	if len(execEnvs) == 0 {
+		return nil, nil, fmt.Errorf("could not set up any Git execution environments")
 	}
 
 	return execEnvs, func() {
-		for i := len(cleanups) - 1; i >= 0; i-- {
-			cleanups[i]()
+		for _, execEnv := range execEnvs {
+			execEnv.Cleanup()
 		}
 	}, nil
 }
@@ -254,21 +234,18 @@ func (cf *ExecCommandFactory) NewWithDir(ctx context.Context, dir string, sc Cmd
 
 // GetExecutionEnvironment returns parameters required to execute Git commands.
 func (cf *ExecCommandFactory) GetExecutionEnvironment(ctx context.Context) ExecutionEnvironment {
-	switch {
-	case cf.execEnvs.bundledGit.BinaryPath != "" && cf.execEnvs.externalGit.BinaryPath != "":
-		if featureflag.UseBundledGit.IsEnabled(ctx) {
-			return cf.execEnvs.bundledGit
+	// We first go through all execution environments and check whether any of them is enabled
+	// in the current context, which most importantly will check their respective feature flags.
+	for _, execEnv := range cf.execEnvs {
+		if execEnv.IsEnabled(ctx) {
+			return execEnv
 		}
-		return cf.execEnvs.externalGit
-	case cf.execEnvs.bundledGit.BinaryPath != "":
-		return cf.execEnvs.bundledGit
-	case cf.execEnvs.externalGit.BinaryPath != "":
-		return cf.execEnvs.externalGit
-	default:
-		// This shouldn't ever happen given that `setupGitExecutionEnvironments()` returns
-		// an error in case no environment was found.
-		panic("no Git execution environment defined")
 	}
+
+	// If none is enabled though, we simply use the first execution environment, which is also
+	// the one with highest priority. This can for example happen in case we only were able to
+	// construct a single execution environment that is currently feature flagged.
+	return cf.execEnvs[0]
 }
 
 // HooksPath returns the path where Gitaly's Git hooks reside.
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 405c35ba7..fe22951cd 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -155,7 +155,12 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 		gitCmdFactory, cleanup, err := git.NewExecCommandFactory(cfg, git.WithSkipHooks())
 		require.NoError(t, err)
 		defer cleanup()
-		require.Equal(t, expectedExecEnv, gitCmdFactory.GetExecutionEnvironment(ctx))
+
+		// We need to compare the execution environments manually because they also have
+		// some private variables which we cannot easily check here.
+		actualExecEnv := gitCmdFactory.GetExecutionEnvironment(ctx)
+		require.Equal(t, expectedExecEnv.BinaryPath, actualExecEnv.BinaryPath)
+		require.Equal(t, expectedExecEnv.EnvironmentVariables, actualExecEnv.EnvironmentVariables)
 	}
 
 	t.Run("set in config", func(t *testing.T) {
@@ -184,7 +189,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 
 		t.Run("missing bin_dir", func(t *testing.T) {
 			_, _, err := git.NewExecCommandFactory(config.Cfg{Git: config.Git{}}, git.WithSkipHooks())
-			require.EqualError(t, err, "setting up Git execution environment: constructing bundled Git environment: cannot use bundled binaries without bin path being set")
+			require.EqualError(t, err, "setting up Git execution environment: constructing Git environment: cannot use bundled binaries without bin path being set")
 		})
 
 		t.Run("missing gitaly-git executable", func(t *testing.T) {
@@ -257,7 +262,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "PATH", "")
 
 		_, _, err := git.NewExecCommandFactory(config.Cfg{}, git.WithSkipHooks())
-		require.EqualError(t, err, "setting up Git execution environment: constructing fallback Git environment: execution environment is not configured: no git executable found in PATH")
+		require.EqualError(t, err, "setting up Git execution environment: could not set up any Git execution environments")
 	})
 }
 
diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go
index a0759c213..71c6621bd 100644
--- a/internal/git/execution_environment.go
+++ b/internal/git/execution_environment.go
@@ -1,6 +1,7 @@
 package git
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"os"
@@ -9,11 +10,36 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 )
 
-// ErrNotConfigured may be returned by an ExecutionEnvironmentConstructor in case an environment
-// was not configured.
-var ErrNotConfigured = errors.New("execution environment is not configured")
+var (
+	// ErrNotConfigured may be returned by an ExecutionEnvironmentConstructor in case an
+	// environment was not configured.
+	ErrNotConfigured = errors.New("execution environment is not configured")
+
+	// ExecutionEnvironmentConstructors is the list of Git environments supported by the Git
+	// command factory. The order is important and signifies the priority in which the
+	// environments will be used: the environment created by the first constructor is the one
+	// that will be preferred when executing Git commands. Later environments may be used in
+	// case `IsEnabled()` returns `false` though.
+	ExecutionEnvironmentConstructors = []ExecutionEnvironmentConstructor{
+		BundledGitEnvironmentConstructor{
+			FeatureFlags: []featureflag.FeatureFlag{
+				featureflag.UseBundledGit,
+			},
+		},
+		DistributedGitEnvironmentConstructor{},
+		FallbackGitEnvironmentConstructor{},
+	}
+)
+
+// ExecutionEnvironmentConstructor is an interface for constructors of Git execution environments.
+// A constructor should be able to set up an environment in which it is possible to run Git
+// executables.
+type ExecutionEnvironmentConstructor interface {
+	Construct(config.Cfg) (ExecutionEnvironment, error)
+}
 
 // ExecutionEnvironment describes the environment required to execute a Git command
 type ExecutionEnvironment struct {
@@ -22,7 +48,8 @@ type ExecutionEnvironment struct {
 	// EnvironmentVariables are variables which must be set when running the Git binary.
 	EnvironmentVariables []string
 
-	cleanup func()
+	isEnabled func(context.Context) bool
+	cleanup   func()
 }
 
 // Cleanup cleans up any state set up by this ExecutionEnvironment.
@@ -32,6 +59,16 @@ func (e ExecutionEnvironment) Cleanup() {
 	}
 }
 
+// IsEnabled checks whether the ExecutionEnvironment is enabled in the given context. An execution
+// environment will typically be enabled by default, except if it's feature-flagged.
+func (e ExecutionEnvironment) IsEnabled(ctx context.Context) bool {
+	if e.isEnabled != nil {
+		return e.isEnabled(ctx)
+	}
+
+	return true
+}
+
 // DistributedGitEnvironmentConstructor creates ExecutionEnvironments via the Git binary path
 // configured in the Gitaly configuration. This expects a complete Git installation with all its
 // components. The installed distribution must either have its prefix compiled into the binaries or
@@ -64,7 +101,12 @@ func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (Executi
 // into Gitaly's binary directory. The binaries must have a `gitaly-` prefix like e.g. `gitaly-git`.
 // Bundled Git installations can be installed with Gitaly's Makefile via `make install
 // WITH_BUNDLED_GIT=YesPlease`.
-type BundledGitEnvironmentConstructor struct{}
+type BundledGitEnvironmentConstructor struct {
+	// FeatureFlags is the set of feature flags which must be enabled in order for the bundled
+	// Git environment to be enabled. Note that _all_ feature flags must be set to `true` in the
+	// context.
+	FeatureFlags []featureflag.FeatureFlag
+}
 
 // Construct sets up an ExecutionEnvironment for a bundled Git installation. Because bundled Git
 // installations are not complete Git installations we need to set up a usable environment at
@@ -155,6 +197,15 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 			"GIT_EXEC_PATH=" + gitExecPath,
 		},
 		cleanup: cleanup,
+		isEnabled: func(ctx context.Context) bool {
+			for _, flag := range c.FeatureFlags {
+				if flag.IsDisabled(ctx) {
+					return false
+				}
+			}
+
+			return true
+		},
 	}, nil
 }
 
@@ -180,5 +231,13 @@ func (c FallbackGitEnvironmentConstructor) Construct(config.Cfg) (ExecutionEnvir
 
 	return ExecutionEnvironment{
 		BinaryPath: resolvedPath,
+		// We always pretend that this environment is disabled. This has the effect that
+		// even if all the other existing execution environments are disabled via feature
+		// flags, we will still not use the fallback Git environment but will instead use
+		// the feature flagged environments. The fallback will then only be used in the
+		// case it is the only existing environment with no other better alternatives.
+		isEnabled: func(context.Context) bool {
+			return false
+		},
 	}, nil
 }