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.

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

egressgw: consider egress interface when deleting stale IP rules #26846

Merged
merged 1 commit into from Jul 17, 2023
Merged

egressgw: consider egress interface when deleting stale IP rules #26846

merged 1 commit into from Jul 17, 2023

Conversation

julianwiedmann
Copy link
Member

When an EgressGW policy was updated to select a different egress interface, delete all IP rules that still point to the old interface.

Fix a bug in the Egress Gateway feature when using the --install-egress-gateway-routes option. Delete stale IP rules after a CiliumEgressGatewayPolicy is updated and selects a different egress network interface.

@julianwiedmann
egressgw: consider egress interface when deleting stale IP rules
885fba6 
When an EgressGW policy was updated to select a different egress interface,
delete all IP rules that still point to the old interface.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. feature/egress-gateway Impacts the egress IP gateway feature. affects/v1.12 This issue affects v1.12 branch needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 15, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.0 Jul 15, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.5 Jul 15, 2023
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann marked this pull request as ready for review July 15, 2023 14:13
@julianwiedmann julianwiedmann requested a review from a team as a code owner July 15, 2023 14:13
aspsk
aspsk approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@aspsk aspsk left a comment

Choose a reason for hiding this comment

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

lgtm, one minor cosmetic comment

pkg/egressgateway/manager_privileged_test.go
@@ -56,6 +57,8 @@ const (

egressIP1 = "192.168.101.1"
egressCIDR1 = "192.168.101.1/24"
egressIP2 = "192.168.102.1"
egressCIDR2 = "192.168.102.1/24"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters, but maybe 192.168.102.0/24 looks more canonical?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would agree, but right now that change causes the tests to fail 🤔. Needs a closer eye why that is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. I think code parses this CIDR as an IP address in func parseIPRule(sourceIP, destCIDR, egressIP string, ifaceIndex int) (it parses it as a CIDR, but then only uses the IP address part), didn't look too precise

@julianwiedmann julianwiedmann merged commit 9e01ae0 into cilium:main Jul 17, 2023
66 checks passed
@julianwiedmann julianwiedmann deleted the 1.15-egressgw-iprule-deletion branch July 17, 2023 10:47
@gandro gandro added the backport/author The backport will be carried out by the author of the PR. label Jul 17, 2023
@gandro
Copy link
Member

gandro commented Jul 17, 2023 •
edited

Marking as backport/author. The matchFunc looks different on v1.13. I could probably resolve them by restructuring the code myself, but it looks like it might miss a dependent PR?

@gandro gandro mentioned this pull request Jul 17, 2023
Merged
6 tasks
@julianwiedmann julianwiedmann mentioned this pull request Jul 20, 2023
Merged
1 task
@julianwiedmann julianwiedmann added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jul 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.5 Jul 20, 2023
@joestringer joestringer mentioned this pull request Jul 25, 2023
Merged
3 tasks
@joestringer joestringer added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.0 Jul 25, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.5 Jul 26, 2023
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jul 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.0 Jul 26, 2023
@gentoo-root gentoo-root mentioned this pull request Jul 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aspsk aspsk aspsk approved these changes

@jibi jibi Awaiting requested review from jibi

Assignees
No one assigned
Labels
affects/v1.12 This issue affects v1.12 branch backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. feature/egress-gateway Impacts the egress IP gateway feature. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.13.5
Backport done to v1.13
1.14.0
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@julianwiedmann @gandro @aspsk @joestringer @aanm