quick fix: flow filter printer - #1679
Draft
daniel-noland wants to merge 18 commits into
Draft
Conversation
The stage-1/stage-2 ACL keys carried their VPC and protocol fields as bare `u32` and `u8`. Type them as `Vni` and `NextHeader` instead, per the development guide's "types as units" (development/code/avoid-global- reasoning.md): a key field should say what it holds, not just how wide it is. The two encodings are byte-identical to what they replace, so the lowered rules, the rte_acl layout and the rule priorities are unchanged -- the reference/DPDK differential and config-oracle property tests pass untouched. Three things fall out of it: 1. `proto_byte` and the `PROTO_OTHER = 0` sentinel are gone. They existed to collapse every non-TCP/UDP protocol onto a byte no rule names, but the collapse was never necessary: rules are only ever (TCP, 0xff), (UDP, 0xff) or (_, 0x00), so a real next-header byte matches the "any" rules and nothing else, purely by their zero mask. The key now carries the packet's actual protocol. The sentinel was safe only for as long as config cannot express a non-TCP/UDP protocol constraint; protocol 0 is IPv6 hop-by-hop, and the day that changes the collapse would have silently misrouted it. 2. `vpcd_u32` becomes `key_vni`, returning a `Vni` rather than a width. It stays a total match, so a second `VpcDiscriminant` variant makes it a compile error -- which is where the table redesign should start. `VpcDiscriminant` remains the type on the interface (`LookupInput`, `Verdict`, `LookupResult`); the VNI is the key encoding, and the two are no longer conflated. 3. `RuleSet::from_overlay` already had a `Vni` in hand from `vpc.vni()` and `get_remote_vni()`, and was wrapping it into a `VpcDiscriminant` only to strip it back to a `u32`. That round trip is deleted. `FixedSize for NextHeader` joins the other impls in net::fixed_size. The `Vni` impl there gains a note on why its key encoding is 4 bytes and not the 24-bit VXLAN wire form: classifier fields must be 1, 2 or 4 bytes wide (acl::dpdk::layout rejects anything else at build time), so there is no 3-byte option to pick, and `write_be` must not be mistaken for a VXLAN header serializer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A rule is lowered to erased `FieldPredicate` bytes before any backend sees it, which leaves anything that wants to *show* a rule guessing what each field means. The two consumers in tree both guess: flow-filter's renderer infers a field's type from its byte width, and acl-filter's decodes by field index against a comment naming the key's field order. Reorder that key and the CLI silently prints the destination VNI as the source VNI. Give the specs a `Display` instead, so a rule can be rendered from the typed form it had before erasure: - `ExactSpec` -> the value - `PrefixSpec` -> `value/len` - `RangeSpec` -> `*` when universal, the port when degenerate, else `a..=b` - `MaskSpec` -> `*` for a zero mask, the bare value for an all-ones mask, and `value/0x<mask>` for a partial one (where neither operand is a meaningful value of `T`, so the mask is shown as the bit pattern it is) The derive gains a matching `Display` for the generated `<Name>Rule`, labelling each field with the struct field's own name, and a `MatchKey::Rule` associated type so a generic table can name the rule form of its key and retain it. `Display` on field types is required rather than opt-in. A classifier an operator cannot inspect is one they cannot debug, and making the dump opt-in would leave that as the default. `TcpPort` and `UdpPort` gain the `Display` they should always have had; the `IpProto` test newtypes gain one each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
`ShowFlowFilter` and the ACL filter's CLI source have been wired up all along, but in production they could only report a rule count per table: `install_table` consumes the rules and `DpdkAclLookup` keeps only the classifier, the actions and the layout. An rte_acl context cannot be asked what is in it, so the rules were simply gone. The full per-rule dump existed only under `cfg(test)`, where the reference backend happens to retain them -- that is, precisely not in the builds where an operator needs it. Retain the rules. Each crate's table becomes a classifier plus the rules it was built from, in match order, so the dump no longer depends on which backend is installed. The rules kept are the *typed* ones, which is both cheaper than keeping the erased predicates (a flat ~16-byte struct against a heap `Vec` of 16-byte `ArrayVec`s) and what lets each field render itself. Retention is unconditional; at 10k rules per table it costs a few MB, doubled by the left-right handoff, on a structure rebuilt only when configuration is applied and never touched on the data path. Rendering then collapses: flow-filter's width-guessing `fmt_predicate` and acl-filter's four positional `decode_*` helpers both go away, along with the `cfg(test)` / `cfg(not(test))` split in each. acl-filter's display drops from 215 lines to 98. While here, `AclKey` gets the same treatment `RemoteKey`/`LocalKey` got: `proto: NextHeader` and `src_vni`/`dst_vni: Vni` instead of `u8`/`u32`. `AclKey::new` already took `Vni` and immediately stripped it, and the key encoding is byte-identical -- but a protocol now renders as `TCP` rather than `6`. `Wildcardable` becomes `IpVersion` and grows the `Prefix -> PrefixSpec<Self>` narrowing that keeps `rule_predicates` monomorphic per IP version. Both crates gain a test asserting the reference and rte_acl backends render identically, which is the property that decayed into "production shows a count" in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the MatchKey ecosystem to retain and render typed rules (rather than decoding erased predicate bytes), enabling consistent human-readable CLI output across both the reference and DPDK (opaque rte_acl) backends.
Changes:
- Add
MatchKey::Ruleand enhance the derive macro to generate a companion*Ruletype that implementsDisplay, enabling typed rule retention and rendering. - Update flow-filter and acl-filter table builders to retain typed rules and render them consistently across backends; add cross-backend display equivalence tests.
- Add/extend
DisplayandFixedSizeimpls for protocol/port types used in match keys.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| net/src/udp/port.rs | Implement Display for UdpPort so it can participate in typed rule rendering. |
| net/src/tcp/port.rs | Implement Display for TcpPort so it can participate in typed rule rendering. |
| net/src/fixed_size.rs | Implement FixedSize for NextHeader and add tests for 1-byte encoding. |
| match-action/tests/derive_roundtrip.rs | Add Display for test key field types to satisfy new derive bounds. |
| match-action/src/predicate.rs | Expose be_bytes to the crate for typed-spec formatting helpers. |
| match-action/src/lib.rs | Add MatchKey::Rule associated type and export the new display module. |
| match-action/src/display.rs | New: Display impls for ExactSpec/PrefixSpec/RangeSpec/MaskSpec to format typed rules. |
| match-action-derive/src/lib.rs | Emit a companion *Rule struct and Display impl; wire it into MatchKey::Rule. |
| flow-filter/src/context/tests.rs | Add test asserting identical display across reference and DPDK backends. |
| flow-filter/src/context/tables.rs | Switch key fields to typed NextHeader/Vni; retain typed rules alongside classifiers. |
| flow-filter/src/context/display.rs | Render full rule dumps from retained typed rules in all builds/backends. |
| config/src/external/overlay/acl.rs | Lower ACL proto matches into MaskSpec<NextHeader> (typed protocol field). |
| acl/tests/eal_install_classify.rs | Add Display for test protocol type to satisfy new derive/display requirements. |
| acl/tests/eal_classify_via_projection.rs | Add Display for test protocol type to satisfy new derive/display requirements. |
| acl/src/dpdk/rule.rs | Update manual MatchKey impl in test to include type Rule. |
| acl-filter/src/tests.rs | Add test asserting identical display across reference and DPDK backends. |
| acl-filter/src/display.rs | Render full rule dumps from retained typed rules (no positional decoding). |
| acl-filter/src/context.rs | Switch key fields to typed NextHeader/Vni; retain typed rules alongside classifiers. |
| let kind = match self { | ||
| AnyTable::Empty => "empty", | ||
| AnyTable::Dpdk(_) => "dpdk", | ||
| let kind = match self.classifier { |
| let kind = match self { | ||
| AnyTable::Empty => "empty", | ||
| AnyTable::Dpdk(_) => "dpdk", | ||
| let kind = match self.classifier { |
daniel-noland
changed the title
mvachhar
requested changes
Jul 31, 2026
mvachhar
left a comment
mvachhar
left a comment
Contributor
There was a problem hiding this comment.
Please fix up some of these comments, plus could you post in the comments some sample output from the Display formatter so we can see what these look like. Also printing in priority order would be very helpful.
The CLI dump led each rule with its priority value. That number is an encoding of (prefix length, port-forwarding bit) produced by `rule_priority` -- `((len + 1) << 1) | port_forwarding`. It means nothing outside the table builder, it is not a stable or externally meaningful construct, and printing it invites an operator to read precedence out of an opaque number, or to compare two of them across releases whose scale has shifted underneath. Print the rank instead. It answers the question the number was standing in for -- which rule wins -- and acl-filter's dump already numbered its rules this way, so the two filters no longer disagree on what the leading bracket means. The rank is a sound precedence claim, not merely a line number: `rule_priority` documents that config validation keeps rules with intersecting match sets from sharing a prefix length, the one exception being a port-forwarding public range overlapping an equal-length masquerade range, which the priority's low tie-break bit resolves. So any two rules a single packet can match have distinct priorities, and the rendered order is the order they are consulted in. Ties are rules that partition the key space, where the display order is arbitrary but unobservable; the sort is stable, so they render in overlay-walk order. The one known equal-priority overlap with differing verdicts -- cross-peering masquerade/masquerade destinations -- was already documented as unspecified and benign in `fuzz_gen`, and is unaffected. `RuleRow` drops the field rather than the format string dropping the value, which keeps the priority contained in `build_table`, where it is actually used (the sort key, and rte_acl's explicit per-rule priority). `display_is_identical_across_backends` checked field rendering but asserted nothing about order, leaving the claim the index makes unpinned; it now requires the port-forwarding /32 to render ahead of the /24s it is nested among. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`MaskSpec`'s `Display` documented that for a partial mask "neither operand is a meaningful value of `T`", and rendered the mask as raw hex on that basis -- then passed the value through `T`'s `Display` anyway. Two things were wrong with the result. The keyword lies. `MaskSpec::new(NextHeader::TCP, 0xf0)` rendered `TCP/0xf0`, but that rule matches every protocol `0x00..=0x0f` -- HOPOPT, ICMP, IGMP, TCP and twelve others. Naming one arbitrary member of the matched set as though it were the whole set is the exact failure mode this crate's typed rendering was introduced to remove; it had simply moved from the erased byte decoder into the typed formatter. Don't-care bits were shown as significant. `MaskSpec::new(0xab, 0xf0)` rendered `171/0xf0` while both backends match on `0xa0/0xf0`. This was display-only, not a matching bug: rte_acl ANDs the value with the mask when it builds the trie (`acl_gen_mask_trie`, acl_bld.c), and the reference backend's `mask_matches` compares `(f & m)` against `(v & m)`. The two agree, so no rule has ever matched wrong here -- and that agreement is what makes rendering `value & mask` right rather than merely tidier: it is precisely what the classifier matches on. A partial mask now renders `0x<value & mask>/0x<mask>`. The full-mask and wildcard branches are untouched, so `proto=TCP` and `proto=*` are unchanged, and since no in-tree rule constructs a partial mask (both `proto_mask` and `AclProtoMatch` emit only 0x00 or 0xff), no existing CLI output moves. Normalizing in `MaskSpec::new` instead would make the invariant structural, but `FixedSize` is write-only and a read-back direction is fallible for exactly the types that matter -- `TcpPort`/`UdpPort` are `NonZero`, `Vni` is range-checked, `UnicastIpv4Addr` rejects multicast. That is a change worth making deliberately, not on the way past. The property test asserts the premise rather than assuming it: for any (value, mask), the raw and masked specs accept all 256 probes identically, and must therefore render identically. It was checked against the previous implementation and fails there, shrinking to (value=1, mask=2) -- `1/0x02` against `0/0x02`. `bolero` becomes a dev-dependency so that test runs under a plain `cargo nextest run`. It was only an optional dependency gating the public `generator` module, so property tests were not otherwise available to this crate's default test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Building an exact-match bitmask required handing `MaskSpec::new` an
all-ones value of the field's own type, so callers had to mint a `T`
that means nothing:
L4Protocol::Tcp => (NextHeader::TCP, NextHeader::new(0xff))
`NextHeader::new(0xff)` is not a protocol; 255 is Reserved. The mask
operand is a bit pattern that happens to be typed like the value it
constrains, and both flow-filter and config carried the same apologetic
comment explaining that to the reader -- a comment being load-bearing
where a constructor should have been.
Add `MaskSpec::exact(value)` and `MaskSpec::wildcard()`, backed by a
`MaskBits` trait supplying `ALL_BITS` / `NO_BITS`. Both duplicated
comments go away, and `proto_mask` now returns the `MaskSpec` itself
rather than a `(value, mask)` tuple that both emitters had to
reassemble.
`MaskBits` lives in `fixed-size` beside `FixedSize`, because `net` needs
to implement it for `NextHeader` and does not depend on `match-action`.
The trait is only implementable by types where every bit pattern of the
type's width is a valid inhabitant -- which is exactly the condition
that makes a type sound to use as a bitmask field at all. `NextHeader`
qualifies (every u8 is some protocol number). `TcpPort` is `NonZero` so
it has no `NO_BITS`, and `Vni` is both non-zero and capped at 24 bits so
it has neither constant; neither can now become a masked field by
accident. The invariant that was prose is now a bound.
`NextHeader::new` becomes `const` so the constants can be built from it.
`IpNumber`'s `From<u8>` is the identity wrapper, so constructing it
directly is the same value by another spelling.
Tests assert behaviour rather than restating the definitions: `exact`
accepts its value and nothing else and `wildcard` accepts everything,
checked exhaustively across all 256 `u8` inputs. `proto_mask`'s unit
test keeps asserting at the byte level, since "every bit"/"no bit" is
what rte_acl actually sees, and the protocol fuzz test now asks the spec
through the same `Accepts` impl the reference backend matches through,
instead of re-deriving the mask comparison alongside it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
match-action/src/lib.rs:59
confidence: 8
tags: [other]
`MatchKey` is a public trait in a published crate (`publish.workspace = true`), and adding the required associated type `Rule` is a semver-breaking change for any downstream manual `impl MatchKey`. If this crate is consumed outside the workspace, this should be paired with an appropriate major version bump / release note (or an explicit statement that the API is not yet stable).
pub trait MatchKey: Sized {
/// The rule (predicate) form of this key: one spec per match field, still carrying each
/// field's type. A table can therefore retain the rules it was built from in a form that
/// renders itself, without having to decode erased bytes back into domain types.
type Rule;
const N: usize;
const KEY_SIZE: usize;
fn field_specs() -> &'static [FieldSpec];
fn as_key_into(&self, out: &mut [u8]);
}
**flow-filter/src/context/display.rs:32**
* ```yaml
confidence: 7
tags: [style]
This private render module isn’t referenced anywhere, and because it only contains impls (no referenced items), it’s likely to trip the dead_code/“module is never used” lint in warning-as-error builds. Consider inlining the contents at the file scope, or explicitly allowing the lint on the module.
mod render {
flow-filter/src/context/display.rs:39
confidence: 9
tags: [style]
`Write` is imported here but never used (the `write!`/`writeln!` macros don’t require the trait in scope), which will trigger an `unused_imports` warning.
use std::fmt::{self, Display, Formatter, Write};
</details>
Not intended to land. Two `printer_sample` functions that assert nothing
and exist only to render a deliberately rich context, so the CLI dump
can be read and argued about without hand-building a config. Drop this
commit before merge.
cargo nextest run -p dataplane-flow-filter --no-capture -E 'test(printer_sample)'
cargo nextest run -p dataplane-acl-filter --no-capture -E 'test(printer_sample)'
They are inert without `--no-capture`: no assertions, so they cannot
fail on an output change, and nextest swallows the stderr. Kept as one
commit, and deliberately not split per crate, so that dropping them is
a single operation.
The fixtures are shaped around what config validation actually permits,
which took a couple of tries and is the main reason these are worth
keeping around rather than rewriting from scratch each time:
- A peering side carrying masquerade or port-forwarding requires the
other side to carry neither (`IncompatibleNatModes`), so the flavors
are spread across three peerings rather than piled into one.
- `AclScope::Flow` is rejected unless the peering uses masquerade or
port-forwarding, so the plain-v4 peering carries packet-scoped rules
and the masqueraded-v6 peering carries the flow-scoped one.
- An ACL rule's src/dst must intersect the manifest of the direction
it is declared in, private (expose) side, not public.
Between them they cover both IP versions, all four NAT modes, the
catch-all default expose, protocol-split port forwarding (TCP and UDP to
the same host), a non-TCP/UDP protocol rendered by keyword (`ICMP`, via
`AclProtoMatch::Other(1)`), both ACL scopes, both ACL actions, and the
per-VPC-pair default actions.
Open readability questions these were built to settle, roughly in
descending order of how cheap they are:
1. acl-filter's lines run to 158 columns, largely because `AclKey`'s
fields are named `src_ip_range` / `dst_port_range`. Renaming them
to `src_ip` / `dst_port` is internal-only, shortens every line by
~24 columns and matches flow-filter's vocabulary.
2. Wildcards outnumber constraints on most lines (`proto=*`,
`src_port_range=*`, `dst_port_range=*`). Eliding universal fields
reads far better, but loses "these are all the key fields", which
is what you want when debugging a key layout rather than a policy.
3. A VNI renders two ways on one line: `src_vni=100` from `Vni` and
`-> VNI(200)` from `VpcDiscriminant`.
4. `NAT: -` reads as missing data rather than "none".
5. Nothing records which peering or expose produced a rule, so the
dump cannot answer "why does this rule exist" -- the first question
an operator asks. The only one of the five that is a design call
rather than formatting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
acl-filter/src/tests.rs:1184
confidence: 9
tags: [style]
`printer_sample` is documented as a manual "printer" (not a real assertion test) but it is still annotated with `#[test]`, so it will run in normal CI by default. Mark it `#[ignore]` (or feature-gate it) so it only runs when explicitly requested.
#[test]
fn printer_sample() {
**flow-filter/src/context/tests.rs:754**
* ```yaml
confidence: 9
tags: [style]
printer_sample is labeled as THROWAWAY: not a test, but it is still annotated with #[test], so it will run in the default test suite (and spam output / add runtime) unless explicitly filtered. Mark it ignored (or gate behind a feature) so it only runs when intentionally requested.
#[test]
fn printer_sample() {
The ACL filter had no property tests at all. Its only backend-level check
was `dpdk_agrees_with_reference`, which cannot see a lowering bug: both
backends consume the same lowered rules, so an error made on the way from
`ValidatedOverlay` to those rules is invisible to it.
Add the oracle that can. `fuzz_gen` generates a valid-by-construction
overlay carrying generated ACLs and runs the *real* config validation on
it (a rejection is a finding: either the generator's model of the rules or
the rules themselves drifted). `fuzz` answers each lookup directly from
the validated config -- first matching rule of the peering that applies in
the packet's direction -- and compares that against the built tables.
The oracle never sees the parts of the lowering most likely to be wrong:
the (src entry x dst entry) cross product, the positional priority
encoding, the per-IP-version table split, the directional dedup, or the
separate default-action map. Each of those is now checked.
Verified with mutation testing rather than assumed. Deliberately breaking
the lowering fails the suite for: an inverted rte_acl priority (caught by
the differential only -- the reference backend is first-match over the
list and never consults a priority, so the ordering test's scope is noted
in its doc comment), a truncated src/dst cross product, an inverted
direction filter, a dropped `scope`, and a dropped `log`. Keying the
default-action map on the reversed VNI pair is *not* caught, and should
not be: each peering is visited once from each end, so both key orders
populate the same set of directed pairs.
Two generator lessons came out of that exercise, both now encoded:
- Probes are anchored to a generated peering direction, with a `Stray`
enum breaking one precondition at a time. Freely drawn probes were
99.98% misses (5 rule hits per run), which spent the whole budget
confirming that unrelated traffic matches nothing.
- A rule's pattern can carry two prefixes per side. With single-entry
patterns the cross product is the identity, and a lowering that kept
only the first entry passed unnoticed.
Coverage counters assert the interesting outcomes were actually reached,
so a time-boxed run cannot pass vacuously.
The generator deliberately duplicates the skeleton of flow-filter's
`fuzz_gen` rather than sharing it: that one needs a pool of disjoint
prefix blocks so expose overlaps arise only where it injects them, while
ACL rules are all about overlapping matches. Sharing would force one into
the other's prefix discipline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The context property tests stop at a `LookupResult`. The step after it -- turning that result into the destination and NAT flags every downstream NF acts on -- had only hand-written coverage, so the mapping was never checked against a configuration nobody chose. `nf_metadata_matches_config_oracle` closes that hop. It runs generated packets through the real NF and compares the stamped metadata against a prediction derived from `context::fuzz::oracle_lookup` -- the same oracle the context suite uses, now `pub(crate)` so the config's meaning of a route stays stated in one place. Only the `LookupResult` -> metadata step is restated. Scope is flowless packets: bypass, invalidation and the reply paths are not functions of the configuration and are covered elsewhere. Verified by mutation: swapping the static src/dst flags, dropping the masquerade-destination check, attaching the flow key whenever NAT is stateful rather than only alongside static NAT, and dropping a source miss with the wrong `DoneReason` all fail the test. Along the way, two fixes the oracle forced out: `ManifestSpec::strip_nat` was over-strict. Config forbids stateful NAT only opposite *stateful* NAT -- `validate_nat_combinations` says outright that "no NAT or static NAT only is compatible with all other modes on the other side", and `static_nat_plus_masquerade_context` in this very file relies on it. The generator stripped *all* NAT from the far side, so masquerade-opposite-static-NAT peerings never appeared in any generated overlay. That is the only shape producing a route that needs both stateful and static NAT, which is the sole case where the NF retains a flow key -- so the flow-key path was unreachable to every property test in the crate. Downgrading to static NAT instead of flattening to plain makes it reachable: the new test now records ~54k flow-key routes per 30s run, and coverage counters assert it stays that way. The comment above `validate_nat_combinations` claimed it rejects "NAT (static or stateful)" on the far side, which contradicts the table and the code directly beneath it. That reading is the most likely source of the generator's mistake, so correct it. Also adds v6 UDP and ICMPv6 packet builders, so a v6 probe is realized as the packet it describes. ICMPv6 carries a different next header than ICMPv4; the probe is adjusted to match whatever the built packet really carries, so the oracle is never asked about a packet that could not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
flow-filter/src/context/tests.rs:754
confidence: 9
tags: [style]
`printer_sample` is described as “not a test, a printer” but it’s still a normal `#[test]`, so it will run in CI/`cargo nextest run` by default and produce noise/time cost. Mark it `#[ignore]` (or gate it behind a feature) so it only runs when explicitly requested.
#[test]
fn printer_sample() {
**acl-filter/src/tests.rs:1184**
* ```yaml
confidence: 9
tags: [style]
printer_sample is explicitly “not a test, a printer” but it will still execute as part of the default unit test suite. Mark it #[ignore] so it doesn’t run in CI unless explicitly selected.
#[test]
fn printer_sample() {
Comment on lines
49
to
+54
| pub trait MatchKey: Sized { | ||
| /// The rule (predicate) form of this key: one spec per match field, still carrying each | ||
| /// field's type. A table can therefore retain the rules it was built from in a form that | ||
| /// renders itself, without having to decode erased bytes back into domain types. | ||
| type Rule; | ||
|
|
`DpdkAclLookup::new` checked that the planned stride was at least the classifier's `min_input_size` -- the invariant that makes the `unsafe` classify calls sound -- but nothing checked it from above. Every buffer on the lookup path is sized by `MAX_USER_KEY_BYTES`, and an oversized stride would take out `ZEROS[..stride]` on the *hot path*, once per packet, two files from the constant that was supposed to prevent it. It is unreachable today, but with no margin at all: `plan_layout` emits at most `MAX_FIELDS` (64) defs of at most 4 bytes, and `4 * 64` is exactly `MAX_USER_KEY_BYTES` (256). Raising `MAX_FIELDS` or lowering `MAX_USER_KEY_BYTES` by one would make a maximal key panic at lookup. Check it in the constructor, so an unusable table fails to build instead, and pin the relationship between the two constants with a test at the planner, where a future edit to either would land. `StrideTooSmall` becomes `StrideError` with `TooSmall`/`TooLarge` variants; the single caller is `install_table`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ader stacks `classify` is the only place this NF reaches into a packet's headers, and every test above it fed exactly four hand-built shapes: v4 TCP, UDP, ICMP, and a bare Ethernet frame. No VLAN tag, no IPv6 extension chain, no fragment, no authentication header, and none of the fuzzed remainder of any header's fields. `net` already ships a bolero header-stack generator; nobody had pointed it at this NF. `arbitrary_header_stacks_uphold_the_config_contract` runs ten stack shapes through the real NF and asserts the stamped metadata equals what the config oracle predicts for the key that stack presents. Addresses are steered into the configured prefixes (a uniformly random address never lands in one, and the run would collapse into destination misses) while every other field stays fuzzed. What the test claims is bounded, and the doc comment says so: `probe_from_packet` extracts the key with the same accessors `classify` uses, so this is not an independent check of that extraction. Its value is that no generated stack panics, the fail-closed invariants hold on shapes nobody picked, and the route -> metadata mapping holds for keys only exotic stacks produce. Coverage counters insist those keys are reached: a typical 1s run sees ~1600 portless IP packets and ~4400 non-transport protocol numbers, neither reachable from the four hand-built shapes. That immediately turned up a behaviour worth pinning on its own, so `ipv6_extension_header_masks_the_transport_protocol` documents it: `Net::next_header()` reports the IPv6 header's own next-header field, so any extension header puts *its* number there -- while `try_transport()` still walks the chain and finds the real ports. TCP behind a hop-by-hop header therefore presents `(proto = HOPOPT, ports = the TCP ports)`. A protocol-restricted expose lowers to an exact match on the protocol byte and cannot match it, so the traffic is dropped rather than forwarded. That is fail-closed, but it is a functional gap: legitimate TCP over IPv6 carrying extension headers cannot reach a TCP-restricted port-forwarding destination. The test pins both the mechanism and the consequence, and shows the same packet routing through an unrestricted expose to confirm the protocol byte is what excluded it. Requires net's "bolero" feature in dev-dependencies for the generator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tables as fans
Every expose the generator emitted was a single block -- one `/24`, one
`/32`. Real exposes carry `not` / `not_as` exclusions, which validation
subtracts and normalizes away, so a `ValidatedExpose` reaching the table
builder is normally a *fan* of disjoint prefixes of differing lengths. The
generator never produced one, and the whole crate leans on that shape:
`rule_priority` orders purely by prefix length.
Add an `ExcludeSel` to each generated expose. Punching a single host out of
a `/24` expands it to prefixes of every length from `/25` to `/32` at once,
which is as hard as the length ordering gets.
Constraints the exclusions have to respect, all encoded and commented:
- Port forwarding forbids exclusions outright, so only the masquerade
half of the Masquerade*PortFw pairs takes one.
- Static NAT requires equal address counts on both sides, so exclusions
are applied symmetrically to the private and public blocks.
- Every variant stays in the block's upper half, because
`derive_routing_probes` aims its guaranteed-routing probes at host `.1`
and `FW_HOST` sits in the lower half -- so neither the derived probes
nor the deliberate nested-overlap cases are disturbed.
`exclusions_reach_the_config_as_multi_length_prefix_fans` asserts the fan
actually materializes (widest spread must reach 8), because an exclusion
that validation collapsed back into its block would leave every test above
it quietly passing on the single-prefix exposes it had before.
Value shown by mutation rather than argued: truncating `RuleSet::from_overlay`
to lower only the *first* prefix of each expose is caught in ~100 iterations
with exclusions on, and passes cleanly with them off. That is the same class
of blind spot the ACL cross-product had -- when every input has one element,
a lowering that drops all but the first is the identity.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…state Both are behaviours the fuzz suites hit thousands of times per run and neither could describe: a property test says "the NF agreed with the config", not "and here is what the config means". Both read like bugs until explained, which is exactly why they should be tests rather than folklore. `portless_packet_cannot_match_a_port_restricted_expose`: a packet with no usable transport ports looks up with port 0, and config forbids port 0 in an expose's ranges, so a port-restricted expose can never match one. Not exotic -- ICMP, non-first fragments, and anything behind an IPv6 extension header all present portless keys. Fail-closed, but it makes port-forwarded destinations unreachable to that traffic, including the ICMP errors path MTU discovery needs. The test shows the same packet routing through an unrestricted expose, so the port is demonstrably what excluded it. `flow_from_a_newer_generation_is_honored_for_bypass`: `dst_vpcd_from_valid_flow` rejects only `flow_genid < genid`, so a flow tagged with a generation this worker has not yet observed short-circuits the tables. That is the transient its comment describes -- the control plane has stamped flows with the new generation before this worker reads it, and treating them as stale would tear down every flow the new config just blessed. It was the one direction of the genid comparison nothing covered. The test aims at a destination the tables do not cover, so honouring the flow is observable rather than incidental. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ering's
A manifest may hold exposes of both IP versions. `ValidatedManifest::is_v4`
is `any`, not `all`, so such a manifest reports `is_v4()` and `is_v6()` at
once, and `validate_ip_version` -- which compares only `is_v4()` -- accepts
the peering.
The lowering filed every rule of a peering under `peering.is_v4()`, which a
dual-stack peering reports as `true` whatever its rules use. A v6 rule
landed in the v4 table, where narrowing the key returned `None` and a
`filter_map` discarded it with no error and no log. Both tables then built
empty and the peering's default admitted the traffic the rule denied.
That is a fail-open reachable from a fully validated configuration: an
operator writes a `Deny`, the config applies cleanly, and the dataplane
never enforces it.
`table_for` now decides the table from the rule's own (source, destination)
prefix pair:
- v4 pair -> v4 table; v6 pair -> v6 table.
- A pair naming both versions at once matches no packet and is dropped
rather than filed under either.
- A rule constraining neither address goes to *both* tables. This is the
only shape that can span versions: config rejects a mixed src/dst
pattern (`have_consistent_ip_version`), so a pattern side is
single-version unless it was empty, in which case validation
substituted the manifest's own coverage. Naming `0.0.0.0/0` (or `::/0`)
on either side pins the rule to that version, because coverage
validation intersects it with the manifest and drops the rest.
Pushing into each list in rule order preserves relative order, so
first-match precedence within every table is unchanged.
`lower_rules` no longer swallows a rule it cannot narrow: it returns an
error, so the whole configuration is refused and the previously applied
tables stay in place. Silence was the actual defect -- a dropped rule is
indistinguishable from a rule that never matched.
Tests: five regression tests covering each case (v6 rule enforced, v4 rule
confined to v4, address-less rule binding both, root prefixes pinning to one
version, and both tables genuinely populated). Reverting `table_for` to the
old bucketing fails four of the five.
The fuzz generator now emits dual-stack peerings (`PeeringSpec::dual_stack`)
with anchors on both versions, so this is covered permanently rather than by
the five examples; under the same mutation the property suite fails in one
iteration. `dual_stack_peerings_reach_both_tables` asserts the shape keeps
being generated -- ~790k dual-stack manifests and ~42k both-table overlays
per 45s run.
Enabling that generation immediately exposed the same latent assumption in
the test oracle: `rule_matches` checked source and destination containment
independently, so on a dual-stack peering it claimed a rule matched a packet
with a v4 source and a v6 destination. No such packet exists, and
`AclTables::lookup` rejects one outright. Guard added.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… carries A default expose names no address at all, so the lowering had to pick the root prefix's IP version from somewhere -- and it picked `ValidatedPeering::is_v4`. That is `any`, not `all`: a manifest holding one expose of each version reports `is_v4()` and `is_v6()` at once, and `validate_ip_version` accepts the peering. On such a peering the catch-all lowered to `0.0.0.0/0` only, `remote_v6` got no `::/0`, and the peering's outbound IPv6 missed stage 1 and dropped. `default_roots` now takes the versions from the manifests' concrete exposes -- a default expose reports neither version itself, since `is_v4`/`is_v6` read the first entry of an empty `ips`. A peering carrying both gets both roots; a single-version peering gets exactly the one it had before, so this is not a widening of existing configurations. The oracle states the same rule via `default_covers`, so the property suite checks the semantics rather than the encoding. This is the sibling of the acl-filter fix in the previous commit and the same underlying cause, but it is fail-closed rather than fail-open: traffic was dropped, not admitted. Tests: `default_expose_on_a_dual_stack_peering_covers_both_versions` pins both halves and routes real v4 and v6 packets through the catch-all; `default_expose_on_a_single_version_peering_stays_single_version` pins that a v4-only peering gains no IPv6 rule. The generator now emits dual-stack peerings (`PeeringSpec::dual_stack`), giving every expose a twin in the other version on the same block number -- the two prefix pools are disjoint, so the twin cannot collide and the block accounting `derive_routing_probes` relies on is unchanged. Derived routing probes are emitted for both versions, so the v6 half of every dual-stack route is exercised. `dual_stack_catch_all_reaches_both_tables` is both a property and a coverage assertion: it checks every generated dual-stack peering with a catch-all has both roots, and fails if the generator ever stops producing that shape (~41k per 45s run). Reverting the fix fails it, the two pinned tests, and the config oracle within 6 iterations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Config models the destination -> VPC relation as one-to-many. In
`config/src/external/overlay/vpcrouting.rs`, `VpcRouteTable` is
`destination prefix -> VpcRouteSet`, and `VpcRouteSet` is a `Vec`:
struct VpcRouteSet(Vec<VpcRoute>);
`VpcRoute::can_overlap` deliberately exempts masquerade/masquerade, so two
peers may masquerade behind one public range and both claim the same
destination prefix.
Stage 1 of the flow-filter lowers that relation into a *function*:
`RemoteKey -> Verdict`, and `Verdict` holds exactly one `dst_vpcd`. Two
peers sharing a range therefore produce two rules with identical match sets
and identical priorities differing only in the destination VPC, and the
collapse is settled by tie-break order -- stable-sort position under the
reference backend, and formally unspecified under rte_acl, so the two
backends need not even agree.
`apply_route` then required the packet's flow to agree with that arbitrary
winner, so every masquerade reply toward the *losing* peer was dropped and
its flow cancelled. Deterministic, but arbitrary: one of the two peers was
simply broken.
The root cause is that a masquerade public address is not a routable
identifier. It names a NAT pool. It can never accept a new connection, so
the only legitimate traffic to it is reply traffic, which by construction
rides a flow -- and the flow is the only thing that can distinguish two
connections to the same address. State is primary for this traffic class;
the table can only ever be a plausibility check.
So stop asking the table an unanswerable question. "Which VPC owns
20.0.0.5?" has no unique answer. "Does vpc3 masquerade 20.0.0.5 toward me?"
does. Stage 3 (`MasqueradeKey`) carries `dst_vni` as an *input*: the flow
supplies the candidate, the table says whether the configuration agrees.
- `LookupInput` carries `flow_dst_vpcd`, so verification stays on the
batched path with the other stages rather than becoming a per-packet
lookup in the NF.
- `LookupResult::MasqueradeDestination(Option<VpcDiscriminant>)` replaces
`Route` with a masquerade `dst_nat`. `Some` means verified; `None` means
no candidate, or one config rejects. There is no source NAT to report --
such a packet rides a flow that already carries its own state.
- Stage 2 and stage 3 are mutually exclusive, so each rte_acl call still
asks exactly one question and the burst still makes three calls.
Note what this does NOT weaken. An earlier proposal was to take the
destination straight from the flow; that is wrong, and it is why this is a
verification and not a substitution. The old equality check was doing double
duty -- a broken disambiguator *and* a working staleness check -- so
removing it outright would have let a flow naming a since-unpeered VPC
forward traffic there. Asking the configuration answers both at once:
`masquerade_reply_is_refused_when_the_flow_names_the_wrong_peer` pins the
staleness half, and the pre-existing `masquerade_reply_with_mismatched_flow_
destination_is_filtered` now passes for a stronger reason (config does not
agree that VPC owns the address, rather than a tie-break disagreeing).
`masquerade_replies_reach_both_peers_sharing_a_public_range` is the
regression test: both peers' replies now work, where one was always dropped.
Reverting the verification to the old equality rule fails it.
Two test-visible consequences of the model change, both intended:
- `dst_side_nat_modes` asserted a masquerade destination "resolves as a
marker" with a destination VPC. It no longer resolves at all; the test
now pins all three verification outcomes.
- Derived routing probes at a masquerade destination never route, so they
are split into `masquerade_probes` carrying the correct candidate and
asserted to verify. That keeps stage 3's *hit* path exercised -- a
verification table matching nothing would satisfy every "must drop"
assertion in the suite (~13k masquerade destinations per 30s run).
Cross-peering overlaps are still absent from the generated overlays, so the
property suite does not yet catch this class on its own; that follows in the
next commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by a long fuzz run, and introduced by the previous commit:
dpdk build: "invalid ACL context name: ACL context name is too long
(32 > 31 bytes)"
`table_name` built `flow_filter_{base}_{seq}` with a decimal counter. The
12-byte prefix plus the new 13-byte `masquerade_v4` base leaves five digits
inside rte_acl's 31-byte limit, so names went over once the counter reached
100_000. The counter is process-global and advances once per table built, so
this is not confined to tests: a long-lived dataplane rebuilding its tables
on every configuration change reaches it too, and when it does *every*
subsequent table build fails -- reconfiguration stops working, with the last
good tables left in place.
The previous bases were short enough (`remote_v4` at 9 bytes) to allow nine
digits, so the ceiling was ~10^9 and effectively unreachable. Adding a
longer base cut it by four orders of magnitude. That is the real defect: the
bound held by accident of how the names happened to be spelled, not by
construction.
Now it holds for every value the counter can take. A 3-byte prefix, a base
capped at 9 bytes, and a hexadecimal counter (16 bytes at most for a `u64`)
give at most 29 bytes. `table_names_fit_the_rte_acl_limit_for_every_counter_value`
asserts this against `u64::MAX` rather than against the counter's current
value, which is the property the decimal scheme lacked, and a `debug_assert`
catches an over-long base at its call site.
Context names are internal to rte_acl's registry -- nothing renders or
persists them -- so renaming is safe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous fix made a masquerade destination something the tables verify
against the flow's candidate rather than resolve from the address. Nothing
in the generated overlays exercised why: the prefix pool gives every expose
its own block, so two peers never shared a masquerade range and every
destination stayed unambiguous. Mutating the verification back to the old
"flow must equal stage 1's arbitrary winner" rule was caught only by the
hand-written regression test.
`OverlaySpec::shared_masquerade_pool` builds the shape. Plain-masquerade
exposes translate into one range shared across the whole overlay, so peers
of the same VPC advertise overlapping masquerade destinations -- the
one-to-many relation config permits and `VpcRouteSet` already models.
Three things this needed, each a lesson about the generator:
- Left to chance the overlap appeared in well under 1% of overlays, since
it needs two peerings of one VPC to *both* draw a plain masquerade
expose. Within the mode it is now arranged: each peering's remote side
leads with masquerade and the local side is stripped of stateful NAT
(config forbids it on both sides, and normalization would otherwise undo
the masquerade). ~14k ambiguous destinations per 60s run.
- No probe could reach the shared pool. It sits outside the `0..blocks`
range probe selectors are reduced into, so the ambiguous destinations
were built and then never looked up. `ProbeSpec::dst_shared_masquerade`
aims at it.
- The oracle's `consider` panicked on any equal-precedence tie. Two
overlapping masquerade exposes are exactly that, and the tie is now
benign because both say "masquerade destination" and neither names the
VPC. `StageOne::Masquerade` carries no VPC, so the two compare equal;
`consider` accepts a tie whose candidates agree and still fails on one
where they disagree, which remains a real ambiguity.
The combined-masquerade variants keep their own block: they exist to pin the
nested and equal-length overlaps, which moving their public side would
dissolve.
Mutation results, all four combinations:
- both lookup paths reverted -> `reference_lookup_matches_config_oracle`
fails in ~400 iterations, and the rte_acl differential fails
- batched path only -> `batched_lookup_matches_single_lookup` fails, which
is what keeps the two paths from drifting apart
- either -> the hand-written regression test fails
Before this commit only the last of those held.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flow-filter/src/context/tests.rs:783
confidence: 8
tags: [style]
`printer_sample` is described as “THROWAWAY: not a test, a printer”, but it is currently a normal `#[test]` and will run in the default CI suite. Mark it `#[ignore]` (and run it explicitly when needed) to avoid adding runtime to routine test runs.
#[test]
fn printer_sample() {
**acl-filter/src/tests.rs:1184**
* ```yaml
confidence: 8
tags: [style]
printer_sample is documented as a “THROWAWAY” printer, but it is currently a normal #[test] and will run in the default CI suite. Mark it #[ignore] (and run it explicitly when needed) to keep routine test runs focused.
#[test]
fn printer_sample() {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Still too much AI in this one. Needs more careful review by me before being up for proper review. Just posting this now to help any discussion of the changes