gitaly: Validation of Storage configuration

The 'cfgerror' package extended with two validation functions. They
should reduce code duplication and also standardize text of the
errors. All frequently used basic errors are defined in the package
as well and now ErrNoUnique add to them as well.

The 'Storage' type fields now have toml tags and a 'Validate()'
method. As in all other cases it returns all found validation
errors.
The validation logic for 'Storage' field placed into
'validateStorages()' and will replace 'validateStorages()' method
of the 'Cfg' afterwards.

5 files changed, 252 insertions, 30 deletions

diff --git a/internal/errors/cfgerror/testhelper_test.go b/internal/errors/cfgerror/testhelper_test.go
new file mode 100644
index 000000000..be6675514
--- /dev/null
+++ b/internal/errors/cfgerror/testhelper_test.go
@@ -0,0 +1,11 @@
+package cfgerror
+
+import (
+	"testing"
+
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+)
+
+func TestMain(m *testing.M) {
+	testhelper.Run(m)
+}
diff --git a/internal/errors/cfgerror/validate.go b/internal/errors/cfgerror/validate.go
index 54a31123b..935caf905 100644
--- a/internal/errors/cfgerror/validate.go
+++ b/internal/errors/cfgerror/validate.go
@@ -3,11 +3,22 @@ package cfgerror
 import (
 	"errors"
 	"fmt"
+	"os"
 	"strings"
 )
 
-// ErrNotSet should be used when the value is not set, but it is required.
-var ErrNotSet = errors.New("not set")
+var (
+	// ErrNotSet should be used when the value is not set, but it is required.
+	ErrNotSet = errors.New("not set")
+	// ErrBlankOrEmpty should be used when non-blank/non-empty string is expected.
+	ErrBlankOrEmpty = errors.New("blank or empty")
+	// ErrDoesntExist should be used when resource doesn't exist.
+	ErrDoesntExist = errors.New("doesn't exist")
+	// ErrNotDir should be used when path on the file system exists, but it is not a directory.
+	ErrNotDir = errors.New("not a dir")
+	// ErrNotUnique should be used when the value must be unique, but there are duplicates.
+	ErrNotUnique = errors.New("not unique")
+)
 
 // ValidationError represents an issue with provided configuration.
 type ValidationError struct {
@@ -96,3 +107,36 @@ func (vs ValidationErrors) Error() string {
 func New() ValidationErrors {
 	return nil
 }
+
+// NotEmpty checks if value is empty.
+func NotEmpty(val string) error {
+	if val == "" {
+		return NewValidationError(ErrNotSet)
+	}
+	return nil
+}
+
+// NotBlank checks the value is not empty or blank.
+func NotBlank(val string) error {
+	if strings.TrimSpace(val) == "" {
+		return NewValidationError(ErrBlankOrEmpty)
+	}
+	return nil
+}
+
+// DirExists checks the value points to an existing directory on the disk.
+func DirExists(path string) error {
+	fs, err := os.Stat(path)
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return NewValidationError(fmt.Errorf("%w: %q", ErrDoesntExist, path))
+		}
+		return err
+	}
+
+	if !fs.IsDir() {
+		return NewValidationError(fmt.Errorf("%w: %q", ErrNotDir, path))
+	}
+
+	return nil
+}
diff --git a/internal/errors/cfgerror/validate_test.go b/internal/errors/cfgerror/validate_test.go
index 847a62cc6..c129a17d7 100644
--- a/internal/errors/cfgerror/validate_test.go
+++ b/internal/errors/cfgerror/validate_test.go
@@ -2,10 +2,15 @@ package cfgerror
 
 import (
 	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestValidationError_Error(t *testing.T) {
@@ -99,3 +104,33 @@ func TestNewValidationError(t *testing.T) {
 	err = NewValidationError(assert.AnError, "outer", "inner")
 	require.Equal(t, ValidationError{Cause: assert.AnError, Key: []string{"outer", "inner"}}, err)
 }
+
+func TestNotEmpty(t *testing.T) {
+	t.Parallel()
+	require.NoError(t, NotEmpty("value"))
+	require.Equal(t, NewValidationError(ErrNotSet), NotEmpty(""))
+}
+
+func TestNotBlank(t *testing.T) {
+	t.Parallel()
+	require.NoError(t, NotBlank("value"))
+	require.Equal(t, NewValidationError(ErrBlankOrEmpty), NotBlank(""))
+	require.Equal(t, NewValidationError(ErrBlankOrEmpty), NotBlank("  \t  \n "))
+}
+
+func TestDirExists(t *testing.T) {
+	t.Parallel()
+
+	filePath := filepath.Join(testhelper.TempDir(t), "tmp-file")
+	require.NoError(t, os.WriteFile(filePath, []byte{}, perm.PublicFile))
+	existing := testhelper.TempDir(t)
+	notExisting := filepath.Join(existing, "bad")
+
+	require.NoError(t, DirExists(existing))
+
+	expectedNotExisting := NewValidationError(fmt.Errorf("%w: %q", ErrDoesntExist, notExisting))
+	require.Equal(t, expectedNotExisting, DirExists(notExisting))
+
+	expectedNotDir := NewValidationError(fmt.Errorf("%w: %q", ErrNotDir, filePath))
+	require.Equal(t, expectedNotDir, DirExists(filePath))
+}
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 7cdaf4cc6..6a39483ac 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -194,8 +194,61 @@ func (cfg GitConfig) GlobalArgs() ([]string, error) {
 
 // Storage contains a single storage-shard
 type Storage struct {
-	Name string
-	Path string
+	Name string `toml:"name"`
+	Path string `toml:"path"`
+}
+
+// Validate runs validation on all fields and compose all found errors.
+func (s Storage) Validate() error {
+	return cfgerror.New().
+		Append(cfgerror.NotEmpty(s.Name), "name").
+		Append(cfgerror.DirExists(s.Path), "path").
+		AsError()
+}
+
+func validateStorages(storages []Storage) error {
+	if len(storages) == 0 {
+		return cfgerror.NewValidationError(cfgerror.ErrNotSet)
+	}
+
+	var errs cfgerror.ValidationErrors
+	for i, s := range storages {
+		errs = errs.Append(s.Validate(), fmt.Sprintf("[%d]", i))
+	}
+
+	for i, storage := range storages {
+		for _, other := range storages[:i] {
+			if other.Name == storage.Name {
+				err := fmt.Errorf("%w: %q", cfgerror.ErrNotUnique, storage.Name)
+				cause := cfgerror.NewValidationError(err, "name")
+				errs = errs.Append(cause, fmt.Sprintf("[%d]", i))
+			}
+
+			if storage.Path == other.Path {
+				// This is weird, but we allow it for legacy gitlab.com reasons.
+				continue
+			}
+
+			if storage.Path == "" || other.Path == "" {
+				// If one of Path-s is not set the code below will produce an error
+				// that only confuses, so we stop here.
+				continue
+			}
+
+			if strings.HasPrefix(storage.Path, other.Path) || strings.HasPrefix(other.Path, storage.Path) {
+				// If storages have the same subdirectory, that is allowed.
+				if filepath.Dir(storage.Path) == filepath.Dir(other.Path) {
+					continue
+				}
+
+				cause := fmt.Errorf("can't nest: %q and %q", storage.Path, other.Path)
+				err := cfgerror.NewValidationError(cause, "path")
+				errs = errs.Append(err, fmt.Sprintf("[%d]", i))
+			}
+		}
+	}
+
+	return errs.AsError()
 }
 
 // Sentry is a sentry.Config. We redefine this type to a different name so
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index f36f3f300..d09ea0b36 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -471,16 +471,16 @@ func TestValidateStorages(t *testing.T) {
 	filePath := filepath.Join(testhelper.TempDir(t), "temporary-file")
 	require.NoError(t, os.WriteFile(filePath, []byte{}, perm.PublicFile))
 
-	invalidDir := filepath.Join(repositories, t.Name())
+	invalidDir := filepath.Join(filepath.Dir(repositories), t.Name())
 
 	testCases := []struct {
-		desc      string
-		storages  []Storage
-		expErrMsg string
+		desc        string
+		storages    []Storage
+		expectedErr error
 	}{
 		{
-			desc:      "no storages",
-			expErrMsg: "no storage configurations found. Are you using the right format? https://gitlab.com/gitlab-org/gitaly/issues/397",
+			desc:        "no storages",
+			expectedErr: cfgerror.NewValidationError(cfgerror.ErrNotSet),
 		},
 		{
 			desc: "just 1 storage",
@@ -510,7 +510,13 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "other", Path: repositories},
 				{Name: "third", Path: nestedRepositories},
 			},
-			expErrMsg: `storage paths may not nest: "third" and "default"`,
+			expectedErr: cfgerror.ValidationErrors{{
+				Key:   []string{"[2]", "path"},
+				Cause: fmt.Errorf(`can't nest: %q and %q`, nestedRepositories, repositories),
+			}, {
+				Key:   []string{"[2]", "path"},
+				Cause: fmt.Errorf(`can't nest: %q and %q`, nestedRepositories, repositories),
+			}},
 		},
 		{
 			desc: "nested paths 2",
@@ -519,7 +525,13 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "other", Path: repositories},
 				{Name: "third", Path: repositories},
 			},
-			expErrMsg: `storage paths may not nest: "other" and "default"`,
+			expectedErr: cfgerror.ValidationErrors{{
+				Key:   []string{"[1]", "path"},
+				Cause: fmt.Errorf(`can't nest: %q and %q`, repositories, nestedRepositories),
+			}, {
+				Key:   []string{"[2]", "path"},
+				Cause: fmt.Errorf(`can't nest: %q and %q`, repositories, nestedRepositories),
+			}},
 		},
 		{
 			desc: "duplicate definition",
@@ -527,7 +539,10 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "default", Path: repositories},
 				{Name: "default", Path: repositories},
 			},
-			expErrMsg: `storage "default" is defined more than once`,
+			expectedErr: cfgerror.ValidationErrors{cfgerror.NewValidationError(
+				fmt.Errorf(`%w: "default"`, cfgerror.ErrNotUnique),
+				"[1]", "name",
+			)},
 		},
 		{
 			desc: "re-definition",
@@ -535,7 +550,12 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "default", Path: repositories},
 				{Name: "default", Path: repositories2},
 			},
-			expErrMsg: `storage "default" is defined more than once`,
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					fmt.Errorf(`%w: "default"`, cfgerror.ErrNotUnique),
+					"[1]", "name",
+				),
+			},
 		},
 		{
 			desc: "empty name",
@@ -543,15 +563,12 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "some", Path: repositories},
 				{Name: "", Path: repositories},
 			},
-			expErrMsg: `empty storage name at declaration 2`,
-		},
-		{
-			desc: "empty path",
-			storages: []Storage{
-				{Name: "some", Path: repositories},
-				{Name: "default", Path: ""},
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					cfgerror.ErrNotSet,
+					"[1]", "name",
+				),
 			},
-			expErrMsg: `empty storage path for storage "default"`,
 		},
 		{
 			desc: "non existing directory",
@@ -559,7 +576,12 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "default", Path: repositories},
 				{Name: "nope", Path: invalidDir},
 			},
-			expErrMsg: fmt.Sprintf(`storage path %q for storage "nope" doesn't exist`, invalidDir),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					fmt.Errorf("%w: %q", cfgerror.ErrDoesntExist, invalidDir),
+					"[1]", "path",
+				),
+			},
 		},
 		{
 			desc: "path points to the regular file",
@@ -567,21 +589,41 @@ func TestValidateStorages(t *testing.T) {
 				{Name: "default", Path: repositories},
 				{Name: "is_file", Path: filePath},
 			},
-			expErrMsg: fmt.Sprintf(`storage path %q for storage "is_file" is not a dir`, filePath),
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					fmt.Errorf("%w: %q", cfgerror.ErrNotDir, filePath),
+					"[1]", "path",
+				),
+			},
+		},
+		{
+			desc: "multiple errors",
+			storages: []Storage{
+				{Name: "", Path: repositories},
+				{Name: "default", Path: "somewhat"},
+			},
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					cfgerror.ErrNotSet,
+					"[0]", "name",
+				),
+				cfgerror.NewValidationError(
+					fmt.Errorf("%w: %q", cfgerror.ErrDoesntExist, "somewhat"),
+					"[1]", "path",
+				),
+			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			cfg := Cfg{Storages: tc.storages}
-
-			err := cfg.validateStorages()
-			if tc.expErrMsg != "" {
-				assert.EqualError(t, err, tc.expErrMsg, "%+v", tc.storages)
+			err := validateStorages(tc.storages)
+			if tc.expectedErr != nil {
+				assert.Equalf(t, tc.expectedErr, err, "%+v", tc.storages)
 				return
 			}
 
-			assert.NoError(t, err, "%+v", tc.storages)
+			assert.NoErrorf(t, err, "%+v", tc.storages)
 		})
 	}
 }
@@ -1696,3 +1738,40 @@ func TestPackObjectsLimiting(t *testing.T) {
 		})
 	}
 }
+
+func TestStorage_Validate(t *testing.T) {
+	t.Parallel()
+
+	dirPath := testhelper.TempDir(t)
+	filePath := filepath.Join(dirPath, "file")
+	require.NoError(t, os.WriteFile(filePath, nil, perm.SharedFile))
+	for _, tc := range []struct {
+		name        string
+		storage     Storage
+		expectedErr error
+	}{
+		{
+			name:    "valid",
+			storage: Storage{Name: "name", Path: dirPath},
+		},
+		{
+			name:    "invalid",
+			storage: Storage{Name: "", Path: filePath},
+			expectedErr: cfgerror.ValidationErrors{
+				cfgerror.NewValidationError(
+					cfgerror.ErrNotSet,
+					"name",
+				),
+				cfgerror.NewValidationError(
+					fmt.Errorf("%w: %q", cfgerror.ErrNotDir, filePath),
+					"path",
+				),
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.storage.Validate()
+			require.Equal(t, tc.expectedErr, err)
+		})
+	}
+}