ip by daniel-noland · Pull Request #1758 · githedgehog/dataplane

daniel-noland · 2026-08-26T02:59:39Z

No description provided.

coderabbitai · 2026-08-26T02:59:46Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-08-26T03:14:31Z

Codecov Report

❌ Patch coverage is 89.41736% with 89 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
hardware/src/scan.rs	0.00%	15 Missing ⚠️
net/src/ip/address.rs	88.46%	15 Missing ⚠️
config/src/external/overlay/vpcpeering.rs	75.60%	7 Missing and 3 partials ⚠️
dataplane/src/packet_processor/ipforward.rs	0.00%	7 Missing ⚠️
cli/bin/main.rs	0.00%	5 Missing ⚠️
nat/src/masquerade/flows.rs	80.00%	5 Missing ⚠️
nat/src/portfw/nf.rs	42.85%	4 Missing ⚠️
nat/src/portfw/portfwtable/setup.rs	73.33%	4 Missing ⚠️
nat/src/masquerade/apalloc/port_alloc.rs	80.00%	3 Missing ⚠️
nat/src/masquerade/packet.rs	40.00%	1 Missing and 2 partials ⚠️
... and 10 more

📢 Thoughts on this report? Let us know!

Four near-identical sealed traits had grown up in the tree: `NatIp`, acl-filter's `IpVersion`, and two copies in acl's tests and benches. This is meant to absorb them, and the folds follow. The `IpAddress<Unicast = Self::Unicast>` bound on the associated type is load-bearing rather than decorative: without it generic code cannot name `Unicast<Ip>` as an address type, so the two constraint strengths exist only at concrete instantiations. The module docs carry the argument. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`max_range` picks up a `NatIpWithBitmap` bound on the way: it builds the largest address, which the unicast forms reject -- 255.255.255.255 is broadcast -- so its `unreachable!` held only because nothing had instantiated it at one. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Only the prefix half stays local: it needs `lpm::Prefix`, and `net` does not depend on `lpm`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Each keeps only the bound that is genuinely local -- bolero's `TypeGenerator` in the test, a `const UNSPECIFIED` in the bench -- and inherits the rest. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`reserve_port` had an arm for a v4 source with a v6 allocation, reachable only through a bug it had no way to describe. `AnyReservation` is now the only way to build the pair, so that arm has no inhabitant and the check happens once, at a caller that can still name the flow. The private address picks up the unicast constraint on the way: `Ipv4::source` already proves it and `FlowKey` drops it again. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`NatTranslationData` was four independent options, so it could say "rewrite the port but not the address". That is a state `nat_translate_icmp_inner` accepts and then discards, because it branches on the address -- a producer setting only a port got silence. Pairing the two makes it unsayable. A `None` port survives the change: it is a real instruction, meaning "translate the address, leave the transport header alone". Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Deriving `BITS` from `FixedSize::SIZE` landed in the previous commit without the `allow` that `clippy::pedantic` wants for the cast. The seal is the argument for it: `SIZE` is 4 or 16 across every implementor. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Validation proves a port-forwarding expose has one prefix and one port range per side, and then re-derives it: four `unreachable!` in the same function that had just checked it, and five more plus three `debug_assert!` in the consumer. `PortForwardExpose` is that narrowing, kept instead of repeated. The one remaining check is in `nat`, where reaching it means validation has a hole -- so it is an error naming the expose rather than a panic raised inside `config`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…atch keys The key port field was a `u16` in which zero meant "this packet has no transport ports". That is sound, but for two reasons neither of which was anywhere near the field: no packet can present zero because `TcpPort` and `UdpPort` are `NonZero`, and no configured range can start at zero because `VpcExpose::validate` rejects it. `KeyPort` is where both now live. The first half was being actively thrown away: both filters read ports through `NonZero::get` on the way in, so the code discarded the proof and then relied on it. Carrying `NonZero` to the key instead deleted the hand-clamps that had grown around the gap -- `sport.max(1)` in two fuzz builders, and the `expected_summary` that existed only to undo one -- and the probe generators no longer spend draws on a port no parser emits. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`NhopKey.ifname` was documented "for diagnostics only" and then included in the derived `Hash`/`Eq`/`Ord`, so the doc comment above it had to warn that populating it would split one next-hop into two. Nothing ever did: every production construction passed `None`, and only test fixtures set it. So the CLI's " interface {ifname}" branch could not fire, the ifname arm of `EgressObject::merge` could not run, and the field cost a `String` comparison per key lookup to carry nothing. Removing it retires the hazard rather than documenting it; showing an interface name is now a question of looking one up from `ifindex` at display time, which needs an interface table the `Display` impls do not currently take. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`VxlanEncapsulation.dmac` was a resolution product living inside `NhopKey`'s derived `Hash`/`Eq`/`Ord`, and `resolve` filled it in place. The stored key survived only because `Encapsulation` is `Copy` and the caller happened to bind `let mut encap_instr = encap`, taking a copy. Nothing said so, and `encapsulation.rs` carries a TODO about giving MPLS labels a real type -- which would make `Encapsulation` non-`Copy` and turn that line into a resolver mutating a key that is live in a map. `ResolvedVxlan` is now what resolution returns rather than what it mutates, and its mac is not optional: resolution failing turns the next-hop into a drop, so an encap instruction with no mac was never reachable. That deletes the re-check the forwarding path was doing per packet in `vxlan_encap`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`FibKey` is what every fib lookup matches on, and it carried a variant no lookup can produce: `Unset` existed because `left_right::new` builds both copies from `Fib::default`, which has no id to give them. A fib's own id is narrower than the key that reaches it -- fibs are created per vrf, and a `FibKey::Vni` is an alias the *table* holds pointing at one. Saying that (`Option<VrfId>`) confines the unset window to the one private field that has it, and takes with it the `unreachable!` in `as_u32`, the "Unset!" arm in the CLI, and `set_id`'s assertion that its argument was the right variant. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`StaticRoute::new` returned a route holding `StaticRouteNhop::Unset` and relied on the caller chaining a `nhop_*` setter; forgetting one produced a route that panicked in the FRR renderer rather than one that failed to build. Not reachable today -- static routes are plumbed as far as `VrfConfig::add_static_routes` but nothing ever populates them, so `StaticRoute` has no production caller at all. Worth closing before it does. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`FlowKey` held `src_ip` and `dst_ip` as `IpAddr`, which let a key pair a v4 source with a v6 destination -- `proto()` then chose between ICMP and ICMPv6 by asking the source alone -- and dropped the unicast guarantee `Ipv4::source` had already established, which `flow_key.rs` then rebuilt with a `try_from(..).unwrap()` a few hundred lines further down. Both ends of `FlowAddrs` are unicast because a flow is bidirectional: the table exists to match return traffic, and `reverse` makes the destination into a source. Building the pair fallibly and reversing it infallibly is the shape that falls out; the first attempt had that backwards and the `Option`s it produced spread through five files before saying so. A packet whose destination is not unicast now has no flow key rather than a key whose reverse can never match. The lookup paths already treat a missing key as "not flow-tracked" and pass the packet on. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`snat` re-derived `UnicastIpAddr` from the translation on every packet of every masqueraded flow, and mapped failure to `UnusableAddress`. Neither source of that address can fail the check: the reverse mapping's comes from a flow key, whose source is unicast by construction, and the forward one comes from a pool built out of expose prefixes, which `reject_special_use` rejects if they overlap multicast or limited broadcast -- exactly what `UnicastIpAddr` excludes. The check now happens once, where the session is created, and says which address failed. `UnusableAddress` had no way left to be constructed and is gone. Port forwarding already held its address this way (`PortFwState::use_ip`); masquerade was the one still carrying an `IpAddr` and paying for it per packet. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`Result<T, ()>` says an operation can fail and declines to say how. Each of the ten was one of three things: - an `Option` in disguise, confirmed by its only caller (`.ok()` in hardware, `if let Ok` in the cli, and a sibling `Observe` impl that already used `Option`); - a reason the callee knew and dropped after logging, which is the log becoming the error channel: a caller cannot branch on it, and the operator gets a message with no idea what was done about it; - one that could not fail at all -- `Manager<Action>::observe` always ends `Ok`, so its observation is now an `ActionBase`. Two failure paths in `re_reserve_ip_and_port` turned out to be silent rather than merely uninformative: a flow was invalidated during a config change and nothing said why, because `ok_or(())` had nothing to carry and no log beside it. Those now have names, and the caller reports the reason once at the point it decides to drop the flow. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 762b44a to 8e60371 Compare August 26, 2026 17:30

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 3c941d2 to 6a95b47 Compare August 26, 2026 17:30

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 8e60371 to e7929b8 Compare August 26, 2026 19:36

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 6a95b47 to 1905efe Compare August 26, 2026 19:36

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from e7929b8 to ff2d1fb Compare August 26, 2026 20:41

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch 2 times, most recently from 68c58e4 to 564a534 Compare August 26, 2026 21:02

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch 2 times, most recently from 55a6199 to c4f741f Compare August 26, 2026 21:13

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 564a534 to 1052b4c Compare August 26, 2026 21:13

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from c4f741f to f560007 Compare August 26, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 1052b4c to 5a8079e Compare August 26, 2026 21:25

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from f560007 to fdcb5f6 Compare August 27, 2026 01:29

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 5a8079e to b08c42c Compare August 27, 2026 01:29

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from fdcb5f6 to bfd0877 Compare August 27, 2026 01:41

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from b08c42c to a774e67 Compare August 27, 2026 01:41

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from bfd0877 to 2e7cfee Compare August 27, 2026 04:34

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from a774e67 to 8fcbc10 Compare August 27, 2026 04:34

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 2e7cfee to ba806ce Compare August 27, 2026 05:10

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 8fcbc10 to 0207c9d Compare August 27, 2026 05:12

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from ba806ce to 4775b0c Compare August 27, 2026 17:59

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 0207c9d to 9b444b3 Compare August 27, 2026 17:59

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 4775b0c to 21e468c Compare August 27, 2026 18:29

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 9b444b3 to 7b60ba9 Compare August 27, 2026 18:29

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch 4 times, most recently from b9defbf to acacc4b Compare August 27, 2026 21:28

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 4db3b53 to bb73757 Compare August 28, 2026 06:02

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from d522540 to 711442d Compare August 28, 2026 06:05

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from bb73757 to c6df3a7 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 711442d to 15ce0ad Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from c6df3a7 to d7a8fed Compare August 28, 2026 07:07

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 15ce0ad to 10bbc35 Compare August 28, 2026 07:07

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from d7a8fed to 68d5d8f Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 10bbc35 to 7300644 Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 68d5d8f to 43af0f0 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 7300644 to 508e5f4 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 43af0f0 to 0ada8c3 Compare August 28, 2026 09:14

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 508e5f4 to 37b1a91 Compare August 28, 2026 09:14

daniel-noland and others added 16 commits August 28, 2026 11:14

refactor(acl-filter): Rebase IpVersion on net::ip::IpAddress

5aed3c5

Only the prefix half stays local: it needs `lpm::Prefix`, and `net` does not depend on `lpm`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

daniel-noland force-pushed the pr/daniel-noland/spec-compliance branch from 0ada8c3 to 2072d87 Compare August 28, 2026 17:15

daniel-noland force-pushed the pr/daniel-noland/ip-address-refactors branch from 37b1a91 to e1937b5 Compare August 28, 2026 17:15

Navigation Menu

Uh oh!

ip - #1758

ip#1758
daniel-noland wants to merge 16 commits into
pr/daniel-noland/spec-compliancefrom
pr/daniel-noland/ip-address-refactors

daniel-noland commented Aug 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 26, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 26, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

daniel-noland commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 26, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

daniel-noland commented Aug 26, 2026 •

edited

Loading

codecov Bot commented Aug 26, 2026 •

edited

Loading