gitaly-lfs-smudge: Refactor config-handling to have a central struct

The gitaly-lfs-smudge binary accepts a set of environment variables to
configure it. Because of this the configuration of the binary is kind of
hard to extend: for every piece of information that is required, we need
to add a new environment variable. Ultimately, this leads to a design
where we have lots of magic environment variables injected with no clear
indicator about which ones are used and which ones aren't, similar to
the case we had with gitaly-hooks before introducing the hooks payload.

Refactor the code and create a central `Config` struct which holds all
the configuration required. This prepares us for a fix where we can do
the same like we did for the hooks payload where we only inject a single
JSON-encoded environment variable that can then be parsed into this
struct.

5 files changed, 203 insertions, 104 deletions

diff --git a/cmd/gitaly-lfs-smudge/config.go b/cmd/gitaly-lfs-smudge/config.go
index d6bd7e139..b22742837 100644
--- a/cmd/gitaly-lfs-smudge/config.go
+++ b/cmd/gitaly-lfs-smudge/config.go
@@ -4,47 +4,49 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"os"
 
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/env"
 )
 
-type configProvider interface {
-	Get(key string) string
+// Config is the configuration used to run gitaly-lfs-smudge. It must be injected via environment
+// variables.
+type Config struct {
+	// GlRepository is the GitLab repository identifier that is required so that we can query
+	// the corresponding Rails' repository for the respective LFS contents.
+	GlRepository string
+	// Gitlab contains configuration so that we can connect to Rails in order to retrieve LFS
+	// contents.
+	Gitlab config.Gitlab
+	// TLS contains configuration for setting up a TLS-encrypted connection to Rails.
+	TLS config.TLS
 }
 
-type envConfig struct{}
+func configFromEnvironment(environment []string) (Config, error) {
+	var cfg Config
 
-func (e *envConfig) Get(key string) string {
-	return os.Getenv(key)
-}
-
-func loadConfig(cfgProvider configProvider) (config.Gitlab, config.TLS, string, error) {
-	var cfg config.Gitlab
-	var tlsCfg config.TLS
-
-	glRepository := cfgProvider.Get("GL_REPOSITORY")
-	if glRepository == "" {
-		return cfg, tlsCfg, "", fmt.Errorf("error loading project: GL_REPOSITORY is not defined")
+	cfg.GlRepository = env.ExtractValue(environment, "GL_REPOSITORY")
+	if cfg.GlRepository == "" {
+		return Config{}, fmt.Errorf("error loading project: GL_REPOSITORY is not defined")
 	}
 
-	u := cfgProvider.Get("GL_INTERNAL_CONFIG")
+	u := env.ExtractValue(environment, "GL_INTERNAL_CONFIG")
 	if u == "" {
-		return cfg, tlsCfg, glRepository, fmt.Errorf("unable to retrieve GL_INTERNAL_CONFIG")
+		return Config{}, fmt.Errorf("unable to retrieve GL_INTERNAL_CONFIG")
 	}
 
-	if err := json.Unmarshal([]byte(u), &cfg); err != nil {
-		return cfg, tlsCfg, glRepository, fmt.Errorf("unable to unmarshal GL_INTERNAL_CONFIG: %v", err)
+	if err := json.Unmarshal([]byte(u), &cfg.Gitlab); err != nil {
+		return Config{}, fmt.Errorf("unable to unmarshal GL_INTERNAL_CONFIG: %w", err)
 	}
 
-	u = cfgProvider.Get("GITALY_TLS")
+	u = env.ExtractValue(environment, "GITALY_TLS")
 	if u == "" {
-		return cfg, tlsCfg, glRepository, errors.New("unable to retrieve GITALY_TLS")
+		return Config{}, errors.New("unable to retrieve GITALY_TLS")
 	}
 
-	if err := json.Unmarshal([]byte(u), &tlsCfg); err != nil {
-		return cfg, tlsCfg, glRepository, fmt.Errorf("unable to unmarshal GITALY_TLS: %w", err)
+	if err := json.Unmarshal([]byte(u), &cfg.TLS); err != nil {
+		return Config{}, fmt.Errorf("unable to unmarshal GITALY_TLS: %w", err)
 	}
 
-	return cfg, tlsCfg, glRepository, nil
+	return cfg, nil
 }
diff --git a/cmd/gitaly-lfs-smudge/config_test.go b/cmd/gitaly-lfs-smudge/config_test.go
new file mode 100644
index 000000000..d5371c37a
--- /dev/null
+++ b/cmd/gitaly-lfs-smudge/config_test.go
@@ -0,0 +1,113 @@
+package main
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
+)
+
+func TestConfigFromEnvironment(t *testing.T) {
+	gitlabCfg := config.Gitlab{
+		URL:             "https://example.com",
+		RelativeURLRoot: "gitlab",
+		HTTPSettings: config.HTTPSettings{
+			ReadTimeout: 1,
+			User:        "user",
+			Password:    "correcthorsebatterystaple",
+			CAFile:      "/ca/file",
+			CAPath:      "/ca/path",
+			SelfSigned:  true,
+		},
+		SecretFile: "/secret/path",
+	}
+
+	tlsCfg := config.TLS{
+		CertPath: "/cert/path",
+		KeyPath:  "/key/path",
+	}
+
+	marshalledGitlabCfg, err := json.Marshal(gitlabCfg)
+	require.NoError(t, err)
+
+	marshalledTLSCfg, err := json.Marshal(tlsCfg)
+	require.NoError(t, err)
+
+	for _, tc := range []struct {
+		desc        string
+		environment []string
+		expectedErr string
+		expectedCfg Config
+	}{
+		{
+			desc:        "empty environment",
+			expectedErr: "error loading project: GL_REPOSITORY is not defined",
+		},
+		{
+			desc: "successful",
+			environment: []string{
+				"GL_REPOSITORY=repo",
+				"GL_INTERNAL_CONFIG=" + string(marshalledGitlabCfg),
+				"GITALY_TLS=" + string(marshalledTLSCfg),
+			},
+			expectedCfg: Config{
+				GlRepository: "repo",
+				Gitlab:       gitlabCfg,
+				TLS:          tlsCfg,
+			},
+		},
+		{
+			desc: "missing GlRepository",
+			environment: []string{
+				"GL_INTERNAL_CONFIG=" + string(marshalledGitlabCfg),
+				"GITALY_TLS=" + string(marshalledTLSCfg),
+			},
+			expectedErr: "error loading project: GL_REPOSITORY is not defined",
+		},
+		{
+			desc: "missing Gitlab config",
+			environment: []string{
+				"GL_REPOSITORY=repo",
+				"GITALY_TLS=" + string(marshalledTLSCfg),
+			},
+			expectedErr: "unable to retrieve GL_INTERNAL_CONFIG",
+		},
+		{
+			desc: "invalid Gitlab config",
+			environment: []string{
+				"GL_REPOSITORY=repo",
+				"GL_INTERNAL_CONFIG=invalid",
+				"GITALY_TLS=" + string(marshalledTLSCfg),
+			},
+			expectedErr: "unable to unmarshal GL_INTERNAL_CONFIG: invalid character 'i' looking for beginning of value",
+		},
+		{
+			desc: "missing TLS config",
+			environment: []string{
+				"GL_REPOSITORY=repo",
+				"GL_INTERNAL_CONFIG=" + string(marshalledGitlabCfg),
+			},
+			expectedErr: "unable to retrieve GITALY_TLS",
+		},
+		{
+			desc: "invalid TLS config",
+			environment: []string{
+				"GL_REPOSITORY=repo",
+				"GL_INTERNAL_CONFIG=" + string(marshalledGitlabCfg),
+				"GITALY_TLS=invalid",
+			},
+			expectedErr: "unable to unmarshal GITALY_TLS: invalid character 'i' looking for beginning of value",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			cfg, err := configFromEnvironment(tc.environment)
+			if tc.expectedErr == "" {
+				require.NoError(t, err)
+			} else {
+				require.EqualError(t, err, tc.expectedErr)
+			}
+			require.Equal(t, tc.expectedCfg, cfg)
+		})
+	}
+}
diff --git a/cmd/gitaly-lfs-smudge/lfs_smudge.go b/cmd/gitaly-lfs-smudge/lfs_smudge.go
index 780b379ae..501e8ca47 100644
--- a/cmd/gitaly-lfs-smudge/lfs_smudge.go
+++ b/cmd/gitaly-lfs-smudge/lfs_smudge.go
@@ -13,14 +13,14 @@ import (
 	"gitlab.com/gitlab-org/labkit/tracing"
 )
 
-func smudge(to io.Writer, from io.Reader, cfgProvider configProvider) (returnedErr error) {
+func smudge(cfg Config, to io.Writer, from io.Reader) (returnedErr error) {
 	// Since the environment is sanitized at the moment, we're only
 	// using this to extract the correlation ID. The finished() call
 	// to clean up the tracing will be a NOP here.
 	ctx, finished := tracing.ExtractFromEnv(context.Background())
 	defer finished()
 
-	output, err := handleSmudge(ctx, to, from, cfgProvider)
+	output, err := handleSmudge(ctx, cfg, to, from)
 	if err != nil {
 		return fmt.Errorf("smudging contents: %w", err)
 	}
@@ -37,7 +37,7 @@ func smudge(to io.Writer, from io.Reader, cfgProvider configProvider) (returnedE
 	return nil
 }
 
-func handleSmudge(ctx context.Context, to io.Writer, from io.Reader, config configProvider) (io.ReadCloser, error) {
+func handleSmudge(ctx context.Context, cfg Config, to io.Writer, from io.Reader) (io.ReadCloser, error) {
 	logger := log.ContextLogger(ctx)
 
 	ptr, contents, err := lfs.DecodeFrom(from)
@@ -48,23 +48,14 @@ func handleSmudge(ctx context.Context, to io.Writer, from io.Reader, config conf
 
 	logger.WithField("oid", ptr.Oid).Debug("decoded LFS OID")
 
-	glCfg, tlsCfg, glRepository, err := loadConfig(config)
-	if err != nil {
-		return io.NopCloser(contents), err
-	}
-
-	logger.WithField("gitlab_config", glCfg).
-		WithField("gitaly_tls_config", tlsCfg).
-		Debug("loaded GitLab API config")
-
-	client, err := gitlab.NewHTTPClient(logger, glCfg, tlsCfg, prometheus.Config{})
+	client, err := gitlab.NewHTTPClient(logger, cfg.Gitlab, cfg.TLS, prometheus.Config{})
 	if err != nil {
 		return io.NopCloser(contents), err
 	}
 
 	qs := url.Values{}
 	qs.Set("oid", ptr.Oid)
-	qs.Set("gl_repository", glRepository)
+	qs.Set("gl_repository", cfg.GlRepository)
 	u := url.URL{Path: "/lfs", RawQuery: qs.Encode()}
 
 	response, err := client.Get(ctx, u.String())
diff --git a/cmd/gitaly-lfs-smudge/lfs_smudge_test.go b/cmd/gitaly-lfs-smudge/lfs_smudge_test.go
index 3bbfaf39b..5d8e50db7 100644
--- a/cmd/gitaly-lfs-smudge/lfs_smudge_test.go
+++ b/cmd/gitaly-lfs-smudge/lfs_smudge_test.go
@@ -2,7 +2,6 @@ package main
 
 import (
 	"bytes"
-	"encoding/json"
 	"net/http"
 	"strings"
 	"testing"
@@ -44,14 +43,6 @@ var defaultOptions = gitlab.TestServerOptions{
 	ServerKeyPath:    keyPath,
 }
 
-type mapConfig struct {
-	env map[string]string
-}
-
-func (m *mapConfig) Get(key string) string {
-	return m.env[key]
-}
-
 func TestSuccessfulLfsSmudge(t *testing.T) {
 	testCases := []struct {
 		desc string
@@ -72,38 +63,36 @@ func TestSuccessfulLfsSmudge(t *testing.T) {
 			var b bytes.Buffer
 			reader := strings.NewReader(tc.data)
 
-			c, cleanup := runTestServer(t, defaultOptions)
+			gitlabCfg, cleanup := runTestServer(t, defaultOptions)
 			defer cleanup()
 
-			cfg, err := json.Marshal(c)
-			require.NoError(t, err)
-
-			tlsCfg, err := json.Marshal(config.TLS{
-				CertPath: certPath,
-				KeyPath:  keyPath,
-			})
-			require.NoError(t, err)
-
-			env := map[string]string{
-				"GL_REPOSITORY":      "project-1",
-				"GL_INTERNAL_CONFIG": string(cfg),
-				"GITALY_TLS":         string(tlsCfg),
+			cfg := Config{
+				GlRepository: "project-1",
+				Gitlab:       gitlabCfg,
+				TLS: config.TLS{
+					CertPath: certPath,
+					KeyPath:  keyPath,
+				},
 			}
-			cfgProvider := &mapConfig{env: env}
 
-			err = smudge(&b, reader, cfgProvider)
-			require.NoError(t, err)
+			require.NoError(t, smudge(cfg, &b, reader))
 			require.Equal(t, testData, b.String())
 		})
 	}
 }
 
 func TestUnsuccessfulLfsSmudge(t *testing.T) {
+	defaultConfig := func(t *testing.T, gitlabCfg config.Gitlab) Config {
+		return Config{
+			GlRepository: "project-1",
+			Gitlab:       gitlabCfg,
+		}
+	}
+
 	testCases := []struct {
 		desc              string
+		setupCfg          func(*testing.T, config.Gitlab) Config
 		data              string
-		missingEnv        string
-		tlsCfg            config.TLS
 		expectedError     bool
 		options           gitlab.TestServerOptions
 		expectedGitalyTLS string
@@ -111,38 +100,50 @@ func TestUnsuccessfulLfsSmudge(t *testing.T) {
 		{
 			desc:          "bad LFS pointer",
 			data:          "test data",
+			setupCfg:      defaultConfig,
 			options:       defaultOptions,
 			expectedError: false,
 		},
 		{
 			desc:          "invalid LFS pointer",
 			data:          invalidLfsPointer,
+			setupCfg:      defaultConfig,
 			options:       defaultOptions,
 			expectedError: false,
 		},
 		{
 			desc:          "invalid LFS pointer with non-hex characters",
 			data:          invalidLfsPointerWithNonHex,
+			setupCfg:      defaultConfig,
 			options:       defaultOptions,
 			expectedError: false,
 		},
 		{
-			desc:          "missing GL_REPOSITORY",
-			data:          lfsPointer,
-			missingEnv:    "GL_REPOSITORY",
+			desc: "missing GL_REPOSITORY",
+			data: lfsPointer,
+			setupCfg: func(t *testing.T, gitlabCfg config.Gitlab) Config {
+				cfg := defaultConfig(t, gitlabCfg)
+				cfg.GlRepository = ""
+				return cfg
+			},
 			options:       defaultOptions,
 			expectedError: true,
 		},
 		{
-			desc:          "missing GL_INTERNAL_CONFIG",
-			data:          lfsPointer,
-			missingEnv:    "GL_INTERNAL_CONFIG",
+			desc: "missing GL_INTERNAL_CONFIG",
+			data: lfsPointer,
+			setupCfg: func(t *testing.T, gitlabCfg config.Gitlab) Config {
+				cfg := defaultConfig(t, gitlabCfg)
+				cfg.Gitlab = config.Gitlab{}
+				return cfg
+			},
 			options:       defaultOptions,
 			expectedError: true,
 		},
 		{
-			desc: "failed HTTP response",
-			data: lfsPointer,
+			desc:     "failed HTTP response",
+			data:     lfsPointer,
+			setupCfg: defaultConfig,
 			options: gitlab.TestServerOptions{
 				SecretToken:   secretToken,
 				LfsBody:       testData,
@@ -153,41 +154,27 @@ func TestUnsuccessfulLfsSmudge(t *testing.T) {
 			expectedError: true,
 		},
 		{
-			desc:          "invalid TLS paths",
-			data:          lfsPointer,
+			desc: "invalid TLS paths",
+			data: lfsPointer,
+			setupCfg: func(t *testing.T, gitlabCfg config.Gitlab) Config {
+				cfg := defaultConfig(t, gitlabCfg)
+				cfg.TLS = config.TLS{CertPath: "fake-path", KeyPath: "not-real"}
+				return cfg
+			},
 			options:       defaultOptions,
-			tlsCfg:        config.TLS{CertPath: "fake-path", KeyPath: "not-real"},
 			expectedError: true,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			c, cleanup := runTestServer(t, tc.options)
+			gitlabCfg, cleanup := runTestServer(t, tc.options)
 			defer cleanup()
 
-			cfg, err := json.Marshal(c)
-			require.NoError(t, err)
-
-			tlsCfg, err := json.Marshal(tc.tlsCfg)
-			require.NoError(t, err)
-
-			env := map[string]string{
-				"GL_REPOSITORY":      "project-1",
-				"GL_INTERNAL_CONFIG": string(cfg),
-				"GITALY_TLS":         string(tlsCfg),
-			}
-
-			if tc.missingEnv != "" {
-				delete(env, tc.missingEnv)
-			}
-
-			cfgProvider := &mapConfig{env: env}
+			cfg := tc.setupCfg(t, gitlabCfg)
 
 			var b bytes.Buffer
-			reader := strings.NewReader(tc.data)
-
-			err = smudge(&b, reader, cfgProvider)
+			err := smudge(cfg, &b, strings.NewReader(tc.data))
 
 			if tc.expectedError {
 				require.Error(t, err)
diff --git a/cmd/gitaly-lfs-smudge/main.go b/cmd/gitaly-lfs-smudge/main.go
index 69e62fe77..397989b3c 100644
--- a/cmd/gitaly-lfs-smudge/main.go
+++ b/cmd/gitaly-lfs-smudge/main.go
@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/env"
 	gitalylog "gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/labkit/log"
 )
@@ -29,20 +30,20 @@ func requireStdin(msg string) {
 func main() {
 	requireStdin("This command should be run by the Git 'smudge' filter")
 
-	closer, err := initLogging(&envConfig{})
+	closer, err := initLogging(os.Environ())
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "error initializing log file for gitaly-lfs-smudge: %v", err)
 	}
 	defer closer.Close()
 
-	if err := run(os.Stdout, os.Stdin, &envConfig{}); err != nil {
+	if err := run(os.Environ(), os.Stdout, os.Stdin); err != nil {
 		log.WithError(err).Error(err)
 		os.Exit(1)
 	}
 }
 
-func initLogging(p configProvider) (io.Closer, error) {
-	path := p.Get(gitalylog.GitalyLogDirEnvKey)
+func initLogging(environment []string) (io.Closer, error) {
+	path := env.ExtractValue(environment, gitalylog.GitalyLogDirEnvKey)
 	if path == "" {
 		return log.Initialize(log.WithWriter(io.Discard))
 	}
@@ -56,8 +57,13 @@ func initLogging(p configProvider) (io.Closer, error) {
 	)
 }
 
-func run(out io.Writer, in io.Reader, cfgProvider configProvider) error {
-	if err := smudge(out, in, cfgProvider); err != nil {
+func run(environment []string, out io.Writer, in io.Reader) error {
+	cfg, err := configFromEnvironment(environment)
+	if err != nil {
+		return fmt.Errorf("loading configuration: %w", err)
+	}
+
+	if err := smudge(cfg, out, in); err != nil {
 		return fmt.Errorf("running smudge filter: %w", err)
 	}