-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added TCP Flags to Flow Record metrics #68
Conversation
@shach33 Thank you for your contribution! In order to facilitate the C code review, could you keep the original indent configuration? It's 4 spaces, no tab. |
@shach33 Thank you very much for this PR! :) IIUC accumulating all the flags of the flowlogs with bitwise OR won't allow me to find which endpoint started the connection. As known, in TCP the 3-step handshake goes like this:
I think that it is most likely that the packets of step 1 and 3 will end up in the same flowlog. In this case, that flowlog will have both SYN and ACK flags making it indistinguishable from the flowlog of step 2. |
Hi @ronensc , Thanks for highlighting that. Yes, you are right, we might need to capture only Syn/Fin/Rst in the MSB of the u16 flag, which might help in getting that info. For eg. The logic in the ebpf code could be :
Does that work? Additionally, We would like to know more such use-cases where aggregation of flags_t is not very useful., so that we can address them here. |
Thanks @praveingk We also talked about that in latency measurements, we need to spot both the SYN and SYN+ACK flowlogs. And in the suggested approach this is not possible. We suggest other alternatives:
|
@ronensc : Yes, I understand the point, and I agree. Can you confirm if the list of combinations that can be incorporated in the eBPF code are below:
|
I think this list is sufficient. |
This is precisely what we need !!! |
@ronensc with respect to:
I have concerns with respect to the implementation on that. AFAIK, eBPF doesn't allow dynamic memory to be created. That would mean that, for each record, you'd need to create a big-enough, fixed-size array to store the flag sequences. I'm afraid this could impact in the overall memory and performance. |
I am working on it. Will push the changes soon. |
Hi @mariomac , including the combinations, we have only 10 possibilities, which can be stored in u16 with a bit mask. That should be ok right? |
@praveingk yes, a 16-bit number it's fine. Maybe I misunderstood @ronensc message and I thought some kind of list/array would be implemented to store that information. |
@mariomac you got me right. I wan't aware of eBPF limitations... |
@shach33 Can you rebase this PR to main branch since there are merge conflicts. After the rebase, I would like to add flags to ipfix export as well. |
fixing indents Indent fixes for flows.c Indent fixes for flows.c-II Still fixing indents
Updated changes according to comments on PR
Rebase merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better, thanks! Few minor comments.
Codecov Report
@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 41.50% 41.60% +0.10%
==========================================
Files 29 29
Lines 1983 1978 -5
==========================================
Hits 823 823
+ Misses 1122 1116 -6
- Partials 38 39 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -97,7 +138,7 @@ static inline int fill_iphdr(struct iphdr *ip, void *data_end, flow_id *id) { | |||
} | |||
|
|||
// sets flow fields from IPv6 header information | |||
static inline int fill_ip6hdr(struct ipv6hdr *ip, void *data_end, flow_id *id) { | |||
static inline int fill_ip6hdr(struct ipv6hdr *ip, void *data_end, flow_id *id, u16 *flags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we call set_flags
here too, in case IPPROTO_TCP
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! We always forget to test IPv6 :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Merging! |
* Test commit * Adding TCP flags * Adding TCP flags to record metrics.TODO: Remove log stmts * Updated tcp flags u32-> u16 * Updated indentation in C file * Update flow.h * Indent fixed. Tab -> 4 space fixing indents Indent fixes for flows.c Indent fixes for flows.c-II Still fixing indents * Removed extra file * Added clang-format config to bpf/. To use clang-format -i <file.c> * Fixed message record member sequences * Added TCP Flags combinations Read from flw Updated changes according to comments on PR * Removed log statements. Rebase merges * Fixing merge conflits * Revert the renaming of grpc_test file * Simplify setting flags for v4,v6 * Add flags to ipfix exporter * Update protobuf * Remove type conversions due to conflicts * Remove .gitignore change * Fix indentation * Remove commented line * Fixed typecast errors since move to go 1.18 * Reverting commit d1e9c44. Added changes as separate PR * set flags for v6 Co-authored-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
* Test commit * Adding TCP flags * Adding TCP flags to record metrics.TODO: Remove log stmts * Updated tcp flags u32-> u16 * Updated indentation in C file * Update flow.h * Indent fixed. Tab -> 4 space fixing indents Indent fixes for flows.c Indent fixes for flows.c-II Still fixing indents * Removed extra file * Added clang-format config to bpf/. To use clang-format -i <file.c> * Fixed message record member sequences * Added TCP Flags combinations Read from flw Updated changes according to comments on PR * Removed log statements. Rebase merges * Fixing merge conflits * Revert the renaming of grpc_test file * Simplify setting flags for v4,v6 * Add flags to ipfix exporter * Update protobuf * Remove type conversions due to conflicts * Remove .gitignore change * Fix indentation * Remove commented line * Fixed typecast errors since move to go 1.18 * Reverting commit d1e9c44. Added changes as separate PR * set flags for v6 Co-authored-by: Pravein Govindan Kannan <pravein.govindan.kannan@ibm.com>
I added a 16 bit flag to every record. The TCP header is read and 8 flags (fin, syn, rst, psh, ack, urg, ece, cwr) are added to bits 0-7 in flags member of metrics struct.
During accumulation, the flags member is bitwise OR'ed to get all flags in bits 0-7.