Allow passing in a custom rustls ClientConfig to Channel

[tiziano88]

cc @ipetr0v @conradgrobler @djc @LucioFranco

Originally posted by @tiziano88 in #859 (comment)

[djc]

I wonder if we could use hyper's connector trait bounds for this? If we just require Connect and Clone, we can pass that into the hyper client builder. In that way, we can conceivably support native-tls as well different rustls versions through their appropriate hyper-rustls integrations.

[LucioFranco]

For now I would suggest following this example #968 this will currently work with tonic 0.7. The future plan for tonic is to actually remove the transport module. I will be submitting a proposal for this soon.

[stuhood]

For now I would suggest following this example #968 this will currently work with tonic 0.7.

That example involves quite a bit of code compared to using a tonic::transport::Endpoint with a custom rustls tls_config in tonic 0.6. I'm not sure that that really seems like a palatable alternative, since it seemingly has to ensure that the hyper and tower layers are configured "just so" for tonic.

[LucioFranco]

While I do agree the ergonomics are not as good, tying tonic to rustls releases is not feasible either.

Currently, the client_rustls example does have some rough edges but these are things we plan to fix in upcoming releases. A lot of the code that made transport easy to use doesn't belong in something like tonic, because it should b extended to use-cases beyond. So removing ClientConfig from the public API is a push in that direction. Now if you are running into specific issues with that example/using hyper etc. I am more than happy to help and understand where those rough edges are.

[bmwill]

Somewhat related to no longer being able to use a custom rustls::ClientConfig we're also unable to use a custom rustls:ServerConfig. From @LucioFranco's most recent comment and looking at some of discussion around this it seems like the main motivation for this change was to not have rustls be apart of the public API of tonic. During some experimentation trying to use a custom config for both client and server with tonic 0.7 I found that rustls is still sort of unintentionally a part of tonic's public api due to the implementation of Connected for tokio_rustls::server::TlsStream.

Specifically if i'm relying on the fact that tokio_rustls::server::TlsStream impls Connected in order to do this:

    // let tls_config = ... some custom rustls ServerConfig
    let listener = tokio_stream::wrappers::TcpListenerStream::new(
        tokio::net::TcpListener::bind("localhost:0")?,
    );
    let tls_acceptor = TlsAcceptor::from(Arc::new(tls_config));
    let listener = listener.and_then(move |stream| tls_acceptor.accept(stream));

    let greeter = MyGreeter::default();
    let server = Server::builder()
        .add_service(GreeterServer::new(greeter));
    // Requires that TlsStream impls `Connected`
    server.serve_with_incoming(listener).await;

Then when tonic bumps its internal rustls version and does a patched release you could break code out in the wild. Of course I may also be misunderstanding what would constitute a semver breaking change given this would be a somewhat obscure breakage.

[LucioFranco]

Thanks @bmwill that was missed, the next breaking release will remove that impl.

[kvc0]

For now I would suggest following this example #968 this will currently work with tonic 0.7. The future plan for tonic is to actually remove the transport module. I will be submitting a proposal for this soon.

Damn this is tough to keep up with. I'm updating a cli application that needs to be able to consume a --insecure option (like curl). Previously this was easy with rustls_client_config on the ClientConfig. Sure it was gross with the leaky transitive dependency and the ergonomics were poor but it was easy and it worked.

In the application I'm updating when --insecure is not passed I need to use rustls-native-certs or the like so I can support local users' certificates. 0.7 ClientTlsConfig, however, breaks both workflows by [1] not providing an override for the TLS verifier, [2] having no answer to RootCertStore for multiple trust roots and [3] offering no escape hatch to rustls.

This facility shouldn't have been removed without an alternative. I'm digging through https://github.com/hyperium/tonic/blob/master/examples/src/tls/client_rustls.rs to see how & whether I can integrate it; but this is a surprising upset in API stability, especially with no observable deprecation period & no discernable communication on the public documentation of a migration path.

I get that your semver leads with 0 so here be dragons and I'll take my licks for using a 0. library; but the Hyper family libraries are becoming ubiquitous and foundational - they're awesome libraries, tonic included: Stability is expected, 0. or not, and orderly migrations are paramount!

Thank you for your stewardship of these libraries. They're no small part of why I love Rust!

[LucioFranco]

Hey! @kvc0 I appreciate the feedback and I do agree there should be more proper paths for migrations and deprecations etc. That said, as you mentioned this project is still below 1.0 and with some of the other work around prost that has taken up most of my time there hasn't been any work towards a migration guides etc. I am currently in the works of writing up the full tonic 1.0 roadmap so hopefully we can get there by end of year. Or at least somewhere close enough where I would be happy to place proper deprecation notices.

That said, I don't believe hyper has bumped a major version in a while so the older versions of tonic should continue to work. If you are interested in helping out smooth the API and write some guides I would totally accept some PRs.

In addition, the plan moving forward for how people will use tonic will look closer to the rustls examples that use hyper directly as the transport module will go away. The reason I removed these APIs from that is to prep for those changes down the line.

[LucioFranco]

I've updated the example here with a more ergonomic change #1017 should make using hyper directly much easier.