From dbe02a55075410ed9d8f0fe35d68ac5c6257a679 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Mon, 10 Jul 2023 15:07:49 +0200 Subject: [PATCH] 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 https://github.com/etcd-io/raft/issues/80. [^1]: https://github.com/cockroachdb/cockroach/issues/105797#issuecomment-1613201509 [^2]: https://github.com/cockroachdb/cockroach/issues/106172#issuecomment-1623568845 Signed-off-by: Tobias Grieger --- raft/raft.go | 56 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/raft/raft.go b/raft/raft.go index 8565982f11b..f760399b5b6 100644 --- a/raft/raft.go +++ b/raft/raft.go @@ -196,6 +196,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 { @@ -275,6 +297,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. @@ -320,20 +345,21 @@ func newRaft(c *Config) *raft { } r := &raft{ - id: c.ID, - lead: None, - isLearner: false, - raftLog: raftlog, - maxMsgSize: c.MaxSizePerMsg, - maxUncommittedSize: c.MaxUncommittedEntriesSize, - prs: tracker.MakeProgressTracker(c.MaxInflightMsgs), - 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: c.MaxSizePerMsg, + maxUncommittedSize: c.MaxUncommittedEntriesSize, + prs: tracker.MakeProgressTracker(c.MaxInflightMsgs), + 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{ @@ -1076,7 +1102,7 @@ func stepLeader(r *raft, m pb.Message) error { refused = "not in joint state; refusing empty conf change" } - if refused != "" { + if refused != "" && !r.disableConfChangeValidation { r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.prs.Config, refused) m.Entries[i] = pb.Entry{Type: pb.EntryNormal} } else {