Add Config.DisableConfChangeValidation

DisableConfChangeValidation turns off propose-time verification of
configuration changes against the currently active configuration of the
raft instance. These checks are generally sensible (cannot leave a joint
config unless in a joint config, et cetera) but they have false positives
because the active configuration may not be the most recent
configuration. This is because configurations are activated during log
application, and even the leader can trail log application by an
unbounded number of entries.
Symmetrically, the mechanism has false negatives - because the check may
not run against the "actual" config that will be the predecessor of the
newly proposed one, the check may pass but the new config may be invalid
when it is being applied. In other words, the checks are best-effort.

Users should *not* use this option unless they have a reliable mechanism
(above raft) that serializes and verifies configuration changes.

In CockroachDB, we doubly hit the false positive case.

In CockroachDB, each proposal (including configuration changes) has a UUID
which the leader tracks while the command is inflight. To detect that a command
is no longer inflight, the leader has to see it apply (or needs some proof that
it will never apply in the future). When a CockroachDB Replica loses leadership
in the middle of a configuration change, a new leader may carry out additional
changes that now take effect, while the old Replica is still trying to finalize
its old, now incompatible, configuration change. What CockroachDB needs is to
see the old proposal in the log so that it can be rejected at apply time, thus
terminating the inflight status of the old configuration change. But since raft
will forward the proposal to the leader where it is rejected (since it's
incompatible with the active, much newer, config), this would never happen,
thus leaking an inflight[^1].

The other case became obvious when we tried to work around the above by
terminating the config change when we noticed that raft rejected it. Because
the rejection is not made against a stable basis (it's against whatever config
is active at the given time), legitimate configuration changes would be
rejected frequently. In particular, this could occur in a way that would still
result in the respective entries becoming applied later! This caused corruption
in CockroachDB[^2] by leading to a divergence between what we thought the config
was and what it actually was.

CockroachDB does have a higher-level protection mechanism (configuration
changes are serialized through transactions reading from and writing to the
same key). The intention is for CockroachDB to use this setting, however it is
likely that other systems are susceptible to similar issues (and perhaps
unknowingly so). This setting is thus of general interest.

Touches etcd-io#80.

[^1]: cockroachdb/cockroach#105797 (comment)
[^2]: cockroachdb/cockroach#106172 (comment)

Signed-off-by: Tobias Grieger <tobias.b.grieger@gmail.com>

raft.go

@@ -257,6 +257,28 @@ type Config struct {
 	// logical clock from assigning the timestamp and then forwarding the data
 	// to the leader.
 	DisableProposalForwarding bool
+
+	// DisableConfChangeValidation turns off propose-time verification of
+	// configuration changes against the currently active configuration of the
+	// raft instance. These checks are generally sensible (cannot leave a joint
+	// config unless in a joint config, et cetera) but they have false positives
+	// because the active configuration may not be the most recent
+	// configuration. This is because configurations are activated during log
+	// application, and even the leader can trail log application by an
+	// unbounded number of entries.
+	// Symmetrically, the mechanism has false negatives - because the check may
+	// not run against the "actual" config that will be the predecessor of the
+	// newly proposed one, the check may pass but the new config may be invalid
+	// when it is being applied. In other words, the checks are best-effort.
+	//
+	// Users should *not* use this option unless they have a reliable mechanism
+	// (above raft) that serializes and verifies configuration changes. If an
+	// invalid configuration change enters the log and gets applied, a panic
+	// will result.
+	//
+	// This option may be removed once false positives are no longer possible.
+	// See: https://github.com/etcd-io/raft/issues/80
+	DisableConfChangeValidation bool
 }
 
 func (c *Config) validate() error {
@@ -359,6 +381,9 @@ type raft struct {
 	// be proposed if the leader's applied index is greater than this
 	// value.
 	pendingConfIndex uint64
+	// disableConfChangeValidation is Config.DisableConfChangeValidation,
+	// see there for details.
+	disableConfChangeValidation bool
 	// an estimate of the size of the uncommitted tail of the Raft log. Used to
 	// prevent unbounded log growth. Only maintained by the leader. Reset on
 	// term changes.
@@ -410,20 +435,21 @@ func newRaft(c *Config) *raft {
 	}
 
 	r := &raft{
-		id:                        c.ID,
-		lead:                      None,
-		isLearner:                 false,
-		raftLog:                   raftlog,
-		maxMsgSize:                entryEncodingSize(c.MaxSizePerMsg),
-		maxUncommittedSize:        entryPayloadSize(c.MaxUncommittedEntriesSize),
-		prs:                       tracker.MakeProgressTracker(c.MaxInflightMsgs, c.MaxInflightBytes),
-		electionTimeout:           c.ElectionTick,
-		heartbeatTimeout:          c.HeartbeatTick,
-		logger:                    c.Logger,
-		checkQuorum:               c.CheckQuorum,
-		preVote:                   c.PreVote,
-		readOnly:                  newReadOnly(c.ReadOnlyOption),
-		disableProposalForwarding: c.DisableProposalForwarding,
+		id:                          c.ID,
+		lead:                        None,
+		isLearner:                   false,
+		raftLog:                     raftlog,
+		maxMsgSize:                  entryEncodingSize(c.MaxSizePerMsg),
+		maxUncommittedSize:          entryPayloadSize(c.MaxUncommittedEntriesSize),
+		prs:                         tracker.MakeProgressTracker(c.MaxInflightMsgs, c.MaxInflightBytes),
+		electionTimeout:             c.ElectionTick,
+		heartbeatTimeout:            c.HeartbeatTick,
+		logger:                      c.Logger,
+		checkQuorum:                 c.CheckQuorum,
+		preVote:                     c.PreVote,
+		readOnly:                    newReadOnly(c.ReadOnlyOption),
+		disableProposalForwarding:   c.DisableProposalForwarding,
+		disableConfChangeValidation: c.DisableConfChangeValidation,
 	}
 
 	cfg, prs, err := confchange.Restore(confchange.Changer{
@@ -1254,7 +1280,7 @@ func stepLeader(r *raft, m pb.Message) error {
 					failedCheck = "not in joint state; refusing empty conf change"
 				}
 
-				if failedCheck != "" {
+				if failedCheck != "" && !r.disableConfChangeValidation {
 					r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.prs.Config, failedCheck)
 					m.Entries[i] = pb.Entry{Type: pb.EntryNormal}
 				} else {

rafttest/interaction_env_handler_add_nodes.go

@@ -50,6 +50,10 @@ func (env *InteractionEnv) handleAddNodes(t *testing.T, d datadriven.TestData) e
 				arg.Scan(t, i, &snap.Data)
 			case "async-storage-writes":
 				arg.Scan(t, i, &cfg.AsyncStorageWrites)
+			case "max-committed-size-per-ready":
+				arg.Scan(t, i, &cfg.MaxCommittedSizePerReady)
+			case "disable-conf-change-validation":
+				arg.Scan(t, i, &cfg.DisableConfChangeValidation)
 			}
 		}
 	}

testdata/confchange_disable_validation.txt

@@ -0,0 +1,84 @@
+# This test verifies the DisableConfChangeValidation setting.
+# With it set, raft should allow configuration changes to enter the log even
+# if they appear to be incompatible with the currently active configuration.
+#
+# The test sets up a single-voter group that applies entries one at a time.
+# Then it proposes a bogus entry followed by a conf change. When the bogus entry
+# has applied, a second (compatible, but the node doesn't know this yet)
+# configuration change is proposed. That configuration change is accepted into
+# the log since due to DisableConfChangeValidation=true.
+add-nodes 1 voters=(1) index=2 max-committed-size-per-ready=1 disable-conf-change-validation=true
+----
+INFO 1 switched to configuration voters=(1)
+INFO 1 became follower at term 0
+INFO newRaft 1 [peers: [1], term: 0, commit: 2, applied: 2, lastindex: 2, lastterm: 1]
+
+campaign 1
+----
+INFO 1 is starting a new election at term 0
+INFO 1 became candidate at term 1
+
+log-level none
+----
+ok
+
+stabilize
+----
+ok (quiet)
+
+# Dummy entry.
+propose 1 foo
+----
+ok
+
+propose-conf-change 1 transition=explicit
+l2 l3
+----
+ok
+
+log-level debug
+----
+ok
+
+# Entries both get appended.
+process-ready 1
+----
+Ready MustSync=true:
+Entries:
+1/4 EntryNormal "foo"
+1/5 EntryConfChangeV2 l2 l3
+
+# Dummy entry comes up for application.
+process-ready 1
+----
+Ready MustSync=false:
+HardState Term:1 Vote:1 Commit:5
+CommittedEntries:
+1/4 EntryNormal "foo"
+
+# Propose new config change. Note how it isn't rejected,
+# which is due to DisableConfChangeValidation=true.
+propose-conf-change 1
+----
+ok
+
+# Turn on autopilot: the first config change applies, the
+# second one gets committed and also applies.
+stabilize
+----
+> 1 handling Ready
+  Ready MustSync=true:
+  Entries:
+  1/6 EntryConfChangeV2
+  CommittedEntries:
+  1/5 EntryConfChangeV2 l2 l3
+  INFO 1 switched to configuration voters=(1)&&(1) learners=(2 3)
+> 1 handling Ready
+  Ready MustSync=false:
+  HardState Term:1 Vote:1 Commit:6
+  CommittedEntries:
+  1/6 EntryConfChangeV2
+  Messages:
+  1->2 MsgApp Term:1 Log:1/5 Commit:5 Entries:[1/6 EntryConfChangeV2]
+  1->3 MsgApp Term:1 Log:1/5 Commit:5 Entries:[1/6 EntryConfChangeV2]
+  INFO 1 switched to configuration voters=(1) learners=(2 3)