chore(tests): add filters for state machine proptests #7148
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
conectado
force-pushed
the
chore/tests/add-filters
branch
from
22e77dc to
9d25446
Compare
|
I wonder if with this tests I should just remove the old tests for filters, I think these should cover everything? Though maybe when introducing the new resource selection algorithm this might get too complex with the whole range of filters. What do you think @thomaseizinger |
There was a problem hiding this comment.
Direction looks good! A few comments regarding the new portal resource types.
For some reason, I thought correctly modelling the filters will be much trickier but it seems to be pretty easy? If this is all we need then I am in favor of deleting the other tests. We should try and get some kind of coverage though to ensure we are actually also sending packets that do make it through the filters.
rust/connlib/tunnel/src/proptest.rs
Outdated
| .boxed() | ||
| use crate::messages::gateway::{Filter, Filters, PortRange}; | ||
|
|
||
| /// Full model of a resource, ressembling what the portal stores, proyections of this are sent to the client and gateways |
There was a problem hiding this comment.
| /// Full model of a resource, ressembling what the portal stores, proyections of this are sent to the client and gateways | |
| /// Full model of a resource, resembling what the portal stores, projections of this are sent to the client and gateways |
There was a problem hiding this comment.
Similar typos further down too.
rust/connlib/tunnel/src/proptest.rs
Outdated
|
|
||
| /// Full model of a resource, ressembling what the portal stores, proyections of this are sent to the client and gateways | ||
| #[derive(Debug, Clone)] | ||
| pub(crate) enum PortalResource { |
There was a problem hiding this comment.
Can we move this to the stub_portal module and just call the enum Resource and the use if with the qualified module name?
rust/connlib/tunnel/src/tests/sut.rs
Outdated
| } | ||
|
|
||
| c.sut.add_resource(resource); | ||
| c.sut.add_resource(ResourceDescription::from(resource)); |
There was a problem hiding this comment.
Why are we converting to the message type here and not directly into client::Resource? If you implement the right trait, you can even pass it directly.
There was a problem hiding this comment.
IMO the ResourceDescription types are only for deserialising from JSON. These tests never deserialise from JSON so they should never need to use these types :)
| @@ -265,6 +291,32 @@ where | |||
| }) | |||
| } | |||
|
|
|||
| fn port_from_resource( | |||
There was a problem hiding this comment.
The idea here is to more often pick ports within the allowed range, otherwise allowed packets w/o internet resource(which in the future shouldn't even happen) would be too sparse
There was a problem hiding this comment.
Should we add some coverage logs to the gateway perhaps?
There was a problem hiding this comment.
Added some coverage for blocked packets here I'll add something for passing packets.
|
Finally tests with 10000 proptest cases are passing! Needs a bit of polish but will be open for review soon, Hardest issue was that we need to precisely track what resources the gateway knows at the time each packet is processed to determine what filters are applied. Will get way easier with #4789 and packet buffering |
conectado
force-pushed
the
chore/tests/add-filters
branch
from
5a88287 to
94e3159
Compare
conectado
force-pushed
the
chore/tests/add-filters
branch
from
1d5b1cb to
6248ee7
Compare
WIP
Tests are working right now but it needs a bit of polish
Fixes #7126