Open
Conversation
Keep the main logic at the top. This brings methods bypass_with_flow_info() and check_packet_flow_info() nearer to similar functions, lower down in the file, and keeps process_packet() with the core logic near the top of the file. Signed-off-by: Quentin Monnet <qmo@qmon.net>
For consistency with the other logs in the rest of the file, use the network function's name in logs from bypass_with_flow_info(). Signed-off-by: Quentin Monnet <qmo@qmon.net>
Make the function a method of struct FlowFilter, for consistency with similar functions called by FlowFilter.process_packet(). Signed-off-by: Quentin Monnet <qmo@qmon.net>
Keep restructuring the code in flow-filter/src/lib.rs, by moving processing function into FlowFilter's implementation. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Fix comments: there's no such thing as "port masquerading", it's supposed to be "port forwarding". Reported-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
When looking for an existing flow to determine where to send a packet, in the case when the flow-filter lookup returned an ambiguous result, we would discard the packet on finding an expired flow. This is incorrect: if the flow is expired, then we cannot use it to determine the destination VPC, but this is not a reason for dropping the packet. Instead we need to carry on and try to determine the destination VPC based on other means, potentially going through stateful NAT as a follow-up step and recreating an up-to-date flow. Also move the status check closer to the flow availability check itself. No need to attempt to take the lock to retrieve the destination VPC if the flow has expired. Fixes: 2eb6274 ("feat(flow-filter): Update logic for port forwarding + masquerade overlap") Signed-off-by: Quentin Monnet <qmo@qmon.net>
In flow-filter's main logic, both functions check_packet_flow_info() and bypass_with_flow_info() check for the availability, and then for the status of flow information for the packet being processed. We have a similar check on flow information availability in set_nat_requirements_from_flow_info(), where we should in fact check for the status as well. And we may yet add another similar occurrence of these checks in future work. Let's move the two checks to a dedicated method from struct Packet in an effort to remove duplicated code. Signed-off-by: Quentin Monnet <qmo@qmon.net>
In an effort to de-duplicate some of the code in the flow-filter processing, move the code to extract and cast the destination VPC discriminant from packet's flow information to a dedicated function that can be called in both check_packet_flow_info() and bypass_with_flow_info(). Signed-off-by: Quentin Monnet <qmo@qmon.net>
We already implement Display for FlowFilterTable, we don't need it for the network function object itself. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Extract the flow-info, and potentially the destination VPC discriminant it contains, only once at the beginning of the flow-filter processing. This avoids repeated checks on the availability and status of the flow-object, and acquiring multiple times the lock for looking at the inner destination VPC information. Suggested-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/cleanup-flow-filter-lib
branch
from
c1c3a06 to
63e20c2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a refactor/cleanup of the flow-filter stage’s flow-info handling, extracting common “active flow-info” logic into net::packet::Packet and reorganizing flow-filter helpers for bypassing and NAT requirement derivation.
Changes:
- Add
Packet::active_flow_info()helper innetto centralize “flow-info exists and is Active” checks. - Refactor
flow-filterto use the new helper, reorganizing the bypass and multiple-match handling paths. - Move NAT-requirements-setting helpers into
FlowFilterimpl and streamline call sites.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
net/src/packet/mod.rs |
Adds active_flow_info() helper for retrieving only Active flow-info entries. |
flow-filter/src/lib.rs |
Refactors flow-info usage to use active_flow_info(), adjusts bypass logic, and reorganizes NAT requirement helpers. |
Comments suppressed due to low confidence (1)
flow-filter/src/lib.rs:29
confidence: 9
tags: [logic]
`FlowInfo`/`Packet::active_flow_info()` uses `concurrency::sync::Arc` (which becomes `shuttle::sync::Arc` under the workspace’s `shuttle` feature), but this file imports `std::sync::Arc` and now uses it in `Option<&Arc<FlowInfo>>` signatures. That will fail to typecheck (and likely to compile) when building with `--features shuttle` (e.g., via `nat`’s shuttle tests), because `PacketMeta.flow_info` is `Option<concurrency::sync::Arc<FlowInfo>>`.
Use `concurrency::sync::Arc` for `FlowInfo`-related arcs here (you may need to add a direct dependency on `concurrency` in `flow-filter/Cargo.toml`), and if you still need `std::sync::Arc` for `PipelineData`, alias one of them (e.g., `StdArc`).
use std::collections::HashSet;
use std::fmt::Display;
use std::net::IpAddr;
use std::num::NonZero;
use std::sync::Arc;
use tracing::{debug, error};
</details>
---
You can also share your feedback on Copilot code review. [Take the survey](https://www.surveymonkey.com/r/XP6L3XJ).
Comment on lines
+363
to
+368
| /// Panic if the lock has been poisoned. | ||
| fn dst_vpcd_from_flow_info(flow_info: Option<&Arc<FlowInfo>>) -> Option<VpcDiscriminant> { | ||
| flow_info? | ||
| .locked | ||
| .read() | ||
| .unwrap() |
Member
Author
There was a problem hiding this comment.
I've been wondering about that, but my understanding is that it's preferable to panic if the lock is poisoned because it means we're doing something very wrong anyway?
Contributor
There was a problem hiding this comment.
We panic in other cases (unwraps()). We should make a decision about this and unify.
For instance, in set_nat_requirements_from_flow_info() you don't panic but err.
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.
Split off from #1341.
Please refer to individual commit logs for details.