From 0402c2b17d93ced57695db3e2134f3211c385e8d Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Mon, 10 Jul 2023 13:14:17 +0200 Subject: [PATCH 1/2] Mechanical rename Signed-off-by: Tobias Grieger --- raft.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/raft.go b/raft.go index 0201ffd7..7e3abee3 100644 --- a/raft.go +++ b/raft.go @@ -1242,17 +1242,17 @@ func stepLeader(r *raft, m pb.Message) error { alreadyJoint := len(r.prs.Config.Voters[1]) > 0 wantsLeaveJoint := len(cc.AsV2().Changes) == 0 - var refused string + var failedCheck string if alreadyPending { - refused = fmt.Sprintf("possible unapplied conf change at index %d (applied to %d)", r.pendingConfIndex, r.raftLog.applied) + failedCheck = fmt.Sprintf("possible unapplied conf change at index %d (applied to %d)", r.pendingConfIndex, r.raftLog.applied) } else if alreadyJoint && !wantsLeaveJoint { - refused = "must transition out of joint config first" + failedCheck = "must transition out of joint config first" } else if !alreadyJoint && wantsLeaveJoint { - refused = "not in joint state; refusing empty conf change" + failedCheck = "not in joint state; refusing empty conf change" } - if refused != "" { - r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.prs.Config, refused) + if failedCheck != "" { + 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 { r.pendingConfIndex = r.raftLog.lastIndex() + uint64(i) + 1 From d8d274b3f0c1f10089541873b8ec61be12149f54 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Mon, 10 Jul 2023 15:07:49 +0200 Subject: [PATCH 2/2] 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.go | 56 ++++++++++---- rafttest/interaction_env_handler_add_nodes.go | 4 + testdata/confchange_disable_validation.txt | 76 +++++++++++++++++++ 3 files changed, 121 insertions(+), 15 deletions(-) create mode 100644 testdata/confchange_disable_validation.txt diff --git a/raft.go b/raft.go index 7e3abee3..45604c37 100644 --- a/raft.go +++ b/raft.go @@ -254,6 +254,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 { @@ -356,6 +378,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. @@ -407,20 +432,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{ @@ -1251,7 +1277,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 { diff --git a/rafttest/interaction_env_handler_add_nodes.go b/rafttest/interaction_env_handler_add_nodes.go index 1c3ed773..fb17badb 100644 --- a/rafttest/interaction_env_handler_add_nodes.go +++ b/rafttest/interaction_env_handler_add_nodes.go @@ -54,6 +54,10 @@ func (env *InteractionEnv) handleAddNodes(t *testing.T, d datadriven.TestData) e arg.Scan(t, i, &cfg.PreVote) case "checkquorum": arg.Scan(t, i, &cfg.CheckQuorum) + case "max-committed-size-per-ready": + arg.Scan(t, i, &cfg.MaxCommittedSizePerReady) + case "disable-conf-change-validation": + arg.Scan(t, i, &cfg.DisableConfChangeValidation) case "read-only": switch arg.Vals[i] { case "safe": diff --git a/testdata/confchange_disable_validation.txt b/testdata/confchange_disable_validation.txt new file mode 100644 index 00000000..1a2bc4fd --- /dev/null +++ b/testdata/confchange_disable_validation.txt @@ -0,0 +1,76 @@ +# 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 + +stabilize log-level=none +---- +ok + +# Dummy entry. +propose 1 foo +---- +ok + +propose-conf-change 1 transition=explicit +l2 l3 +---- +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)