Fix issues with config update and flow filter/flow table invalidation - #1691
Open
Fredi-raspall wants to merge 6 commits into
Open
Fix issues with config update and flow filter/flow table invalidation#1691Fredi-raspall wants to merge 6 commits into
Fredi-raspall wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe PR changes route results to a named ChangesFlow-filter routing and lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts flow-filter’s handling of stateful NAT flows (especially masquerade) during config generation changes, and refactors the routing lookup return type from a tuple into a named Route struct to improve clarity and reduce tuple destructuring.
Changes:
- Refactor
FlowFilterContextrouting results to return aRoutestruct (dst_vpcd,dst_nat_mode,src_nat_mode) instead of a(VpcDiscriminant, NatMode, NatMode)tuple. - Modify flow-bypass logic to allow masquerade flows to bypass the flow-filter even when their flow
genidis older than the pipelinegenid. - Update/add tests to reflect the new masquerade/port-forwarding behaviors across config updates/removals.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| flow-filter/src/lib.rs | Changes bypass logic for flows and updates lookup-result handling for the new Route struct; adjusts logging for missing dst_vpcd. |
| flow-filter/src/tests.rs | Updates existing tests and adds a new port-forwarding genid/config-removal test; updates expectations for masquerade behavior. |
| flow-filter/src/context/tests.rs | Updates context tests to use the new Route struct fields (*_nat_mode). |
| flow-filter/src/context/tables.rs | Introduces the Route struct and updates lookup code to return it. |
| flow-filter/src/context/fuzz.rs | Updates fuzz oracle to return LookupResult::Route(Route::new(...)). |
Comment on lines
669
to
670
| // The peering is gone from the new config: even an active, state-consistent flow must not let | ||
| // reply traffic through (stage 1 finds no marker to trust), and the flow pair is invalidated. |
Comment on lines
342
to
346
| if flow_genid < genid && !flow_summary.needs_masquerade { | ||
| // If a packet belongs to a masqueraded flow, we have to let it through, temporarily, even if the | ||
| // flow is out-dated in terms of generation Id: the flow filter does not have the knowledge to | ||
| // forbid that flow. That's the responsibility of the masquerade stage. | ||
| debug!( |
Comment on lines
+629
to
+633
| // The packet hits a flow that is masquerading. The flow-filter will not set the verdict but be bypassed. | ||
| let flow = attach_flow(&mut p, Some(vpcd(300)), true, true, false); | ||
| let out = run(&mut flow_filter, p); | ||
| assert_eq!(out.get_done(), Some(DoneReason::Filtered)); | ||
| assert_eq!(flow.status(), FlowStatus::Cancelled); | ||
| assert_eq!(out.get_done(), None); | ||
| assert_eq!(flow.status(), FlowStatus::Active); |
| /// results can be extracted and the context guard dropped before packet metadata is mutated. | ||
| type Route = (VpcDiscriminant, NatMode, NatMode); | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flow-filter/src/tests.rs (1)
668-682: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale test name and comment contradict the new masquerade-flow behavior.
The masquerade flow behavior changed to remain active across destination and configuration mismatches, but two spots in this file still describe or imply the old "filtered/invalidated" behavior. This creates a mismatch between documentation and the actual test assertions, and can mislead future readers debugging failures here.
flow-filter/src/tests.rs#L668-L682: Update the comment at lines 669-670 to state that the masquerade flow remains active and reply traffic bypasses the filter after peering removal, matching the assertions at lines 680-681. The current text ("must not let reply traffic through ... flow pair is invalidated") is the opposite of what the test verifies.flow-filter/src/tests.rs#L622-L634: Renamemasquerade_reply_with_mismatched_flow_destination_is_filteredto reflect the bypass outcome (for example,masquerade_reply_with_mismatched_flow_destination_bypasses_filter), consistent with the sibling rename applied at line 668.🤖 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 668 - 682, The test documentation contradicts the actual behavior across two locations. At flow-filter/src/tests.rs lines 669-670 (anchor), update the comment for masquerade_flow_is_left_untouched_on_config_removal to state that the masquerade flow remains active and reply traffic bypasses the filter after peering removal, removing the incorrect text that says "must not let reply traffic through" and "flow pair is invalidated" which contradicts the assertions at lines 680-681. At flow-filter/src/tests.rs lines 622-634 (sibling), rename the test function masquerade_reply_with_mismatched_flow_destination_is_filtered to a name that reflects the bypass outcome (such as masquerade_reply_with_mismatched_flow_destination_bypasses_filter) to align with the corrected behavior description at the anchor site.
🤖 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.
Outside diff comments:
In `@flow-filter/src/tests.rs`:
- Around line 668-682: The test documentation contradicts the actual behavior
across two locations. At flow-filter/src/tests.rs lines 669-670 (anchor), update
the comment for masquerade_flow_is_left_untouched_on_config_removal to state
that the masquerade flow remains active and reply traffic bypasses the filter
after peering removal, removing the incorrect text that says "must not let reply
traffic through" and "flow pair is invalidated" which contradicts the assertions
at lines 680-681. At flow-filter/src/tests.rs lines 622-634 (sibling), rename
the test function masquerade_reply_with_mismatched_flow_destination_is_filtered
to a name that reflects the bypass outcome (such as
masquerade_reply_with_mismatched_flow_destination_bypasses_filter) to align with
the corrected behavior description at the anchor site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf865b71-c182-455d-b3ae-50d6e9cc1d6d
📒 Files selected for processing (5)
flow-filter/src/context/fuzz.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/lib.rsflow-filter/src/tests.rs
mvachhar
changed the title
Contributor
|
Does this deal with the port forward case too? |
Contributor
|
Supercedes #1689 |
If a packet hits a valid flow whose generation id does not match the current, and the flow is masquerading, let the packet use that flow even if outdated. Otherwise, the flow filter would drop packets belonging to still-allowed flows unnecessarily. The flow filter does not have the knowledge that those flows should be allowed or not. It is the masquerade stage that is responsible for that. Also, rename variable and change debug! for error! in a case that should never occur. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Avoids ordering issues in NatMode and duplicated test type. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Flows live in pairs in the flow table. When exporting them, we will want to export them "in pairs", because having only one of them is of little use to forward the associated traffic. E.g. if flows F1 and F2 are related, we may export some (F1,F2). However, there is no notion of "forward" or "reverse". Both F1 and F2 are currently equally important. This creates a problem to export them in pairs: we may create some export object for pair (F1, F2) when scanning the flow table, but then need a way to know that (F2, F1) needs not be exported, to avoid sending the information twice. To solve this, we add a flag "master" to each flow and make sure that the flag is only set for one of the flows in a pair. This way, we only need to export "master" flows. The "master" flag has no semantic other than that, but could always be set to the flow corresponding to the packet that triggered the creation of the flow pair. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Summary of changes (conceptual):
* let stage-1 not have entries where remote requires masquerade
as that would provide multiple answers due to overlap.
* let stage-2 have all (local) entries, including port-forwarding
* revisit flow-filter logic:
- packets with no flow or non-active flow are always checked.
- only packets with active, up-to-date flows can bypass filter.
- packets with active but non-up-to-date flows cannot bypass
since we don't know if the flow would still be valid with a
new configuration. So, the packet/flow needs to be re-evaluated.
As before, the re-evaluation may not be complete because the
flow filter does not have full visibility to the NF state.
But it should disqualify when it is possible to ensure that
packets are delivered to the intended recipient and marked to
get the treatment according to the configuration.
- A packet may not always allow us to validate a flow, because
it may come from the direction where the flow-filter lacks
the knowledge to unambiguosly tell what to do. In those cases,
when attempting to validate an existing flow created under
a prior config, we ask the flow filter to tell us if the
packet that would have initiated that flow would still be
allowed; instead of checking the packet that we received.
If that prior packet would be allowed and the flow that the
received packet rides on now is compatible with that verdict,
the flow is considered valid and guided to the respective NFs,
which have the last word to tell if the flow is valid or not.
There is no duplication here: the flow-filter has to determine
what NFs need to process the packet.
Code changes:
- removed the zipping of the two iterators and use a single index.
- use the flowsummary to annotate if the flow is "master" (initiator)
and let it include src vpcd.
- add logic to build input lookup key from master flow when needed.
- propagate the input key to the result processing logic.
- rework the logic to accept a flow/packet depending on the case.
- Adapt fuzzer, tests and some table utils (Claude).
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/flow-filter-fix
branch
from
August 6, 2026 16:03
6e68434 to
a3b3183
Compare
Fredi-raspall
added
ci:+release
Fredi-raspall
marked this pull request as ready for review
August 6, 2026 16:09
Fredi-raspall
requested review from
daniel-noland
and removed request for
a team
August 6, 2026 16:09
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
flow-filter/src/tests.rs:877
confidence: 8
tags: [style]
This test prints the full table to stdout unconditionally; that’s noisy in CI. Reuse `show_flow_filter(&flow_filter)` (which can be gated) or remove the print.
let table = flow_filter.tables.load();
println!("{table}");
**flow-filter/src/tests.rs:283**
* ```yaml
confidence: 8
tags: [style]
show_flow_filter prints the full tables unconditionally, which makes cargo test noisy and can hide real failures in CI logs. Consider only emitting this dump when explicitly requested (e.g. via an env var), or switch to tracing::debug! behind a filter.
This issue also appears on line 876 of the same file.
fn show_flow_filter(flow_filter: &FlowFilter) {
let tables = flow_filter.tables.load();
println!("{tables}");
}
flow-filter/src/tests.rs:833
confidence: 9
tags: [style]
Typo in comment: “droppped” → “dropped”.
// no flow with masquerade, packet is droppped
**flow-filter/src/lib.rs:214**
* ```yaml
confidence: 8
tags: [style]
Spelling/grammar typos in this comment block (“possiblities”, “can’t'”) make the explanation harder to read.
// If the packet matched an active flow, there are two possiblities:
// 1) the flow is up-to-date (in terms of genid): the packet can bypass the flow filter confidently.
// 2) the flow is not up-to-date: the packet can't' bypass the flow filter since we don't know if
// the flow should still be allowed nor the current treatment.
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
|
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 (5)
net/src/flows/flow_info.rs (1)
149-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTake
selfby value for consistency with the sibling accessors.
FlowInfoFlagsisCopy.requires_static_nat_srcandrequires_static_nat_dsttakeself.is_pair_mastertakes&self. Use the same receiver.♻️ Proposed change
- pub const fn is_pair_master(&self) -> bool { + pub const fn is_pair_master(self) -> bool { self.contains(FlowInfoFlags::IS_PAIR_MASTER) }🤖 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 `@net/src/flows/flow_info.rs` around lines 149 - 154, Update FlowInfoFlags::is_pair_master to take self by value instead of &self, matching the receiver style of requires_static_nat_src and requires_static_nat_dst while preserving its existing contains check.flow-filter/src/tests.rs (2)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the referenced function name.
The comment names
lookup_input_from_flow. The function inflow-filter/src/lib.rsisbuild_lookup_key_from_flow.🤖 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 42 - 45, Update the comment describing the flow pair to reference build_lookup_key_from_flow instead of lookup_input_from_flow, leaving the rest of the explanation unchanged.
856-869: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test names after the semantics change. Three test names still describe the previous behavior, while the assertions describe the new behavior. Rename each test to match what it now asserts.
flow-filter/src/tests.rs#L856-L869: renamemasquerade_reply_with_mismatched_flow_destination_is_filtered; the body assertsget_done() == NoneandFlowStatus::Active, which is a bypass.flow-filter/src/tests.rs#L649-L663: renameoutdated_flow_missing_port_forwarding_state_is_invalidated, or restore an assertion on the flow status; the body no longer asserts cancellation.flow-filter/src/context/tests.rs#L509-L510: renamesource_port_forwarding_is_excluded_and_falls_back_to_masquerade; port-forwarding sources are now installed in the lowest-priority band rather than excluded.🤖 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 856 - 869, Rename the three stale tests to match their current semantics: in flow-filter/src/tests.rs:856-869, rename masquerade_reply_with_mismatched_flow_destination_is_filtered to describe bypass with an active flow; in flow-filter/src/tests.rs:649-663, rename outdated_flow_missing_port_forwarding_state_is_invalidated to reflect the asserted behavior unless cancellation is restored; and in flow-filter/src/context/tests.rs:509-510, rename source_port_forwarding_is_excluded_and_falls_back_to_masquerade to indicate installation in the lowest-priority band rather than exclusion.flow-filter/src/context/display.rs (1)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the absent-NAT placeholder with the existing table dump.
nat_mode_labelrenders an absent NAT mode as--. The existing helpernat_modeat Line 145 renders the same condition as-. An operator reads both strings in the same CLI output. Use one placeholder for both.♻️ Proposed change
fn nat_mode_label(mode: crate::NatMode) -> &'static str { match mode { Some(nat) => nat.label(), - None => "--", + None => "-", } }🤖 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/display.rs` around lines 27 - 32, Update nat_mode_label to use the same absent-NAT placeholder as the existing nat_mode helper: return "-" instead of "--" for None, while preserving the label returned for Some(nat).flow-filter/src/context/mod.rs (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a crate-visible re-export for
Route.
contextis private, so external crates cannot access this re-export. Changepub use tables::{FlowFilterContext, Route};topub(crate) use ...to matchRoute’s crate-only API.🤖 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/mod.rs` at line 19, Change the re-export in the context module from public to crate-visible by using pub(crate) for Route (and the accompanying FlowFilterContext re-export), matching Route’s crate-only API.
🤖 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 137-158: Update build_lookup_key_from_flow to handle a missing
source VPC discriminant safely: replace the key.src_vpcd().unwrap_or_else(||
unreachable!()) assumption with an Option-based early return of None. Preserve
the existing LookupInput construction for keys that include src_vpcd so classify
can drop packets only when the input is unavailable.
- Around line 313-326: Move the port-forwarding eligibility check in the
route-processing flow before the destination annotation and NAT requirement
updates in the surrounding method. Ensure packets dropped by the
has_active_pfw_flow check are marked filtered and returned without populating
packet.meta_mut().dst_vpcd, while preserving annotation behavior for accepted
packets.
---
Nitpick comments:
In `@flow-filter/src/context/display.rs`:
- Around line 27-32: Update nat_mode_label to use the same absent-NAT
placeholder as the existing nat_mode helper: return "-" instead of "--" for
None, while preserving the label returned for Some(nat).
In `@flow-filter/src/context/mod.rs`:
- Line 19: Change the re-export in the context module from public to
crate-visible by using pub(crate) for Route (and the accompanying
FlowFilterContext re-export), matching Route’s crate-only API.
In `@flow-filter/src/tests.rs`:
- Around line 42-45: Update the comment describing the flow pair to reference
build_lookup_key_from_flow instead of lookup_input_from_flow, leaving the rest
of the explanation unchanged.
- Around line 856-869: Rename the three stale tests to match their current
semantics: in flow-filter/src/tests.rs:856-869, rename
masquerade_reply_with_mismatched_flow_destination_is_filtered to describe bypass
with an active flow; in flow-filter/src/tests.rs:649-663, rename
outdated_flow_missing_port_forwarding_state_is_invalidated to reflect the
asserted behavior unless cancellation is restored; and in
flow-filter/src/context/tests.rs:509-510, rename
source_port_forwarding_is_excluded_and_falls_back_to_masquerade to indicate
installation in the lowest-priority band rather than exclusion.
In `@net/src/flows/flow_info.rs`:
- Around line 149-154: Update FlowInfoFlags::is_pair_master to take self by
value instead of &self, matching the receiver style of requires_static_nat_src
and requires_static_nat_dst while preserving its existing contains check.
🪄 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: e2b33f15-bdec-4237-82fe-4ea2f71c7dec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
flow-filter/Cargo.tomlflow-filter/src/context/display.rsflow-filter/src/context/fuzz.rsflow-filter/src/context/mod.rsflow-filter/src/context/tables.rsflow-filter/src/context/tests.rsflow-filter/src/fuzz_gen.rsflow-filter/src/lib.rsflow-filter/src/tests.rsnet/src/flows/flow_info.rs
Comment on lines
+137
to
+158
| fn build_lookup_key_from_flow(summary: &FlowSummary) -> Option<LookupInput> { | ||
| let flow_info = summary.flow_info.related.as_ref().and_then(Weak::upgrade)?; | ||
| if !flow_info.get_flags().is_pair_master() { | ||
| error!("Related flow of a non-master flow is not the master. This is a bug"); | ||
| return None; | ||
| } | ||
| // related flow could be inactive (unlikely) | ||
| if flow_info.status() != FlowStatus::Active { | ||
| debug!("Won't use related flow: it is not active"); | ||
| return None; | ||
| } | ||
| let key = flow_info.flowkey(); | ||
| let input = LookupInput { | ||
| src_vpcd, | ||
| src_vpcd: key.src_vpcd().unwrap_or_else(|| unreachable!()), | ||
| src_ip: *key.src_ip(), | ||
| dst_ip: *key.dst_ip(), | ||
| proto: key.proto(), | ||
| ports: key.ports().map(|(s, d)| (s.get(), d.get())), | ||
| }; | ||
| debug!("Will validate flow {key}"); | ||
| Some(input) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not assume the related master key carries a source VPC discriminant.
Line 150 unwraps key.src_vpcd() with unreachable!(). The key here belongs to the related master flow, not to the flow the packet matched. FlowSummary::from_flow_info validates src_vpcd only for the attached flow, so nothing in this path proves the master key carries one. A master flow created without a source VPC discriminant panics the data path.
Return None instead and let classify drop the packet, which it already does for a missing input.
🛡️ Proposed fix
let key = flow_info.flowkey();
+ let Some(src_vpcd) = key.src_vpcd() else {
+ error!("Related master flow has no src vpc discriminant. This is a bug");
+ return None;
+ };
let input = LookupInput {
- src_vpcd: key.src_vpcd().unwrap_or_else(|| unreachable!()),
+ src_vpcd,
src_ip: *key.src_ip(),
dst_ip: *key.dst_ip(),
proto: key.proto(),
ports: key.ports().map(|(s, d)| (s.get(), d.get())),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn build_lookup_key_from_flow(summary: &FlowSummary) -> Option<LookupInput> { | |
| let flow_info = summary.flow_info.related.as_ref().and_then(Weak::upgrade)?; | |
| if !flow_info.get_flags().is_pair_master() { | |
| error!("Related flow of a non-master flow is not the master. This is a bug"); | |
| return None; | |
| } | |
| // related flow could be inactive (unlikely) | |
| if flow_info.status() != FlowStatus::Active { | |
| debug!("Won't use related flow: it is not active"); | |
| return None; | |
| } | |
| let key = flow_info.flowkey(); | |
| let input = LookupInput { | |
| src_vpcd, | |
| src_vpcd: key.src_vpcd().unwrap_or_else(|| unreachable!()), | |
| src_ip: *key.src_ip(), | |
| dst_ip: *key.dst_ip(), | |
| proto: key.proto(), | |
| ports: key.ports().map(|(s, d)| (s.get(), d.get())), | |
| }; | |
| debug!("Will validate flow {key}"); | |
| Some(input) | |
| } | |
| fn build_lookup_key_from_flow(summary: &FlowSummary) -> Option<LookupInput> { | |
| let flow_info = summary.flow_info.related.as_ref().and_then(Weak::upgrade)?; | |
| if !flow_info.get_flags().is_pair_master() { | |
| error!("Related flow of a non-master flow is not the master. This is a bug"); | |
| return None; | |
| } | |
| // related flow could be inactive (unlikely) | |
| if flow_info.status() != FlowStatus::Active { | |
| debug!("Won't use related flow: it is not active"); | |
| return None; | |
| } | |
| let key = flow_info.flowkey(); | |
| let Some(src_vpcd) = key.src_vpcd() else { | |
| error!("Related master flow has no src vpc discriminant. This is a bug"); | |
| return None; | |
| }; | |
| let input = LookupInput { | |
| src_vpcd, | |
| src_ip: *key.src_ip(), | |
| dst_ip: *key.dst_ip(), | |
| proto: key.proto(), | |
| ports: key.ports().map(|(s, d)| (s.get(), d.get())), | |
| }; | |
| debug!("Will validate flow {key}"); | |
| Some(input) | |
| } |
🤖 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 137 - 158, Update
build_lookup_key_from_flow to handle a missing source VPC discriminant safely:
replace the key.src_vpcd().unwrap_or_else(|| unreachable!()) assumption with an
Option-based early return of None. Preserve the existing LookupInput
construction for keys that include src_vpcd so classify can drop packets only
when the input is unavailable.
Comment on lines
+313
to
+326
| // Annotate destination and requirements in packet | ||
| packet.meta_mut().dst_vpcd = Some(route.dst_vpcd); | ||
| Self::set_nat_requirements(packet.meta_mut(), route.src_nat_mode, route.dst_nat_mode); | ||
|
|
||
| // if originator requires port-forwarding and the packet has no active port-forwarding flow, | ||
| // drop the packet since port-forwarding should not initiate flows. | ||
| if route.src_nat_mode == Some(NatRequirement::PortForwarding) | ||
| && !has_active_pfw_flow(flow_summary) | ||
| { | ||
| debug!("{nfi}: dropping packet without active port-forwarding flow"); | ||
| packet.done(DoneReason::Filtered); | ||
| packet.invalidate_flows(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear the destination annotation when the port-forwarding check drops the packet.
Line 314 sets packet.meta_mut().dst_vpcd before the port-forwarding check. If has_active_pfw_flow returns false, the packet is marked DoneReason::Filtered while dst_vpcd stays populated. The packet is then both resolved and done. burst_processing_upholds_structural_invariants in flow-filter/src/tests.rs asserts the opposite (resolved XOR done); the current generator does not produce a port-forwarding source route, so the violation is not caught today.
Move the check before the annotation.
🐛 Proposed fix
- // Annotate destination and requirements in packet
- packet.meta_mut().dst_vpcd = Some(route.dst_vpcd);
- Self::set_nat_requirements(packet.meta_mut(), route.src_nat_mode, route.dst_nat_mode);
-
// if originator requires port-forwarding and the packet has no active port-forwarding flow,
// drop the packet since port-forwarding should not initiate flows.
if route.src_nat_mode == Some(NatRequirement::PortForwarding)
&& !has_active_pfw_flow(flow_summary)
{
debug!("{nfi}: dropping packet without active port-forwarding flow");
packet.done(DoneReason::Filtered);
packet.invalidate_flows();
return;
}
+
+ // Annotate destination and requirements in packet
+ packet.meta_mut().dst_vpcd = Some(route.dst_vpcd);
+ Self::set_nat_requirements(packet.meta_mut(), route.src_nat_mode, route.dst_nat_mode);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Annotate destination and requirements in packet | |
| packet.meta_mut().dst_vpcd = Some(route.dst_vpcd); | |
| Self::set_nat_requirements(packet.meta_mut(), route.src_nat_mode, route.dst_nat_mode); | |
| // if originator requires port-forwarding and the packet has no active port-forwarding flow, | |
| // drop the packet since port-forwarding should not initiate flows. | |
| if route.src_nat_mode == Some(NatRequirement::PortForwarding) | |
| && !has_active_pfw_flow(flow_summary) | |
| { | |
| debug!("{nfi}: dropping packet without active port-forwarding flow"); | |
| packet.done(DoneReason::Filtered); | |
| packet.invalidate_flows(); | |
| return; | |
| } | |
| // if originator requires port-forwarding and the packet has no active port-forwarding flow, | |
| // drop the packet since port-forwarding should not initiate flows. | |
| if route.src_nat_mode == Some(NatRequirement::PortForwarding) | |
| && !has_active_pfw_flow(flow_summary) | |
| { | |
| debug!("{nfi}: dropping packet without active port-forwarding flow"); | |
| packet.done(DoneReason::Filtered); | |
| packet.invalidate_flows(); | |
| return; | |
| } | |
| // Annotate destination and requirements in packet | |
| packet.meta_mut().dst_vpcd = Some(route.dst_vpcd); | |
| Self::set_nat_requirements(packet.meta_mut(), route.src_nat_mode, route.dst_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/lib.rs` around lines 313 - 326, Move the port-forwarding
eligibility check in the route-processing flow before the destination annotation
and NAT requirement updates in the surrounding method. Ensure packets dropped by
the has_active_pfw_flow check are marked filtered and returned without
populating packet.meta_mut().dst_vpcd, while preserving annotation behavior for
accepted packets.
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.
No description provided.