fix(config,acl-filter): a manifest may not mix IP versions; refuse rules that cannot be lowered - #1688
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
daniel-noland
added
dont-merge
3722537 to
e9b759e
Compare
There was a problem hiding this comment.
Pull request overview
This PR tightens overlay/config invariants around IP-version consistency and makes ACL rule lowering fail closed (refuse configuration) rather than silently dropping rules that cannot be filed into the intended per-IP-version tables. It also adds/extends Bolero-based property suites to exercise these invariants and the resulting dataplane metadata behavior under generated overlays and adversarial packet/header shapes.
Changes:
- Enforce “single IP version per manifest” during manifest validation, with regression tests covering mixed-version and default-expose cases.
- Change ACL rule lowering to return an error when rule prefixes cannot be narrowed to the target table’s IP version (instead of dropping those rules).
- Add substantial property-test coverage for flow-filter metadata stamping and ACL lowering/lookup semantics using generated overlays, probes, exclusions, and header stacks.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| flow-filter/src/tests.rs | Adds property tests and edge-case tests validating flowless-packet metadata and classification behavior against a config oracle. |
| flow-filter/src/test_utils.rs | Adds IPv6 UDP and ICMPv6 packet builders used by the new test suites. |
| flow-filter/src/lib.rs | Clarifies bypass comment to include newer-generation flows. |
| flow-filter/src/fuzz_gen.rs | Extends overlay/probe generation to cover exclusions and port-forwarding probe derivation; adjusts normalization around stateful NAT constraints. |
| flow-filter/src/context/mod.rs | Makes the fuzz oracle module accessible to crate tests (while still cfg(test)-gated). |
| flow-filter/src/context/fuzz.rs | Exposes the route-lookup oracle for reuse and adds a property ensuring exclusions generate multi-length prefix fans. |
| flow-filter/Cargo.toml | Enables net’s bolero feature for generated header-stack tests. |
| config/src/external/overlay/vpcpeering.rs | Validates that a manifest cannot mix IPv4 and IPv6 exposes. |
| config/src/external/overlay/vpc.rs | Updates NAT-combination comment to match the enforced compatibility model. |
| config/src/external/overlay/validation_tests.rs | Adds regression tests for rejecting mixed-version manifests and allowing default exposes to remain version-agnostic. |
| Cargo.lock | Records new dependency usage (bolero) for the ACL filter crate. |
| acl-filter/src/lib.rs | Wires in new cfg(test) fuzz modules. |
| acl-filter/src/fuzz.rs | Adds ACL lowering/lookup property tests comparing tables/backends against a config-semantics oracle. |
| acl-filter/src/fuzz_gen.rs | Adds generators for valid-by-construction overlays with ACLs and probes that exercise ordering, overlap, directionality, and miss paths. |
| acl-filter/src/context.rs | Makes rule lowering return an error (fail closed) when rules cannot be lowered into the intended IP-version table. |
| acl-filter/src/access.rs | Updates test-only context builder expectation message for the now-fallible lowering stage. |
| acl-filter/Cargo.toml | Adds bolero dev-dependency for the new property test suite. |
b01461b to
ab42e83
Compare
e9b759e to
ddc447f
Compare
Generate valid ACL overlays and compare reference-table lookups with an independent oracle over the validated config. Cover ordering, direction, prefix cross-products, protocols, metadata, IP versions, and defaults. Compare the same cases with rte_acl to cover backend encoding and priority. Coverage counters prevent vacuous short runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Expose validation checks one expose at a time, so a manifest could still combine IPv4 and IPv6. Filters choose one table version per peering and could omit rules for the other version. Require one IP version across a manifest's non-default exposes. Add tests for both expose orders, default exposes, and valid single-version manifests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
lower_rules previously dropped rules that did not match the selected table's IP version. Return FailureApply instead so an invariant violation rejects reconfiguration rather than silently omitting a rule. Validated manifests prevent this case; the check is defense in depth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
ddc447f to
b0ed0e4
Compare
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
Good catch for the IP version fix, thank you!
Not sure whether the 3rd commit is necessary, but I've got no objection.
|
Does it need to be on top of the #1687? It seems the two PRs are entirely distinct? I'd send to merge queue if this one was based on |
mvachhar
left a comment
mvachhar
left a comment
There was a problem hiding this comment.
This LGTM, but I want to confirm that we decided the fix for the IPv4 vs. IPv6 ACL any match issue was to reject (for now) manifests that have both IPv4 and IPv6 in the same peering?
@mvachhar Sorry, what issue is that, exactly? |
1ab1345
into
pr/daniel-noland/flow-filter-coverage
Similar to #1687, this is more testing logic and some fixes for the acl-filter.
Again, I don't love the structure of these fuzzers, but this is the version which doesn't require extra refactor of the production logic. The types should be reworked later so that we compose better and to promote reuse.