validate election strategy configuration

Praefect currently does not validate election strategy configuration.
This commit adds validation for the config option.

6 files changed, 114 insertions, 35 deletions

diff --git a/cmd/praefect/main.go b/cmd/praefect/main.go
index f6d8ce33e..51b6aba51 100644
--- a/cmd/praefect/main.go
+++ b/cmd/praefect/main.go
@@ -266,14 +266,11 @@ func run(cfgs []starter.Config, conf config.Config) error {
 	}
 
 	var (
-		repositorySpecificPrimariesEnabled = false
-		healthChecker                      praefect.HealthChecker
-		nodeSet                            praefect.NodeSet
-		router                             praefect.Router
+		healthChecker praefect.HealthChecker
+		nodeSet       praefect.NodeSet
+		router        praefect.Router
 	)
-	if conf.Failover.ElectionStrategy == "per_repository" {
-		repositorySpecificPrimariesEnabled = true
-
+	if conf.Failover.ElectionStrategy == config.ElectionStrategyPerRepository {
 		nodeSet, err = praefect.DialNodes(ctx, conf.VirtualStorages, protoregistry.GitalyProtoPreregistered, errTracker)
 		if err != nil {
 			return fmt.Errorf("dial nodes: %w", err)
@@ -358,7 +355,12 @@ func run(cfgs []starter.Config, conf config.Config) error {
 
 	if db != nil {
 		prometheus.MustRegister(
-			datastore.NewRepositoryStoreCollector(logger, conf.VirtualStorageNames(), db, repositorySpecificPrimariesEnabled),
+			datastore.NewRepositoryStoreCollector(
+				logger,
+				conf.VirtualStorageNames(),
+				db,
+				conf.Failover.ElectionStrategy == config.ElectionStrategyPerRepository,
+			),
 		)
 	}
 
@@ -434,7 +436,14 @@ func run(cfgs []starter.Config, conf config.Config) error {
 		if conf.MemoryQueueEnabled {
 			logger.Warn("Disabled automatic reconciliation as it is only implemented using SQL queue and in-memory queue is configured.")
 		} else {
-			r := reconciler.NewReconciler(logger, db, healthChecker, conf.StorageNames(), conf.Reconciliation.HistogramBuckets, repositorySpecificPrimariesEnabled)
+			r := reconciler.NewReconciler(
+				logger,
+				db,
+				healthChecker,
+				conf.StorageNames(),
+				conf.Reconciliation.HistogramBuckets,
+				conf.Failover.ElectionStrategy == config.ElectionStrategyPerRepository,
+			)
 			prometheus.MustRegister(r)
 			go r.Run(ctx, helper.NewTimerTicker(interval))
 		}
diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index ca03b7831..ebca60f43 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -16,12 +16,35 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config/sentry"
 )
 
+// ElectionStrategy is a Praefect primary election strategy.
+type ElectionStrategy string
+
+// validate validates the election strategy is a valid one.
+func (es ElectionStrategy) validate() error {
+	switch es {
+	case ElectionStrategyLocal, ElectionStrategySQL, ElectionStrategyPerRepository:
+		return nil
+	default:
+		return fmt.Errorf("invalid election strategy: %q", es)
+	}
+}
+
+const (
+	// ElectionStrategyLocal configures a single node, in-memory election strategy.
+	ElectionStrategyLocal = "local"
+	// ElectionStrategySQL configures an SQL based strategy that elects a primary for a virtual storage.
+	ElectionStrategySQL = "sql"
+	// ElectionStrategyPerRepository configures an SQL based strategy that elects different primaries per repository.
+	ElectionStrategyPerRepository = "per_repository"
+)
+
 type Failover struct {
-	Enabled                  bool            `toml:"enabled"`
-	ElectionStrategy         string          `toml:"election_strategy"`
-	ErrorThresholdWindow     config.Duration `toml:"error_threshold_window"`
-	WriteErrorThresholdCount uint32          `toml:"write_error_threshold_count"`
-	ReadErrorThresholdCount  uint32          `toml:"read_error_threshold_count"`
+	Enabled bool `toml:"enabled"`
+	// ElectionStrategy is the strategy to use for electing primaries nodes.
+	ElectionStrategy         ElectionStrategy `toml:"election_strategy"`
+	ErrorThresholdWindow     config.Duration  `toml:"error_threshold_window"`
+	WriteErrorThresholdCount uint32           `toml:"write_error_threshold_count"`
+	ReadErrorThresholdCount  uint32           `toml:"read_error_threshold_count"`
 	// BootstrapInterval allows set a time duration that would be used on startup to make initial health check.
 	// The default value is 1s.
 	BootstrapInterval config.Duration `toml:"bootstrap_interval"`
@@ -52,8 +75,6 @@ func (f Failover) ErrorThresholdsConfigured() (bool, error) {
 	return true, nil
 }
 
-const sqlFailoverValue = "sql"
-
 // Reconciliation contains reconciliation specific configuration options.
 type Reconciliation struct {
 	// SchedulingInterval the interval between each automatic reconciliation run. If set to 0,
@@ -122,7 +143,7 @@ func FromFile(filePath string) (Config, error) {
 		Reconciliation: DefaultReconciliationConfig(),
 		Replication:    DefaultReplicationConfig(),
 		// Sets the default Failover, to be overwritten when deserializing the TOML
-		Failover: Failover{Enabled: true, ElectionStrategy: sqlFailoverValue},
+		Failover: Failover{Enabled: true, ElectionStrategy: ElectionStrategySQL},
 	}
 	if err := toml.Unmarshal(b, conf); err != nil {
 		return Config{}, err
@@ -153,6 +174,10 @@ var (
 
 // Validate establishes if the config is valid
 func (c *Config) Validate() error {
+	if err := c.Failover.ElectionStrategy.validate(); err != nil {
+		return err
+	}
+
 	if c.ListenAddr == "" && c.SocketPath == "" && c.TLSListenAddr == "" {
 		return errNoListener
 	}
@@ -209,8 +234,7 @@ func (c *Config) Validate() error {
 
 // NeedsSQL returns true if the driver for SQL needs to be initialized
 func (c *Config) NeedsSQL() bool {
-	return !c.MemoryQueueEnabled ||
-		(c.Failover.Enabled && c.Failover.ElectionStrategy == sqlFailoverValue)
+	return !c.MemoryQueueEnabled || (c.Failover.Enabled && c.Failover.ElectionStrategy != ElectionStrategyLocal)
 }
 
 func (c *Config) setDefaults() {
diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go
index be87c9f89..fb65e80f7 100644
--- a/internal/praefect/config/config_test.go
+++ b/internal/praefect/config/config_test.go
@@ -27,21 +27,49 @@ func TestConfigValidation(t *testing.T) {
 		{Storage: "internal-3.1", Address: "localhost:33458", Token: "secret-token-2"},
 	}
 
+	fromValidConfig := func(changeFunc func(*Config)) Config {
+		cfg := Config{
+			ListenAddr:  "localhost:1234",
+			Replication: DefaultReplicationConfig(),
+			VirtualStorages: []*VirtualStorage{
+				{Name: "default", Nodes: vs1Nodes},
+				{Name: "secondary", Nodes: vs2Nodes},
+			},
+			Failover: Failover{ElectionStrategy: ElectionStrategySQL},
+		}
+
+		changeFunc(&cfg)
+
+		return cfg
+	}
+
 	testCases := []struct {
 		desc   string
 		config Config
 		errMsg string
 	}{
 		{
-			desc: "Valid config with ListenAddr",
-			config: Config{
-				ListenAddr:  "localhost:1234",
-				Replication: DefaultReplicationConfig(),
-				VirtualStorages: []*VirtualStorage{
-					{Name: "default", Nodes: vs1Nodes},
-					{Name: "secondary", Nodes: vs2Nodes},
-				},
-			},
+			desc:   "Valid config with ListenAddr",
+			config: fromValidConfig(func(*Config) {}),
+		},
+		{
+			desc: "Valid config with local elector",
+			config: fromValidConfig(func(cfg *Config) {
+				cfg.Failover.ElectionStrategy = ElectionStrategyLocal
+			}),
+		},
+		{
+			desc: "Valid config with per repository elector",
+			config: fromValidConfig(func(cfg *Config) {
+				cfg.Failover.ElectionStrategy = ElectionStrategyPerRepository
+			}),
+		},
+		{
+			desc: "Invalid election strategy",
+			config: fromValidConfig(func(cfg *Config) {
+				cfg.Failover.ElectionStrategy = "invalid-strategy"
+			}),
+			errMsg: `invalid election strategy: "invalid-strategy"`,
 		},
 		{
 			desc: "Valid config with TLSListenAddr",
@@ -51,6 +79,7 @@ func TestConfigValidation(t *testing.T) {
 				VirtualStorages: []*VirtualStorage{
 					{Name: "default", Nodes: vs1Nodes},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 		},
 		{
@@ -61,6 +90,7 @@ func TestConfigValidation(t *testing.T) {
 				VirtualStorages: []*VirtualStorage{
 					{Name: "default", Nodes: vs1Nodes},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 		},
 		{
@@ -71,6 +101,7 @@ func TestConfigValidation(t *testing.T) {
 				VirtualStorages: []*VirtualStorage{
 					{Name: "default", Nodes: vs1Nodes},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: "replication batch size was 0 but must be >=1",
 		},
@@ -84,6 +115,7 @@ func TestConfigValidation(t *testing.T) {
 					{Name: "default", Nodes: vs1Nodes},
 				},
 				Replication: DefaultReplicationConfig(),
+				Failover:    Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: "no listen address or socket path configured",
 		},
@@ -92,6 +124,7 @@ func TestConfigValidation(t *testing.T) {
 			config: Config{
 				ListenAddr:  "localhost:1234",
 				Replication: DefaultReplicationConfig(),
+				Failover:    Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: "no virtual storages configured",
 		},
@@ -109,6 +142,7 @@ func TestConfigValidation(t *testing.T) {
 						}),
 					},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storage "default": internal gitaly storages are not unique`,
 		},
@@ -129,6 +163,7 @@ func TestConfigValidation(t *testing.T) {
 						},
 					},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storage "default": all gitaly nodes must have a storage`,
 		},
@@ -149,6 +184,7 @@ func TestConfigValidation(t *testing.T) {
 						},
 					},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storage "default": all gitaly nodes must have an address`,
 		},
@@ -160,6 +196,7 @@ func TestConfigValidation(t *testing.T) {
 				VirtualStorages: []*VirtualStorage{
 					{Name: "", Nodes: vs1Nodes},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storages must have a name`,
 		},
@@ -172,6 +209,7 @@ func TestConfigValidation(t *testing.T) {
 					{Name: "default", Nodes: vs1Nodes},
 					{Name: "default", Nodes: vs2Nodes},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storage "default": virtual storages must have unique names`,
 		},
@@ -184,6 +222,7 @@ func TestConfigValidation(t *testing.T) {
 					{Name: "default", Nodes: vs1Nodes},
 					{Name: "secondary", Nodes: nil},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `virtual storage "secondary": no primary gitaly backends configured`,
 		},
@@ -196,6 +235,7 @@ func TestConfigValidation(t *testing.T) {
 					{Name: "default", Nodes: vs1Nodes},
 					{Name: "secondary", Nodes: append(vs2Nodes, vs1Nodes[1])},
 				},
+				Failover: Failover{ElectionStrategy: ElectionStrategySQL},
 			},
 			errMsg: `multiple storages have the same address`,
 		},
@@ -283,7 +323,7 @@ func TestConfigParsing(t *testing.T) {
 				Replication: Replication{BatchSize: 1},
 				Failover: Failover{
 					Enabled:                  true,
-					ElectionStrategy:         sqlFailoverValue,
+					ElectionStrategy:         ElectionStrategySQL,
 					ErrorThresholdWindow:     config.Duration(20 * time.Second),
 					WriteErrorThresholdCount: 1500,
 					ReadErrorThresholdCount:  100,
@@ -319,7 +359,7 @@ func TestConfigParsing(t *testing.T) {
 				Replication:         DefaultReplicationConfig(),
 				Failover: Failover{
 					Enabled:           true,
-					ElectionStrategy:  sqlFailoverValue,
+					ElectionStrategy:  ElectionStrategySQL,
 					BootstrapInterval: config.Duration(time.Second),
 					MonitorInterval:   config.Duration(3 * time.Second),
 				},
@@ -437,17 +477,22 @@ func TestNeedsSQL(t *testing.T) {
 		},
 		{
 			desc:     "Failover enabled with SQL election strategy",
-			config:   Config{Failover: Failover{Enabled: true, ElectionStrategy: "sql"}},
+			config:   Config{Failover: Failover{Enabled: true, ElectionStrategy: ElectionStrategyPerRepository}},
 			expected: true,
 		},
 		{
 			desc:     "Both PostgresQL and SQL election strategy enabled",
-			config:   Config{Failover: Failover{Enabled: true, ElectionStrategy: "sql"}},
+			config:   Config{Failover: Failover{Enabled: true, ElectionStrategy: ElectionStrategyPerRepository}},
 			expected: true,
 		},
 		{
 			desc:     "Both PostgresQL and SQL election strategy enabled but failover disabled",
-			config:   Config{Failover: Failover{Enabled: false, ElectionStrategy: "sql"}},
+			config:   Config{Failover: Failover{Enabled: false, ElectionStrategy: ElectionStrategyPerRepository}},
+			expected: true,
+		},
+		{
+			desc:     "Both PostgresQL and per_repository election strategy enabled but failover disabled",
+			config:   Config{Failover: Failover{Enabled: false, ElectionStrategy: ElectionStrategyPerRepository}},
 			expected: true,
 		},
 	}
diff --git a/internal/praefect/nodes/manager.go b/internal/praefect/nodes/manager.go
index d20355ff9..59c8f4378 100644
--- a/internal/praefect/nodes/manager.go
+++ b/internal/praefect/nodes/manager.go
@@ -173,7 +173,7 @@ func NewManager(
 		}
 
 		if c.Failover.Enabled {
-			if c.Failover.ElectionStrategy == "sql" {
+			if c.Failover.ElectionStrategy == config.ElectionStrategySQL {
 				strategies[virtualStorage.Name] = newSQLElector(virtualStorage.Name, c, db, log, ns)
 			} else {
 				strategies[virtualStorage.Name] = newLocalElector(virtualStorage.Name, log, ns)
diff --git a/internal/praefect/nodes/manager_test.go b/internal/praefect/nodes/manager_test.go
index cfe13abfa..453d6d1f0 100644
--- a/internal/praefect/nodes/manager_test.go
+++ b/internal/praefect/nodes/manager_test.go
@@ -119,7 +119,7 @@ func TestManagerFailoverDisabledElectionStrategySQL(t *testing.T) {
 	defer srv1.Stop()
 
 	conf := config.Config{
-		Failover:        config.Failover{Enabled: false, ElectionStrategy: "sql"},
+		Failover:        config.Failover{Enabled: false, ElectionStrategy: config.ElectionStrategySQL},
 		VirtualStorages: []*config.VirtualStorage{virtualStorage},
 	}
 	nm, err := NewManager(testhelper.DiscardTestEntry(t), conf, nil, nil, promtest.NewMockHistogramVec(), protoregistry.GitalyProtoPreregistered, nil)
diff --git a/internal/testhelper/testserver.go b/internal/testhelper/testserver.go
index 0deca476a..b6dab7ed9 100644
--- a/internal/testhelper/testserver.go
+++ b/internal/testhelper/testserver.go
@@ -162,6 +162,7 @@ func (p *TestServer) Start() error {
 		MemoryQueueEnabled: true,
 		Failover: praefectconfig.Failover{
 			Enabled:           true,
+			ElectionStrategy:  praefectconfig.ElectionStrategyLocal,
 			BootstrapInterval: config.Duration(time.Microsecond),
 			MonitorInterval:   config.Duration(time.Second),
 		},