feat: implement checksum #325
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jokestax The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
|
Heya 👋 I noticed this was |
|
Hello,I’d appreciate a review on this draft to ensure I’m heading in the right direction before proceeding with similar changes for ICMP |
| bpf_csum_diff( | ||
| mem::MaybeUninit::zeroed().assume_init(), | ||
| 0, | ||
| ip_hdr as *mut u32, | ||
| Ipv4Hdr::LEN as u32, | ||
| 0, |
There was a problem hiding this comment.
You'll see in the libbpf documentation that the bpf_l4_csum_replace function is intended for use with the bpf_csum_diff function:
https://docs.ebpf.io/linux/helper-function/bpf_l4_csum_replace/
So I'm not sure we want to remove this here. We should probably take a look at other solutions and glean what we can from how they're doing things:
There was a problem hiding this comment.
Just a thought that occurred to me: it would probably be good to take a peek at what Cilium is doing as well and cross reference between this and calico (and maybe others) to glean some insights.
| bpf_l3_csum_replace( | ||
| ctx.skb.skb, | ||
| 4u32, | ||
| old_src_addr as u64, | ||
| lb_mapping.backend_key.ip.to_be() as u64, | ||
| 4, |
There was a problem hiding this comment.
Aya provides helper methods for this that chain all the way up to the TcContext, so you don't have to include these functions directly but could instead do ctx.l3_csum_replace.
| bpf_l4_csum_replace( | ||
| ctx.skb.skb, | ||
| tcp_header_offset as u32, | ||
| old_src_port as u64, | ||
| lb_mapping.backend_key.port.to_be() as u64, | ||
| 2, | ||
| ) |
There was a problem hiding this comment.
Same as above, you should be able to do ctx.l4_csum_replace.
Description
bpf_l3_csum_replaceandbpf_l4_csum_replaceinstead ofbpf_csum_diff.