feat(s2n-quic): allow disabling active connection migration support #2182
Conversation
| /// | ||
| /// If set to false, the `disable_active_migration` transport parameter will be sent to the | ||
| /// peer, and any attempt by the peer to perform an active connection migration will be ignored. | ||
| pub fn with_connection_migration(mut self, enabled: bool) -> Result<Self, ValidationError> { |
There was a problem hiding this comment.
Does it make sense to expose a non-exhaustive enum here instead if its functionally equivalent?
Imagining a wild future where this gets expanded to other values (maybe not):
- allow active mig (covered in this api)
- deny active mig (covered in this api)
- deny passive mig
- allow up to 5 active mig
- allow active mig from these IPs
- ..etc
There was a problem hiding this comment.
I think it would be better to call this with_active_connection_migration, since we're not completely disabling migration, right?
There was a problem hiding this comment.
yeah, I went back and forth on that a bit. I think I ended up on thinking that the application shouldn't be too concerned about "passive" connection migration, so I left the "active" out of it. But thinking about it, its more obviously related to the disable_active_migration transport parameter with active in it, so I can add it
There was a problem hiding this comment.
Regarding the wild future you are talking about @toidiu, I think at that point we would have to stabilize the path migration validator and add more of the configuration in there. This one is really tied to the transport parameter, which doesn't have any of that configurability
| /// | ||
| /// If set to false, the `disable_active_migration` transport parameter will be sent to the | ||
| /// peer, and any attempt by the peer to perform an active connection migration will be ignored. | ||
| pub fn with_connection_migration(mut self, enabled: bool) -> Result<Self, ValidationError> { |
There was a problem hiding this comment.
I think it would be better to call this with_active_connection_migration, since we're not completely disabling migration, right?
| #[derive(Clone, Copy, Debug, Default, PartialEq)] | ||
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub enum MigrationSupport { | ||
| #[default] |
There was a problem hiding this comment.
why did we need to change this?
There was a problem hiding this comment.
|
Kani failures will be fixed by model-checking/kani#3144 |
Co-authored-by: toidiu <apoorv@toidiu.com>
Description of changes:
This change adds a
with_connection_migration(enabled: bool)setting to the default connection limiter provider. When set tofalse, thedisable_active_migrationtransport parameter will be sent to the peer, and any attempt by the peer to perform an active connection migration will be ignored.The definition of "active connection migration" is the peer sending from a different source address while also changing the destination connection ID it was using previously. This is in contrast to a passive cnonection migration (caused by a NAT rebind or similar), where the peer would not know its source address changed, and thus would not change the destination connection ID.
Call-outs:
I went back and forth on whether this belonged in the path migration validator provider, but ultimately decided against it since:
Testing:
Added unit tests
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed? -->
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.