Support for default exposes #1198
Merged
Conversation
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
Fredi-raspall
requested review from
qmonnet and
sergeymatov
and removed request for
a team and
sergeymatov
qmonnet
approved these changes
Jan 15, 2026
qmonnet
added a commit
that referenced
this pull request
Jan 16, 2026
Because the follow-up commits should be based on the changes from #1198, but we just need this tiny change to get the rest to compile.
Fredi-raspall
force-pushed
the
pr/fredi/expose_refined
branch
from
cfe7896 to
e08e39b
Compare
qmonnet
added a commit
that referenced
this pull request
Jan 17, 2026
Because the follow-up commits should be based on the changes from #1198, but we just need this tiny change to get the rest to compile.
Fredi-raspall
force-pushed
the
pr/fredi/expose_refined
branch
from
e08e39b to
bfc4dfe
Compare
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Extends the conversion from CRD to internal type to allow the support of default exposes. A default expose cannot contain any ip/nots or nat configuration. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Since we keep at most one config, there's no need to clear intermediate collections. Also, reorganize the code so that adding validations is clearer. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
- Forbid prefixes 0/0 or ::/0 in ip/nat/nots/as-not's in exposes - Do not allow default exposes to have ip/nat/nots/not-as Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Reorganize code so that we validate the `Peering` objects collected in Vpcs instead of the undirected `VpcPeering` objects learnt from the CRD. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adds a method to return the set of prefixes that should be advertised for an expose. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Adapt the logic to determine prefixes to be advertised for a given peering expose. Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Fredi-raspall
force-pushed
the
pr/fredi/expose_refined
branch
from
bfc4dfe to
a43d269
Compare
qmonnet
reviewed
Jan 19, 2026
Member
qmonnet
left a comment
qmonnet
left a comment
There was a problem hiding this comment.
I took another look at the PR today and I've got a couple more comments on the validation - nothing really blocking, though
Comment on lines
+318
to
+339
| if self.ips.iter().any(|p| p.prefix().is_root()) { | ||
| return Err(ConfigError::Forbidden( | ||
| "Expose: root prefix as 'ip' forbidden", | ||
| )); | ||
| } | ||
| if self.nots.iter().any(|p| p.prefix().is_root()) { | ||
| return Err(ConfigError::Forbidden( | ||
| "Expose: root prefix as 'not' is forbidden", | ||
| )); | ||
| } | ||
| if let Some(nat) = &self.nat { | ||
| if nat.as_range.iter().any(|p| p.prefix().is_root()) { | ||
| return Err(ConfigError::Forbidden( | ||
| "Expose: root prefix as NAT 'as' is forbidden", | ||
| )); | ||
| } | ||
| if nat.not_as.iter().any(|p| p.prefix().is_root()) { | ||
| return Err(ConfigError::Forbidden( | ||
| "Expose: root prefix as NAT 'as-not' is forbidden", | ||
| )); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Nit: these are not for validating the default expose, should they be in a separate function named differently? (Probably doesn't matter much though)
Contributor
Author
There was a problem hiding this comment.
yeah, the name is not ideal. Let me think of a better one...
The type doesn't seem necessary at this time, let's remove it to help
with clarity.
[ Quentin:
Remove VpcdLookupResult from comments in flow-filter/src/tables.rs ]
Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Make sure that we account for "default" VpcExpose when we retrieve the destination VPC discriminant for the packet. Internally, we don't add a prefix for the default destination into the tables, because we'd have to handle overlap to some extent. Instead, we add a per-source-prefix value to use as a fallback when no other destination prefix match. At the moment, the code assumes that each VPC accepts at most one "default" destination. Signed-off-by: Quentin Monnet <qmo@qmon.net>
Add unit test cases to validate the behaviour in the case of
default-destination VpcExpose objects. In such case, NAT must use the
relevant default destination to find what transformation rules it should
apply.
As it turns out, there is no change required in the code for NAT:
- For stateful NAT:
- We don't currently support destination NAT with stateful NAT. So
when a packet is sent to a default-destination VPC, we may apply
stateful NAT for the source as usual, and won't apply destination
NAT in the case of the default.
- When the packet comes back, we have an entry in the session table
so there's no need to look at the context from the config anyway.
- For stateless NAT, the stage does not enforce any validation on
whether packets match existing local and remote prefixes (this is the
role of the flow-filter stage instead). When we reaching the stateless
NAT stage we already know the destination VPC (also from the
flow-filter stage): if we find no matching prefix for destination NAT,
we do not NAT. Given that default-destination VPCs don't support NAT,
we're good.
Signed-off-by: Quentin Monnet <qmo@qmon.net>
Be more explicit in the stateless NAT lookups, especially if those do not provide a value. Signed-off-by: Fredi Raspall <fredi@githedgehog.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>
Make sure that we do not expose overlapping prefixes to a given VPC. The only supported exception is when one of the expose is a `default` expose destination. Note: The implementation is currently rather inefficient as we have seven nested loops (counting the loop on VPCs for validation). We should look into some structure where to insert prefixes as we encounter them, like for check_peering_count(), but it's not trivial to identify overlapping prefixes (accounting for both IP prefixes and port ranges) in a structure without looping on them anyway, so we stick to the naive implementation for now. Signed-off-by: Quentin Monnet <qmo@qmon.net>
We decided to reject, for now at least, configurations where multiple "default" VpcExpose objects are exposed to a single VPC. This commit adjusts validation accordingly, and add the related unit tests. Also reject the configuration if a peering (not a VPC) contains multiple "default" expose blocks. Signed-off-by: Quentin Monnet <qmo@qmon.net>
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.
defaultflag fabricator#1313 gets merged (?)