Skip to content

Allow preconfigured TLS with feature = "http3"#1850

Merged
seanmonstar merged 1 commit intoseanmonstar:masterfrom
daxpedda:h3-preconfigured-tls
Jul 6, 2023
Merged

Allow preconfigured TLS with feature = "http3"#1850
seanmonstar merged 1 commit intoseanmonstar:masterfrom
daxpedda:h3-preconfigured-tls

Conversation

@daxpedda
Copy link
Copy Markdown
Contributor

Currently it's impossible to use ClientBuilder::use_preconfigured_tls() in combination with the http3 feature:

h3_connector.expect("missing HTTP/3 connector"),

This PR addresses this by requiring to pass quinn::TransportConfig as well when using the http3 feature.

#[cfg(feature = "http3")]
{
if let Some(conn) = (&mut tls as &mut dyn Any)
.downcast_mut::<Option<(rustls::ClientConfig, quinn::TransportConfig)>>()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'd want to do this, because I would like to make it easier to swap in s2n-quic or similar for the QUIC implementation. Is there need to configure the TransportConfig yourself, or can we just figure it out internally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'd want to do this, because I would like to make it easier to swap in s2n-quic or similar for the QUIC implementation.

How does this prevent swapping in s2n-quic?

Is there need to configure the TransportConfig yourself, or can we just figure it out internally?

There's a bunch of configuration options that aren't exposed through ClientBuilder.
Personally I don't need it and we can definitely figure it out internally.

I will adjust the PR to generate TransportConfig instead of having to pass it through.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@daxpedda daxpedda force-pushed the h3-preconfigured-tls branch from 17b943f to 258d384 Compare May 28, 2023 19:19
@daxpedda
Copy link
Copy Markdown
Contributor Author

CI currently fails because of https://github.com/bluejekyll/trust-dns/issues/1946.

@daxpedda daxpedda force-pushed the h3-preconfigured-tls branch from 258d384 to f172314 Compare May 28, 2023 19:28
@daxpedda daxpedda force-pushed the h3-preconfigured-tls branch from f172314 to e226767 Compare June 14, 2023 08:32
@daxpedda
Copy link
Copy Markdown
Contributor Author

CI fails because log bumped their MSRV to v1.60, reqwests is v1.57, in v0.4.18, but they didn't use the rust-version tag, but they added that in v0.4.19: https://github.com/rust-lang/log/blob/master/CHANGELOG.md#0419---2023-06-10.

@daxpedda daxpedda requested a review from seanmonstar June 14, 2023 08:49
@daxpedda daxpedda force-pushed the h3-preconfigured-tls branch from e226767 to 025192c Compare July 6, 2023 10:33
@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jul 6, 2023

CI currently fails because of rust-lang/rust#113152.

@seanmonstar
Copy link
Copy Markdown
Owner

Wow, an instance of testing minimal-versions biting us.

@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jul 6, 2023

Wow, an instance of testing minimal-versions biting us.

Happy to make a PR fixing it, just not sure how. I could just bump the dependency beforehand like here:

- name: Make sure log v0.4.18 is used
run: |
cargo update
cargo update -p log --precise 0.4.18

See #1879.

@seanmonstar
Copy link
Copy Markdown
Owner

Yea that seems fine. (Or we could put it in dev-dependencies...)

@daxpedda daxpedda force-pushed the h3-preconfigured-tls branch from 025192c to e6dea55 Compare July 6, 2023 13:28
@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jul 6, 2023

@seanmonstar friendly ping, CI passes, PR is ready to be reviewed again.

@seanmonstar seanmonstar merged commit 89df5d3 into seanmonstar:master Jul 6, 2023
Nutomic pushed a commit to Nutomic/reqwest that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants