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

Implement checksums properly in dataplane #25

Open
2 tasks
shaneutt opened this issue Nov 14, 2022 · 10 comments
Open
2 tasks

Implement checksums properly in dataplane #25

shaneutt opened this issue Nov 14, 2022 · 10 comments
Labels
area/dataplane area/maintenance help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@shaneutt
Copy link
Member

shaneutt commented Nov 14, 2022

Problem Statement

We currently don't handle checksums properly or at all in several places. The purpose of this task is to get that sorted out.

Acceptance Criteria

  • find all the places we are currently handling, or NOT handling L3 and L4 checksums and get them consistent
  • use helpers like bpf_l3_cksum_replace to properly recalculate checksums
@shaneutt shaneutt added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. area/dataplane area/maintenance labels Nov 14, 2022
@astoycos astoycos self-assigned this Nov 14, 2022
@astoycos astoycos removed their assignment Dec 6, 2022
@astoycos
Copy link
Member

astoycos commented Dec 6, 2022

Opening this up as a good issue for newcomers interested in getting involved in the ebpf dataplane bits :)

@LCaparelli
Copy link

Would like to work on this one o/

@LCaparelli LCaparelli self-assigned this Dec 7, 2022
@astoycos
Copy link
Member

astoycos commented Dec 8, 2022

@LCaparelli I would recommend joining the AYA discord If you're going to dive in here -> https://aya-rs.dev/community/

I already asked how to fix this bug here https://discord.com/channels/855676609003651072/1039654899408982077

@LCaparelli
Copy link

Yup, the nice folks over there already gave me a hand on the other issue I'm working on to improve the build process, thanks for the heads up @astoycos

@shaneutt shaneutt changed the title Add bpf_l3_cksum_replace instead of custom checksums Implement checksums properly in dataplane May 3, 2023
@shaneutt
Copy link
Member Author

shaneutt commented May 3, 2023

@LCaparelli as I understand it you may not have time for this one going forward so I'm opening it back up, however if you still want to take this one on please just let us know! 🖖

@shaneutt shaneutt removed the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label May 3, 2023
shaneutt added a commit that referenced this issue May 3, 2023
This adds TCP support to the dataplane and includes integration tests to
verify `TCPRoute` functionality.

Checksums and conntrack cleanup will be handled by follow-up issues:

- #25
- #85
@shaneutt shaneutt added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Oct 12, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 30, 2024
@shaneutt
Copy link
Member Author

Not stale, we still intend this for the v0.7.0 milestone:

/remove-lifecycle stale

However, I suspect @LCaparelli is no longer working on this (do correct me if you still wanted this)? So I think we're back to help wanted.

/help

@k8s-ci-robot
Copy link
Contributor

@shaneutt:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Not stale, we still intend this for the v0.7.0 milestone:

/remove-lifecycle stale

However, I suspect @LCaparelli is no longer working on this (do correct me if you still wanted this)? So I think we're back to help wanted.

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 30, 2024
@maborosii
Copy link
Contributor

maborosii commented Sep 5, 2024

I have some questions about this issue, I found some functions implemented with bpf_l4_cksum_replace and bpf_l3_cksum_replace like set_ipv4_dest_port and set_ipv4_ip_dst in tc_ingress module, so you mean that we should keep consistent in other module like tc_egress and etc. ?

@shaneutt
Copy link
Member Author

shaneutt commented Sep 5, 2024

Consistency for sure, but I also think we just have a lot of hacks in place. I removed the good-first-issue on this one some time ago because basically what we need is someone to deep dive in and suss it all out first and actually figure out exactly what is needed, and then make the patch. So just know that we're happy to support you in this one but it's a bit of a deeper dive!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dataplane area/maintenance help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
Development

No branches or pull requests

6 participants