flow-filter: Fix flow validation on config update - #1701
Open
qmonnet wants to merge 6 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesFlow classification derives destination-VPC and NAT-mode hints from eligible outdated active flows. Routing tables perform ordinary lookups followed by revalidation passes. Generated probes, differential tests, and end-to-end tests cover the new paths. Route revalidation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the flow-filter table build/lookup logic so that outdated-flow revalidation during config updates can disambiguate cases where multiple peerings expose overlapping masqueraded prefixes (per issue #1694).
Changes:
- Extends lookup inputs to optionally carry revalidation context from an outdated flow (destination VPC and source NAT mode).
- Updates remote/local table keys and lookup logic to do a second-pass lookup gated by that revalidation context (dst-vni for masquerade destinations; nat-mode for port-forwarding-only sources).
- Updates fuzz/properties and unit tests to generate and validate lookups that include revalidation information.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| flow-filter/src/lib.rs | Threads revalidation metadata from attached (outdated) flows into table lookups; adds NAT-mode encoding helper. |
| flow-filter/src/context/tables.rs | Adds dst-vni/nat-mode into table keys and implements second-pass lookups for revalidation in both single and batch paths. |
| flow-filter/src/context/tests.rs | Updates test helpers and assertions to use the new lookup signature and table display columns. |
| flow-filter/src/fuzz_gen.rs | Enhances probe derivation/generation to include revalidation inputs for masquerade/port-forwarding edge cases. |
| flow-filter/src/context/fuzz.rs | Updates the config oracle + fuzz properties to model and assert the new revalidation-aware semantics. |
| flow-filter/Cargo.toml | Adds tracing-test as a dev-dependency. |
| Cargo.lock | Updates lockfile for the new flow-filter dev-dependency. |
Suppressed comments (1)
flow-filter/src/context/tables.rs:780
confidence: 8
tags: [logic]
Same as the IPv4 branch: the stage-2 second-pass local lookup is always attempted even when `nat_mode` encodes to 0, which is redundant. Add a `nat_mode != 0` guard before doing the revalidation lookup.
None => {
match self.local_v6.lookup(&LocalKey {
proto,
src_vni,
dst_vni: key_vni(verdict.dst_vpcd),
</details>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
flow-filter/src/lib.rs (2)
333-354: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn both revalidation hints, and rename the function.
Two points on this function:
- The
else ifchain drops the port-forwarding hint when a flow carries bothneeds_masqueradeandneeds_port_forwarding. In that case stage 2 receivesnat_mode: None, so a port-forwarding source does not resolve and the lookup returnsSourceMiss.apply_routestill admits the packet throughactive_stateful_flow, so this is not a user-visible failure today. Supplying both hints keeps the tables authoritative instead of relying on the fallback.- The name
revalidation_dst_vpcdno longer describes the return value, which is a(Option<VpcDiscriminant>, Option<NatRequirement>)pair.♻️ Suggested change
- fn revalidation_dst_vpcd( + fn revalidation_hints( &self, flow_summary: &FlowSummary, genid: i64, ) -> (Option<VpcDiscriminant>, Option<NatRequirement>) { let nfi = &self.name; if flow_summary.flow_info.status() != FlowStatus::Active { debug!("{nfi}: Packet has inactive flow information"); return (None, None); } let flow_genid = flow_summary.flow_info.genid(); if flow_genid >= genid { return (None, None); } - if flow_summary.needs_masquerade { - (flow_summary.dst_vpcd, None) - } else if flow_summary.needs_port_forwarding { - (None, Some(NatRequirement::PortForwarding)) - } else { - (None, None) - } + let dst_vpcd = flow_summary.needs_masquerade.then_some(flow_summary.dst_vpcd).flatten(); + let nat_mode = flow_summary + .needs_port_forwarding + .then_some(NatRequirement::PortForwarding); + (dst_vpcd, nat_mode) }Update the call site in
classifyif you rename.🤖 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/lib.rs` around lines 333 - 354, Update revalidation_dst_vpcd to return both independent hints when flow_summary.needs_masquerade and flow_summary.needs_port_forwarding are true, preserving the destination VPC discriminant and NatRequirement::PortForwarding together; avoid the mutually exclusive else-if chain. Rename the function to reflect its tuple of revalidation hints and update its call site in classify accordingly.
464-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTake
selfby value inas_u8.
NatRequirementis a fieldless enum used by value elsewhere in this crate (for example inVerdictand inconvert_option).fn as_u8(&self)forces a borrow at every call site for no benefit.♻️ Suggested change
- fn as_u8(&self) -> u8 { + fn as_u8(self) -> u8 { match self {🤖 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/lib.rs` around lines 464 - 478, Change NatRequirement::as_u8 to take self by value instead of borrowing &self, preserving the existing variant-to-byte mapping and allowing convert_option to call it directly on the owned requirement.flow-filter/src/context/tests.rs (1)
27-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a NAT-mode parameter for source-side revalidation coverage.
route_revalidatealways passesNonefornat_mode. The destination-side revalidation pass now has a unit test, but the source-side port-forwarding pass inflow-filter/src/context/tables.rshas coverage only from the fuzz oracle. Anat_modeparameter here would letdst_side_nat_modes(or a sibling test) assert that a port-forwarding source resolves only when the lookup carriesNatRequirement::PortForwarding.🤖 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/tests.rs` around lines 27 - 60, The route_revalidate helper currently hardcodes nat_mode as None, preventing source-side NAT revalidation tests. Add a nat-mode parameter to route_revalidate, pass it through to context.lookup, and update route to supply None; then use the parameter in dst_side_nat_modes or a sibling test to verify port-forwarding source lookups require NatRequirement::PortForwarding.flow-filter/src/context/tables.rs (2)
670-790: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the two-pass lookup into one generic helper.
The v4 and v6 arms are identical except for the table pair and the address type.
lookup_versionedalready proves the generic form works for the batch path. Alookup_versioned_single<I>(remote, local, ...)helper would remove the duplicated two-pass structure and keep the single-key oracle aligned with the batch path when either pass changes. This path is#[cfg(test)], so the change is optional.🤖 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/tables.rs` around lines 670 - 790, The IPv4 and IPv6 branches in Tables::lookup duplicate the same two-pass remote/local lookup logic. Optionally extract that logic into a generic lookup_versioned_single helper, modeled on lookup_versioned, parameterized by the address type and remote/local table pair, then have both arms delegate to it while preserving DestinationMiss, SourceMiss, and Route results.
924-952: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the shadowed retry bindings to record their index space.
The stage-1
revalidate_posholds positions inq_chunk. The stage-2revalidate_posholds positions inlocal_keys/nat_modes, which is a different index space. The second pair shadows the first inside the same loop body. The current indexing is correct, but the shared names make a future edit likely to mix the two spaces.♻️ Suggested renaming
- let mut revalidate_pos = Vec::new(); - let mut revalidate_keys = Vec::new(); + let mut revalidate_hit = Vec::new(); + let mut revalidate_local_keys = Vec::new(); for (pos, (nat_mode, &q_pos)) in nat_modes.iter().zip(hit_pos.iter()).enumerate() { let revalidated = q_chunk[q_pos].nat_mode; if nat_mode.is_none() && revalidated != 0 { - revalidate_pos.push(pos); - revalidate_keys.push(LocalKey { + revalidate_hit.push(pos); + revalidate_local_keys.push(LocalKey { nat_mode: revalidated, ..local_keys[pos].clone() }); } } - let mut revalidate_nat_modes = vec![None; revalidate_keys.len()]; - local.lookup_batch(&revalidate_keys, &mut revalidate_nat_modes); - for (pos, nat_mode) in revalidate_pos.into_iter().zip(revalidate_nat_modes) { - nat_modes[pos] = nat_mode; + let mut revalidate_nat_modes = vec![None; revalidate_local_keys.len()]; + local.lookup_batch(&revalidate_local_keys, &mut revalidate_nat_modes); + for (hit, nat_mode) in revalidate_hit.into_iter().zip(revalidate_nat_modes) { + nat_modes[hit] = nat_mode; }🤖 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/tables.rs` around lines 924 - 952, Rename the stage-2 retry bindings in the revalidation block to distinguish their local_keys/nat_modes index space from the stage-1 q_chunk positions, including the declaration, pushes, and final zip loop; preserve the existing indexing and behavior.
🤖 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/Cargo.toml`:
- Line 36: Remove the unused tracing-test dependency declaration from
flow-filter/Cargo.toml, leaving the remaining workspace dependencies unchanged.
In `@flow-filter/src/lib.rs`:
- Around line 339-342: Remove the redundant debug log from classify’s
inactive-flow branch. Keep the status check and return (None, None) unchanged,
relying on dst_vpcd_from_valid_flow to emit the existing “Packet has inactive
flow information” message.
---
Nitpick comments:
In `@flow-filter/src/context/tables.rs`:
- Around line 670-790: The IPv4 and IPv6 branches in Tables::lookup duplicate
the same two-pass remote/local lookup logic. Optionally extract that logic into
a generic lookup_versioned_single helper, modeled on lookup_versioned,
parameterized by the address type and remote/local table pair, then have both
arms delegate to it while preserving DestinationMiss, SourceMiss, and Route
results.
- Around line 924-952: Rename the stage-2 retry bindings in the revalidation
block to distinguish their local_keys/nat_modes index space from the stage-1
q_chunk positions, including the declaration, pushes, and final zip loop;
preserve the existing indexing and behavior.
In `@flow-filter/src/context/tests.rs`:
- Around line 27-60: The route_revalidate helper currently hardcodes nat_mode as
None, preventing source-side NAT revalidation tests. Add a nat-mode parameter to
route_revalidate, pass it through to context.lookup, and update route to supply
None; then use the parameter in dst_side_nat_modes or a sibling test to verify
port-forwarding source lookups require NatRequirement::PortForwarding.
In `@flow-filter/src/lib.rs`:
- Around line 333-354: Update revalidation_dst_vpcd to return both independent
hints when flow_summary.needs_masquerade and flow_summary.needs_port_forwarding
are true, preserving the destination VPC discriminant and
NatRequirement::PortForwarding together; avoid the mutually exclusive else-if
chain. Rename the function to reflect its tuple of revalidation hints and update
its call site in classify accordingly.
- Around line 464-478: Change NatRequirement::as_u8 to take self by value
instead of borrowing &self, preserving the existing variant-to-byte mapping and
allowing convert_option to call it directly on the owned requirement.
🪄 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: 44af8cb4-25c7-43ee-a24d-5241e7f6326a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
flow-filter/Cargo.tomlflow-filter/src/context/fuzz.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rs
qmonnet
force-pushed
the
pr/qmonnet/flow-filter-table
branch
from
August 6, 2026 09:08
5384435 to
c6e05ec
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
qmonnet
force-pushed
the
pr/qmonnet/flow-filter-table
branch
from
August 6, 2026 10:48
c6e05ec to
aecd774
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/lib.rs`:
- Around line 308-315: Update the revalidation-hint selection around
tag_for_bypass/set_nat_requirements so masquerade and port-forwarding hints are
computed independently and both are returned for combined NAT flows; do not let
the masquerade branch suppress NatRequirement::PortForwarding. Add a regression
test covering return traffic from a port-forwarded host to a masqueraded peer
and verify both hints prevent the established flow from being filtered after
configuration update.
🪄 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: ec8efa3c-5ee8-44ea-bdff-7651dbc6b3db
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
flow-filter/Cargo.tomlflow-filter/src/context/fuzz.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rsflow-filter/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- flow-filter/Cargo.toml
- flow-filter/src/fuzz_gen.rs
- flow-filter/src/context/tables.rs
- flow-filter/src/context/fuzz.rs
Reply traffic for masqueraded flows don't usually require a flow-table lookup, because the associated flow table entry allows us to bypass the lookup under normal conditions. However, if the flow has an old genid (this may happen if we bump the genid for the flow-filter pipeline stage between the flow lookup and the flow-filter processing for the packet, for example), then we need to do the lookup to re-validate the flow. The issue with this lookup is that we may have colliding entries, if multiple VPCs expose (via masquerade) overlapping prefixes to a given VPC. To avoid that, we add the destination VNI as part of the key in the remote lookup table. The destination VNI is only used for reply traffic for masqueraded flows; for all other entries, it remains at 0 (invalid VNI number). On finding a packet with outdated flow-info for masquerade, we may run two consecutive lookups: one using the destination VNI in the key, and if it fails, a second one without it, for forward traffic (we cannot use the destination VNI for forward traffic entries, or we'd be unable to validate packets initiating new flows and without flow information). Signed-off-by: Quentin Monnet <qmo@qmon.net>
A port-forwarding source cannot initiate a connection, so it was left out of the local table altogether. The reply direction of a forwarded session then gets no answer from the tables, when we try to re-validate it after a configuration update: it comes back as a source miss. Instead, add back the related entries to the local table; but also make the NAT mode part of the key to use it in that case (and only in the case of reply traffic for port-forwarding flows): we need it to dissociate these entries from potential colliding entries associated with overlapping masqueraded prefixes. Similarly to what we do for the remote lookup table, we have a two-step lookup: first look with the NAT mode, to catch an entry associated with port-forwarding reply traffic, then retry without it for "forward" traffic. Signed-off-by: Quentin Monnet <qmo@qmon.net>
The tables now hold rules that only a lookup carrying flow revalidation information can reach. The property tests still asked every question without it, so the config oracle described semantics the tables no longer have, and the probes derived to route by construction stopped routing. Those derived probes are also the only ones that reach the gated rules at all: the address, port and NAT mode a revalidated route needs never come up together by random generation. They now go through the batch path and the oracle too, not just the single lookup, and the run fails if it never resolved a route through revalidation. Also fix some comments still describing masquerade destinations and port-forwarding sources as marker-only or absent from the tables, and document the ordering. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/flow-filter-table
branch
from
August 6, 2026 16:07
aecd774 to
32f5d8a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
flow-filter/src/tests.rs (2)
780-801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the copied comment and remove the debug print.
Line 782 describes vpc2 and vpc3 exposing masqueraded prefixes. This test declares only vpc1 and vpc2, and the overlap is between local port-forwarding and masquerade exposures in vpc1. Line 801 prints the context on every run, which is a leftover debug artifact.
♻️ Proposed fix
let ctx = context( &[("vpc1", 100), ("vpc2", 200)], - // vpc2 and vpc3 both expose the same masqueraded prefixes towards the same vpc1 prefix + // vpc1 exposes the same prefix twice: port-forwarding on /24 and masquerade on the /25 vec![peering(- println!("{ctx}"); let (mut flow_filter, writer) = make_flow_filter(ctx);🤖 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 780 - 801, Correct the comment above the peering setup to describe the actual overlap between vpc1’s port-forwarding and masquerade exposures, and remove the println!("{ctx}") debug statement while retaining the test setup unchanged.
776-778: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
#[traced_test]attribute and the mid-file import.The test body never calls
logs_containorlogs_assert. The attribute therefore adds atracing-testdev-dependency and a per-test subscriber with no assertion value. If you keep the attribute, add a log assertion. If you do not need log assertions, remove the attribute, the import, and thetracing-testdev-dependency entry.♻️ Proposed cleanup
-use tracing_test::traced_test; -#[traced_test] #[test] fn revalidation_works_in_case_of_local_masquerade_portforwarding_overlap() {🤖 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 776 - 778, Remove the unused traced_test attribute and the mid-file tracing_test import from the affected test, and remove the tracing-test dev-dependency entry since no log assertions are used. Keep the test itself unchanged.
🤖 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/tests.rs`:
- Around line 931-943: Update the final port-forwarding scenario around
flow_filter to set the generation ID to 6 instead of 5, ensuring the flow is
outdated after the peering removal. In the attach_flow call for this packet, set
port_fw_state to true while preserving nat_state as true, matching the
established port-forwarding flow state.
---
Nitpick comments:
In `@flow-filter/src/tests.rs`:
- Around line 780-801: Correct the comment above the peering setup to describe
the actual overlap between vpc1’s port-forwarding and masquerade exposures, and
remove the println!("{ctx}") debug statement while retaining the test setup
unchanged.
- Around line 776-778: Remove the unused traced_test attribute and the mid-file
tracing_test import from the affected test, and remove the tracing-test
dev-dependency entry since no log assertions are used. Keep the test itself
unchanged.
🪄 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: f7a98731-84c6-4a64-b349-b0436a2f5778
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
flow-filter/Cargo.tomlflow-filter/src/context/fuzz.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rsflow-filter/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- flow-filter/Cargo.toml
- flow-filter/src/lib.rs
- flow-filter/src/context/fuzz.rs
- flow-filter/src/fuzz_gen.rs
- flow-filter/src/context/tests.rs
- flow-filter/src/context/tables.rs
qmonnet
force-pushed
the
pr/qmonnet/flow-filter-table
branch
from
August 6, 2026 16:15
32f5d8a to
6ab2cf2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
flow-filter/src/tests.rs (1)
776-779: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
traced_testinstrumentation, or assert on the captured logs.
#[traced_test]installs a tracing subscriber for this test, but no assertion inspects the output. The attribute therefore adds a dependency and setup cost with no verification value. Theusestatement also sits between two test functions instead of at the top of the module.If you intend to verify revalidation log output, add a
logs_contain(...)assertion. If not, remove both lines.♻️ Proposed cleanup
-use tracing_test::traced_test; -#[traced_test] #[test] fn revalidation_works_in_case_of_local_masquerade_portforwarding_overlap() {🤖 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 776 - 779, Remove the unused traced_test import and #[traced_test] attribute from revalidation_works_in_case_of_local_masquerade_portforwarding_overlap unless the test is updated to assert captured logs with logs_contain(...); keep the import organized with the module’s other imports if instrumentation is retained.
🤖 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/tests.rs`:
- Around line 945-953: Update the final masquerade packet in the test around the
existing flow setup to target the same public address and port used by the
earlier vpc2-to-vpc1 replies, preserving the established 5-tuple. Keep the
cancellation and filtering assertions unchanged so they verify cancellation of
the previously validated masquerade flow rather than an unrelated packet.
- Around line 780-802: Update the comment in the test context setup to describe
the actual vpc1 local port-forwarding versus local masquerade overlap, rather
than the unrelated vpc2/vpc3 scenario. Remove the debug println!(“{ctx}”) call
before make_flow_filter.
---
Nitpick comments:
In `@flow-filter/src/tests.rs`:
- Around line 776-779: Remove the unused traced_test import and #[traced_test]
attribute from
revalidation_works_in_case_of_local_masquerade_portforwarding_overlap unless the
test is updated to assert captured logs with logs_contain(...); keep the import
organized with the module’s other imports if instrumentation is retained.
🪄 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: fb7b0388-2344-4543-b222-5f584d66fec7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
flow-filter/Cargo.tomlflow-filter/src/context/fuzz.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rsflow-filter/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- flow-filter/Cargo.toml
- flow-filter/src/lib.rs
- flow-filter/src/fuzz_gen.rs
- flow-filter/src/context/fuzz.rs
- flow-filter/src/context/tests.rs
- flow-filter/src/context/tables.rs
Make sure flow revalidation when flow info is outdated (with regards to flow-filter's genid) behaves as we expect, in particular for the two following (distinct) cases: - We have overlapping, masqueraded prefixes exposed by multiples VPCs to a given VPC - We have overlapping prefixes for port-forwarding and masquerading, within a given manifest. For both port-forwarding and masquerade in this setting, check the behaviour for the initial packet, follow-up replies in both directions; then update the genid and make sure that we still resolve for the different packets as expected. Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/flow-filter-table
branch
from
August 6, 2026 16:21
6ab2cf2 to
93684c1
Compare
We recently added the destination VNI as part of the key in the remote table for the flow-filter lookup, to use only in the case when we want to revalidate the flow for reply masqueraded traffic with outdated flow information. Rather than using a u32 (and 0 when we don't want to use the VNI), use an Option<Vni>, wrapped withing a GateVni type so we can implement FixedSize for it. No functional change. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
We used to represent the NAT mode in the local table key with a u8; use a dedicated "gate" instead (a flag to turn on and off the lookups for entries for reply traffic with port forwarding for revalidating flows with outdated information, in the local-side context table), similarly to what we did with GateVni for the remote table. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
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.
This is one proposed solution for #1694.
It needs a bit of polishing (variables names need a second pass) and some flow-filter unit tests probably need to be adjusted to better validate the new logic, but at the moment it seems to be working, with tests passing, and it's a (relatively) small change-set (fuzzing tests set aside). It's also self-contained in the flow-filter tables build/lookup logic, which is nice given that we intend to replace this eventually so it avoids spreading modifications for flow re-validation in other crates.