Open
Conversation
We want to make the code for the flow-filter tests less verbose. Add some helpers to determine what NAT mode is required for a packet exiting the flow-filter stage. Contrary to the existing methods for metadata .requires_...(), these helpers check that a given flag is set but also that flags for other modes are not set. This avoids checking three different requirements each time we assess a packet in tests. Signed-off-by: Quentin Monnet <qmo@qmon.net>
We want to make the code for the flow-filter tests less verbose. To that end, we condense the assessment of the packets processed by the flow-filter, in flow-filter tests: we directly assume there's only one packet in the output and don't validate the length, making the code slightly simpler and easier to read. Given that it's less dense, we also remove some blank lines to condense the tests more. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the flow-filter crate’s unit tests by moving the large #[cfg(test)] mod tests block out of lib.rs into a dedicated tests.rs module and introducing small helper utilities to reduce verbosity. As part of making port-forwarding tests less ad-hoc, it also extends VpcExpose::make_port_forwarding to accept an optional L4 protocol and updates call sites accordingly.
Changes:
- Move
flow-filtertests fromflow-filter/src/lib.rsintoflow-filter/src/tests.rsand simplify assertions/helpers. - Extend
VpcExpose::make_port_forwardingto acceptproto: Option<L4Protocol>and apply it tonat.proto. - Update all in-repo call sites/tests to the new
make_port_forwardingsignature.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| nat/src/stateful/apalloc/setup.rs | Updates test call sites to new make_port_forwarding(None, None) signature. |
| flow-filter/src/tests.rs | New test module containing the moved/condensed flow-filter tests. |
| flow-filter/src/setup.rs | Updates test call sites to new make_port_forwarding(None, None) signature. |
| flow-filter/src/lib.rs | Removes inline tests module and wires in #[cfg(test)] mod tests;. |
| config/src/external/overlay/vpcpeering.rs | Updates VpcExpose::make_port_forwarding API to accept optional protocol and sets nat.proto. |
| config/src/converters/k8s/config/expose.rs | Updates converter call site to pass the new proto argument. |
We want to make the code for the flow-filter tests less verbose. Let's make it possible to specify the L4 protocol to use for port forwarding rules with the VpcExpose method .make_port_forwarding(). This way, we can pass expose definitions inline to peering_table.add(), even if we need to define a port forwarding block for TCP only or UDP only. It's more consistent, and less verbose. Signed-off-by: Quentin Monnet <qmo@qmon.net>
The tests have grown quite a lot in flow-filter/src/lib.rs and it is not very convenient to have such a large portion of the file occupied by tests. Let's move them to a separate file. Submodules retain their own tests. Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/simplify-flow-filter-tests
branch
from
d4b9ba6 to
7d931a9
Compare
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.
Tests in flow-filter's lib.rs have grown quite a lot, I tried to "condense" them a little by making them slightly less verbose. They were still taking a huge portion of the file, so I also moved them to a separate file.
There's no functional change (even in tests), although we do add an argument to
.make_port_forwarding().