gitaly: Validation of Git configuration

We want to make errors of configuration validation more verbose
and flexible to operate with. The idea is to produce a machine
friendly output that is why the ValidationError structure is defined.
The next steps are to refactor existing validation methods to return
errors as instances of the ValidationError combined into
ValidationErrors.

The first refactored validation is for the Git type. It now has a
new Validate method that returns all the validation errors found.

5 files changed, 279 insertions, 23 deletions

diff --git a/internal/errors/cfgerror/validate.go b/internal/errors/cfgerror/validate.go
new file mode 100644
index 000000000..54a31123b
--- /dev/null
+++ b/internal/errors/cfgerror/validate.go
@@ -0,0 +1,98 @@
+package cfgerror
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+)
+
+// ErrNotSet should be used when the value is not set, but it is required.
+var ErrNotSet = errors.New("not set")
+
+// ValidationError represents an issue with provided configuration.
+type ValidationError struct {
+	// Key represents a path to the field.
+	Key []string
+	// Cause contains a reason why validation failed.
+	Cause error
+}
+
+// Error to implement an error standard interface.
+// The string representation can have 3 different formats:
+// - when Key and Cause is set: "outer.inner: failure cause"
+// - when only Key is set: "outer.inner"
+// - when only Cause is set: "failure cause"
+func (ve ValidationError) Error() string {
+	if len(ve.Key) != 0 && ve.Cause != nil {
+		return fmt.Sprintf("%s: %v", strings.Join(ve.Key, "."), ve.Cause)
+	}
+	if len(ve.Key) != 0 {
+		return strings.Join(ve.Key, ".")
+	}
+	if ve.Cause != nil {
+		return fmt.Sprintf("%v", ve.Cause)
+	}
+	return ""
+}
+
+// NewValidationError creates a new ValidationError with provided parameters.
+func NewValidationError(err error, keys ...string) ValidationError {
+	return ValidationError{Key: keys, Cause: err}
+}
+
+// ValidationErrors is a list of ValidationError-s.
+type ValidationErrors []ValidationError
+
+// Append adds provided error into current list by enriching each ValidationError with the
+// provided keys or if provided err is not an instance of the ValidationError it will be wrapped
+// into it. In case the nil is provided nothing happens.
+func (vs ValidationErrors) Append(err error, keys ...string) ValidationErrors {
+	switch terr := err.(type) {
+	case nil:
+		return vs
+	case ValidationErrors:
+		for _, err := range terr {
+			vs = append(vs, ValidationError{
+				Key:   append(keys, err.Key...),
+				Cause: err.Cause,
+			})
+		}
+	case ValidationError:
+		vs = append(vs, ValidationError{
+			Key:   append(keys, terr.Key...),
+			Cause: terr.Cause,
+		})
+	default:
+		vs = append(vs, ValidationError{
+			Key:   keys,
+			Cause: err,
+		})
+	}
+
+	return vs
+}
+
+// AsError returns nil if there are no elements and itself if there is at least one.
+func (vs ValidationErrors) AsError() error {
+	if len(vs) != 0 {
+		return vs
+	}
+	return nil
+}
+
+// Error transforms all validation errors into a single string joined by newline.
+func (vs ValidationErrors) Error() string {
+	var buf strings.Builder
+	for i, ve := range vs {
+		if i != 0 {
+			buf.WriteString("\n")
+		}
+		buf.WriteString(ve.Error())
+	}
+	return buf.String()
+}
+
+// New returns uninitialized ValidationErrors object.
+func New() ValidationErrors {
+	return nil
+}
diff --git a/internal/errors/cfgerror/validate_test.go b/internal/errors/cfgerror/validate_test.go
new file mode 100644
index 000000000..847a62cc6
--- /dev/null
+++ b/internal/errors/cfgerror/validate_test.go
@@ -0,0 +1,101 @@
+package cfgerror
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidationError_Error(t *testing.T) {
+	t.Parallel()
+
+	require.Equal(t, "", ValidationError{}.Error())
+	require.Equal(t, "1.2", ValidationError{Key: []string{"1", "2"}}.Error())
+	require.Equal(t, "err", ValidationError{Cause: errors.New("err")}.Error())
+	require.Equal(t, "1.2: err", ValidationError{
+		Key:   []string{"1", "2"},
+		Cause: errors.New("err"),
+	}.Error())
+}
+
+func TestValidationErrors_Append(t *testing.T) {
+	t.Parallel()
+
+	err1 := NewValidationError(errors.New("bad-1"), "added")
+
+	t.Run("add nil", func(t *testing.T) {
+		var errs ValidationErrors
+		require.Equal(t, New(), errs.Append(nil, "some"))
+	})
+
+	t.Run("add ValidationError type", func(t *testing.T) {
+		var errs ValidationErrors
+		require.Equal(t, ValidationErrors{err1}, errs.Append(err1))
+
+		require.Equal(t, ValidationErrors{
+			{
+				Cause: err1.Cause,
+				Key:   append([]string{"2"}, err1.Key...),
+			},
+		}, errs.Append(err1, "2"))
+	})
+
+	t.Run("add ValidationErrors type", func(t *testing.T) {
+		err2 := NewValidationError(errors.New("bad-2"), "nested")
+		err3 := NewValidationError(errors.New("bad-2"), "root", "outer", "nested")
+
+		var errs ValidationErrors
+		require.Equal(t, ValidationErrors{err1}, errs.Append(ValidationErrors{err1}))
+
+		require.Equal(t, ValidationErrors{err3}, errs.Append(ValidationErrors{err2}, "root", "outer"))
+	})
+
+	t.Run("add not ValidationError(s) type", func(t *testing.T) {
+		var errs ValidationErrors
+		expected := ValidationErrors{{Cause: assert.AnError, Key: []string{"any"}}}
+		require.Equal(t, expected, errs.Append(assert.AnError, "any"))
+	})
+}
+
+func TestValidationErrors_AsError(t *testing.T) {
+	t.Parallel()
+
+	t.Run("empty", func(t *testing.T) {
+		err := ValidationErrors{}.AsError()
+		require.NoError(t, err)
+	})
+
+	t.Run("non empty", func(t *testing.T) {
+		err := ValidationErrors{NewValidationError(errors.New("msg"), "err")}.AsError()
+		require.Equal(t, ValidationErrors{{Key: []string{"err"}, Cause: errors.New("msg")}}, err)
+	})
+}
+
+func TestValidationErrors_Error(t *testing.T) {
+	t.Parallel()
+
+	t.Run("serialized", func(t *testing.T) {
+		require.Equal(t, "1.2: msg1\n1: msg2",
+			ValidationErrors{
+				NewValidationError(errors.New("msg1"), "1", "2"),
+				NewValidationError(errors.New("msg2"), "1"),
+			}.Error(),
+		)
+	})
+
+	t.Run("nothing to present", func(t *testing.T) {
+		require.Equal(t, "", ValidationErrors{}.Error())
+	})
+}
+
+func TestNewValidationError(t *testing.T) {
+	t.Parallel()
+
+	err := NewValidationError(assert.AnError)
+	require.Equal(t, ValidationError{Cause: assert.AnError}, err)
+
+	err = NewValidationError(assert.AnError, "outer", "inner")
+	require.Equal(t, ValidationError{Cause: assert.AnError, Key: []string{"outer", "inner"}}, err)
+}
diff --git a/internal/git/command_options_test.go b/internal/git/command_options_test.go
index a961d77c4..7d0a81160 100644
--- a/internal/git/command_options_test.go
+++ b/internal/git/command_options_test.go
@@ -3,11 +3,11 @@ package git
 import (
 	"bytes"
 	"encoding/base64"
-	"errors"
 	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/errors/cfgerror"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
@@ -159,19 +159,28 @@ func TestGlobalOption(t *testing.T) {
 			expectedArgs: []string{"-c", "http.https://*.example.com/.proxy=http://proxy.example.com"},
 		},
 		{
-			desc:        "config pair with invalid section format",
-			option:      ConfigPair{Key: "foo", Value: "value"},
-			expectedErr: fmt.Errorf("invalid configuration key %q: %w", "foo", errors.New("key must contain at least one section")),
+			desc:   "config pair with invalid section format",
+			option: ConfigPair{Key: "foo", Value: "value"},
+			expectedErr: fmt.Errorf(
+				"invalid configuration key \"foo\": %w",
+				cfgerror.NewValidationError(fmt.Errorf("key %q must contain at least one section", "foo"), "key"),
+			),
 		},
 		{
-			desc:        "config pair with leading whitespace",
-			option:      ConfigPair{Key: " foo.bar", Value: "value"},
-			expectedErr: fmt.Errorf("invalid configuration key %q: %w", " foo.bar", errors.New("key failed regexp validation")),
+			desc:   "config pair with leading whitespace",
+			option: ConfigPair{Key: " foo.bar", Value: "value"},
+			expectedErr: fmt.Errorf(
+				"invalid configuration key \" foo.bar\": %w",
+				cfgerror.NewValidationError(fmt.Errorf("key %q failed regexp validation", " foo.bar"), "key"),
+			),
 		},
 		{
-			desc:        "config pair with disallowed character in key",
-			option:      ConfigPair{Key: "foo.b=r", Value: "value"},
-			expectedErr: fmt.Errorf("invalid configuration key %q: %w", "foo.b=r", errors.New("key cannot contain assignment")),
+			desc:   "config pair with disallowed character in key",
+			option: ConfigPair{Key: "foo.b=r", Value: "value"},
+			expectedErr: fmt.Errorf(
+				"invalid configuration key \"foo.b=r\": %w",
+				cfgerror.NewValidationError(fmt.Errorf("key %q cannot contain \"=\"", "foo.b=r"), "key"),
+			),
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 256e2ce6a..7cdaf4cc6 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -18,6 +18,7 @@ import (
 
 	"github.com/pelletier/go-toml/v2"
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/errors/cfgerror"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/auth"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 	internallog "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/log"
@@ -127,6 +128,16 @@ type Git struct {
 	SigningKey         string      `toml:"signing_key,omitempty" json:"signing_key"`
 }
 
+// Validate runs validation on all fields and compose all found errors.
+func (g Git) Validate() error {
+	var errs cfgerror.ValidationErrors
+	for _, gc := range g.Config {
+		errs = errs.Append(gc.Validate(), "config")
+	}
+
+	return errs.AsError()
+}
+
 // GitConfig contains a key-value pair which is to be passed to git as configuration.
 type GitConfig struct {
 	// Key is the key of the config entry, e.g. `core.gc`.
@@ -140,20 +151,32 @@ func (cfg GitConfig) Validate() error {
 	// Even though redundant, this block checks for a few things up front to give better error
 	// messages to the administrator in case any of the keys fails validation.
 	if cfg.Key == "" {
-		return errors.New("key cannot be empty")
+		return cfgerror.NewValidationError(cfgerror.ErrNotSet, "key")
 	}
 	if strings.Contains(cfg.Key, "=") {
-		return errors.New("key cannot contain assignment")
+		return cfgerror.NewValidationError(
+			fmt.Errorf(`key %q cannot contain "="`, cfg.Key),
+			"key",
+		)
 	}
 	if !strings.Contains(cfg.Key, ".") {
-		return errors.New("key must contain at least one section")
+		return cfgerror.NewValidationError(
+			fmt.Errorf("key %q must contain at least one section", cfg.Key),
+			"key",
+		)
 	}
 	if strings.HasPrefix(cfg.Key, ".") || strings.HasSuffix(cfg.Key, ".") {
-		return errors.New("key must not start or end with a dot")
+		return cfgerror.NewValidationError(
+			fmt.Errorf("key %q must not start or end with a dot", cfg.Key),
+			"key",
+		)
 	}
 
 	if !configKeyRegex.MatchString(cfg.Key) {
-		return fmt.Errorf("key failed regexp validation")
+		return cfgerror.NewValidationError(
+			fmt.Errorf("key %q failed regexp validation", cfg.Key),
+			"key",
+		)
 	}
 
 	return nil
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index eebe460aa..f36f3f300 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/pelletier/go-toml/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/errors/cfgerror"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/auth"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/cgroups"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
@@ -638,7 +639,7 @@ value = "second-value"
 	}, cfg.Git)
 }
 
-func TestValidateGitConfig(t *testing.T) {
+func TestGit_Validate(t *testing.T) {
 	testCases := []struct {
 		desc        string
 		configPairs []GitConfig
@@ -658,36 +659,60 @@ func TestValidateGitConfig(t *testing.T) {
 			configPairs: []GitConfig{
 				{Value: "value"},
 			},
-			expectedErr: fmt.Errorf("invalid configuration key \"\": %w", errors.New("key cannot be empty")),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					errors.New("not set"),
+					"config", "key",
+				),
+			},
 		},
 		{
 			desc: "key has no section",
 			configPairs: []GitConfig{
 				{Key: "foo", Value: "value"},
 			},
-			expectedErr: fmt.Errorf("invalid configuration key \"foo\": %w", errors.New("key must contain at least one section")),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					errors.New(`key "foo" must contain at least one section`),
+					"config", "key",
+				),
+			},
 		},
 		{
 			desc: "key with leading dot",
 			configPairs: []GitConfig{
 				{Key: ".foo.bar", Value: "value"},
 			},
-			expectedErr: fmt.Errorf("invalid configuration key \".foo.bar\": %w", errors.New("key must not start or end with a dot")),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					errors.New(`key ".foo.bar" must not start or end with a dot`),
+					"config", "key",
+				),
+			},
 		},
 		{
 			desc: "key with trailing dot",
 			configPairs: []GitConfig{
 				{Key: "foo.bar.", Value: "value"},
 			},
-			expectedErr: fmt.Errorf("invalid configuration key \"foo.bar.\": %w", errors.New("key must not start or end with a dot")),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					errors.New(`key "foo.bar." must not start or end with a dot`),
+					"config", "key",
+				),
+			},
 		},
 		{
 			desc: "key has assignment",
 			configPairs: []GitConfig{
 				{Key: "foo.bar=value", Value: "value"},
 			},
-			expectedErr: fmt.Errorf("invalid configuration key \"foo.bar=value\": %w",
-				errors.New("key cannot contain assignment")),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					errors.New(`key "foo.bar=value" cannot contain "="`),
+					"config", "key",
+				),
+			},
 		},
 		{
 			desc: "missing value",
@@ -700,7 +725,7 @@ func TestValidateGitConfig(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			cfg := Cfg{BinDir: testhelper.TempDir(t), Git: Git{Config: tc.configPairs}}
-			require.Equal(t, tc.expectedErr, cfg.validateGit())
+			require.Equal(t, tc.expectedErr, cfg.Git.Validate())
 		})
 	}
 }