mgmt: Validate VpcPeering & friends #445
Conversation
|
I'm not rebasing on the top of three different PRs. Rebases in any of the three will cause me to need new rebases. (Although, there doesn't seem to be that many conflicts between your code and mine - mostly the changes and renaming of If you prefer your sets to go in first - totally understandable - I'll wait for them to get merged, and then rebase. |
Fredi-raspall
force-pushed
the
pr/qmonnet/nat-validation
branch
from
4da325d to
3898e9c
Compare
|
Oh, you did rebase 🤯 |
Fredi-raspall
force-pushed
the
pr/qmonnet/nat-validation
branch
4 times, most recently
from
31b64ef to
6c1bc96
Compare
Fredi-raspall
force-pushed
the
pr/fredi/mgmt-cleanup
branch
from
0a28eb3 to
e382a20
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
6c1bc96 to
6ea7f0b
Compare
|
OK, we should be good for a second review |
| @@ -187,6 +281,144 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_validate_expose() { | |||
There was a problem hiding this comment.
two things
- why not make these distinct tests?
- why not use bolero?
There was a problem hiding this comment.
-
Not sure what you mean, you propose to move each of the different case to a separate function? They felt close enough (all about testing the validation for
VpcExpose) that I could group them in a single function, but if you think it's better to have them separated into many smaller functions, I guess that's also a possibility. -
Because I'm not familiar with bolero and haven't looked into it (or even thought about it, for that matter 🫣). I can, if you think it's worth it.
There was a problem hiding this comment.
Oh, I won't reject the PR on this basis.
I just think that bolero would be notably easier and more effective than this path.
Perhaps I should do a tutorial on how to use it.
There was a problem hiding this comment.
I think you did a presentation on it at some point? I need to find the link again. If I'm getting confused and you haven't, then yes, I'd gladly watch/read it!
| @@ -144,10 +144,21 @@ impl VpcManifest { | |||
| } | |||
| } | |||
| pub fn add_expose(&mut self, expose: VpcExpose) -> ConfigResult { | |||
| expose.validate()?; | |||
There was a problem hiding this comment.
is it your intention to make it possible to construct invalid configurations?
Why remove the validation check here?
There was a problem hiding this comment.
This is based on Fredi's feedback from my first version:
- mgmt: Validate VpcPeering & friends #445 (comment)
- mgmt: Validate VpcPeering & friends #445 (comment)
The gist is that we prefer having validation in a single place, and that, yes, it's acceptable to build a broken config that we will reject at validation time, if only to show it back to the user.
There was a problem hiding this comment.
Ok.
I can live with that.
It might be good to have a validated type which wraps this one in that case to express the static requirement that you can't feed an invalid config to the dataplane worker
There was a problem hiding this comment.
Yes, I can look into it as a follow-up.
Fredi-raspall
force-pushed
the
pr/fredi/mgmt-cleanup
branch
2 times, most recently
from
a2d87d8 to
fac46e6
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
2 times, most recently
from
db6bb9f to
dae4cfd
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
dae4cfd to
5f9497c
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
5f9497c to
6be6b4c
Compare
Fredi-raspall
force-pushed
the
pr/fredi/management-processor
branch
from
141df4a to
570027f
Compare
Fredi-raspall
force-pushed
the
pr/qmonnet/nat-validation
branch
2 times, most recently
from
281b10c to
aef176f
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
aef176f to
9f1686b
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
3 times, most recently
from
f9f7223 to
fe9eb86
Compare
|
I've incorporated Fredi's feedback I think, this is ready for another review. |
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
fe9eb86 to
f536525
Compare
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
2 times, most recently
from
cd27db0 to
a89f119
Compare
Implement the skeleton function to validate a VpcExpose object. This
validation function is automatically called when adding a VpcExpose to a
VpcManifest object. The function performs the following steps:
1. Make sure that all prefixes and exclusion prefixes for this
VpcExpose are of the same IP version.
2. Make sure that all prefixes (or exclusion prefixes) in each list
(ips/nots/as_range/not_as) don't overlap with other prefixes (or
exclusion prefixes, respectively) of this list.
3. Make sure that all exclusion prefixes are contained within existing
prefixes, unless the list of allowed elements is empty.
4. Make sure exclusion prefixes in a list don't exclude all of the
prefixes in the associated prefixes list.
5. Make sure we have the same number of addresses available on each
side (public/private), taking exclusion prefixes into account.
Add related ConfigError enum variants, as necessary to cover the
different error cases.
Add unit tests to check validation for a number of VpcExpose objects.
Some other unit tests in the crate need minor adjustments, because they
use incorrect VpcExpose objects with exclusion prefixes not covering
allowed prefixes, or with a mismatch between the number of addresses in
the public and private sets (which we reject because of static NAT).
Signed-off-by: Quentin Monnet <[email protected]>
Just like we did for VpcExpose, implement validation for VpcManifest. There's not much to do this time, because most of the work consists in validating the VpcExpose objects. Make sure the name is not empty, make sure the lists of VpcExpose objects are not empty, and done. Note: We should also prevent prefix overlap between the different exposes in the manifest; this is not trivial, and will be added as a separate commit. Fix some tests that would use manifests with empty VpcExpose lists. Signed-off-by: Quentin Monnet <[email protected]>
When validating a VpcExpose object, we ensure that the prefixes in each category (ips, as_range, nots, not_as) do not overlap. However, within a VpcManifest, we must also ensure that prefixes from different VpcExpose objects do not collide. In particular, we want to make sure that the available addresses in the "ips" lists do not collide with "ips" lists from other VpcExpose (or if the prefixes collide, they should also be covered by exclusion prefixes). Same applies to the "as" lists. Introduce a function to check overlap between two sets of addresses (one set = allowed prefixes + exclusion prefixes). See comments in the code for details on the implementation. Call this function as part of the validation process for a VpcManifest, to ensure there is no collision. Signed-off-by: Quentin Monnet <[email protected]>
When we try to insert a peering with a name that already exists for another peering in a VpcPeeringTable, this results in an error, but it also results in the peering being inserted anyway, as we only detect the name collision when inserting (and we do not implement rollback). As a consequence, we keep an incorrect peering entry in the table. This can cause unexpected collisions on prefixes when trying to add subsequent, legitimate peerings to the table, for example. Instead of inserting unconditionally, let's check first for the existence of the peering name in the table. Fixes: e45c0ef (feat(mgmt): define external model) Signed-off-by: Quentin Monnet <[email protected]>
In previous commits, we introduced validation for VpcExpose and VpcManifest objects; now we do the same for a VpcTable. Here, we simply need to ensure, after building the table, that for any two given Vpcs, we have at most a single Peering object between the two: having more than one is not currently supported. Signed-off-by: Quentin Monnet <[email protected]>
qmonnet
force-pushed
the
pr/qmonnet/nat-validation
branch
from
a89f119 to
3f13088
Compare
This Pull Request implements some validation for
VpcExpose,VpcManifest,VpcPeering, andVpcTableobjects received from the gRPC server.Fixes: #442
Please refer to individual commit descriptions for details.