Fix Vxlan code + small additions #509

Fredi-raspall · 2025-05-21T20:49:42Z

No description provided.

Fredi-raspall · 2025-05-21T20:55:40Z

Marking as dont-merge until I double check that these are the good fixes and @daniel-noland approves.

qmonnet

Changes look OK to me

daniel-noland · 2025-05-22T19:52:20Z

net/src/headers.rs

@@ -319,6 +319,11 @@ impl Headers {
        Headers::default()
    }

+    /// Add / Replace Ethernet header
+    pub fn set_eth(&mut self, eth: Eth) {


technically this method is unsafe in the sense that you can set an ethernet header with an ethertype which doesn't match the lower layers of the frame. Will think about a good way to fix this.

Having thought about it some more, I don't think we can fix this safety issue without requiring the user to go through a strongly typed builder API on every packet manipulation.

The actual fix here should be to make the API contract of the packet / headers types free form and then create a validated type wrapper which does not allow mutations without consuming the structure back into the mutable form.

That will prevent us from sending (or accepting) total nonsense while still giving us the latitude to actually manipulate packets.

That said, wherever possible, we should still forbid creating nonsense packets; moving errors closer to their actual cause point is just good practice anyway.

I would say it's the caller's responsibility to set the ether-type. I mean, IMO, the net crate should provide the machinery to build packets, but can't be foreseeing all possible combinations and, if it ought to, it should probably be after the fact maybe when deparsing(), or via some validation method to be called explicitly?

daniel-noland · 2025-05-22T19:53:37Z

net/src/ipv4/mod.rs

+    /// This method _will not_ update the checksum of the header.
+    /// # Errors
+    ///    This method returns [`Ipv4LengthError`] if the value is too big
+    pub fn set_payload_len(&mut self, payload_len: u16) -> Result<(), Ipv4LengthError> {


again, technically an unsafe method. Invariants pushed to the caller.

Yes, this is probably a distinct case. Here consistency matters a bit more. There's distinct ways we could enforce correctness. Either with validation or by "encapsulation"; i.e. somewhat emulate a stack by wrapping layers. But this requires a significant refactor and APIs? E.g. you could have something like packet.eth_encap( Params ), with packet being anything above L2 (ARP, IPv4, IPv6). This would get the packet, automatically determine ethertype and use params to set src/dst macs. Similarly you could have something like packet.ip_encap( Params ) (where packet would be assumed to contain L4 stuff maybe) and the params contain the fields that would make sense for the user to set. Protocol, header-length, packet length, and so on could be set automatically. So you could do things like:

udp_packet.ip_encap( l3-params ).eth_encap( l2-params )

daniel-noland · 2025-05-22T19:55:09Z

net/src/packet/mod.rs

@@ -74,6 +74,11 @@ impl<Buf: PacketBufferMut> Packet<Buf> {
        &self.payload
    }

+    /// Add / Replace Ethernet header
+    pub fn set_eth(&mut self, eth: Eth) {


technically unsafe

How do you want to proceed? You want me to annotate all those methods as unsafe?

daniel-noland · 2025-05-22T19:56:30Z

net/src/packet/mod.rs

@@ -133,10 +138,7 @@ impl<Buf: PacketBufferMut> Packet<Buf> {
    /// ```
    pub fn vxlan_decap(&mut self) -> Option<Result<Vxlan, ParseError<EthError>>> {
        match self.headers.try_vxlan() {
-            None => {


why are we removing the log here?

We call vxlan_decap for every packet destined to the gateway, but the gateway may receive packets that are not vxlan (control-plane, icmp, etc..). So the prior log would show for every packet sent to it.

net/src/packet/mod.rs

daniel-noland · 2025-05-22T19:59:56Z

net/src/packet/mod.rs

+
+    /// Wraps a packet in an `Option` depending on the metadata:
+    /// If [`Packet`] is to be dropped, returns `None`. Else, `Some`.
+    pub fn enforce(self) -> Option<Self> {


I'm not sure I follow the logic for the naming here. Why enforce?

When we iterate over a burst of packets, we pervasively use filter_map which requires returning Some(packet) or None to indicate if the packet should be returned or not. The routing stages (as currently defined) annotate in the packet metadata what to do next: 1) no annotation, 2) some drop condition (there are plenty) 3) whether the packet can be delivered etc. These annotations happen inside the pipeline functions. The enforce is just some sugar syntax to, based on those, determine if Some(packet) / None is to be returned. So, most stages end up calling packet.enforce(). This allows hiding all that logic to the stages and allowing to change it without needing to change them. That's the explanation. Given this purpose, do you have suggestions for the naming? You're the native speaker here!

daniel-noland

I think we need to consider some API issues here, but my only actual hard concern is the naming of the enforce method. Given that this is easily refactored later, I'm ok with approving this.

That said, we should talk through my thoughts on a validated packet type at some point

Fredi-raspall · 2025-05-22T20:48:35Z

I think we need to consider some API issues here, but my only actual hard concern is the naming of the enforce method. Given that this is easily refactored later, I'm ok with approving this.

That said, we should talk through my thoughts on a validated packet type at some point

I can easily change the naming for enforce if we find a better name. Not that I like it either....
How about:

conclude()
decide()
resolve()
determine()

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Either Daniel forgot these in the final PR, or we developed it on the phone 😂 Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

There are several changes in this function. The only one that is surely needed is the overwrite of headers with the cloned-then-modified headers at the end of the method. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Malformed => used when the packet does not seem to be right. MissingEtherType => used when we can't determine ethertype to use Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

The alternative is to log from the caller if needed. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Adds a method to compute a hash over invariant packet fields, including ethernet header fields and vlan plus the fields consi- dered already for ecmp hashing. A second method leverages the aforementioned to build an entropy source UDP port suitable for VxLAN encapsulations. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

To encapsulate a packet in VxLAN, a UDP header needs to be built. In the current API, the caller is requested to build and pass the UDP information. All of the information in the UDP header, however, can be automatically be set by the API. This includes: * dst port, which should almost always be the well-known value. * source port, which can be computed from the payload itself. * length * checksums (optional for ipv4) This patch changes the API to not require UDP header and build it automatically, so that callers need not have to set those. This may be useful when needing to encapsulate traffic e.g. for L2 EVIs. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Fredi-raspall requested a review from daniel-noland May 21, 2025 20:49

Fredi-raspall requested a review from a team as a code owner May 21, 2025 20:49

Fredi-raspall added the dont-merge Do not merge this Pull Request label May 21, 2025

Fredi-raspall force-pushed the pr/fredi/recovery_vxlan branch from 2ee184c to c3a73b2 Compare May 21, 2025 20:59

qmonnet approved these changes May 22, 2025

View reviewed changes

Fredi-raspall force-pushed the pr/fredi/recovery_vxlan branch 3 times, most recently from 6686ec8 to 659328e Compare May 22, 2025 18:32

daniel-noland reviewed May 22, 2025

View reviewed changes

net/src/packet/mod.rs Outdated Show resolved Hide resolved

daniel-noland reviewed May 22, 2025

View reviewed changes

daniel-noland approved these changes May 22, 2025

View reviewed changes

Fredi-raspall force-pushed the pr/fredi/recovery_vxlan branch from 659328e to 43fd427 Compare May 22, 2025 20:53

This was referenced May 23, 2025

Small VxLAN API change #511

Merged

Update IP checksum when decrementing TTL #513

Merged

Fredi-raspall added 9 commits May 23, 2025 13:46

feat(net): make dscp constructor public

4408829

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add missing enforce() packet method

13f11a5

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add ipv4 length and checksum methods

3d432ee

Either Daniel forgot these in the final PR, or we developed it on the phone 😂 Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): fix Packet::vxlan_encap()

577ff7b

There are several changes in this function. The only one that is surely needed is the overwrite of headers with the cloned-then-modified headers at the end of the method. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add 2 values for DoneReason

0b015af

Malformed => used when the packet does not seem to be right. MissingEtherType => used when we can't determine ethertype to use Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add methods to add an Eth header

ad3ab16

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): impl Display for packet UdpEncap

dca01fd

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): remove log that is misleading

37f6836

The alternative is to log from the caller if needed. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add utils to set src/dst eth macs

caaf7bf

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

Fredi-raspall force-pushed the pr/fredi/recovery_vxlan branch from 43fd427 to caaf7bf Compare May 23, 2025 11:46

mvachhar removed the dont-merge Do not merge this Pull Request label May 23, 2025

mvachhar added this pull request to the merge queue May 23, 2025

mvachhar removed this pull request from the merge queue due to a manual request May 23, 2025

Fredi-raspall added 4 commits May 23, 2025 10:11

feat(net): update checksum when decrementing TTL

4fdcb6b

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

feat(net): add sanity for vxlan encap

99674c5

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>

mvachhar enabled auto-merge May 23, 2025 16:18

mvachhar added this pull request to the merge queue May 23, 2025

Merged via the queue into main with commit a0f844e May 23, 2025
14 checks passed

mvachhar deleted the pr/fredi/recovery_vxlan branch May 23, 2025 16:28

Fix Vxlan code + small additions #509

Fix Vxlan code + small additions #509

Uh oh!

Conversation

Fredi-raspall commented May 21, 2025

Uh oh!

Fredi-raspall commented May 21, 2025

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-noland left a comment

Choose a reason for hiding this comment

Uh oh!

Fredi-raspall commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fredi-raspall commented May 22, 2025 •

edited

Loading