From dc771c508b6fe07c593ea96625d0039eeba7e48a Mon Sep 17 00:00:00 2001 From: SherlockShemol Date: Sun, 30 Nov 2025 15:11:09 +0800 Subject: [PATCH] raft: return error when ConfChange is rejected due to pending change Previously, when a ConfChange proposal was rejected (e.g., due to an existing pending config change), the proposal was silently converted to a no-op entry without returning any error. This silent failure could lead to security issues where administrators believe a configuration change succeeded when it was actually ignored. This commit adds error notification while preserving the original behavior of appending a no-op entry for log consistency: 1. When a ConfChange is rejected, it is still converted to a no-op (maintaining backward compatibility and log consistency) 2. BUT now returns ErrProposalDropped to notify the caller This allows callers to: - Detect when their ConfChange was rejected - Take appropriate action (e.g., wait and retry) - Log warnings or alerts The fix is conservative: it maintains full backward compatibility with existing log structures while adding explicit error notification. Signed-off-by: SherlockShemol --- raft.go | 12 ++++++++++++ raft_test.go | 7 +++++-- testdata/confchange_v2_add_single_explicit.txt | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/raft.go b/raft.go index 94c2363d..56f53139 100644 --- a/raft.go +++ b/raft.go @@ -1298,6 +1298,10 @@ func stepLeader(r *raft, m pb.Message) error { return ErrProposalDropped } + // Track if any ConfChange was rejected so we can return an error + // while still appending the noop to maintain log consistency. + var confChangeRejected bool + for i := range m.Entries { e := &m.Entries[i] var cc pb.ConfChangeI @@ -1330,7 +1334,10 @@ func stepLeader(r *raft, m pb.Message) error { if failedCheck != "" && !r.disableConfChangeValidation { r.logger.Infof("%x ignoring conf change %v at config %s: %s", r.id, cc, r.trk.Config, failedCheck) + // Convert to noop to maintain log consistency, but mark as rejected + // so we can return an error to notify the caller. m.Entries[i] = pb.Entry{Type: pb.EntryNormal} + confChangeRejected = true } else { r.pendingConfIndex = r.raftLog.lastIndex() + uint64(i) + 1 traceChangeConfEvent(cc, r) @@ -1342,6 +1349,11 @@ func stepLeader(r *raft, m pb.Message) error { return ErrProposalDropped } r.bcastAppend() + // Return error if any ConfChange was rejected, so the caller knows + // the config change didn't take effect (even though a noop was appended). + if confChangeRejected { + return ErrProposalDropped + } return nil case pb.MsgReadIndex: // only one voting member (the leader) in the cluster diff --git a/raft_test.go b/raft_test.go index 85bbcce7..675e3c36 100644 --- a/raft_test.go +++ b/raft_test.go @@ -2687,7 +2687,7 @@ func TestStepConfig(t *testing.T) { // TestStepIgnoreConfig tests that if raft step the second msgProp in // EntryConfChange type when the first one is uncommitted, the node will set -// the proposal to noop and keep its original state. +// the proposal to noop, keep its original state, and return an error. func TestStepIgnoreConfig(t *testing.T) { // a raft that cannot make progress r := newTestRaft(1, 10, 1, newTestMemoryStorage(withPeers(1, 2))) @@ -2696,7 +2696,10 @@ func TestStepIgnoreConfig(t *testing.T) { r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}}) index := r.raftLog.lastIndex() pendingConfIndex := r.pendingConfIndex - r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}}) + // Second ConfChange should be converted to noop but return an error + err := r.Step(pb.Message{From: 1, To: 1, Type: pb.MsgProp, Entries: []pb.Entry{{Type: pb.EntryConfChange}}}) + assert.Equal(t, ErrProposalDropped, err) + // A noop entry should still be appended (for log consistency) wents := []pb.Entry{{Type: pb.EntryNormal, Term: 1, Index: 3, Data: nil}} ents, err := r.raftLog.entries(index+1, noLimit) require.NoError(t, err) diff --git a/testdata/confchange_v2_add_single_explicit.txt b/testdata/confchange_v2_add_single_explicit.txt index 1d390e55..01a11ba1 100644 --- a/testdata/confchange_v2_add_single_explicit.txt +++ b/testdata/confchange_v2_add_single_explicit.txt @@ -103,6 +103,7 @@ propose-conf-change 1 v3 v4 v5 ---- INFO 1 ignoring conf change {ConfChangeTransitionAuto [{ConfChangeAddNode 3} {ConfChangeAddNode 4} {ConfChangeAddNode 5}] []} at config voters=(1 2)&&(1): must transition out of joint config first +raft proposal dropped # Propose a transition out of the joint config. We'll see this at index 6 below. propose-conf-change 1 @@ -165,6 +166,7 @@ stabilize propose-conf-change 1 ---- INFO 1 ignoring conf change {ConfChangeTransitionAuto [] []} at config voters=(1 2): not in joint state; refusing empty conf change +raft proposal dropped # Finishes work for the empty entry we just proposed. stabilize