Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use generic HTTP connector in Client #27

Merged
merged 3 commits into from
Dec 30, 2021
Merged

Use generic HTTP connector in Client #27

merged 3 commits into from
Dec 30, 2021

Conversation

iwinux
Copy link
Contributor

@iwinux iwinux commented Dec 14, 2021 •
edited
Loading

Motivation

Need to connect to ClickHouse via TLS, but the hyper::Client<HttpConnector> used in clickhouse::Client::default() cannot handle https:// URLs.

Solution

  1. Make Client generic over type param C, which should implement the hyper::client::connect::Connect trait.
  2. Allow injection of hyper::Client via with_hyper_client().

With these updates, users of clickhouse::Client can connect to ClickHouse via TLS with something like hyper::Client<hyper_tls::HttpsConnector<HttpConnector>>. For example:

let mut http_connector = HttpConnector::new();
http_connector.enforce_http(false); // allow https URLs

let tls_connector = native_tls::TlsConnector::builder().build()?.into();
let https_connector = HttpsConnector::from((http_connector, tls_connector));
let hyper_client = hyper::client::Client::builder().build(connector);

let client = clickhouse::Client::with_hyper_client(http).with_url("https://...");

@iwinux
Copy link
Contributor Author

iwinux commented Dec 14, 2021

@loyd please take a look and leave comments when you have time :)

@loyd
Copy link
Collaborator

loyd commented Dec 16, 2021 •
edited
Loading

Thanks for contributing, too useful feature!

However, I don't want to add a generic parameter because, firstly, it pollutes all structs and, secondly, it exposes hyper-specific types and traits. I have plans to support other frameworks (to support different runtimes), so this solution complicates the transition later.

Instead, I suggest using dynamic dispatching (some custom dyn HttpClient) to hide details. The possible impact is insignificant here and it doesn't pollute signatures.

So, with_hyper_client(client: hyper::Client<C>) should be translated into with_http_client(impl HttpClient), where HttpClient is implemented for any hyper::Client<C>:

// http_client.rs
use crate::sealed::Sealed;

trait HttpClient: Sealed {
    /// Private API.
    #[doc(hidden)]
    fn request(&self, req: Request<Body>) -> ResponseFuture;
}

impl<C> Sealed for hyper::Client<C> {}

impl<C> HttpClient for hyper::Client<C> {
  ...
}

What do you think about it?

@iwinux
Copy link
Contributor Author

iwinux commented Dec 17, 2021

Your design looks much cleaner! I'll update the implementation.

@iwinux
Copy link
Contributor Author

iwinux commented Dec 22, 2021

@loyd I've updated the code. Please take a look :)

Note: it might not be as extensible as intended:

  • Body and ResponseFuture are hyper-specific types
  • Sealed prevents the trait from being implemented outside of the crate

@loyd
Copy link
Collaborator

loyd commented Dec 28, 2021 •
edited
Loading

Thanks for the updates!

Sealed prevents the trait from being implemented outside of the crate

Exactly what we want, because internally the crate should know about implementation until h2 is used.

loyd
loyd reviewed Dec 28, 2021
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@loyd
Copy link
Collaborator

loyd commented Dec 30, 2021

Tests passed, code is fine, LGTM.
Ready to merge?

@iwinux
Copy link
Contributor Author

iwinux commented Dec 30, 2021

Yeah it's good to merge :)

@loyd loyd merged commit 0daaab5 into ClickHouse:master Dec 30, 2021
@stonecharioteer
Copy link

Hi @iwinux I'm trying to use this feature. Could you point me to some documentation that walks me through using a https only connection with clickhouse?

@loyd loyd mentioned this pull request Jan 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loyd loyd loyd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iwinux @loyd @stonecharioteer