egressgw: drop traffic if no gateway is found by MrFreezeex · Pull Request #24835 · cilium/cilium

MrFreezeex · 2023-04-12T11:08:01Z

Switch to dropping traffic when no gateway are found for an egressgw instead of the previous behavior consisting of allowing traffic without the snat.

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Fixes: #issue-number

Drop traffic matching an egress gateway policy when no gateway are found

bpf/tests/tc_egressgw_redirect.c

MrFreezeex · 2023-04-12T11:30:04Z

Hi @jibi 👋, it's me again for the second part of the work to actually drop traffic that doesn't match any gateway :D (for reference/other people tuning in you will be able to find more context here #24449 (comment)).

pchaigno · 2023-04-12T15:45:35Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig MonitorAggregation Checks that monitor aggregation flags send notifications

Failure Output

FAIL: Timed out after 240.001s.

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/898/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

jibi

Looks good! Just a couple of small comments

bpf/bpf_lxc.c

bpf/lib/egress_policies.h

bpf/tests/tc_egressgw_redirect.c

jibi · 2023-04-18T12:06:45Z

🤔

2023-04-18T11:55:51.342505519Z level=fatal msg="auto-direct-node-routes cannot be used with tunneling. Packets must be routed through the tunnel device." subsys=daemon

can you try rebasing on top of master please?

rolinh

Hubble API changes lgtm.

jibi

We should also update docs (both the egressgw one as well as the upgrade guide) to point out that from now on we'll start dropping packets in case no gateway is found, but this can be done in a follow up PR since CI is already green

christarazi · 2023-04-18T19:38:25Z

One second thought since I saw the upgrade-impact label, should we include some note in the upgrade guide about this?
cc @pchaigno @jibi

@christarazi we should add some notes in the upgrade guide as we are changing the behavior of the no gateway case, see my previous message #24835 (review).

But I don't think the upgrade-impact label is still accurate. When I added it I thought the feature that this PR (and the previous related one) are changing was already in v1.13, but that's not the case

Oops, you caught me not reading other reviews 😅.

I'll keep my review as "requesting changes" because otherwise the merge button is green. Thanks!

jibi · 2023-04-18T19:45:05Z

I'll keep my review as "requesting changes" because otherwise the merge button is green. Thanks!

fair enough 👍 @MrFreezeex could you please add a note about the new behaviour:

in the 1.14 upgrade guide: https://github.com/cilium/cilium/blob/main/Documentation/operations/upgrade.rst#114-upgrade-notes
as well as in the egressgw guide: https://github.com/cilium/cilium/blob/main/Documentation/network/egress-gateway.rst (perhaps in the section about the gateway node selection)

christarazi

Thanks!

zacharysarah

@MrFreezeex Thanks for the update. Minor edits, otherwise LGTM

Documentation/network/egress-gateway.rst

Documentation/operations/upgrade.rst

Switch to dropping traffic when no gateway are found for an egressgw instead of the previous behavior consisting of allowing traffic without the snat. It also adds a new drop reason (DROP_NO_EGRESS_GATEWAY) for this specific case. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>

jibi · 2023-04-19T08:23:40Z

/test

MrFreezeex · 2023-04-19T08:32:40Z

Conformance job failure (https://github.com/cilium/cilium/actions/runs/4740958766/jobs/8417418731?pr=24835) seems related to #24622

jibi · 2023-04-20T07:25:43Z

All good here with reviews and CI, merging this 🚢 thanks again for the great work!

MrFreezeex requested review from a team as code owners April 12, 2023 11:08

MrFreezeex requested a review from christarazi April 12, 2023 11:08

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 12, 2023

MrFreezeex commented Apr 12, 2023

View reviewed changes

bpf/tests/tc_egressgw_redirect.c Outdated Show resolved Hide resolved

MrFreezeex force-pushed the egressgw-drop-no-gateway branch from 786a3ff to 1c8fb00 Compare April 12, 2023 12:47

jibi self-requested a review April 12, 2023 14:45

jibi added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 12, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023

jibi added upgrade-impact This PR has potential upgrade or downgrade impact. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. feature/egress-gateway Impacts the egress IP gateway feature. labels Apr 12, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023

jibi reviewed Apr 18, 2023

View reviewed changes

bpf/bpf_lxc.c Outdated Show resolved Hide resolved

bpf/lib/egress_policies.h Outdated Show resolved Hide resolved

bpf/tests/tc_egressgw_redirect.c Outdated Show resolved Hide resolved

MrFreezeex force-pushed the egressgw-drop-no-gateway branch from 1c8fb00 to acb187d Compare April 18, 2023 10:37

MrFreezeex requested review from a team as code owners April 18, 2023 10:37

MrFreezeex requested review from nathanjsweet and rolinh April 18, 2023 10:37

MrFreezeex force-pushed the egressgw-drop-no-gateway branch 3 times, most recently from c4b6f89 to 2e9ad4a Compare April 18, 2023 10:58

MrFreezeex force-pushed the egressgw-drop-no-gateway branch from 2e9ad4a to bd991e1 Compare April 18, 2023 12:08

rolinh approved these changes Apr 18, 2023

View reviewed changes

jibi approved these changes Apr 18, 2023

View reviewed changes

christarazi approved these changes Apr 18, 2023

View reviewed changes

MrFreezeex force-pushed the egressgw-drop-no-gateway branch from bd991e1 to 2b80b7e Compare April 18, 2023 20:00

MrFreezeex requested a review from a team as a code owner April 18, 2023 20:00

MrFreezeex requested a review from qmonnet April 18, 2023 20:00

christarazi approved these changes Apr 18, 2023

View reviewed changes

zacharysarah suggested changes Apr 19, 2023

View reviewed changes

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved

MrFreezeex force-pushed the egressgw-drop-no-gateway branch from 2b80b7e to 064cf32 Compare April 19, 2023 08:03

jibi requested a review from zacharysarah April 19, 2023 11:19

zacharysarah approved these changes Apr 19, 2023

View reviewed changes

zacharysarah added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Apr 19, 2023

jibi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 20, 2023

jibi merged commit a292a75 into cilium:main Apr 20, 2023

jibi added backport/author The backport will be carried out by the author of the PR. needs-backport/1.13 labels Apr 20, 2023

maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Apr 20, 2023

MrFreezeex mentioned this pull request Apr 20, 2023

v1.13 backports 2023-04-20 #24999

Merged

1 task

dylandreimerink mentioned this pull request Apr 21, 2023

Assume Ingress identity also for cluster internal traffic via Ingress #24826

Merged

jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 labels Apr 21, 2023

thorn3r mentioned this pull request May 17, 2023

Prepare for release v1.13.3 #25514

Merged

Conversation

MrFreezeex commented Apr 12, 2023

Uh oh!

Uh oh!

MrFreezeex commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno commented Apr 12, 2023 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

jibi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jibi commented Apr 18, 2023

Uh oh!

rolinh left a comment

Choose a reason for hiding this comment

Uh oh!

jibi left a comment

Choose a reason for hiding this comment

Uh oh!

christarazi commented Apr 18, 2023

Uh oh!

jibi commented Apr 18, 2023

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

zacharysarah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jibi commented Apr 19, 2023

Uh oh!

MrFreezeex commented Apr 19, 2023

Uh oh!

jibi commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MrFreezeex commented Apr 12, 2023 •

edited

Loading

pchaigno commented Apr 12, 2023 •

edited by maintainer-s-little-helper bot

Loading