Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: ignore raft messages if member id mismatch #17078

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

overvenus
Copy link
Contributor

@overvenus overvenus commented Dec 7, 2023

Ignore Raft messages when the To field mismatches the local member ID. In cases where incorrect Raft messages are dispatched, potentially due to a malfunctioning switch, this proactive check prevents panics, such as "tocommit is out of range".

Fix #16220
Fix #17081

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Ignore Raft messages when the `To` field mismatches the local member ID.
In cases where incorrect Raft messages are dispatched, potentially due
to a malfunctioning switch, this proactive check prevents panics,
such as "tocommit is out of range".

Signed-off-by: Neil Shen <overvenus@gmail.com>
@k8s-ci-robot
Copy link

Hi @overvenus. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High level the change looks good.

I also did some digging ito Process validation.
It was introduced in #1343 (comment) with promised followups. Don't think they were realised though.

CCing the people involved with the original discussion to get their opinion:
cc @yichengq @jonboulle @xiang90

@serathius
Copy link
Member

cc @ahrtr

// Mock a mad switch dispatching messages to wrong node.
m := raftpb.Message{
Type: raftpb.MsgHeartbeat,
To: 2, // Wrong ID.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me some time to get what's current member's ID. You do not explicitly specify a member ID for current member, so it's 0. Please let's explicitly set a member ID for current member to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9f82390, actually I want the server member ID to be 1.

server/etcdserver/server_test.go Outdated Show resolved Hide resolved
Signed-off-by: Neil Shen <overvenus@gmail.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We also need to backport the fix to both 3.4 and 3.5

@ahrtr
Copy link
Member

ahrtr commented Dec 7, 2023

Followup:

  • backport the fix to both 3.5 and 3.4
  • update changelog for both 3.5 and 3.4

@overvenus could you please raise a ticket to track the todo items?

@overvenus
Copy link
Contributor Author

Certainly! I've created ticket #17081. Feel free to review and comments.

@serathius serathius merged commit 902436e into etcd-io:main Dec 7, 2023
35 checks passed
@jkumarpa
Copy link

@overvenus @ahrtr
Could you please rollout these changes ? I see it's not yet backported

@ahrtr
Copy link
Member

ahrtr commented Apr 17, 2024

@overvenus @ahrtr Could you please rollout these changes ? I see it's not yet backported

Thanks for the reminder. Contribution is welcome for #17081

@henrybear327
Copy link
Contributor

henrybear327 commented Apr 17, 2024

@overvenus @ahrtr Could you please rollout these changes ? I see it's not yet backported

Thanks for the reminder. Contribution is welcome for #17081

Hey @ahrtr I can take this backporting task :)

@henrybear327 henrybear327 removed their assignment Apr 17, 2024
henrybear327 added a commit to henrybear327/etcd that referenced this pull request Apr 17, 2024
henrybear327 added a commit to henrybear327/etcd that referenced this pull request Apr 17, 2024
…#17078

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
henrybear327 added a commit to henrybear327/etcd that referenced this pull request Apr 17, 2024
…d-io#17078

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
henrybear327 added a commit to henrybear327/etcd that referenced this pull request Apr 17, 2024
…d-io#17078

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants