Skip to content

node: Remove permanent ARP entry when remote node is deleted #10227

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 25, 2020
Merged

node: Remove permanent ARP entry when remote node is deleted #10227

merged 2 commits into from
Feb 25, 2020

Conversation

brb
Copy link
Member

@brb brb commented Feb 18, 2020 •
edited
Loading

When IPsec or NodePort is enabled, we add a permanent ARP entry (remote node IP => remote node MAC addr) upon receiving a NodeUpdate event. The entry is needed to facilitate calls to fib_lookup() from the datapath.

Up until now, the permanent entry was not removed when the remote node was deleted. This could lead to a problem, when a packet destined to a new node which reused the IP addr of the deleted node was dropped due to the wrong MAC addr until NodeUpdate event for the new node had been received.

This PR fixes the problem by removing obsolete ARP entries.

Tested manually.

Reviewable per commit.

Fixes #10197.

This change is Reviewable

@brb
node: Do not try to insert ARP entry if IPv4 is disabled
fd449c2 
We manage only ARP entries, which is IPV4-only, so it doesn't make sense
to do it if IPv4 is disabled.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb added wip area/daemon Impacts operation of the Cilium daemon. labels Feb 18, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

4 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@brb
Copy link
Member Author

brb commented Feb 18, 2020

test-me-please

@brb brb added the release-note/misc This PR makes changes that have no direct user impact. label Feb 18, 2020
@brb brb force-pushed the pr/brb/rm-arp-entry branch from 1926876 to 2d9ff70 Compare February 18, 2020 10:46
@brb
node: Remove permanent ARP entry when remote node is deleted
2d9ff70 
When IPsec or NodePort is enabled, we add a permanent ARP entry (remote
node IP => remote node MAC addr) upon receiving a NodeUpdate event.
The entry is needed to facilitate calls to fib_lookup() from the
datapath.

Up until now, the permanent entry was not removed when the remote node
was deleted. This could lead to a problem, when a packet destined to a
new node which reused the IP addr of the deleted node was dropped due to
the wrong MAC addr until NodeUpdate event for the new node had been
received.

This commit fixes the problem by removing obsolete ARP entries.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Feb 18, 2020

test-me-please

@brb brb added pending-review and removed wip labels Feb 18, 2020
@brb brb marked this pull request as ready for review February 18, 2020 11:02
@brb brb requested a review from a team February 18, 2020 11:02
@coveralls
Copy link

coveralls commented Feb 18, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.01%) to 44.461% when pulling 2d9ff70 on pr/brb/rm-arp-entry into b6f2e75 on master.

@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 18, 2020
@aanm aanm added this to the 1.8 milestone Feb 18, 2020
@aanm aanm removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 18, 2020
aanm
aanm requested changes Feb 18, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Can't this be unit tested since we have a pkg/datapath/linux/node_test.go file for this?

jrfastab
jrfastab approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

LGTM thanks for doing this.

@brb
Copy link
Member Author

brb commented Feb 24, 2020

@aanm Can we merge this? I'm going to add the tests after I've finished my work on a major feature.

@aanm
Copy link
Member

aanm commented Feb 24, 2020

@aanm Can we merge this? I'm going to add the tests after I've finished my work on a major feature.

I'll defer that decision to @joestringer.

@joestringer
Copy link
Member

joestringer commented Feb 25, 2020 •
edited
Loading

@brb please file an issue to follow up on the tests just to make sure it doesn't get dropped. Manual testing of something like this is as good as no testing, it could regress at any time. Unit tests at the very least should not require a large commitment.

The change itself looks pretty straightforward and has relevant signoffs so LGTM.

@joestringer joestringer merged commit 6c06c51 into master Feb 25, 2020
@joestringer joestringer deleted the pr/brb/rm-arp-entry branch February 25, 2020 21:12
@joestringer joestringer added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Feb 25, 2020
@joestringer
Copy link
Member

Also, I marked this as a bugfix as this seems like it could cause neighbour cache to grow unchecked if there is much node churn.

@brb
Copy link
Member Author

brb commented Feb 26, 2020

@joestringer @aanm Created an issue for testing: #10341.

@joestringer joestringer mentioned this pull request Feb 27, 2020
Merged
@brb brb mentioned this pull request Feb 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm requested changes

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

@jrfastab jrfastab jrfastab approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

Remove permanent ARP entry when node is deleted
6 participants
@brb @coveralls @aanm @joestringer @borkmann @jrfastab