Replace read-only feature flag with a config toggle

Removes the feature-flag controlling read-only enforcement. The
feature flag only controlled enforcement in RPCs, making future
upgrades more difficult as some storages could already be in
read-only mode when the feature flag is removed. The feature flag
is replaced with a config toggle that also controls whether the
storages should be set read-only on a failover.

11 files changed, 50 insertions, 46 deletions

diff --git a/config.praefect.toml.example b/config.praefect.toml.example
index 534f9b9a2..cec16030a 100644
--- a/config.praefect.toml.example
+++ b/config.praefect.toml.example
@@ -29,6 +29,7 @@ listen_addr = "127.0.0.1:2305"
 [failover]
 enabled = true
 election_strategy = "local" # Options: local, sql
+read_only_after_failover = false # Switch the virtual storage to read-only mode after after a failover.
 
 [[virtual_storage]]
 name = 'praefect'
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 8daa998ef..e1dbb3098 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -18,8 +18,6 @@ const (
 	GoUpdateHook = "go_update_hook"
 	// RemoteBranchesLsRemote will use `ls-remote` for remote branches
 	RemoteBranchesLsRemote = "ruby_remote_branches_ls_remote"
-	// EnforceReadOnly enforces storage read-only mode after failover.
-	EnforceReadOnly = "enforce_read_only"
 )
 
 const (
diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go
index fb28f2a20..510eada28 100644
--- a/internal/praefect/config/config.go
+++ b/internal/praefect/config/config.go
@@ -15,8 +15,9 @@ import (
 )
 
 type Failover struct {
-	Enabled          bool   `toml:"enabled"`
-	ElectionStrategy string `toml:"election_strategy"`
+	Enabled               bool   `toml:"enabled"`
+	ElectionStrategy      string `toml:"election_strategy"`
+	ReadOnlyAfterFailover bool   `toml:"read_only_after_failover"`
 }
 
 // Config is a container for everything found in the TOML config file
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index f4dff2e4b..977d66498 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -9,7 +9,6 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
@@ -144,7 +143,7 @@ func (c *Coordinator) mutatorStreamParameters(ctx context.Context, call grpcCall
 		return nil, fmt.Errorf("mutator call: get shard: %w", err)
 	}
 
-	if featureflag.IsEnabled(ctx, featureflag.EnforceReadOnly) && shard.IsReadOnly {
+	if c.conf.Failover.ReadOnlyAfterFailover && shard.IsReadOnly {
 		return nil, helper.ErrPreconditionFailed(ReadOnlyStorageError(call.targetRepo.GetStorageName()))
 	}
 
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 16b67a561..a713bb715 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -11,7 +11,6 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/internal/praefect/datastore"
@@ -60,33 +59,34 @@ func (m *mockNode) GetConnection() *grpc.ClientConn { return nil }
 
 func TestStreamDirectorReadOnlyEnforcement(t *testing.T) {
 	for _, tc := range []struct {
-		readOnly         bool
-		readOnlyEnforced bool
-		shouldError      bool
+		readOnly        bool
+		readOnlyEnabled bool
+		shouldError     bool
 	}{
 		{
-			readOnly:         false,
-			readOnlyEnforced: true,
-			shouldError:      false,
+			readOnly:        false,
+			readOnlyEnabled: true,
+			shouldError:     false,
 		},
 		{
-			readOnly:         true,
-			readOnlyEnforced: true,
-			shouldError:      true,
+			readOnly:        true,
+			readOnlyEnabled: true,
+			shouldError:     true,
 		},
 		{
-			readOnly:         false,
-			readOnlyEnforced: false,
-			shouldError:      false,
+			readOnly:        false,
+			readOnlyEnabled: false,
+			shouldError:     false,
 		},
 		{
-			readOnly:         true,
-			readOnlyEnforced: false,
-			shouldError:      false,
+			readOnly:        true,
+			readOnlyEnabled: false,
+			shouldError:     false,
 		},
 	} {
-		t.Run(fmt.Sprintf("read-only: %v, enabled: %v", tc.readOnly, tc.readOnlyEnforced), func(t *testing.T) {
+		t.Run(fmt.Sprintf("read-only: %v, enabled: %v", tc.readOnly, tc.readOnlyEnabled), func(t *testing.T) {
 			conf := config.Config{
+				Failover: config.Failover{ReadOnlyAfterFailover: tc.readOnlyEnabled},
 				VirtualStorages: []*config.VirtualStorage{
 					&config.VirtualStorage{
 						Name: "praefect",
@@ -120,9 +120,6 @@ func TestStreamDirectorReadOnlyEnforcement(t *testing.T) {
 
 			ctx, cancel := testhelper.Context()
 			defer cancel()
-			if tc.readOnlyEnforced {
-				ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.EnforceReadOnly)
-			}
 
 			frame, err := proto.Marshal(&gitalypb.CleanupRequest{Repository: &gitalypb.Repository{
 				StorageName:  storageName,
diff --git a/internal/praefect/nodes/local_elector.go b/internal/praefect/nodes/local_elector.go
index ec4246a92..04711f760 100644
--- a/internal/praefect/nodes/local_elector.go
+++ b/internal/praefect/nodes/local_elector.go
@@ -20,13 +20,14 @@ type nodeCandidate struct {
 // shard. It does NOT support multiple Praefect nodes or have any
 // persistence. This is used mostly for testing and development.
 type localElector struct {
-	m               sync.RWMutex
-	failoverEnabled bool
-	shardName       string
-	nodes           []*nodeCandidate
-	primaryNode     *nodeCandidate
-	isReadOnly      bool
-	log             logrus.FieldLogger
+	m                     sync.RWMutex
+	failoverEnabled       bool
+	shardName             string
+	nodes                 []*nodeCandidate
+	primaryNode           *nodeCandidate
+	readOnlyAfterFailover bool
+	isReadOnly            bool
+	log                   logrus.FieldLogger
 }
 
 // healthcheckThreshold is the number of consecutive healthpb.HealthCheckResponse_SERVING necessary
@@ -63,7 +64,7 @@ func (n *nodeCandidate) isHealthy() bool {
 	return true
 }
 
-func newLocalElector(name string, failoverEnabled bool, log logrus.FieldLogger, ns []*nodeStatus) *localElector {
+func newLocalElector(name string, failoverEnabled, readOnlyAfterFailover bool, log logrus.FieldLogger, ns []*nodeStatus) *localElector {
 	nodes := make([]*nodeCandidate, len(ns))
 	for i, n := range ns {
 		nodes[i] = &nodeCandidate{
@@ -72,11 +73,12 @@ func newLocalElector(name string, failoverEnabled bool, log logrus.FieldLogger,
 	}
 
 	return &localElector{
-		shardName:       name,
-		failoverEnabled: failoverEnabled,
-		log:             log,
-		nodes:           nodes,
-		primaryNode:     nodes[0],
+		shardName:             name,
+		failoverEnabled:       failoverEnabled,
+		log:                   log,
+		nodes:                 nodes,
+		primaryNode:           nodes[0],
+		readOnlyAfterFailover: readOnlyAfterFailover,
 	}
 }
 
@@ -147,7 +149,7 @@ func (s *localElector) checkNodes(ctx context.Context) error {
 	}
 
 	s.primaryNode = newPrimary
-	s.isReadOnly = true
+	s.isReadOnly = s.readOnlyAfterFailover
 
 	return nil
 }
diff --git a/internal/praefect/nodes/local_elector_test.go b/internal/praefect/nodes/local_elector_test.go
index 7c56ed664..3b8e40f46 100644
--- a/internal/praefect/nodes/local_elector_test.go
+++ b/internal/praefect/nodes/local_elector_test.go
@@ -30,7 +30,7 @@ func setupElector(t *testing.T) (*localElector, []*nodeStatus, *grpc.ClientConn,
 	secondary := newConnectionStatus(models.Node{Storage: storageName}, cc, testhelper.DiscardTestEntry(t), mockHistogramVec1)
 	ns := []*nodeStatus{cs, secondary}
 	logger := testhelper.NewTestLogger(t).WithField("test", t.Name())
-	strategy := newLocalElector(storageName, true, logger, ns)
+	strategy := newLocalElector(storageName, true, true, logger, ns)
 
 	strategy.bootstrap(time.Second)
 
diff --git a/internal/praefect/nodes/manager.go b/internal/praefect/nodes/manager.go
index b28616b1d..f2c041158 100644
--- a/internal/praefect/nodes/manager.go
+++ b/internal/praefect/nodes/manager.go
@@ -115,7 +115,7 @@ func NewManager(log *logrus.Entry, c config.Config, db *sql.DB, latencyHistogram
 		if c.Failover.Enabled && c.Failover.ElectionStrategy == "sql" {
 			strategies[virtualStorage.Name] = newSQLElector(virtualStorage.Name, c, defaultFailoverTimeoutSeconds, defaultActivePraefectSeconds, db, log, ns)
 		} else {
-			strategies[virtualStorage.Name] = newLocalElector(virtualStorage.Name, c.Failover.Enabled, log, ns)
+			strategies[virtualStorage.Name] = newLocalElector(virtualStorage.Name, c.Failover.Enabled, c.Failover.ReadOnlyAfterFailover, log, ns)
 		}
 	}
 
diff --git a/internal/praefect/nodes/manager_test.go b/internal/praefect/nodes/manager_test.go
index fccd60105..669b163e9 100644
--- a/internal/praefect/nodes/manager_test.go
+++ b/internal/praefect/nodes/manager_test.go
@@ -218,7 +218,7 @@ func TestNodeManager(t *testing.T) {
 
 	confWithFailover := config.Config{
 		VirtualStorages: virtualStorages,
-		Failover:        config.Failover{Enabled: true},
+		Failover:        config.Failover{Enabled: true, ReadOnlyAfterFailover: true},
 	}
 	confWithoutFailover := config.Config{
 		VirtualStorages: virtualStorages,
diff --git a/internal/praefect/nodes/sql_elector.go b/internal/praefect/nodes/sql_elector.go
index e84decbc8..3bb7d79ce 100644
--- a/internal/praefect/nodes/sql_elector.go
+++ b/internal/praefect/nodes/sql_elector.go
@@ -83,6 +83,7 @@ type sqlElector struct {
 	log                   logrus.FieldLogger
 	failoverSeconds       int
 	activePraefectSeconds int
+	readOnlyAfterFailover bool
 }
 
 func newSQLElector(name string, c config.Config, failoverTimeoutSeconds int, activePraefectSeconds int, db *sql.DB, log logrus.FieldLogger, ns []*nodeStatus) *sqlElector {
@@ -105,6 +106,7 @@ func newSQLElector(name string, c config.Config, failoverTimeoutSeconds int, act
 		activePraefectSeconds: activePraefectSeconds,
 		nodes:                 nodes,
 		primaryNode:           nodes[0],
+		readOnlyAfterFailover: c.Failover.ReadOnlyAfterFailover,
 	}
 }
 
@@ -423,11 +425,11 @@ func (s *sqlElector) electNewPrimary(candidates []*sqlCandidate) error {
 	DO UPDATE SET elected_by_praefect = EXCLUDED.elected_by_praefect
 				, node_name = EXCLUDED.node_name
 				, elected_at = EXCLUDED.elected_at
-				, read_only = true
+				, read_only = $5
 				, demoted = false
 	   WHERE shard_primaries.elected_at < now() - $4::INTERVAL SECOND
 	`
-	_, err = s.db.Exec(q, s.praefectName, s.shardName, newPrimaryStorage, s.failoverSeconds)
+	_, err = s.db.Exec(q, s.praefectName, s.shardName, newPrimaryStorage, s.failoverSeconds, s.readOnlyAfterFailover)
 
 	if err != nil {
 		s.log.Errorf("error updating new primary: %s", err)
diff --git a/internal/praefect/nodes/sql_elector_test.go b/internal/praefect/nodes/sql_elector_test.go
index 2b73c46dd..e5f9d3870 100644
--- a/internal/praefect/nodes/sql_elector_test.go
+++ b/internal/praefect/nodes/sql_elector_test.go
@@ -70,6 +70,9 @@ func TestBasicFailover(t *testing.T) {
 
 	conf := config.Config{
 		SocketPath: socketName,
+		Failover: config.Failover{
+			ReadOnlyAfterFailover: true,
+		},
 	}
 
 	internalSocket0, internalSocket1 := testhelper.GetTemporaryGitalySocketFileName(), testhelper.GetTemporaryGitalySocketFileName()
@@ -288,7 +291,8 @@ func TestElectNewPrimary(t *testing.T) {
 		t.Run(testCase.desc, func(t *testing.T) {
 			db.TruncateAll(t)
 
-			elector := newSQLElector(shardName, config.Config{}, failoverTimeSeconds, defaultActivePraefectSeconds, db.DB, testhelper.DiscardTestLogger(t), ns)
+			conf := config.Config{Failover: config.Failover{ReadOnlyAfterFailover: true}}
+			elector := newSQLElector(shardName, conf, failoverTimeSeconds, defaultActivePraefectSeconds, db.DB, testhelper.DiscardTestLogger(t), ns)
 
 			require.NoError(t, elector.electNewPrimary(candidates))
 			primary, readOnly, err := elector.lookupPrimary()