Update config validation (peering, expose), add tests#1326
Merged
Fredi-raspall merged 8 commits intomainfrom Mar 13, 2026
Merged
Update config validation (peering, expose), add tests#1326Fredi-raspall merged 8 commits intomainfrom
Fredi-raspall merged 8 commits intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates overlay config validation around VpcExpose/peering semantics (per #1150) and adds a large dedicated validation test suite to lock in the intended API behavior.
Changes:
- Relax/adjust
VpcExposevalidation: allow root prefixes inips/as_range, forbid emptyipsand emptyas_rangewhen NAT is enabled, allow “useless” exclusion prefixes (warn instead of error), and forbid default-to-default peerings. - Simplify prefix-collapsing utilities:
collapse_prefixes_peeringis now infallible and call sites no longer map/propagate util errors. - Add extensive new validation tests and update existing tests to match the new validation rules.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| nat/src/stateless/setup/mod.rs | Adjusts to infallible collapse_prefixes_peering and removes now-unused peering error variant. |
| nat/src/stateful/apalloc/setup.rs | Removes error mapping around prefix-collapsing since collapse is now infallible. |
| flow-filter/src/setup.rs | Updates default-expose handling (no longer fallible) and uses infallible prefix collapsing. |
| flow-filter/src/lib.rs | Updates one test to bypass overlay validation when constructing now-forbidden default-to-default peering config. |
| config/src/utils/overlap.rs | Switches overlap helper inputs to PrefixPortsSet to match the current expose field types. |
| config/src/utils/collapse.rs | Makes collapse_prefixes_peering return Peering directly and factors out per-expose collapsing. |
| config/src/external/overlay/vpcpeering.rs | Implements new VpcExpose/manifest validation rules; warns on non-overlapping excludes; enforces single default expose per manifest. |
| config/src/external/overlay/vpc.rs | Adds explicit validation forbidding default-to-default peerings. |
| config/src/external/overlay/validation_tests.rs | New, comprehensive validation-focused test suite for exposes/peerings/overlay. |
| config/src/external/overlay/tests.rs | Updates existing tests to match new validation semantics (empty expose rejected; out-of-range excludes allowed). |
| config/src/external/overlay/mod.rs | Exposes the new validation test module. |
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
5e32fae to
450912d
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
450912d to
e152c99
Compare
Member
Author
|
Fixed merge conflict caused by #1330 |
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
e152c99 to
c112a3d
Compare
Member
Author
|
And by #1319 |
When moving from BTreeSet<PrefixWithOptionalPorts> to PrefixPortsSet for the lists of prefixes in VpcExpose objects, we forgot to update function check_prefixes_dont_overlap() in the config crate. Fix it now. Fixes: 9fee292 ("refactor(config): Use wrapper type for lists of prefixes") Signed-off-by: Quentin Monnet <qmo@qmon.net>
Following a recent discussion, we agreed to allow the use of root prefixes (0.0.0.0/0, ::/0) in expose blocks. Notes: - A root prefix covers all IP addresses, but does not have the same semantic meaning as a "default" expose, because it cannot overlap with other exposed addresses (the "default" expose acts as a fallback in case prefixes in another expose on the same end of the manifest are in use). - Root prefixes as exclusion prefixes are forbidden anyway, because they mask all exposed prefixes. - Root prefixes might be disabled again soon, because we plan to forbid the use of some reserved IP addresses. Link: #1150 Signed-off-by: Quentin Monnet <qmo@qmon.net>
We should check that a given manifest has at most one default expose when validating the manifest object, not when looking for its default expose while building the flow-filter stage context. Let's clean it up. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Forbid peering a manifest with a default expose blocks with another manifest that also contains a default expose blocks. There is no strong motivation for rejecting this case, but discussing it we agreed that it sounds fishy, and we prefer to conservatively reject it at the moment. Might change in the future. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Apart from "default" expose blocks, it makes no sense to have an expose block with an empty ips list. Similarly, if NAT is set up for an expose, its as_range list should not be empty. Adjust validation accordingly. Signed-off-by: Quentin Monnet <qmo@qmon.net>
We used to reject configuration for expose blocks where an exclusion prefix would not be fully covered by any of the allowed prefixes. Stop rejecting them: it's OK to have these useless exclusion prefixes, although we want to warn users in such a case. We also change the check, to warn when there's no overlap (rather than when the exclusion prefix is not fully contained by any of the allowed prefixes). The reason for this change is the following: we could have an exclusion prefixes that is not fully contained in any allowed prefixes, but that could be contained within the union of allowed prefixes. In such case, the exclusion prefix remains in use (although if the user specifies multiple prefixes to then negate them all at once, it might be suspicious). We could look at whether the full exclusion prefix is covered by the union of the allowed prefixes, but this sounds too expensive for the gain. Signed-off-by: Quentin Monnet <qmo@qmon.net>
The function does not fail and does not need to wrap its return value in a Result<_>. Let's also split it, so it's easier in the future to add another processing method while iterating over the exposes in the peering, for example to remove duplicate prefixes. Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
c112a3d to
699db2b
Compare
Add checks related to validation for peerings and expose blocks, to make sure we comply with the expected rules and limitations we've settled one for accepting a configuration. We already have tests in file tests.rs in the same directory: whereas existing tests focus on simpler consistency checks and on checking the validation behaves as we expect, the tests in the new file are more oriented towards checking that semantics and restrictions discussed for peerings (see link below) are respected. There is some overlap between the two files though, we may remove some duplicate tests in the future. Link: #1150 Signed-off-by: Quentin Monnet <qmo@qmon.net>
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
699db2b to
8e4e98e
Compare
Member
Author
|
And by #1332 |
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 PR adds many tests for validation for peering and expose objects, to ensure that validation abides by the permissions and restrictions discussed in #1150. The tests are added in a new file; some of these tests overlap with the existing ones in tests.rs, but overall they focus more on compliance with the API decisions we've made, rather than checking the validation logic like the existing tests do.
As for the validation updates, this PR contains:
ipsoras_rangelists for expose blocksHowever, the following items are NOT implemented:
Forbidding reserved IPs (0.0.0.0, 255.255.255.255, loopback prefix, multicast prefix...) and ports (0): some investigation work is required to figure out what addresses to block, exactly, and how to deal with prefixes containing blocked addresses.
Allowing overlapping prefixes within a set of prefixes (
ipsoras_rangeor exclusion lists): this requires merging prefixes together, and possibly splitting them (when port ranges are used, if overlap is partial and full merge is not doable), which we already do later when building the flow-filter context. We don't want to repeat the process at the validation step: we should refactor the code to only to it once, but this is left for a future PR.Allowing exclusion prefixes to cover full allowed prefixes, as long as not all allowed prefixs are completely masked: this requires "applying" the exclusion prefixes to the allowed prefixes, which we already do later when building NAT or flow-filter context. We don't want to repeat the process at the validation step: we should refactor the code to only do it once, but this is left for a future PR.