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: drop traffic if no gateway is found #24835

Merged
merged 1 commit into from
Apr 20, 2023
Merged

egressgw: drop traffic if no gateway is found #24835

merged 1 commit into from
Apr 20, 2023

Conversation

MrFreezeex
Copy link
Member

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

@MrFreezeex MrFreezeex requested review from a team as code owners April 12, 2023 11:08
@maintainer-s-little-helper 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 github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 12, 2023
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Apr 12, 2023 •
edited

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)).

@jibi jibi self-requested a review April 12, 2023 14:45
@jibi 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 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 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 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
@pchaigno
Copy link
Member

pchaigno commented Apr 12, 2023 •
edited by maintainer-s-little-helper bot

/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
jibi reviewed Apr 18, 2023
View reviewed changes
Copy link
Member

@jibi jibi left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of small comments

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 Show resolved Hide resolved
@MrFreezeex MrFreezeex requested review from a team as code owners April 18, 2023 10:37
@MrFreezeex MrFreezeex force-pushed the egressgw-drop-no-gateway branch 3 times, most recently from c4b6f89 to 2e9ad4a Compare April 18, 2023 10:58
@jibi
Copy link
Member

jibi commented Apr 18, 2023

🤔

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
rolinh approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Hubble API changes lgtm.

jibi
jibi approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@jibi jibi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

jibi commented Apr 18, 2023

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:

@MrFreezeex MrFreezeex requested a review from a team as a code owner April 18, 2023 20:00
@MrFreezeex MrFreezeex requested a review from qmonnet April 18, 2023 20:00
christarazi
christarazi approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Thanks!

zacharysarah
zacharysarah requested changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

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

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
@MrFreezeex
egressgw: drop traffic if no gateway is found
064cf32 
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
Copy link
Member

jibi commented Apr 19, 2023

/test

@MrFreezeex
Copy link
Member Author

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

@jibi jibi requested a review from zacharysarah April 19, 2023 11:19
@zacharysarah zacharysarah added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Apr 19, 2023
@jibi 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
Copy link
Member

jibi commented Apr 20, 2023

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

@jibi jibi merged commit a292a75 into cilium:main Apr 20, 2023
41 of 42 checks passed
@jibi jibi added backport/author The backport will be carried out by the author of the PR. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 20, 2023
@maintainer-s-little-helper 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 MrFreezeex mentioned this pull request Apr 20, 2023
Merged
1 task
This was referenced Apr 20, 2023
Merged
Merged
Merged
Merged
@dylandreimerink dylandreimerink mentioned this pull request Apr 21, 2023
Merged
@jibi jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 21, 2023
@thorn3r thorn3r mentioned this pull request May 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@rolinh rolinh rolinh approved these changes

@zacharysarah zacharysarah zacharysarah approved these changes

@christarazi christarazi christarazi approved these changes

@nathanjsweet nathanjsweet Awaiting requested review from nathanjsweet nathanjsweet is a code owner automatically assigned from cilium/api

@qmonnet qmonnet Awaiting requested review from qmonnet qmonnet is a code owner automatically assigned from cilium/docs-structure

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. 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. feature/egress-gateway Impacts the egress IP gateway feature. kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MrFreezeex @pchaigno @jibi @christarazi @rolinh @zacharysarah