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.

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

bpf: nat: tolerate unhandled protocol types in revSNAT path #25740

Merged

Conversation

julianwiedmann
Copy link
Member

In the past we would just pass up packets with unknown protocols, and let the next layer (or the stack) deal with them.

#19753 recently added error handling for the revSNAT paths, so that we can catch errors while eg. rewriting the packet headers.

But we want to preserve the old behaviour of tolerating unknown protocol types. So return NAT_PUNT_TO_STACK from a few additional ICMP paths.

In the past we would just pass up packets with unknown protocols, and let
the next layer (or the stack) deal with them.

cilium#19753 recently added error handling
for the revSNAT paths, so that we can catch errors while eg. rewriting the
packet headers.

But we want to preserve the old behaviour of tolerating unknown protocol
types. So return NAT_PUNT_TO_STACK from a few additional ICMP paths.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. labels May 29, 2023
@julianwiedmann julianwiedmann requested a review from a team as a code owner May 29, 2023 09:43
@julianwiedmann
Copy link
Member Author

julianwiedmann commented May 29, 2023

/test

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

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) with L7 policy Tests NodePort with L7 Policy

Failure Output

FAIL: Request from testclient-ffqvl pod to service tftp://[fd04::12]:30606/hello failed

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

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

Then please upload the Jenkins artifacts to that issue.

@julianwiedmann
Copy link
Member Author

@julianwiedmann
Copy link
Member Author

/test-1.25-4.19

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 30, 2023
@julianwiedmann julianwiedmann merged commit 3f04408 into cilium:main May 30, 2023
63 checks passed
@julianwiedmann julianwiedmann deleted the 1.14-bpf-revsnat-toleration branch May 30, 2023 08:31
@brb
Copy link
Member

brb commented Jun 5, 2023

@julianwiedmann Too late to the party, but do you plan to do the same for snat_v4_nat()?

@julianwiedmann
Copy link
Member Author

@julianwiedmann Too late to the party, but do you plan to do the same for snat_v4_nat()?

We should make the same change, but not sure whether I will get to it. It's also not quite as trivial (as we should at least consider whether all types of traffic are ok to leave the node untranslated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants