Add TlsConfigHelper for additional TLS configurability#5246
Add TlsConfigHelper for additional TLS configurability#5246jack-berg merged 14 commits intoopen-telemetry:mainfrom
Conversation
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
I like this idea. Maybe we can prototype the API by offering an internal API for users to play around with and confirm all our bases are covered. Maybe something like the internal API for setting retry policy.
| } | ||
|
|
||
| /** Configures TLS for the provided OkHttp client builder by setting the SSL Socket Factory. */ | ||
| public void configure(OkHttpClient.Builder clientBuilder) { |
There was a problem hiding this comment.
How about having this accept a Consumer<SSLSocketFactory> so this isn't coupled to okhttp?
There was a problem hiding this comment.
Yeah totally....I started down this path when I realized there are like 2 or 3 other implementations doing exactly the same thing.
There was a problem hiding this comment.
So please have a look at how it ended up. Unfortunately, the managed channel stuff inside GRPC uses a different approach and I had to create two different callback interfaces. I think it worked out ok. I also wanted to be able to handle SSLException in the helper, which required the checked exception to be on the interface method.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5246 +/- ##
============================================
- Coverage 91.10% 91.03% -0.07%
- Complexity 4876 4891 +15
============================================
Files 549 550 +1
Lines 14420 14456 +36
Branches 1371 1368 -3
============================================
+ Hits 13137 13160 +23
- Misses 890 901 +11
- Partials 393 395 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Small comments plus an approach that would allow us to simplify by removing TlsConfigHelper#configureWithKeyManager.
| tlsConfigHelper.configureWithKeyManager( | ||
| (tm, km) -> | ||
| ManagedChannelUtil.setClientKeysAndTrustedCertificatesPem( | ||
| managedChannelBuilder, tm, km)); |
There was a problem hiding this comment.
Its strange that we for the other gRPC clients (otlp, jaeger) we don't bother trying to configure the managed channel with certificates. We take the stance that if you bring your own ManagedChannel, you're responsible for configuring it.
In fact, this is the only usage of ManagedChannelUtil.setClientKeysAndTrustedCertificatesPem outside of tests.
We should adopt the same policy here. Doing so allows us to move ManagedChannelUtil.setClientKeysAndTrustedCertificatesPem to test code, where its used by ManagedChannelTelemetryExporterBuilder. This allows us to simplify tlsConfigHelper as we can get rid of the configureWithKeyManager method.
There was a problem hiding this comment.
Agreed, this is interesting and I would love to simplify. I was absolutely bummed when I discovered this "other path" for configuring TLS related stuff, and unifying/simplifying would be great.
I think that removing configureWithKeyManager() from the helper would then require the DefaultGrpcServiceBuilder to call setClientKeysAndTrustedCertificatesPem() with a key manager and trust manager that it has constructed itself. I'm not sure immediately how to do with without duplicating some of the guts of TlsConfigHelper.
There was a problem hiding this comment.
I think that removing configureWithKeyManager() from the helper would then require the DefaultGrpcServiceBuilder to call setClientKeysAndTrustedCertificatesPem() with a key manager and trust manager that it has constructed itself.
I'm suggesting we drop any configuration of ManagedChannel in DefaultGrpcServiceBuilder, as we've done with the other grpc clients that allow you to "bring your own channel". If we do so, the call to setClientKeysAndTrustedCertificatesPem goes away completely. I'm happy to do this in a separate PR to avoid expanding the scope of this PR.
There was a problem hiding this comment.
I support a separate PR (which I could also do if you want), since I think it will be a user-facing change.
There was a problem hiding this comment.
Can we merge this first? There are a lot of cherries on this tree now....
|
@jkwatson please take a look when you have a chance |
seems fine to me. No API changes, so as long as it's working and it seems fine, then I'm totally fine with it. |
3 participants
Related to #5211.
Before touching the exporter builder API surface, I wanted to try and get some of these changes in first. The main goal was to allow configuring some of the TLS components directly, but it turned out to also consolidate a small amount of duplicated code between the internal GPRC and HTTP builders.
There are warnings logged if the helper is not used in an expected way (ie. parts are configured without the trust manager [the primary important thing!] being configured).
The hope is that this can pave a way forward to add a few methods to the exporter builders, that will eventually allow users to bring their own:
X509TrustManagerX509KeyManagerSSLSocketFactory...eventually providing more complete configurability of TLS within HTTP behavior, while also keeping the simple things easy.