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

bpf/nat: implement support of ICMP4 fragmentation needed at egress #25054

Merged
merged 1 commit into from Apr 28, 2023
Merged

bpf/nat: implement support of ICMP4 fragmentation needed at egress #25054

merged 1 commit into from Apr 28, 2023

Conversation

liuyuan10
Copy link
Contributor

@liuyuan10 liuyuan10 commented Apr 21, 2023 •
edited

PR #18414 added support for ingress ICMP "fragmentation needed" support to hande those sent by remote routers. This commit mirrors it to support such ICMP sent from endpoints.

The use case is that pod is redirecting traffic from the world into a tunnel, which has smaller MTU. It may return a ICMP "frag needed" to the remote server that requires SNAT to happen properly on both outer and inner headers.

Address IPv4 "fragmentation needed" part of : #23955

Supports IPv4 ICMP "fragmentation needed" in egress SNAT

@liuyuan10 liuyuan10 requested a review from a team as a code owner April 21, 2023 22:02
@maintainer-s-little-helper
Copy link

Commit 554b8c509166cdc94e61a1a41015f93f80f63a2b does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 21, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 21, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 21, 2023
@liuyuan10 liuyuan10 force-pushed the icmp-nat-upstream branch 3 times, most recently from f954e1f to 0937f33 Compare April 21, 2023 23:53
dylandreimerink
dylandreimerink requested changes Apr 24, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Thank you for this work. Seems very nice overall, just a few small things.

I do have some doubts surrounding the complexity/use-case trade off though.

bpf/lib/nat.h Show resolved Hide resolved
bpf/lib/nat.h Outdated Show resolved Hide resolved
bpf/lib/nat.h Show resolved Hide resolved
@pchaigno
Copy link
Member

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

/test

Job 'Cilium-PR-K8s-1.27-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

Failure Output

FAIL: Can not connect to service "http://192.168.56.11:32657" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.27-kernel-net-next/112/

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

@liuyuan10
Copy link
Contributor Author

I do have some doubts surrounding the complexity/use-case trade off though.

I agree this is not a common use case. But I do think it's good to support ICMP packets in SNAT (e.g. Pods may have iptables rules rejecting traffic with ICMP). With this change, it's just a few lines more to support other ICMP codes

@dylandreimerink
Copy link
Member

I do have some doubts surrounding the complexity/use-case trade off though.

I agree this is not a common use case. But I do think it's good to support ICMP packets in SNAT

The "Datapath BPF Complexity" test passed on the previous iteration with room to spare in the NAT related tail calls, so I think we are good.

@liuyuan10
bpf/nat: implement support of ICMP4 fragmentation needed at egress
18627d8 
PR cilium#18414 added support for ingress ICMP "need to frag" support to hande
those sent by remote routers. This commit mirrors it to support such
ICMP sent from endpoints.

The use case is that pod is redirecting traffic from the world into a
tunnel, which has smaller MTU. It may return a ICMP "frag needed" to the
remote server that requires SNAT to happen properly on both outer and
inner headers.

Signed-off-by: Yuan Liu <liuyuan@google.com>
@liuyuan10
Copy link
Contributor Author

@dylandreimerink what's the next step for this PR? I don't see unresolved comments

dylandreimerink
dylandreimerink approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

https://github.com/cilium/cilium/pull/25054/files#r1175491486 was not resolved and you had not re-requested a review, so I thought you might still have some amendments, hence the delay. Though this is such a minor thing that I think everything is good as it is.

Next step is the setting the correct labels which I will do and to run the full test suite once more on the latest changes

@dylandreimerink dylandreimerink added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 27, 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 27, 2023
@dylandreimerink
Copy link
Member

dylandreimerink commented Apr 27, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Encapsulation Check iptables masquerading with random-fully

Failure Output

FAIL: Error creating resource /home/jenkins/workspace/Cilium-PR-K8s-1.24-kernel-5.4/src/github.com/cilium/cilium/test/k8s/manifests/l3-policy-demo.yaml: Cannot retrieve "cilium-2qxhr"'s policy revision: cannot get policy revision: ""

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-5.4/1874/

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

Then please upload the Jenkins artifacts to that issue.

@liuyuan10
Copy link
Contributor Author

https://github.com/cilium/cilium/pull/25054/files#r1175491486 was not resolved and you had not re-requested a review, so I thought you might still have some amendments

Sorry I didn't realize I need to re-request a review. I expect my reply addressed your comment and please mark it resolved if it's ok with you. or let me know if you have other concerns

@dylandreimerink
Copy link
Member

dylandreimerink commented Apr 28, 2023 •
edited by maintainer-s-little-helper bot

/mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4

👍 created #25187

@dylandreimerink
Copy link
Member

/test-1.24-5.4

@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 Apr 28, 2023
@pchaigno pchaigno merged commit c31688a into cilium:main Apr 28, 2023
56 checks passed
@liuyuan10 liuyuan10 deleted the icmp-nat-upstream branch April 28, 2023 16:55
@julianwiedmann julianwiedmann mentioned this pull request May 8, 2023
Closed
@julianwiedmann julianwiedmann mentioned this pull request May 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylandreimerink dylandreimerink dylandreimerink approved these changes

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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
3 participants
@liuyuan10 @pchaigno @dylandreimerink