test(flow-filter): carry the config oracle through to the NF, and widen the generators - #1687
Conversation
📝 WalkthroughWalkthroughThe PR adds stricter overlay and ACL validation, expands ACL and flow-filter generators, and adds property tests for routing, filtering, NAT, IPv4/IPv6 parsing, packet shapes, and configuration-generation bypass. ChangesOverlay and filter validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
daniel-noland
added
dont-merge
d64d4ed to
dab0fef
Compare
dab0fef to
7ec284c
Compare
7ec284c to
b01461b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
flow-filter/src/tests.rs:1145
confidence: 8
tags: [logic]
`NfOutcome::Dropped` is documented as having “no destination stamped”, but `observed_outcome()` currently ignores `meta.dst_vpcd` for done packets, so this test won’t catch a regression where `dst_vpcd` is incorrectly set on filtered/not-ip packets. Adding an assertion here tightens the contract without changing the oracle shape.
fn observed_outcome(pkt: &Packet) -> NfOutcome {
if pkt.is_done() {
return NfOutcome::Dropped(pkt.get_done());
}
</details>
Compare flowless packet metadata with the config routing oracle. Extend generated probes and packet builders to cover all NAT modes and IPv6 traffic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Generate VLAN, IPv4, IPv6, extension, fragment, and transport headers. Check each packet with the config oracle and require every shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Generate not/not_as exclusions and require multi-length prefix fans. Preserve static NAT address counts and port-forwarding probe hosts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Cover IPv6 extension headers, portless packets, and newer flows. Clarify which flow generations bypass the filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
b01461b to
ab42e83
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
flow-filter/src/tests.rs (1)
1404-1418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
BY_SHAPEindexing depends on an implicit discriminant contract.
BY_SHAPE[*shape as usize]on Line 1523 andBY_SHAPE[shape as usize]on Line 1583 assume that each variant's discriminant equals its position inShape::ALL. The comment on Line 1405 records the requirement, but nothing enforces it. If a variant is inserted or reordered without updatingALL, the counters silently attribute packets to the wrong shape.A debug assertion makes the contract self-checking.
♻️ Proposed self-check
impl Shape { /// Every shape, in selector and counter order. const ALL: [Shape; 10] = [ Shape::NoIp, Shape::V4Tcp, Shape::V4Udp, Shape::V4Icmp, Shape::VlanV4Tcp, Shape::V4AuthTcp, Shape::V6Tcp, Shape::V6Udp, Shape::V6HopByHopTcp, Shape::V6FragmentUdp, ]; + + /// `ALL` must be ordered by discriminant so `shape as usize` indexes the counters. + const _ORDER_CHECK: () = { + let mut i = 0; + while i < Self::ALL.len() { + assert!(Self::ALL[i] as usize == i, "Shape::ALL is out of order"); + i += 1; + } + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flow-filter/src/tests.rs` around lines 1404 - 1418, Add a debug assertion near the Shape::ALL definition or before the BY_SHAPE indexing paths to verify every Shape variant’s discriminant matches its position in Shape::ALL. Keep the existing indexing behavior, but make mismatches fail during debug/test execution rather than silently misattribute counters.flow-filter/src/context/fuzz.rs (1)
382-397: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEach peering is inspected twice.
vpc_table().values()yields every VPC, and each VPC lists the peerings it takes part in. A peering between two VPCs therefore appears in both lists, so this loop visits the same expose set twice.fetch_maxis idempotent, so the assertion result does not change. The extra traversal only costs generator time.If the duplication is intentional, no change is needed. Otherwise iterate the peering table once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flow-filter/src/context/fuzz.rs` around lines 382 - 397, Update the traversal around the VPC peering inspection to iterate the peering table once rather than visiting each peering through every VPC in vpc_table().values(). Preserve the existing expose, IP-set, prefix-length, and WIDEST_SPREAD calculation while eliminating duplicate processing of the same peering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@flow-filter/src/context/fuzz.rs`:
- Around line 382-397: Update the traversal around the VPC peering inspection to
iterate the peering table once rather than visiting each peering through every
VPC in vpc_table().values(). Preserve the existing expose, IP-set,
prefix-length, and WIDEST_SPREAD calculation while eliminating duplicate
processing of the same peering.
In `@flow-filter/src/tests.rs`:
- Around line 1404-1418: Add a debug assertion near the Shape::ALL definition or
before the BY_SHAPE indexing paths to verify every Shape variant’s discriminant
matches its position in Shape::ALL. Keep the existing indexing behavior, but
make mismatches fail during debug/test execution rather than silently
misattribute counters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9061116f-9ff1-4856-bde6-02850a4bea6a
📒 Files selected for processing (8)
config/src/external/overlay/vpc.rsflow-filter/Cargo.tomlflow-filter/src/context/fuzz.rsflow-filter/src/context/mod.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rsflow-filter/src/test_utils.rsflow-filter/src/tests.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flow-filter/src/tests.rs:1583
confidence: 7
tags: [style]
`Shape::ALL.map(|shape| ... BY_SHAPE[shape as usize] ...)` has the same implicit-discriminant indexing assumption as above. Index `BY_SHAPE` by the array position instead, so `Shape` changes can’t desynchronize the counters.
let by_shape =
Shape::ALL.map(|shape| (shape, BY_SHAPE[shape as usize].load(Ordering::Relaxed)));
**flow-filter/src/tests.rs:1523**
* ```yaml
confidence: 7
tags: [style]
BY_SHAPE[*shape as usize] relies on the enum’s implicit discriminant values matching the intended indexing order; adding/reordering variants can silently break coverage accounting. Use an explicit match-to-index mapping so the compiler forces updates when Shape changes.
This issue also appears on line 1582 of the same file.
BY_SHAPE[*shape as usize].fetch_add(1, Ordering::Relaxed);
|
Note: It may be worth waiting for the flow-filter fixes to make it first before merging this PR |
|
Removing from merge queue, I think it's worth waiting for the flow-filter fixes first (but open to discussion) |
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>
|
We agreed to merge the fuzz tests first |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acl-filter/src/fuzz.rs`:
- Around line 161-181: Update Coverage::assert_reached and the fuzz probe
generation flow to make bucket coverage deterministic: add explicit iterations
or targeted ProbeSpec cases that exercise rule allows, rule denies, default
fallbacks, and unconfigured pairs, rather than relying on random ProbeSpec.stray
generation. Preserve the existing counters and assertions, but ensure each
bucket is reached before assert_reached performs its checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 725792c0-18ab-4c5e-b228-a65a459bc1a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
acl-filter/Cargo.tomlacl-filter/src/access.rsacl-filter/src/context.rsacl-filter/src/fuzz.rsacl-filter/src/fuzz_gen.rsacl-filter/src/lib.rsconfig/src/external/overlay/validation_tests.rsconfig/src/external/overlay/vpcpeering.rsflow-filter/src/fuzz_gen.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- flow-filter/src/fuzz_gen.rs
| fn assert_reached(&self, label: &str) { | ||
| let (allows, denies) = ( | ||
| self.rule_allows.load(Ordering::Relaxed), | ||
| self.rule_denies.load(Ordering::Relaxed), | ||
| ); | ||
| let (defaults, unconfigured) = ( | ||
| self.default_falls.load(Ordering::Relaxed), | ||
| self.unconfigured.load(Ordering::Relaxed), | ||
| ); | ||
| eprintln!( | ||
| "{label} coverage: {allows} rule allows, {denies} rule denies, \ | ||
| {defaults} default fallbacks, {unconfigured} unconfigured pairs" | ||
| ); | ||
| assert!(allows >= 1, "{label}: no rule ever allowed a packet"); | ||
| assert!(denies >= 1, "{label}: no rule ever denied a packet"); | ||
| assert!(defaults >= 1, "{label}: never fell through to a default"); | ||
| assert!( | ||
| unconfigured >= 1, | ||
| "{label}: never probed a pair with no ACL at all" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Inspect the VpcTable container type and its values() iteration order.
fd -t f 'vpc.rs' | xargs rg -n -C6 'struct VpcTable|fn values|BTreeMap|HashMap|IndexMap'
# 2) Look for a configured bolero iteration bound in the workspace.
fd -t f -e toml -e sh -e yml -e yaml | xargs rg -n -i 'bolero|BOLERO_'
rg -n 'with_iterations|check!\(\)' --type=rustRepository: githedgehog/dataplane
Length of output: 14344
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- fuzz.rs target sections ---'
cat -n acl-filter/src/fuzz.rs | sed -n '1,210p;330,390p'
printf '%s\n' '--- VNI declarations and table construction ---'
rg -n -C8 'VNIS|vpc_table\(\)|generated_overlays_use_the_declared_vnis|Coverage|assert_reached|bolero::check|check!' acl-filter/src/fuzz.rs acl-filter/Cargo.toml Cargo.toml
printf '%s\n' '--- local Bolero configuration or test bounds ---'
rg -n -i -C3 'iterations|cases|timeout|bolero' .cargo Cargo.toml acl-filter/Cargo.toml 2>/dev/null || trueRepository: githedgehog/dataplane
Length of output: 22784
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- fuzz generator ---'
fd -t f -i '*fuzz*' acl-filter/src | sort
rg -n -C10 'pub const VNIS|const VNIS|struct OverlaySpec|impl OverlaySpec|fn build|vni\(' acl-filter/src/fuzz_gen.rs acl-filter/src
printf '%s\n' '--- dependency lock information ---'
rg -n -C4 'name = "bolero"|name = "bolero-generator"|name = "bolero-engine"' Cargo.lock
printf '%s\n' '--- Bolero usage and explicit run bounds ---'
rg -n -C3 'with_iterations|with_cases|iterations|cases|BOLERO' --glob '*.rs' --glob '*.toml' --glob '*.yml' --glob '*.yaml' --glob '*.sh' .Repository: githedgehog/dataplane
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- OverlaySpec::build construction ---'
cat -n acl-filter/src/fuzz_gen.rs | sed -n '259,385p'
printf '%s\n' '--- Bolero lock and local documentation references ---'
sed -n '468,525p' Cargo.lock
rg -n -C5 'with_iterations|auto-iterations|BOLERO_MAX|BOLERO_ITERATIONS|default.*iteration' acl-filter k8s-intf concurrency --glob '*.rs' --glob '*.toml'Repository: githedgehog/dataplane
Length of output: 9117
🌐 Web query:
Bolero 0.13.4 Rust check! default iterations with_type for_each
💡 Result:
In Bolero 0.13.4, the bolero::check! macro is used for property and fuzz testing [1][2]. Default Iterations: When using bolero::check!, the number of iterations is controlled by the underlying engine [3][4]. While Bolero does not explicitly document a single global "default" iteration count for all environments, you can manually configure the number of iterations for a test target by using the.with_iterations(n) method on the test target builder [5][3][4]. with_type: The.with_type:: method is used to configure the test to generate structured inputs of a specific type T [2]. For this to work, the type T must implement the generator::TypeGenerator trait [2]. This mode is commonly used when you need to test logic that requires complex or structured input rather than raw bytes [2]. for_each: The.for_each(|input| {... }) method is the terminal operation of the bolero::check! pipeline [2]. It accepts a closure that receives the generated input and contains the test logic or assertions to be executed for each generated case [2]. Example Usage: use bolero::check; check!.with_type::<(u8, u16)>.for_each(|(a, b)| { // Assertions and test logic here assert!(a as u32 + b as u32 <= 65535); }); For more granular control over input generation (such as constraints on collection lengths), you can use.with_generator instead of.with_type [2][6].
Citations:
- 1: https://docs.rs/crate/bolero/0.13.4
- 2: https://docs.rs/bolero/latest/bolero/macro.check.html
- 3: https://docs.rs/bolero/latest/bolero/struct.TestTarget.html
- 4: https://docs.rs/bolero/latest/src/bolero/lib.rs.html
- 5: https://docs.rs/bolero/latest/bolero/struct.DefaultEngine.html
- 6: https://camshaft.github.io/bolero/features/structured-testing.html
🌐 Web query:
site:docs.rs/bolero/0.13.4 bolero TestTarget with_iterations default iterations DefaultEngine
💡 Result:
For bolero 0.13.4:
TestTarget::with_iterations(iterations)sets the number of generated test cases executed. (docs.rs)- With the default
DefaultEngine/TestEngine, the iteration count comes fromrng_cfg.iterations_or_default(). (docs.rs) - In
fuzzing_randommode, omittingwith_iterationschanges the count tousize::MAX—effectively running until stopped. (docs.rs) - The docs do not expose a numeric default iteration count for normal test mode; use
.with_iterations(n)when you need deterministic behavior.
Example:
bolero::check!()
.with_iterations(10_000)
.for_each(|input| {
// test input
});Citations:
- 1: https://docs.rs/bolero/latest/bolero/struct.TestTarget.html
- 2: https://docs.rs/bolero/latest/src/bolero/test/mod.rs.html
- 3: https://docs.rs/bolero/latest/src/bolero/test/mod.rs.html
🌐 Web query:
github bolero 0.13.4 "auto-iterations" "with_iterations"
💡 Result:
The terms auto-iterations and with_iterations are associated with the Bolero library for the Rust programming language, which is a fuzz and property testing framework [1][2]. They are not related to the Bolero F# web framework [3][4]. In the Rust Bolero crate, with_iterations is a method used to configure the number of iterations executed during testing [5][6][7]. Specifically, it allows you to set a fixed number of iterations for a test target [5][7]. The term auto-iterations does not appear as a standard API method in the Bolero Rust documentation [5][6][2]. It is possible that it is either a misunderstanding of how the engine automatically handles iteration limits or refers to internal engine behavior. The Bolero engine is designed to handle execution, and configuration methods like with_iterations are provided to override or set specific constraints [6][7]. If you are seeing references to auto-iterations in the context of Bolero 0.13.4, it is likely related to how the testing engine manages its internal loop or automatic scaling of test inputs, rather than a user-facing configuration parameter [8][1].
Citations:
- 1: https://docs.rs/crate/bolero/0.13.4
- 2: https://docs.rs/bolero/latest/bolero/
- 3: https://github.com/fsbolero/Bolero
- 4: https://fsbolero.io/
- 5: https://docs.rs/bolero/latest/bolero/struct.TestTarget.html
- 6: https://docs.rs/bolero/latest/bolero/struct.DefaultEngine.html
- 7: https://docs.rs/bolero/latest/src/bolero/lib.rs.html
- 8: camshaft/bolero@f0d4332
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- ProbeSpec and probe resolution ---'
rg -n -C12 'struct ProbeSpec|enum Stray|impl.*ProbeSpec|fn resolve|TypeGenerator for ProbeSpec|Anchor' acl-filter/src/fuzz_gen.rs
printf '%s\n' '--- generator defaults and distributions ---'
cat -n acl-filter/src/fuzz_gen.rs | sed -n '120,258p;430,562p'Repository: githedgehog/dataplane
Length of output: 16676
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
gen = Path("acl-filter/src/fuzz_gen.rs").read_text()
vpc = Path("config/src/external/overlay/vpc.rs").read_text()
vnis = re.search(r'pub\(crate\) const VNIS: \[u32; 3\] = \[([0-9, ]+)\]', gen).group(1)
names = re.findall(r'format!\("vpc\{}", index \+ 1\)', gen)
insert_loop = re.search(r'for \(i, id\) in VNIS\.iter\(\)\.enumerate\(\)', gen)
has_stray_option = "stray: Option<Stray>" in gen
stray_changes_vni = "Some(Stray::SrcVni)" in gen and "Some(Stray::DstVni)" in gen
uses_btree = "vpcs: BTreeMap<String, Vpc>" in vpc
assert vnis == "100, 200, 300"
assert names and insert_loop
assert uses_btree
assert has_stray_option and stray_changes_vni
print("VpcTable order: deterministic BTreeMap order matches vpc1/vpc2/vpc3 insertion keys.")
print("Coverage: no generator invariant guarantees all four outcome buckets.")
PYRepository: githedgehog/dataplane
Length of output: 313
Make coverage checks deterministic. VpcTable uses a BTreeMap, and OverlaySpec::build inserts vpc1–vpc3 for [100, 200, 300]; sorting is not needed. Coverage::assert_reached has no equivalent guarantee because ProbeSpec.stray is random and no generator invariant reaches all four buckets. Add explicit iterations and targeted coverage cases before asserting every bucket.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acl-filter/src/fuzz.rs` around lines 161 - 181, Update
Coverage::assert_reached and the fuzz probe generation flow to make bucket
coverage deterministic: add explicit iterations or targeted ProbeSpec cases that
exercise rule allows, rule denies, default fallbacks, and unconfigured pairs,
rather than relying on random ProbeSpec.stray generation. Preserve the existing
counters and assertions, but ensure each bucket is reached before assert_reached
performs its checks.
Pull request was closed
Fuzz tests for flow filter.
I don't love everything about these tests (I wish the typing in particular were cleaner), but this is the version which doesn't require refactors outside of the tests themselves.