Repository navigation
Conversation
|
To clarify, I mentioned some items here that AI found while I was working on this, but all the code and work was done by me. AI was only used for review and to help figure out how the different repos work together |
43a07c0 to
18a2b61
Compare
Adds new request_{arp,ndp} actions to control resolver, which are now
the default actions for the resolver_v{4,6} tables.
Runs the unicast reflection check before the resolver, so a packet
routed back out its ingress port is dropped rather than punted. Moves
the mac rewrite for packets from userspace into control ingress after
fwd_from_scrimlet(), and skips the mac rewrite when a resolver miss
punts the packet to the scrimlet. This brings sidecar-lite behavior into
alignment with sidecar.p4.
Adds unit tests that verify routed IPv4/IPv6 packets whose nexthops are
unresolved will trigger a sidecar packet to the scrimlet with the proper
sc_code, sc_egress, and sc_payload (nexthop ip) values, that reflected
packets are dropped on a resolver miss, and that a punted packet keeps
its source MAC until userspace sends it back.
Fixes: oxidecomputer#25
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
18a2b61 to
0bf3081
Compare
|
Latest version of the commit addresses some corner cases that AI code review found, namely around MAC rewrite ordering for punted packets and ensuring that reflected packets are dropped before the resolver code runs and changes the egress_port. It also adds a couple more test cases and refactors the test helpers to keep things more concise. |
`0bf3081` (parent) moved `mac.apply()` onto the from-scrimlet path, which would have given scrimlet-sent multicast the port MAC. We keep the userspace MACs for `ff02::/16` multicast traffic from the scrimlet, while dropping IPv4 multicast and other IPv6 multicast on this path.
[multicast] restrict scrimlet multicast to IPv6 link-local
|
I've merged in a change from zeeshan that inlines the multicast check into |
[test] update tests
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
8d070e9 to
a3bb85b
Compare
|
Manual testing of this PR alongside oxidecomputer/dendrite#414 is looking good! Logs from v4 routes: Logs from v6 routes: Logs from v4-via-v6 routes: |
Can you help me understand what I'm looking at here? For the first entry I see a log entry for resolution needed for the address |
Post Ry's review comments, e.g., oxidecomputer#181 (comment), we shouldn't have a rigid policy here. Nonetheless, we have to take care of the sidecar header, which sidecar.p4 currently leaves on these packets. Includes: - scrimlet packets w/ `ff02` (IPv6 link-local mcast) destinations go out `sc_egress` w/ the userspace MACs (just like mainline) - other scrimlet mcast destinations get their sidecar header stripped (`decap_sidecar`), and then the usual DMAC/TTL checks, NAT + replication - They are dropped on a DMAC mismatch, TTL <= 1, or no group - the scrimlet worked below the DMAC check (`ff02` gets it too then) - Note: unicast unchanged
| // Check if this is a packet coming from the scrimlet. If so just | ||
| // forward it out the source port indicated by the sidecar header. | ||
| // | ||
| if (hdr.sidecar.isValid()) { fwd_from_scrimlet(); return; } |
There was a problem hiding this comment.
It's still not clear to me why this logic should change. If a packet is coming from the switch zone, it should just be sent out the port the driver on the host identifies in the sidecar header. There is no resolution to do and any multicast should be handled within the host itself. e.g. we are acting more as a gigantic NIC in this case than a switch.
There was a problem hiding this comment.
After some discussion with @rcgoodfellow, including:
yeah i'm fine with rewrite being at the end of the pipeline and happening on every packet except for those with multcast macs - that makes sense. What I was taking issue with was the seemingly very contrived logic on mac rewrites at the head of the pipeline from userland.
I'm going with the giant-NIC model. The next push will return the switch-zone path back to main's fwd_from_scrimlet (i.e., no replication/TTL/DMAC handling for anything coming from the switch zone itself). Instead, we'll move the SMAC rewrite to the end of the ingress control for non-mcast packets that make it through to that point and aren't dropped or headed to the scrimlet, akin to DPD's current sidecar approach. tfportd's re-injected packets get their SMAC applied there too, which is how sidecar.p4 already handles them.
… ingress ~ match sidecar.p4
Per Ry's review (giant-NIC approach/model) + chat:
- I've placed the scrimlet path back to main's `fwd_from_scrimlet`
(i.e., no replication, NAT, TTL or DMAC handling for anything
coming from the switch zone), and `decap_sidecar` has
been folded back into `fwd_from_scrimlet` as such.
- The SMAC rewrite has been moved to the end of the ingress control
for non-mcast packets (by IP dst) that aren't tagged to drop and
aren't headed to the scrimlet (port 0).
- Unlike main, of course, this also covers switch-zone unicast/ARP/LLDP.
- This matches up w/ the [sidecar.p4's end-of-Ingress `mac_rewrite`][sidecar],
which is also what covers tfportd's re-injected packets
[sidecar]: https://github.com/oxiecomputer/dendrite/blob/f8d4418a9a08741a2965eb45e7bf33333f0387a2/dpd/p4/sidecar.p4#L2201-L2205
Note: the body diff under `if (hdr.sidecar.isValid()) { ... } else { ...
}` is mostly a reindent (use `-w` in jj for example).
22f2ba0 to
840ce97
Compare
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
This is much better. Thank you.
Last thing is my question on the debug output
These are tfportd logs showing the packets punted with The 3 sets of logs show that we are correctly triggering resolution for a v4->v4 flow, a v6->v6 flow, and a v4->v6 flow. |
Adds new request_{arp,ndp} actions to control resolver, which are now the default actions for the resolver_v{4,6} tables.
Adds unit tests that verify routed IPv4/IPv6 packets whose nexthops are unresolved will trigger a sidecar packet to the scrimlet with the proper sc_code, sc_egress, and sc_payload (nexthop ip) values.
Fixes: #25