feat(s2n-quic): unstable dc provider #2210
Conversation
quic/s2n-quic-core/src/dc/mod.rs
Outdated
|
|
||
| /// Returns the stateless reset tokens to include in a `DC_STATELESS_RESET_TOKENS` | ||
| /// frame sent to the peer. | ||
| fn stateless_reset_tokens(&mut self) -> &[stateless_reset::Token]; |
There was a problem hiding this comment.
Trying to decide if this is going to be an issue if we're making the provider own the tokens (since they're being borrowed out of this function).
There was a problem hiding this comment.
I'm not sure, but I think we can get a little further in the integration and revisit
quic/s2n-quic-core/src/dc/mod.rs
Outdated
| @@ -0,0 +1,146 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
I've been preferring moving away from mod.rs and using the actual name, since it gets annoying having a million mod.rs files in the project.
| pub struct DisabledPath(()); | ||
|
|
||
| impl Path for DisabledPath { | ||
| fn on_path_secrets_ready(&mut self, _session: &impl TlsSession) {} |
There was a problem hiding this comment.
Will any of these methods actually get called? Might be a good idea to put an unimplemented!() or at the very least a debug_assert!(false).
There was a problem hiding this comment.
If I make new_path return Option, then I can just have the disabled endpoint return None, in which case these will not be called
| @@ -52,6 +52,15 @@ cfg_if!( | |||
| } | |||
| ); | |||
|
|
|||
| cfg_if!( | |||
| if #[cfg(any(test, feature = "unstable-provider-dc"))] { | |||
There was a problem hiding this comment.
Are you wanting to add this feature in this PR?
There was a problem hiding this comment.
knew I was forgetting something :-)
There was a problem hiding this comment.
also needed to add the provider to the server and client builders
quic/s2n-quic-core/src/dc/mod.rs
Outdated
| type Path: Path; | ||
|
|
||
| /// Called when a dc version has been negotiated for the given `ConnectionInfo` | ||
| fn new_path(&mut self, connection_info: &ConnectionInfo) -> Self::Path; |
There was a problem hiding this comment.
Do we want to have this return an Option<Self::Path>?
There was a problem hiding this comment.
I was planning on not calling it at all if no dc version was negotiated, so it didn't seem necessary. But I guess it might still be useful to give the provider the opportunity to not use dc even if a dc version was negotiated if it was running in some mixed dc/non-dc environment.
quic/s2n-quic/src/provider/dc.rs
Outdated
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| use s2n_quic_core::dc::{Disabled, Endpoint}; |
There was a problem hiding this comment.
Looks like this is missing re-exports for the s2n-quic-core internals? Not sure how much it will matter but historically it has been painful to maintain those versions (since every bump requires bump toml or being hit with build failures, even though you may not be directly affected by the changes in the bump).
There was a problem hiding this comment.
good catch, it wasn't intentional, I'll add the re-exports
Description of changes:
This change adds an unstable dc provider for configuring dc functionality. The provider will not actually be used until subsequent PRs.
Call-outs:
I've implemented
PathforOption<P>as I was planning to have thedc::Manageroperate on anOption<Path>rather than justPath. This is because dc can end up disabled for a path in 2 ways:Disabledprovider is used (which has an associated type ofDisabledPath)Pathtype of the dc provider is used but shouldn't actually do anything.I didn't want the implementor of the provider to have to handle when no dc version is negotiated, so instead I'll have the
dc::Managerhandle that case.Testing:
Will add tests after further integration of the provider
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.