-
Notifications
You must be signed in to change notification settings - Fork 586
build: replicate tonic tls features #3180
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
base: main
Are you sure you want to change the base?
build: replicate tonic tls features #3180
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3180 +/- ##
=====================================
Coverage 80.5% 80.5%
=====================================
Files 126 126
Lines 22238 22238
=====================================
Hits 17921 17921
Misses 4317 4317 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jeremie Drouet <[email protected]>
Signed-off-by: Jeremie Drouet <[email protected]>
d55af6e to
af00d1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jdrouet thanks for raising this!
Have put a couple comments inline; perhaps we can do this in a non-breaking way without it getting too messy
| gzip-http = ["flate2"] | ||
| zstd-http = ["zstd"] | ||
| tls = ["tonic/tls-ring"] | ||
| tls = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tonic feature flags impl referenced:
_tls-any = ["dep:tokio", "tokio?/rt", "tokio?/macros", "tls-connect-info"] # Internal. Please choose one of `tls-ring` or `tls-aws-lc`
tls-ring = ["_tls-any", "tokio-rustls/ring"]
tls-aws-lc = ["_tls-any", "tokio-rustls/aws-lc-rs"]
I believe the intention is that "_tls-any" is an "internal" feature, not something the user is intended to rely on, and is what's then used to guard the backend-independent TLS code.
With this in mind, and with an eye to maintaining backward compatibility, how about:
_tls-any = [] # Internal feature for TLS guards
tls-ring = ["_tls-any", "tonic/tls-ring"] # User opts into ring
tls-aws-lc = ["_tls-any", "tonic/tls-aws-lc"] # User opts into aws-lc
tls = ["tls-ring"] # Default to existing behaviour
This would need code changes too to move from tls to _tls-any in the feature guards - e.g. a bunch of stuff in exporter/tonic/mod:
opentelemetry-rust/opentelemetry-otlp/src/exporter/tonic/mod.rs
Lines 11 to 12 in 51eac97
| #[cfg(feature = "tls")] | |
| use tonic::transport::ClientTlsConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this suggestion stretches the scope a little here, it would be great if one of the others piped in - @lalitb @bantonsson , does this seem reasonable to you?
Changes
ringis not maintained anymore and could lead to compilation issues. We need a way to use the tonic exporter with https without relying onring. This PR replicates the waytonichandles TLS by separating theringfeature from the others.This PR also introduces a
tls-aws-lcfeature so that users can rely on it.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes