Merge branch 'zj-gitaly-dont-start-no-storage' into 'master'

Require storage directories to exist at startup

Closes #1341

See merge request gitlab-org/gitaly!675

5 files changed, 42 insertions, 13 deletions

diff --git a/changelogs/unreleased/zj-gitaly-dont-start-no-storage.yml b/changelogs/unreleased/zj-gitaly-dont-start-no-storage.yml
new file mode 100644
index 000000000..fd85c6891
--- /dev/null
+++ b/changelogs/unreleased/zj-gitaly-dont-start-no-storage.yml
@@ -0,0 +1,5 @@
+---
+title: Require storage directories to exist at startup
+merge_request: 675
+author:
+type: changed
diff --git a/internal/config/config.go b/internal/config/config.go
index eb796acd3..9e8ed6566 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -146,6 +146,10 @@ func validateStorages() error {
 			return fmt.Errorf("empty storage path in %v", storage)
 		}
 
+		if fs, err := os.Stat(storage.Path); err != nil || !fs.IsDir() {
+			return fmt.Errorf("storage paths have to exist %v", storage)
+		}
+
 		stPath := filepath.Clean(storage.Path)
 		for j := 0; j < i; j++ {
 			other := Config.Storages[j]
@@ -160,7 +164,11 @@ func validateStorages() error {
 			}
 
 			if strings.HasPrefix(stPath, otherPath) || strings.HasPrefix(otherPath, stPath) {
-				return fmt.Errorf("storage paths may not nest: %q and %q", storage.Name, storage.Name)
+				// If storages have the same sub directory, that is allowed
+				if filepath.Dir(stPath) == filepath.Dir(otherPath) {
+					continue
+				}
+				return fmt.Errorf("storage paths may not nest: %q and %q", storage.Name, other.Name)
 			}
 		}
 	}
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index c842c6d1e..f02221f4e 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -158,6 +159,14 @@ func TestValidateStorages(t *testing.T) {
 		Config.Storages = oldStorages
 	}(Config.Storages)
 
+	repositories, err := filepath.Abs("testdata/repositories")
+	require.NoError(t, err)
+
+	repositories2, err := filepath.Abs("testdata/repositories2")
+	require.NoError(t, err)
+
+	invalidDir := path.Join(repositories, t.Name())
+
 	testCases := []struct {
 		desc     string
 		storages []Storage
@@ -166,23 +175,22 @@ func TestValidateStorages(t *testing.T) {
 		{
 			desc: "just 1 storage",
 			storages: []Storage{
-				{Name: "default", Path: "/home/git/repositories"},
+				{Name: "default", Path: repositories},
 			},
 		},
 		{
 			desc: "multiple storages",
 			storages: []Storage{
-				{Name: "default", Path: "/home/git/repositories1"},
-				{Name: "other", Path: "/home/git/repositories2"},
-				{Name: "third", Path: "/home/git/repositories3"},
+				{Name: "default", Path: repositories},
+				{Name: "other", Path: repositories2},
 			},
 		},
 		{
 			desc: "multiple storages pointing to same directory",
 			storages: []Storage{
-				{Name: "default", Path: "/home/git/repositories"},
-				{Name: "other", Path: "/home/git/repositories"},
-				{Name: "third", Path: "/home/git/repositories"},
+				{Name: "default", Path: repositories},
+				{Name: "other", Path: repositories},
+				{Name: "third", Path: repositories},
 			},
 		},
 		{
@@ -206,23 +214,23 @@ func TestValidateStorages(t *testing.T) {
 		{
 			desc: "duplicate definition",
 			storages: []Storage{
-				{Name: "default", Path: "/home/git/repositories"},
-				{Name: "default", Path: "/home/git/repositories"},
+				{Name: "default", Path: repositories},
+				{Name: "default", Path: repositories},
 			},
 			invalid: true,
 		},
 		{
 			desc: "re-definition",
 			storages: []Storage{
-				{Name: "default", Path: "/home/git/repositories1"},
-				{Name: "default", Path: "/home/git/repositories2"},
+				{Name: "default", Path: repositories},
+				{Name: "default", Path: repositories2},
 			},
 			invalid: true,
 		},
 		{
 			desc: "empty name",
 			storages: []Storage{
-				{Name: "", Path: "/home/git/repositories1"},
+				{Name: "", Path: repositories},
 			},
 			invalid: true,
 		},
@@ -233,6 +241,14 @@ func TestValidateStorages(t *testing.T) {
 			},
 			invalid: true,
 		},
+		{
+			desc: "non existing directory",
+			storages: []Storage{
+				{Name: "default", Path: repositories},
+				{Name: "nope", Path: invalidDir},
+			},
+			invalid: true,
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/config/testdata/repositories/.gitkeep b/internal/config/testdata/repositories/.gitkeep
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/internal/config/testdata/repositories/.gitkeep
diff --git a/internal/config/testdata/repositories2/.gitkeep b/internal/config/testdata/repositories2/.gitkeep
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/internal/config/testdata/repositories2/.gitkeep