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.

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

fix(tonic-build,tonic) Add back TLS handling in generated Client::connect code #1866

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

mkatychev
Copy link
Contributor

@mkatychev mkatychev commented Aug 9, 2024

Motivation

The recent breaking change in #1731 removed the ability for generated client functions to connect to HTTPS endpoints through strings alone.
For example the doctest below would fail in 0.12.x tonic-build connection code:

/// ```
/// # use tonic::transport::Endpoint;
/// Endpoint::from_static("https://example.com");
/// ```

Solution

This PR introduces a best attempt at picking up tls feature flags in Endpoint::new to allow successfully connecting to strings that contain https:// once more

@mkatychev mkatychev force-pushed the fix/tls-codegen-failure branch from a8737f5 to 55bb186 Compare August 9, 2024 19:51
@mkatychev
Copy link
Contributor Author

@tottoto @djc Let me know if there is a better approach to handle generated clients. 0.12 ::connect does not work with TLS anymore.

@mkatychev mkatychev changed the title Add back TLS handling in genereated Client::connect code fix(tonic-build) Add back TLS handling in genereated Client::connect code Aug 9, 2024
@mkatychev
Copy link
Contributor Author

mkatychev commented Aug 9, 2024

Though this fix is in tonic, as mentioned in

// FIXME: determine if we want to expose this or not. This is really
// just used in codegen for a shortcut.

Endpoint::new is mainly used in generated code through tonic-build.

@mkatychev mkatychev changed the title fix(tonic-build) Add back TLS handling in genereated Client::connect code fix(tonic-build,tonic) Add back TLS handling in genereated Client::connect code Aug 9, 2024
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Seems a little ugly but probably still better to fix the regression for now.

@djc djc added this pull request to the merge queue Aug 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 19, 2024
…onnect` code (#1866)

* tls feature flag for Endpoint::new

* added unit test
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 19, 2024
@tottoto
Copy link
Collaborator

tottoto commented Aug 20, 2024

Personally, I do not like this kind of implicit configuration and would prefer that users explicitly configure TLS. It seems better to update the documentation instead.

@djc
Copy link
Contributor

djc commented Aug 20, 2024

Personally, I do not like this kind of implicit configuration and would prefer that users explicitly configure TLS. It seems better to update the documentation instead.

I'm sympathetic to this but I think regressing user's code in an upgrade is worse, and I don't think adding documentation is enough. We could improve this via the type system instead.

@mkatychev
Copy link
Contributor Author

@tottoto I would be happy to hear of alternatives, #1731 for better or worse took a lot of convenience out of a TLS approach that "just works".
Long term, From<Uri> for Endpoint should be where a TLS config decision is made, this should enable getting rid of Endpoint::new entirely.
A Uri's scheme should determine whether TLS is enabled if some feature flag like auto_tls is enabled. It's fine to be explicit about configuring TLS but I would rather not do it on each connection and a non-default feature flag seems explicit enough.

@mkatychev mkatychev force-pushed the fix/tls-codegen-failure branch from 8688bf9 to 74a42fb Compare August 20, 2024 14:07
@tottoto
Copy link
Collaborator

tottoto commented Aug 20, 2024

It's fine to be explicit about configuring TLS but I would rather not do it on each connection and a non-default feature flag seems explicit enough.

You can define your own constructor such as function to construct connection with your config.

@mkatychev
Copy link
Contributor Author

It's fine to be explicit about configuring TLS but I would rather not do it on each connection and a non-default feature flag seems explicit enough.

You can define your own constructor such as function to construct connection with your config.

That is not the case for generated code.

@djc djc added this pull request to the merge queue Aug 21, 2024
Merged via the queue into hyperium:master with commit 22475d8 Aug 21, 2024
16 checks passed
@djc djc changed the title fix(tonic-build,tonic) Add back TLS handling in genereated Client::connect code fix(tonic-build,tonic) Add back TLS handling in generated Client::connect code Aug 21, 2024
@djc djc mentioned this pull request Aug 21, 2024
1 task
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.

3 participants