Add support for "default" destination to flow-filter stage #1205
Conversation
qmonnet
force-pushed
the
pr/qmonnet/handle-default
branch
2 times, most recently
from
f5d2633 to
5053ce2
Compare
Fredi-raspall
force-pushed
the
pr/qmonnet/handle-default
branch
from
5053ce2 to
647b8ad
Compare
|
There's a bug in the logic, I need to address it. |
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>
qmonnet
force-pushed
the
pr/qmonnet/handle-default
branch
from
647b8ad to
3b75fa7
Compare
Now fixed, and validated with a test. I'm working on the NAT parts. |
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>
mvachhar
left a comment
mvachhar
left a comment
There was a problem hiding this comment.
This PR looks good, but I just want a discussion of the multiple defaults first.
| pub fn empty() -> Self { | ||
| Self::default() | ||
| } | ||
| #[must_use] |
There was a problem hiding this comment.
Is the assumption that there is only a single default valid even in the presence of stateful NAT? For example, if connections originate from the default blocks, then stateful NAT would be able to resolve any ambiguity. What should the right behavior be in that case here? Should we just leave the destination vpc field empty? Do we have to handle that case now?
There was a problem hiding this comment.
Yes, the code assumes that there is a single default at the moment. We still need to add the appropriate validation for that.
For example, if [...]
Not supported at the moment. Having multiple defaults would typically require a step to disambiguate the destination VPC - but we haven't thought out all the details yet. We could also rely on stateful NAT, but this is not the case at the moment (and not everybody agrees about it).
Should we just leave the destination vpc field empty
No, at the moment the destination VPC field is never empty when the packet leaves the flow-filter stage (unless we drop the packet). Note that following the change of rules regarding overlapping prefixes, we removed the code for setting the destination VPC from stateful NAT (69a5f6b).
Do we have to handle that case now?
I'd definitely defer to a future release rather than trying to hack something quick here, it feels like it requires more thinking at the moment.
There was a problem hiding this comment.
Is the assumption that there is only a single default valid even in the presence of stateful NAT? For example, if connections originate from the default blocks, then stateful NAT would be able to resolve any ambiguity. What should the right behavior be in that case here? Should we just leave the destination vpc field empty? Do we have to handle that case now?
We could handle multiple defaults, but we'd probably need to augment the API to do the "right" thing. This was mentioned / asked in githedgehog/gateway#275 but there was no real discussion IIRC
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>
qmonnet
force-pushed
the
pr/qmonnet/handle-default
branch
from
96ec87f to
32803f2
Compare
|
@Fredi-raspall I kept but changed your commit to keep just one line of log per lookup, let me know if you really prefer your version. |
|
NAT needed no change after all, it looks like this is ready for review 🎉 |
I preferred mine (even if more verbose) because I dislike debug prints |
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>
qmonnet
force-pushed
the
pr/qmonnet/handle-default
branch
from
32803f2 to
07b892f
Compare
|
Discussed today in meeting - We support at most one |
qmonnet
force-pushed
the
pr/qmonnet/handle-default
branch
from
bf1f81b to
07b892f
Compare
Work in progress.
This needs to be rebased on top of #1198, once it is merged.NOTE: this is now rebased on #1198 and could be merged to that PR so that we keep a consistent history of PRs vs features in github.
Currently, this PR also misses: