We should not blindly trust pending peers. This makes the cluster vulnerable during membership changes to node failures. The way Riak uses riak_ensemble largely avoids this issue in practice, but there are corner cases where it can still be an issue.
Pending peers should check the backend module for trust the same as normal peers do, and move to sync if necessary. As written, there are a few things that would need to be handled.
First, a pending peer can leave the pending state from either a commit or prepare message. A commit message moves the peer straight to following. A prepare moves to prefollow, and ultimately to following via a future commit. Regardless of the path, the peer should move to sync before following if it is untrusted. Perhaps the easiest approach is to make following(init) check that the peer is in-fact trusted, and bail out to sync if not. This shouldn't matter for normal peers where we already check this property (trust == true) in maybe_follow.
Second, the check_quorum, count_quorum, etc support API calls that are used for support, debugging, and to make deterministic tests currently rely upon commit messages to determine peer count. Untrusted peers should not count. As currently written, only trusted peers accept commit messages (since pending peers are considered trusted by default). Once we make pending peers accept commits before being trusted, this is incorrect. Therefore, we need to change the support API calls from using commit to using a new message that is similar except that untrusted peers nack instead of accept.
/cc basho/riak#536
FYI, this issue is mentioned in a source code TODO at riak_ensemble_peer.erl#L324-L327:
%% TODO: Trusting pending peers makes ensemble vulnerable to concurrent
%% node failures during membership changes. Change to move to
%% syncing state before moving to following.
{next_state, pending, State2#state{trust=true}};
We should not blindly trust pending peers. This makes the cluster vulnerable during membership changes to node failures. The way Riak uses
riak_ensemblelargely avoids this issue in practice, but there are corner cases where it can still be an issue.Pending peers should check the backend module for trust the same as normal peers do, and move to
syncif necessary. As written, there are a few things that would need to be handled.First, a pending peer can leave the
pendingstate from either acommitorpreparemessage. Acommitmessage moves the peer straight tofollowing. Apreparemoves toprefollow, and ultimately tofollowingvia a futurecommit. Regardless of the path, the peer should move tosyncbeforefollowingif it is untrusted. Perhaps the easiest approach is to makefollowing(init)check that the peer is in-fact trusted, and bail out tosyncif not. This shouldn't matter for normal peers where we already check this property (trust == true) inmaybe_follow.Second, the
check_quorum, count_quorum, etcsupport API calls that are used for support, debugging, and to make deterministic tests currently rely uponcommitmessages to determine peer count. Untrusted peers should not count. As currently written, only trusted peers accept commit messages (since pending peers are considered trusted by default). Once we make pending peers accept commits before being trusted, this is incorrect. Therefore, we need to change the support API calls from usingcommitto using a new message that is similar except that untrusted peers nack instead of accept./cc basho/riak#536
FYI, this issue is mentioned in a source code TODO at riak_ensemble_peer.erl#L324-L327: