Praefect: configuration verification

Praefect uses a .toml file as a source of configuration.
Before praefect starts handle requests the verification
must be executed to check if configuration is correct.
All `gitaly`-s must use distinct addresses.
Any virtual node must have at least one gitaly behind it.
Each virtual node must be named.

2 files changed, 148 insertions, 69 deletions

diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index 3508c4a23..fb28f2a20 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -72,8 +72,9 @@ var (
 	errNoListener               = errors.New("no listen address or socket path configured")
 	errNoPrimaries              = errors.New("no primaries designated")
 	errNoVirtualStorages        = errors.New("no virtual storages configured")
-	errStorageAddressMismatch   = errors.New("storages with the same name must have the same address")
+	errStorageAddressDuplicate  = errors.New("multiple storages have the same address")
 	errVirtualStoragesNotUnique = errors.New("virtual storages must have unique names")
+	errVirtualStorageUnnamed    = errors.New("virtual storages must have a name")
 )
 
 // Validate establishes if the config is valid
@@ -86,55 +87,55 @@ func (c Config) Validate() error {
 		return errNoVirtualStorages
 	}
 
-	allStorages := make(map[string]string)
-	virtualStorages := make(map[string]struct{})
+	allAddresses := make(map[string]struct{})
+	virtualStorages := make(map[string]struct{}, len(c.VirtualStorages))
 
 	for _, virtualStorage := range c.VirtualStorages {
-		if _, ok := virtualStorages[virtualStorage.Name]; ok {
-			return errVirtualStoragesNotUnique
+		if virtualStorage.Name == "" {
+			return errVirtualStorageUnnamed
+		}
+
+		if len(virtualStorage.Nodes) == 0 {
+			return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errNoGitalyServers)
 		}
 
+		if _, ok := virtualStorages[virtualStorage.Name]; ok {
+			return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errVirtualStoragesNotUnique)
+		}
 		virtualStorages[virtualStorage.Name] = struct{}{}
 
-		storages := make(map[string]struct{})
-		var primaries int
+		storages := make(map[string]struct{}, len(virtualStorage.Nodes))
+		primaries := 0
 		for _, node := range virtualStorage.Nodes {
 			if node.DefaultPrimary {
 				primaries++
 			}
 
 			if primaries > 1 {
-				return fmt.Errorf("virtual storage %s: %v", virtualStorage.Name, errMoreThanOnePrimary)
+				return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errMoreThanOnePrimary)
 			}
 
 			if node.Storage == "" {
-				return errGitalyWithoutStorage
+				return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errGitalyWithoutStorage)
 			}
 
 			if node.Address == "" {
-				return errGitalyWithoutAddr
+				return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errGitalyWithoutAddr)
 			}
 
 			if _, found := storages[node.Storage]; found {
-				return errDuplicateStorage
+				return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errDuplicateStorage)
 			}
+			storages[node.Storage] = struct{}{}
 
-			if address, found := allStorages[node.Storage]; found {
-				if address != node.Address {
-					return errStorageAddressMismatch
-				}
-			} else {
-				allStorages[node.Storage] = node.Address
+			if _, found := allAddresses[node.Address]; found {
+				return fmt.Errorf("virtual storage %q: address %q : %w", virtualStorage.Name, node.Address, errStorageAddressDuplicate)
 			}
-
-			storages[node.Storage] = struct{}{}
+			allAddresses[node.Address] = struct{}{}
 		}
 
 		if primaries == 0 {
-			return fmt.Errorf("virtual storage %s: %v", virtualStorage.Name, errNoPrimaries)
-		}
-		if len(storages) == 0 {
-			return fmt.Errorf("virtual storage %s: %v", virtualStorage.Name, errNoGitalyServers)
+			return fmt.Errorf("virtual storage %q: %w", virtualStorage.Name, errNoPrimaries)
 		}
 	}
 
diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go
index 041c30188..0077e6094 100644
--- a/internal/praefect/config/config_test.go
+++ b/internal/praefect/config/config_test.go
@@ -1,7 +1,6 @@
 package config
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -13,59 +12,92 @@ import (
 )
 
 func TestConfigValidation(t *testing.T) {
-	nodes := []*models.Node{
-		{Storage: "internal-1", Address: "localhost:23456", Token: "secret-token", DefaultPrimary: true},
-		{Storage: "internal-2", Address: "localhost:23457", Token: "secret-token"},
-		{Storage: "internal-3", Address: "localhost:23458", Token: "secret-token"},
+	vs1Nodes := []*models.Node{
+		{Storage: "internal-1.0", Address: "localhost:23456", Token: "secret-token-1", DefaultPrimary: true},
+		{Storage: "internal-2.0", Address: "localhost:23457", Token: "secret-token-1"},
+		{Storage: "internal-3.0", Address: "localhost:23458", Token: "secret-token-1"},
+	}
+
+	vs2Nodes := []*models.Node{
+		// storage can have same name as storage in another virtual storage, but all addresses must be unique
+		{Storage: "internal-1.0", Address: "localhost:33456", Token: "secret-token-2", DefaultPrimary: true},
+		{Storage: "internal-2.1", Address: "localhost:33457", Token: "secret-token-2"},
+		{Storage: "internal-3.1", Address: "localhost:33458", Token: "secret-token-2"},
 	}
 
 	testCases := []struct {
 		desc   string
 		config Config
-		err    error
+		errMsg string
 	}{
 		{
-			desc:   "No ListenAddr or SocketPath",
-			config: Config{ListenAddr: "", VirtualStorages: []*VirtualStorage{&VirtualStorage{Nodes: nodes}}},
-			err:    errNoListener,
+			desc: "Valid config with ListenAddr",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+					{Name: "secondary", Nodes: vs2Nodes},
+				},
+			},
 		},
 		{
-			desc:   "Only a SocketPath",
-			config: Config{SocketPath: "/tmp/praefect.socket", VirtualStorages: []*VirtualStorage{&VirtualStorage{Nodes: nodes}}},
-			err:    nil,
+			desc: "Valid config with SocketPath",
+			config: Config{
+				SocketPath: "/tmp/praefect.socket",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+				},
+			},
+		},
+		{
+			desc: "No ListenAddr or SocketPath",
+			config: Config{
+				ListenAddr: "",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+				},
+			},
+			errMsg: "no listen address or socket path configured",
 		},
 		{
-			desc:   "No servers",
+			desc:   "No virtual storages",
 			config: Config{ListenAddr: "localhost:1234"},
-			err:    errNoVirtualStorages,
+			errMsg: "no virtual storages configured",
 		},
 		{
 			desc: "duplicate storage",
 			config: Config{
 				ListenAddr: "localhost:1234",
 				VirtualStorages: []*VirtualStorage{
-					&VirtualStorage{Nodes: append(nodes, &models.Node{Storage: nodes[0].Storage, Address: nodes[1].Address})},
+					{
+						Name: "default",
+						Nodes: append(vs1Nodes, &models.Node{
+							Storage: vs1Nodes[0].Storage,
+							Address: vs1Nodes[1].Address,
+						}),
+					},
 				},
 			},
-			err: errDuplicateStorage,
+			errMsg: `virtual storage "default": internal gitaly storages are not unique`,
 		},
 		{
-			desc:   "Valid config",
-			config: Config{ListenAddr: "localhost:1234", VirtualStorages: []*VirtualStorage{&VirtualStorage{Nodes: nodes}}},
-			err:    nil,
-		},
-		{
-			desc:   "No designated primaries",
-			config: Config{ListenAddr: "localhost:1234", VirtualStorages: []*VirtualStorage{&VirtualStorage{Nodes: nodes[1:]}}},
-			err:    errNoPrimaries,
+			desc: "No designated primaries",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes[1:]},
+				},
+			},
+			errMsg: `virtual storage "default": no primaries designated`,
 		},
 		{
 			desc: "More than 1 primary",
 			config: Config{
 				ListenAddr: "localhost:1234",
 				VirtualStorages: []*VirtualStorage{
-					&VirtualStorage{
-						Nodes: append(nodes,
+					{
+						Name: "default",
+						Nodes: append(vs1Nodes,
 							&models.Node{
 								Storage:        "internal-4",
 								Address:        "localhost:23459",
@@ -75,56 +107,102 @@ func TestConfigValidation(t *testing.T) {
 					},
 				},
 			},
-			err: errMoreThanOnePrimary,
+			errMsg: `virtual storage "default": only 1 node can be designated as a primary`,
 		},
 		{
-			desc: "Node storage not unique",
+			desc: "Node storage has no name",
 			config: Config{
 				ListenAddr: "localhost:1234",
 				VirtualStorages: []*VirtualStorage{
-					&VirtualStorage{Name: "default", Nodes: nodes},
-					&VirtualStorage{
-						Name: "backup",
+					{
+						Name: "default",
 						Nodes: []*models.Node{
-							&models.Node{
-								Storage:        nodes[0].Storage,
-								Address:        "some.other.address",
-								DefaultPrimary: true},
+							{
+								Storage:        "",
+								Address:        "localhost:23456",
+								Token:          "secret-token-1",
+								DefaultPrimary: true,
+							},
 						},
 					},
 				},
 			},
-			err: errStorageAddressMismatch,
+			errMsg: `virtual storage "default": all gitaly nodes must have a storage`,
 		},
 		{
-			desc: "Node storage not unique",
+			desc: "Node storage has no address",
 			config: Config{
 				ListenAddr: "localhost:1234",
 				VirtualStorages: []*VirtualStorage{
-					&VirtualStorage{Name: "default", Nodes: nodes},
-					&VirtualStorage{
+					{
 						Name: "default",
 						Nodes: []*models.Node{
-							&models.Node{
-								Storage:        nodes[0].Storage,
-								Address:        "some.other.address",
-								DefaultPrimary: true}},
+							{
+								Storage:        "internal",
+								Address:        "",
+								Token:          "secret-token-1",
+								DefaultPrimary: true,
+							},
+						},
 					},
 				},
 			},
-			err: errVirtualStoragesNotUnique,
+			errMsg: `virtual storage "default": all gitaly nodes must have an address`,
+		},
+		{
+			desc: "Virtual storage has no name",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "", Nodes: vs1Nodes},
+				},
+			},
+			errMsg: `virtual storages must have a name`,
+		},
+		{
+			desc: "Virtual storage not unique",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+					{Name: "default", Nodes: vs2Nodes},
+				},
+			},
+			errMsg: `virtual storage "default": virtual storages must have unique names`,
+		},
+		{
+			desc: "Virtual storage has no nodes",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+					{Name: "secondary", Nodes: nil},
+				},
+			},
+			errMsg: `virtual storage "secondary": no primary gitaly backends configured`,
+		},
+		{
+			desc: "Node storage has address duplicate",
+			config: Config{
+				ListenAddr: "localhost:1234",
+				VirtualStorages: []*VirtualStorage{
+					{Name: "default", Nodes: vs1Nodes},
+					{Name: "secondary", Nodes: append(vs2Nodes, vs1Nodes[1])},
+				},
+			},
+			errMsg: `multiple storages have the same address`,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			err := tc.config.Validate()
-			if tc.err == nil {
+			if tc.errMsg == "" {
 				assert.NoError(t, err)
 				return
 			}
 
-			assert.True(t, strings.Contains(err.Error(), tc.err.Error()))
+			assert.Contains(t, err.Error(), tc.errMsg)
 		})
 	}
 }