[Design Discussion] Looking for feedback on ICMP API

robs-zeynet commented 2 years ago

Hello - from https://github.com/JulianSchmid/etherparse/issues/10#issuecomment-748440396, a little over a year ago, I volunteered to add support for ICMP parsing. So, a year is a long time - my bad - but I'm starting to dig into it now.

But as I dig into it, there are some messy API questions that I thought I would circulate before I did too much coding. The main issue is that may of the messages in Icmp4 and Icmp6 are similar in function but different in implementation. For example:

Icmp v4 (ipv4.proto = 1) and Icmp v6 (ipv6.next-header = 58 !?) actually use different protocol numbers
Icmp v4 echo request/reply use different icmp_types than Icmp v6 (8 vs. 128)
Most messages support similar functions (e.g., everything has a type/code/checksum) but some messages have fairly complex payloads that would be super useful to decode, e.g., unreachables with their encapsulated ip header from the unreachable packet
Some message types are v6 only and are also quite complicated, e.g., router advertisement

Long story short: there are a few ways to encode/abstract hide this info from the caller and I wanted to get folks opinion about their preferences before I did the work. Maybe there are even some better ways that folks could suggest - I'm still new to rust and learning the idiomatic magic. I'm trying to optimize on "what is most useful to the caller" (rather than what's easiest to implement) and even that's not particularly clear to me given that a good API. An apparent tension seems to be making something easier to parse (e.g., fewer nested match() statements) vs. trying to have the compiler prevent you from doing things you're not supposed to do (e.g., create a v6 router advertisement in a v4 packet).

Proposal 1 : "Fat struct"

The idea here is that there is just one struct for all ICMP messages (v4, v6, etc.) and all of the fields are Option<> and only populated if they make sense. This is great for parsing (only one match() statement) but horrible for preventing you from doing, e.g., you could add an unreachable IP header to an echo request packet.

pub struct Icmp {
     icmp_type: u8,
     icmp_code: u8,
    checksum: u8,
    // these are only valid for EchoRequest/Reply
    identifier: Option<u16>,
    sequence_number: Option<u16>,
    // these are only valid for unreachables 
     unreachable_ip: Option<IpHeader>,
   // ...
}

Proposal 2 : "Nested enums"


pub enum TransportHeader {
    Icmp(IcmpHeader),  // new!
    Udp(udp::UdpHeader),
    Tcp(tcp::TcpHeader)
}
enum IcmpHeader {  // nested once
      Icmp4(Icmp4Header),
      Icmp6(Icmp6Header),
}

enum Icmp4Header { // nested again
    EchoRequest4(EchoRequest4Msg),
    EchoReply4(EchoReply4Msg),
   //...
}

enum Icmp6Header {
    DestUnreachable6(DestUnreachable6Msg),
    EchoRequest6(EchoRequest6Msg),
    EchoReply6(EchoRequest6Msg),
    //...
}

// unique structs for each v4 and v6 msgs + some Traits for common operations

This is cleaner from an API standpoint, but is a PITA for parsing, e.g.,

match sliced_packet.ip.transport {
     Udp(_) => ...,
     Tcp(_) => ..., 
     Icmp(msg) => match msg {  // three levels of match just to get to the structs/data!
          Icmp4(icmp4) => match icmp4 { ...},
          Icmp6(icmp6) => match icmp6 {...},
     },
}

Proposal 3 : transport_v4 vs. transport_v6

We could split the crate::transport::TransportHeader into separate v4 and v6 instances:

pub enum TransportHeader_v4 {
    Icmp(icmp::Icmpv4Header),
    Udp(udp::UdpHeader),
    Tcp(tcp::TcpHeader),
}

pub enum TransportHeader_v6 {
    Icmp(icmp::Icmpv6Header),
    Udp(udp::UdpHeader),
    Tcp(tcp::TcpHeader),
}

but this also seems horrible and has the additional damage of breaking a lot of existing code.

Long story short I'm not happy with any of these options so I wanted to float this to the community to get feedback.



## References
* v4: https://en.wikipedia.org/wiki/Internet_Control_Message_Protocol
* v6 : https://en.wikipedia.org/wiki/Internet_Control_Message_Protocol_for_IPv6

robs-zeynet commented 2 years ago

I never got any feedback so I picked a hybrid between the fat structure and nested, and put up a partially working pull request: https://github.com/JulianSchmid/etherparse/pull/22 . Feedback welcome!

JulianSchmid commented 2 years ago

Hi @robs-zeynet ,

First thanks for the PR. I like the approach you took. In general ICMP is a bit of a tricky topic, as ICMP is such a wide ranging protocol (especially when it comes to ICMPv6). Getting that structured consistently is something I always struggled with. So I am also not 100% sure what the best approach is, but I think adding the two enum values to the transport enums is for sure the right one.

I will post a code review with some more detailed feedback in your PR (hopefully shortly).

Greets & Thanks again Julian

JulianSchmid / etherparse