test(flow-filter): Add non-regression test - #1680
Conversation
As described in the test's comment, we recently discovered a bug in the previous flow-filter implementation. The new version is not affected, but let's introduce a non-regression test to be sure of it. Signed-off-by: Quentin Monnet <qmo@qmon.net>
📝 WalkthroughWalkthroughThis change adds a multi-prefix test helper and a regression test for overlapping and contiguous peerings. The test verifies forward and reverse lookups, unset NAT requirements, and local and remote rules without ChangesPrefix overlap validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@flow-filter/src/context/tests.rs`:
- Around line 547-578: The routing test around route must cover both contiguous
addresses, 10.0.2.2 and 10.0.2.3, in both forward and reverse directions and
verify each result. Replace the weak assert_ne! checks over reference_rules with
assertions that the relevant local and remote rules contain the expected
normalized 10.0.2.2/31 prefix, while still validating the intended rule
collections are present.
🪄 Autofix (Beta)
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: 6dd81d13-00a4-4560-a13a-25aef101d6e8
📒 Files selected for processing (2)
flow-filter/src/context/tests.rsflow-filter/src/test_utils.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
flow-filter/src/context/tests.rs:511
confidence: 10
tags: [docs]
Typo in the regression-test comment: “discrepency” should be “discrepancy”.
// then we would merge entries in the temporary list of overlapping prefixes used to compute table
// entries, resulting in a discrepency between (merged) prefixes for vpc2's context and (split)
// prefixes for vpc3's context. In our example here, the context table would contain, for source
**flow-filter/src/context/tests.rs:508**
* ```yaml
confidence: 8
tags: [docs]
The explanatory comment describes “vpc2 and vpc3 exposing overlapping prefixes to vpc1” and gives an example where, for source vpc1, the destination-prefix table has 10.0.2.x entries resolving to vpc2/vpc3. In this test setup, 10.0.2.x prefixes are on vpc1’s local/source exposes (towards vpc2/vpc3), while vpc2/vpc3 expose 20.0.0.0/24 and 30.0.0.0/24 to vpc1 (no overlap). Please either adjust the setup to match the described scenario, or update/rewrite the comment so it matches what the test actually exercises and asserts.
This issue also appears on line 509 of the same file.
// - vpc2 and vpc3 exposing overlapping prefixes to vpc1, and
// - vpc3's exposed prefixes being contained within vpc2's exposed prefixes to vpc1, and
// - vpc2's exposed prefixes containing contiguous prefixes that could be merged into a single
// parent prefix,
//
As described in the test's comment, we recently discovered a bug in the previous flow-filter implementation. The new version is not affected, but let's introduce a non-regression test to be sure of it.