Clean up stale ENI rules upon endpoint deletion #11163

christarazi · 2020-04-27T04:47:19Z

See commit messages, reviewable per-commit.

Note: commits starting with "dropme!" are meant to be dropped for the final change. These changes are pending in #11073. They can be ignored for the review.

TODO:

Remove ifindex from function arg to enirouting.Delete (as it is always -1 anyway)
Add IPv6 tests for listing rules
Rebase on top of master when Fix cilium-health healthchecking remote endpoint in ENI mode #11073 is merged and drop "dropme!" commits
Upstream PR to netlink: Add ability to list rules in a filtered manner vishvananda/netlink#538 🎉
Edit commit before merge to include issue number

Fixes: #11041

Remove stale rules for endpoints upon deletion in ENI mode

maintainer-s-little-helper · 2020-04-27T04:47:21Z

Please set the appropriate release note label.

christarazi · 2020-04-27T19:57:42Z

test-me-please

christarazi · 2020-04-27T22:24:03Z

test-me-please

coveralls · 2020-04-27T22:51:12Z

Coverage decreased (-0.1%) to 44.507% when pulling e22f554 on christarazi:pr/christarazi/delete-stale-rules-eni into ba3a74b on cilium:master.

christarazi · 2020-04-28T03:42:30Z

test-me-please

joestringer

LGTM.

All of the below are just minor nits that would help code maintainability for the future.

pkg/datapath/linux/route/route_linux.go

pkg/datapath/linux/route/route_linux_test.go

pkg/datapath/linux/route/route_linux.go

pkg/aws/eni/routing/routing.go

christarazi · 2020-04-30T22:36:43Z

test-me-please

joestringer

LGTM. Go mod bot looks like legitimate change needed but modulo that I think we're good to merge.

christarazi · 2020-04-30T22:48:17Z

test-me-please

christarazi · 2020-04-30T22:54:27Z

test-me-please

Edit: provisioning failure https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/19357/

christarazi · 2020-04-30T23:56:57Z

test-me-please

christarazi · 2020-05-01T04:28:41Z

Build green. Editing commit message mentioned to tag issue number.

Currently, this facility doesn't exist in the upstream netlink library (https://github.com/vishvananda/netlink). This commit adds it here so that we don't have to wait to utilize it when it's merged. Once it's merged, then we can replace this. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This makes log messages containing the rule much easier to read when debugging. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Previously, ENI rules were created, but never cleaned up. This can result in an increasing number of stale rules over the lifetime of a node. This commit adds the ability to delete these rules (ingress and egress). Note that routes are deliberately not deleted as they can be reused for future endpoints on the same node. This is due to the routes being created with the ENI device "ifindex" as the table ID. Fixes cilium#11041 Signed-off-by: Chris Tarazi <chris@isovalent.com>

Signed-off-by: Chris Tarazi <chris@isovalent.com>

jaffcheng · 2020-07-29T04:40:15Z

FWIW, in case someone here from commit message is trying to clean up stale ip rules, this may save you some effort:
(tested on cilium v1.7.4)

# ip rule backup
ip rule list > /tmp/ip-rule-list
cilium endpoint list | grep -v IDENTITY | awk '{if ($6 != "") {print $6}}' > /tmp/running-iplist

# check stale ingress rules before deletion
ip rule list | grep -w "20:" | awk '{if (system("grep -w "$5" /tmp/running-iplist > /dev/null")==1) {system("echo stale ip rule found: "$0)} }'
# clean up ingress rules
ip rule list | grep -w "20:" | awk '{if (system("grep -w "$5" /tmp/running-iplist > /dev/null")==1) {system("ip rule del from all to "$5";echo deleted ip rule from all to "$5)} }'

# check stale egress rules before deletion
ip rule list | grep -w "110:" | awk '{if (system("grep -w "$3" /tmp/running-iplist > /dev/null")==1) {system("echo stale ip rule found: "$0)} }'
# clean up egress rules
ip rule list | grep -w "110:" | awk '{if (system("grep -w "$3" /tmp/running-iplist > /dev/null")==1) {system("ip rule del from "$3";echo deleted ip rule from "$3)} }'

christarazi · 2020-07-29T04:52:26Z

@jaffcheng Nice! I'll also point you to https://github.com/cilium/stale-rules 😉.

jaffcheng · 2020-07-29T05:36:19Z

@christarazi Thanks! I should have found that earlier 😄 .

maintainer-s-little-helper bot added the dont-merge/needs-release-note label Apr 27, 2020

christarazi changed the title ~~Clean up stale ENI rules for endpoint traffic~~ Clean up stale ENI rules upon endpoint deletion Apr 27, 2020

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from 6ee91f7 to 699cbdb Compare April 27, 2020 05:01

christarazi added kind/bug This is a bug in the Cilium logic. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 27, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note label Apr 27, 2020

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from 699cbdb to 3c56387 Compare April 27, 2020 19:52

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from 3c56387 to feb589f Compare April 27, 2020 22:22

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from feb589f to ac42c62 Compare April 28, 2020 03:41

christarazi marked this pull request as ready for review April 28, 2020 03:46

christarazi requested review from a team April 28, 2020 03:46

christarazi requested review from a team as code owners April 28, 2020 03:46

christarazi requested a review from a team April 28, 2020 03:46

christarazi requested a review from a team as a code owner April 28, 2020 03:46

christarazi requested a review from a team April 28, 2020 03:46

christarazi removed the wip label Apr 28, 2020

christarazi marked this pull request as draft April 28, 2020 16:20

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from ac42c62 to d77765f Compare April 28, 2020 16:46

joestringer requested changes Apr 30, 2020

View reviewed changes

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from b4c6885 to 133a129 Compare April 30, 2020 22:36

christarazi requested a review from joestringer April 30, 2020 22:37

joestringer approved these changes Apr 30, 2020

View reviewed changes

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from 133a129 to 119cbe1 Compare April 30, 2020 22:47

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from 119cbe1 to c53a64a Compare April 30, 2020 22:54

christarazi requested a review from a team as a code owner April 30, 2020 22:54

christarazi mentioned this pull request Apr 30, 2020

Validate when Cilium is in ENI mode that IPv4 is enabled #11272

Closed

christarazi added 4 commits April 30, 2020 21:29

Implement Rule.String for string repr of Rule

a585721

This makes log messages containing the rule much easier to read when debugging. Signed-off-by: Chris Tarazi <chris@isovalent.com>

Move rule priority constants to linux_defaults

e22f554

Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/delete-stale-rules-eni branch from c53a64a to e22f554 Compare May 1, 2020 04:30

christarazi added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed pending-review labels May 1, 2020

qmonnet merged commit 83fc45a into cilium:master May 1, 2020

christarazi deleted the pr/christarazi/delete-stale-rules-eni branch May 1, 2020 17:51

christarazi mentioned this pull request May 8, 2020

v1.7 backports 2020-05-08 #11441

Merged

christarazi added backport-pending/1.7 and removed needs-backport/1.7 labels May 8, 2020

aanm added backport-done/1.7 and removed backport-pending/1.7 labels May 13, 2020

Clean up stale ENI rules upon endpoint deletion #11163

Clean up stale ENI rules upon endpoint deletion #11163

Uh oh!

Conversation

christarazi commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Apr 27, 2020

Uh oh!

christarazi commented Apr 27, 2020

Uh oh!

christarazi commented Apr 27, 2020

Uh oh!

coveralls commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi commented Apr 28, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christarazi commented Apr 30, 2020

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi commented Apr 30, 2020

Uh oh!

christarazi commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi commented Apr 30, 2020

Uh oh!

christarazi commented May 1, 2020

Uh oh!

jaffcheng commented Jul 29, 2020

Uh oh!

christarazi commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaffcheng commented Jul 29, 2020

Uh oh!

Uh oh!

christarazi commented Apr 27, 2020 •

edited

Loading

coveralls commented Apr 27, 2020 •

edited

Loading

christarazi commented Apr 30, 2020 •

edited

Loading

christarazi commented Jul 29, 2020 •

edited

Loading