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

ipsec: Fix incorrect trace in from-network #18643

Merged
merged 2 commits into from
Feb 1, 2022
Merged

ipsec: Fix incorrect trace in from-network #18643

merged 2 commits into from
Feb 1, 2022

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Jan 27, 2022 •
edited

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.
  • 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.
  • Thanks for contributing!

In from-network, there is a wrong send_trace_notify which sets
TRACE_REASON_ENCRYPTED for decrypted packets.

Fix incorrect packet trace for encrypted packets received from the network

@YutaroHayakawa YutaroHayakawa requested a review from a team January 27, 2022 14:23
@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 Jan 27, 2022
@pchaigno pchaigno self-requested a review January 27, 2022 15:18
@pchaigno pchaigno added area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. needs-backport/1.11 release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 27, 2022
@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 Jan 27, 2022
@pchaigno pchaigno added the kind/bug This is a bug in the Cilium logic. label Jan 27, 2022
pchaigno
pchaigno reviewed Jan 27, 2022
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment •
edited

Choose a reason for hiding this comment

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

Fix incorrect trace in from-network

from-network won't mean anything to users (though they could guess :) ). What about Fix incorrect packet trace for encrypted packets received from the network?

bpf/bpf_network.c Outdated Show resolved Hide resolved
bpf/bpf_network.c Outdated Show resolved Hide resolved
@YutaroHayakawa
Copy link
Member Author

Fix incorrect trace in from-network

from-network won't mean anything to users (though they could guess :) ). What about Fix incorrect packet trace for encrypted packets received from the network?

Yep, sounds more user-friendly. I used that.

pchaigno
pchaigno reviewed Jan 31, 2022
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.

I think the third commit is enough (changes made in the first commit are overwritten by the third anyway). Also, don't we need the same fix as in commit 3 for bpf_overlay? Changes in bpf_network look good to me 👍

bpf/bpf_network.c Outdated Show resolved Hide resolved
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Jan 31, 2022 •
edited

I think the third commit is enough (changes made in the first commit are overwritten by the third anyway).

Yep, let me clean up the commit.

Also, don't we need the same fix as in commit 3 for bpf_overlay?

Yes, I'm now working for the overlay as well now. Hopefully, I can make a commit in short.

@YutaroHayakawa YutaroHayakawa force-pushed the no-issue-fix-wrong-trace-in-from-network branch from 9ca7d11 to 2ba897c Compare January 31, 2022 15:37
@YutaroHayakawa
ipsec: Trace ESP packets coming from the network as encrypted
a9be513 
When IPSec is enabled, it is helpful to trace ESP packets as encrypted.
Fix the current program to do that. In non-IPSec mode, it won't happen
because we are not interested in the encrypted packets. Please see the
comment as well for more details.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the no-issue-fix-wrong-trace-in-from-network branch from 2ba897c to b34476d Compare January 31, 2022 15:49
@YutaroHayakawa
Copy link
Member Author

Now overlay side is done and commits are cleaned up. Unfortunately, the overlay side became a bit wasteful program since it adds one extra packet parsing just for tracing. If it seems not good, I'm happy to remove it actually :(

@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Jan 31, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sLRPTests Checks local redirect policy LRP connectivity

Failure Output

FAIL: Cannot retrieve services on cilium pod

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sLRPTests Checks local redirect policy LRP connectivity

Failure Output

FAIL: Cannot retrieve services on cilium pod

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sLRPTests Checks local redirect policy LRP connectivity

Failure Output

FAIL: Cannot retrieve services on cilium pod

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

pchaigno
pchaigno approved these changes Jan 31, 2022
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.

Only minor comments this time.

If you only push fixes for those without rebasing, I don't think we'll need to rerun the end-to-end tests (assuming they pass now).

bpf/bpf_overlay.c Outdated Show resolved Hide resolved
bpf/bpf_overlay.c Outdated Show resolved Hide resolved
@YutaroHayakawa
ipsec: Trace ESP packets coming from the overlay as encrypted
8bad03c 
When IPSec is enabled, it is helpful to trace ESP packets as encrypted.
Fix the current program to do that. In non-IPSec mode, it won't happen
because we are not interested in the encrypted packets. Please see the
comment as well for more details.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the no-issue-fix-wrong-trace-in-from-network branch from b34476d to 8bad03c Compare February 1, 2022 01:22
@YutaroHayakawa
Copy link
Member Author

Fixed based on Paul's comment.

@pchaigno
Copy link
Member

pchaigno commented Feb 1, 2022

The cilium/bpf review request is covered. Tests were passing before the last push, except for K8sLRPTests Checks local redirect policy LRP connectivity which failed due to #15474. The last push has only minor changes and the compile tests are passing. So I think this is ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 1, 2022
@glibsm glibsm merged commit 2cfff53 into cilium:master Feb 1, 2022
@joamaki joamaki mentioned this pull request Feb 7, 2022
Merged
@joamaki joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022
@joestringer joestringer mentioned this pull request Feb 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@kkourt kkourt Awaiting requested review from kkourt

Assignees

@kkourt kkourt

Labels
area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@YutaroHayakawa @pchaigno @glibsm @kkourt @joamaki