fix(renderer): add more common protocols to protoname renderer #341
Conversation
…nassigned, experimental and reserved to protoname renderer
Co-authored-by: marioschaefer <[email protected]>
Co-authored-by: marioschaefer <[email protected]>
|
@lspgn what do you think? |
| 40: "IL", | ||
| 41: "IPv6", | ||
| 42: "SDRP", | ||
| 43: "IPv6-Route", |
There was a problem hiding this comment.
IPv6-Route is the official IANA name, same as with ICMPv6 vs. IPv6-ICMP - we can deviate from the standard, as you like.
But in my opinion, we should stick to the standard.
--- update ---
commit was not planned, i thought i deleted my change request... will change again, according to your decision.
There was a problem hiding this comment.
+1 to sticking to official IANA protocol names
|
Looks good to me. Just a small note: |
| 55: "Min-IPv4", | ||
| 56: "TLSP", | ||
| 57: "SKIP", | ||
| 58: "IPv6-ICMP", |
There was a problem hiding this comment.
Would rather keep ICMPv6 since it's already being used
There was a problem hiding this comment.
While I agree it's IANA, I would prefer not breaking existing implementations
There was a problem hiding this comment.
my 2 cents:
- I dont think we can mitigate the absence of information without some kind of breakage of existing implementations. As the Protocol Name is the only hint to the protocol field of the actually sampled packet (when utilizing JSON output) I'd prefer IANA names.
- imho a viable alternative would be to add a field to the json output (proto_num?) with the int32 protonum - which would allow a clear mapping to official protocol names (for example in an enricher) and wont break existing implementations relying on the strings
I'd keep all other protocols in protoName with their official names - with "ICMPv6" as only deviation.
@lspgn @marioschaefer wdyt
There was a problem hiding this comment.
@DerInti
point 1:
i'm on the complete same page
point 2:
I also thought about that and its probably the cleanest solution, bcs you have the number and the name.
But this will also break the existing implementation, since we will stick to the IANA-Names and the existing implementation doesn't use them.
on the other hand: how much longer do we want to carry the wrong name around with us. sooner or later it will be changed. the longer you wait, the more it hurts. that's why i'm a big fan of "sooner rather than later".
| 55: "Min-IPv4", | ||
| 56: "TLSP", | ||
| 57: "SKIP", | ||
| 58: "IPv6-ICMP", |
There was a problem hiding this comment.
| 58: "IPv6-ICMP", | |
| 58: "ICMPv6", |
|
@lspgn whats the status on this PR? |
|
@marioschaefer I'll do this shortly, was focused on #342 which has also some protocol naming I could adapt (6bfc106). |
* fix(renderer): add more common protocols to protoname renderer, add unassigned, experimental and reserved to protoname renderer --------- Co-authored-by: marioschaefer <[email protected]>
As it yields an empty string in JSON output for "proto" (to which we rely heavily on):
I refrained to add the whole IANA Protocol Numbers list due to reasons of usability and clarity.