Skip to content

pkg: add missing xfrm-no-track rules from ipv6 #24557

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
Mar 27, 2023
Merged

pkg: add missing xfrm-no-track rules from ipv6 #24557

merged 2 commits into from
Mar 27, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Mar 24, 2023 •
edited by julianwiedmann
Loading

By right there should be a rule to let ipsec skb bypass conntrack:

-A CILIUM_PRE_raw -m mark --mark 0xd00/0xf00 -m comment --comment "cilium-xfrm-notrack:" -j CT --notrack

However ipv6 missed it and this commit adds the rule back.

Fixes: #23481

Add missing xfrm-no-track rules for IPv6 IPSec. This fixes a connectivity issue for IPv6 IPSec with externalTrafficPolicy=local.

@jschwinger233
pkg: add missing xfrm-no-track rules from ipv6
6f83126 
By right there should be a rule to let ipsec skb bypass conntrack:

-A CILIUM_PRE_raw -m mark --mark 0xd00/0xf00 -m comment --comment "cilium-xfrm-notrack:" -j CT --notrack

However ipv6 missed it and this commit adds the rule back.

Fixes: cilium#23481

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@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 Mar 24, 2023
@jschwinger233 jschwinger233 added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 24, 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 Mar 24, 2023
@jschwinger233
Revert "test: Do not test E/W eTP=Local with IPsec"
32f0493 
This reverts commit b1b9fbd and
re-enables test for IPv6 externalTrafficPolicy=Local E/W for IPsec.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233 jschwinger233 marked this pull request as ready for review March 24, 2023 13:05
@jschwinger233 jschwinger233 requested review from a team as code owners March 24, 2023 13:05
@jschwinger233 jschwinger233 requested review from aspsk and tklauser March 24, 2023 13:05
@julianwiedmann
Copy link
Member

/test

@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. feature/ipv6 Relates to IPv6 protocol support labels Mar 24, 2023
pchaigno
pchaigno reviewed Mar 24, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for tracking and fixing this!

I see it's labeled as release-note/minor, but it feels more like a release-note/bug, no? The release note should also describe the user impact. It currently describes the change itself.
Finally, I think we should figure out where we need to backport.

@jschwinger233 jschwinger233 added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 27, 2023
@jschwinger233
Copy link
Member Author

jschwinger233 commented Mar 27, 2023 •
edited
Loading

I think we should figure out where we need to backport.

I noticed there is no PR labelled with backport/1.10 and earlier versions, so I add needs-backport/1.11 for this, if it's not appropriate please let me know.

brb
brb approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

🚀

@julianwiedmann
Copy link
Member

[word-smith'ed the release-note. Also adding backport labels for 1.12 and 1.13]

@julianwiedmann julianwiedmann added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. needs-backport/1.12 labels Mar 27, 2023
@pchaigno pchaigno merged commit dd0a8f5 into cilium:master Mar 27, 2023
@jschwinger233
Copy link
Member Author

Backport PR will be taken care of by renovate bot, right? Is there anything I should follow up?

@pchaigno
Copy link
Member

Backport PR will be taken care of by renovate bot, right? Is there anything I should follow up?

Backport PRs will be opened by the backporter. If that person can't backport themselves because it's too involved, they will ping you, in the backport PR or here.

@jschwinger233 jschwinger233 deleted the gray/fix-ipsec-eTP branch March 27, 2023 10:48
@julianwiedmann julianwiedmann mentioned this pull request Mar 28, 2023
Closed
@mhofstetter mhofstetter mentioned this pull request Mar 28, 2023
Merged
@tklauser tklauser mentioned this pull request Mar 28, 2023
Merged
5 tasks
@tklauser tklauser mentioned this pull request Mar 28, 2023
Merged
6 tasks
@tklauser tklauser mentioned this pull request Mar 28, 2023
Merged
9 tasks
@tklauser tklauser added backport-pending/1.13 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed needs-backport/1.13 labels Mar 28, 2023
@YutaroHayakawa YutaroHayakawa mentioned this pull request Mar 29, 2023
Merged
@tklauser tklauser added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Mar 29, 2023
This was referenced Apr 14, 2023
Merged
Merged
@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 labels Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@pchaigno pchaigno pchaigno approved these changes

@aspsk aspsk Awaiting requested review from aspsk aspsk was automatically assigned from cilium/sig-lb

@tklauser tklauser Awaiting requested review from tklauser tklauser was automatically assigned from cilium/ci-structure

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. feature/ipv6 Relates to IPv6 protocol support release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ipsec: IPv6 externalTrafficPolicy=Local E/W is broken when accessed from non-backend node
5 participants
@jschwinger233 @julianwiedmann @pchaigno @brb @tklauser