microsoft / retina

eBPF distributed networking observability tool for Kubernetes
https://retina.sh
MIT License
2.68k stars 198 forks source link

Don't check endieness on the host #139

Closed anubhabMajumdar closed 3 months ago

anubhabMajumdar commented 5 months ago

Describe the bug Currently Retina checks host endianness to enrich port/IP addresses. That is not correct, network is big-endian. Handle conversion in bpf code.

To Reproduce

Ref: https://github.com/microsoft/retina/blob/61e115278c478191910d3038e07e7137a96b65d2/pkg/utils/utils_linux.go#L78

Expected behavior Network is big-endian. Handle conversion as such bpf code.

Platform (please complete the following information):

rbtr commented 5 months ago

@hainenber can you comment here if you want this issue assigned to you also?

hainenber commented 5 months ago

Yes, please :D

anubhabMajumdar commented 5 months ago

What we want is to handle host to network conversion in bpf code and not handle it in userspace, making the code more efficient. Ideally, we won't need HostToNetwork and IntToIp won't need endianness check.

Some references for issue owner to consider:

hainenber commented 5 months ago

Oh that wasn't what I expect at first 😆 . I was just thinking the conversion is already somewhere in eBPF codebase.

Thanks for the lead, I'll try getting my hands wet on this one as I didn't work on low-level code before :D

rectified95 commented 5 months ago

Hand-rolled byte order swap for IPv4 (32-bit addresses):

uint32_t s = 0x80010000;
s = (s & 0x0000FFFF) << 16 | (s & 0xFFFF0000) >> 16;
s = (s & 0x00FF00FF) << 8 | (s & 0xFF00FF00) >> 8; 
cout << setw(8) << setfill('0') << s << endl;
rbtr commented 4 months ago

Hand-rolled byte order swap for IPv4 (32-bit addresses):

uint32_t s = 0x80010000;
s = (s & 0x0000FFFF) << 16 | (s & 0xFFFF0000) >> 16;
s = (s & 0x00FF00FF) << 8 | (s & 0xFF00FF00) >> 8; 
cout << setw(8) << setfill('0') << s << endl;

@rectified95 maybe you can pick this up now? 🙂

nddq commented 3 months ago

just want to dust off this issue, when we are getting the src and dst ips from the iphdr image they might already are big-endian? image so we might not need to do conversation at all? correct me if im wrong 🙂 @anubhabMajumdar

rectified95 commented 3 months ago

@nddq Yes, those values are always big-endian and need to be converted to little-endian so they can be used by the hosts. I'm working on this now. The question is whether doing this simple operation in bpf is much faster than in Go, but it def wouldn't hurt.