fix(tonic-build,tonic) Add back TLS handling in generated `Client::connect` code by mkatychev · Pull Request #1866 · hyperium/tonic

mkatychev · 2024-08-09T19:47:25Z

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:

tonic/tonic/src/transport/channel/endpoint.rs

Lines 79 to 82 in 03913b9

    
               /// ``` 
        
               /// # 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 · 2024-08-09T20:06:05Z

@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 · 2024-08-09T20:10:29Z

Though this fix is in tonic, as mentioned in

tonic/tonic/src/transport/channel/endpoint.rs

Lines 43 to 44 in decbf61

    
           // 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.

djc

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

…onnect` code (#1866) * tls feature flag for Endpoint::new * added unit test

tottoto · 2024-08-20T10:32:11Z

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 · 2024-08-20T10:34:08Z

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 · 2024-08-20T13:51:19Z

@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.

tottoto · 2024-08-20T14:27:58Z

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 · 2024-08-20T14:39:29Z

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.

PR hyperium#1866 fixed the breaking change introduced in hyperium#1731, but resets the `tls_config` instead of adding the tls roots to existing config. This patch resolves the regression and also restores expected behaviour.

PR hyperium#1866 fixed the breaking change introduced in hyperium#1731, but resets the TLS config without checking if `tls` is set. This patch resolves the regression and restores expected behaviour.

* fix: tls config overwrite in endpoint PR #1866 fixed the breaking change introduced in #1731, but resets the TLS config without checking if `tls` is set. This patch resolves the regression and restores expected behaviour. * fix: cargo fmt whitespace check --------- Co-authored-by: vigneshwar.sm <[email protected]> Co-authored-by: Lucio Franco <[email protected]>

* Add From<T> for Response<T> (#1064) Co-authored-by: tottoto <[email protected]> * chore: Add getrandom and wasi crate to cargo-deny skip config (#2169) * chore(examples): Update to rand 0.9 (#2168) * chore(interop): Replace repeat and take with repeat_n (#2170) * Update LICENSE (#2167) * chore(transport): Update url to http crate document (#2173) * chore: Refactor redundant pattern match (#2174) * chore(transport): Remove redundant type reconstruct (#2176) * chore: Update to strum 0.27 (#2180) * feat: optional `SSLKEYLOGFILE` support (#1539) Add a `use_key_log` option to server and client TLS configs that -- when set -- will enable rustls's `SSLKEYLOGFILE` handling. This is helpful when you want to intercept TLS traffic for debugging and is generally supported by many libraries and browsers. Also see: https://wiki.wireshark.org/TLS#using-the-pre-master-secret * chore: Remove html_root_url (#2184) * chore: Remove unused mutability (#2183) * chore: Update generated code (#2222) * chore: Update cargo-deny config (#2210) * chore: Add rustix and linux-raw-sys crate to cargo-deny skip config * chore: Ignore RUSTSEC-2024-0436 * Remove unnecessary mut (#2219) * remove unnecessary mut * remove unnecessary mut for health_reporter --------- Co-authored-by: tottoto <[email protected]> * chore: fix some comments (#2224) Signed-off-by: jimmycathy <[email protected]> Co-authored-by: tottoto <[email protected]> * feat: Allow convert i32 to Code in const context (#2195) * chore: Disable unused tower feature (#2196) * chore(router): Remove unnecessary body type converting (#2214) * chore(server): Use standard library pin macro (#2212) * chore(build): Use idiomatic api (#2211) * feat(tonic): Exclude benches-disabled to remove Apache-2.0 resource (#2204) * chore(ci): Add concurrency group to cancel old ci (#2202) * chore(test): Use library crate in test (#2201) * chore: Remove unused rand crate from dev-dependencies (#2198) * chore: Remove documentation config in manifest (#2193) * chore(test): Remove unnecessary macro_use (#2200) * feat: Add proto header to generated code (#2205) * chore(router): Use upstream poll_ready to implement service (#2215) * feat(router): Use infallible as error type (#2232) * chore: Remove unnecessary license file from private crate (#2203) * chore: update changelog to point to releases (#2235) * chore: fix changelog header * chore(server): Remove import sleep and pending function (#2234) * chore(server): Refactor default http2 keepalive timeout config (#2213) * chore: Remove unnecessary docs.rs config (#2223) * feat(transport): add support for uds, unix domain socket (#2218) * feat(transport): add support for uds, unix domain socket (#2218) Previously the uds support lives as an example in the `example/src/uds` folder. Endpoint is refactored to support multiple endpoint types, including Uri and Uds. The supported unix domain socket URI follows RFC-3986 which is aligned with the gRPC naming convention. - unix:relative_path - unix:///absolute_path References: - https://datatracker.ietf.org/doc/html/rfc3986 - https://github.com/grpc/grpc/blob/master/doc/naming.md * fix feature flag error * fix windows build * fix windows build 2 * fix windows build 3 * fix windows build 4 * fix windows build 5 --------- Co-authored-by: Lucio Franco <[email protected]> * Handle stream error correctly (#2199) Co-authored-by: Lucio Franco <[email protected]> * chore: Remove resolved cargo-deny config (#2230) * Create place for grpc crate and initial contents (#2192) * Create place for grpc crate and initial contents * Cargo.toml fixes * clippy * clippy 2 * 3 * grpc-web: relax bounds for inner service's response body (#2245) * grpc-web: relax bounds for inner service's response body * address feedback * chore(test): Allow clippy::doc_overindented_list_items lint in generated code (#2246) * chore(test): Update to rand 0.9 (#2236) * chore(router): Remove unnecessary type converting (#2237) * chore(ci): Update to nightly-2025-03-27 on udeps ci (#2242) * chore(codegen): Update to protox 0.8 (#2254) * chore(ci): Remove deny job (#2255) Removing the deny ci job it has become more of a pain to manage than actually helpful. * feat: preserve request user-agent (#2250) Co-authored-by: Lucio Franco <[email protected]> * feat(server): Add method to get local addr to TcpIncoming (#2233) * feat: expose Status as a Response extension (#2145) Co-authored-by: Lucio Franco <[email protected]> * chore(server): Remove unnecessary await service ready (#2258) * chore: Use symbolic link for license file (#2241) * chore: Use inline format argument (#2260) * chore: Add `flake.nix` (#2261) * chore: Fix interop test certs (#2262) * chore: Fix interop test certs * fix bash script: * fix: tls config overwrite in endpoint (#2252) * fix: tls config overwrite in endpoint PR #1866 fixed the breaking change introduced in #1731, but resets the TLS config without checking if `tls` is set. This patch resolves the regression and restores expected behaviour. * fix: cargo fmt whitespace check --------- Co-authored-by: vigneshwar.sm <[email protected]> Co-authored-by: Lucio Franco <[email protected]> * chore(tonic-bench): Fix failing bench (#2207) Co-authored-by: Lucio Franco <[email protected]> * feat: expose creation of HealthService and HealthReporter (#2251) * Expose creation of HealthService and HealthReporter * add default impl for HealthReporter * [spr] initial version (#2264) Created using spr 1.3.6-beta.1 * Revert "[spr] initial version (#2264)" (#2265) * chore: Prepare `v0.13.1` release Reviewers: Pull Request: #2266 --------- Signed-off-by: jimmycathy <[email protected]> Co-authored-by: Amr Hassan <[email protected]> Co-authored-by: tottoto <[email protected]> Co-authored-by: Maxim Evtush <[email protected]> Co-authored-by: Marco Neumann <[email protected]> Co-authored-by: DAKAI, TZOU <[email protected]> Co-authored-by: jimmycathy <[email protected]> Co-authored-by: Adam Basfop Cavendish <[email protected]> Co-authored-by: Jakub Łabor <[email protected]> Co-authored-by: Doug Fawley <[email protected]> Co-authored-by: Brandon Williams <[email protected]> Co-authored-by: Darren Bolduc <[email protected]> Co-authored-by: Ferenc Tamás <[email protected]> Co-authored-by: Vigneshwar S <[email protected]> Co-authored-by: vigneshwar.sm <[email protected]> Co-authored-by: Rafael RL <[email protected]> Co-authored-by: Leon Hartley <[email protected]>

* chore: Use symbolic link for license file (hyperium#2241) * chore: Use inline format argument (hyperium#2260) * chore: Add `flake.nix` (hyperium#2261) * chore: Fix interop test certs (hyperium#2262) * chore: Fix interop test certs * fix bash script: * fix: tls config overwrite in endpoint (hyperium#2252) * fix: tls config overwrite in endpoint PR hyperium#1866 fixed the breaking change introduced in hyperium#1731, but resets the TLS config without checking if `tls` is set. This patch resolves the regression and restores expected behaviour. * fix: cargo fmt whitespace check --------- Co-authored-by: vigneshwar.sm <[email protected]> Co-authored-by: Lucio Franco <[email protected]> * chore(tonic-bench): Fix failing bench (hyperium#2207) Co-authored-by: Lucio Franco <[email protected]> * feat: expose creation of HealthService and HealthReporter (hyperium#2251) * Expose creation of HealthService and HealthReporter * add default impl for HealthReporter * [spr] initial version (hyperium#2264) Created using spr 1.3.6-beta.1 * Revert "[spr] initial version (hyperium#2264)" (hyperium#2265) * chore: Prepare `v0.13.1` release Reviewers: Pull Request: hyperium#2266 * chore: Disable unused tower feature (hyperium#2270) * chore(ci): Set RUSTFLAGS only on check job (hyperium#2271) * chore: Update to webpki-roots 1 (hyperium#2269) * chore(ci): Update to cargo-check-external-types 0.2 (hyperium#2272) * chore(ci): Set token permission to read (hyperium#2275) * chore(test): Remove cargo-machete config (hyperium#2278) * chore: Remove unused dependency (hyperium#2277) * feat: add support for tower's load-shed layer (hyperium#2189) Refs: hyperium#1616 * chore: box Status contents (hyperium#2253) (hyperium#2282) * chore: box Status contents (hyperium#2253) * chore: use private into_status method * chore(doc): Fix outdated limit in comment (hyperium#2297) * fix outdated limit in comment This mention was missed when the default changed in hyperium#1335 * Add backquotes Co-authored-by: tottoto <[email protected]> --------- Co-authored-by: tottoto <[email protected]> * feat: Update to prost 0.14 (hyperium#2300) * chore: Start development of version 0.14 (hyperium#2303) * chore(build): Make empty client and server modules private (hyperium#2291) Co-authored-by: Lucio Franco <[email protected]> Co-authored-by: tottoto <[email protected]> * chore(test): Simplify skip debug test (hyperium#2305) * chore(ci): Exclude semver check of unreleased crate (hyperium#2304) * feat(types): Update error_details.proto to a56cbf3b (hyperium#2286) (hyperium#2306) * chore(test): Remove unnecessary prost-build dependency (hyperium#2307) * feat(transport): Allow setting TCP_KEEPINTVL and TCP_KEEPCNT (hyperium#2299) * Allow setting TCP_KEEPINTVL and TCP_KEEPCNT * fix windows * fix clippy windows * fix(tonic): make `Streaming` `Sync` again (hyperium#2293) The boxed `Decoder` inside `Streaming` need not be `Sync` since hyperium#804. Unfortunately, that makes `Streaming` non-`Sync`, meaning that all the generated `tonic` futures cannot be awaited in `Sync` futures. In fact, the only times we use the `Decoder`, we have a `&mut` unique reference to it, so we are guaranteed not to require synchronization. The `sync_wrapper` crate encodes this reasoning, allowing us to safely make the `Streaming` type `Sync` regardless of whether the contained `Decoder` is `Sync` or not. * feat: preserve user-agent header (hyperium#2290) Co-authored-by: Lucio Franco <[email protected]> * chore: Make publish script portable (hyperium#2313) * feat(tls): Add tls handshake timeout support (hyperium#2309) Co-authored-by: tottoto <[email protected]> * docs(tonic-build): remove doc-difference between `lib.rs` and Readme (hyperium#2308) * doc: try to improve the doc-disparity between `tonic-build`'s `lib.rs` and the readme * Fix indentation * fix the doctests * fix a typo and retes an assumption * Change `compile_fail` to `ignore` Co-authored-by: tottoto <[email protected]> --------- Co-authored-by: tottoto <[email protected]> * Merge changes from next to master branch (hyperium#2315) Co-authored-by: Arjan Singh Bal <[email protected]> Co-authored-by: Easwar Swaminathan <[email protected]> * chore(interop): Update to console 0.16 (hyperium#2318) * chore(test): Remove unnecessary tokio-stream dependency (hyperium#2311) * chore(test): Refactor default stubs test (hyperium#2310) * chore(test): Use tempfile crate to handle temporary file (hyperium#2325) * chore(grpc): Update to rand 0.9 (hyperium#2324) --------- Co-authored-by: tottoto <[email protected]> Co-authored-by: Lucio Franco <[email protected]> Co-authored-by: Vigneshwar S <[email protected]> Co-authored-by: vigneshwar.sm <[email protected]> Co-authored-by: Rafael RL <[email protected]> Co-authored-by: Leon Hartley <[email protected]> Co-authored-by: Joe Roback <[email protected]> Co-authored-by: Raphael Taylor-Davies <[email protected]> Co-authored-by: Alexis Darrasse <[email protected]> Co-authored-by: Alex Steele <[email protected]> Co-authored-by: Kristopher Wuollett <[email protected]> Co-authored-by: Makro <[email protected]> Co-authored-by: James Kay <[email protected]> Co-authored-by: Darren Bolduc <[email protected]> Co-authored-by: Honsun Zhu <[email protected]> Co-authored-by: Frank Elsinga <[email protected]> Co-authored-by: Arjan Singh Bal <[email protected]> Co-authored-by: Easwar Swaminathan <[email protected]>

mkatychev added 2 commits August 9, 2024 14:39

tls feature flag for Endpoint::new

03913b9

added unit test

55bb186

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

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 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

djc approved these changes Aug 19, 2024

View reviewed changes

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

fix(tonic-build,tonic) Add back TLS handling in genereated `Client::c…

7831249

…onnect` code (#1866) * tls feature flag for Endpoint::new * added unit test

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 19, 2024

mkatychev added 2 commits August 20, 2024 09:06

Simplified Endpoint::new initialization

de5cec8

Merge branch 'master' into fix/tls-codegen-failure

74a42fb

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

djc approved these changes Aug 21, 2024

View reviewed changes

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

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 mentioned this pull request Aug 21, 2024

chore: prepare 0.12.2 release #1881

Merged

1 task

str4d mentioned this pull request Aug 30, 2024

0.12 reports invalid cert where 0.11 succeeded #1904

Closed

vigneshs-12 mentioned this pull request Apr 11, 2025

fix: tls config overwrite in endpoint #2252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

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

fix(tonic-build,tonic) Add back TLS handling in generated `Client::connect` code#1866
djc merged 4 commits intohyperium:masterfrom
knox-networks:fix/tls-codegen-failure

mkatychev commented Aug 9, 2024 •

edited

Loading

Uh oh!

mkatychev commented Aug 9, 2024

Uh oh!

mkatychev commented Aug 9, 2024 •

edited

Loading

Uh oh!

djc left a comment

Uh oh!

Uh oh!

tottoto commented Aug 20, 2024

Uh oh!

djc commented Aug 20, 2024 •

edited

Loading

Uh oh!

mkatychev commented Aug 20, 2024

Uh oh!

tottoto commented Aug 20, 2024

Uh oh!

mkatychev commented Aug 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Comments

Conversation

mkatychev commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Uh oh!

mkatychev commented Aug 9, 2024

Uh oh!

mkatychev commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tottoto commented Aug 20, 2024

Uh oh!

djc commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkatychev commented Aug 20, 2024

Uh oh!

tottoto commented Aug 20, 2024

Uh oh!

mkatychev commented Aug 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkatychev commented Aug 9, 2024 •

edited

Loading

mkatychev commented Aug 9, 2024 •

edited

Loading

djc commented Aug 20, 2024 •

edited

Loading