Add runtime TLS diagnostics for gRPC and HTTPS#17559
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds runtime TLS diagnostics logging to help identify SSL/TLS configuration issues, particularly for Platform-FIPS-JDK deployments. It introduces centralized diagnostic utilities that warn when non-JDK SSL providers are configured and logs once per context which JSSE provider, protocol, and enabled protocols are active.
Changes:
- Introduced centralized TLS diagnostic utilities in
TlsUtilsfor logging SSL provider/protocol information and warning about non-JDK providers - Added diagnostic logging at gRPC server/client and HTTPS initialization points
- Replaced inline SSL provider warning in
BrokerGrpcServerwith centralized diagnostic utilities
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pinot-common/src/main/java/org/apache/pinot/common/utils/tls/TlsUtils.java | Core implementation of TLS diagnostic utilities including once-per-context logging and FIPS deployment warnings |
| pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java | Adds diagnostic logging for gRPC query server TLS context |
| pinot-common/src/main/java/org/apache/pinot/common/utils/grpc/BaseGrpcQueryClient.java | Adds diagnostic logging for gRPC query client TLS context |
| pinot-broker/src/main/java/org/apache/pinot/broker/grpc/BrokerGrpcServer.java | Replaces inline warning with centralized diagnostic utilities |
| + "sslProvider='JDK' (avoid OpenSSL).", contextName, configured); | ||
| } | ||
| } catch (Exception e) { | ||
| // If config is invalid, let existing code fail where it parses/builds the context. |
There was a problem hiding this comment.
The empty catch block swallows all exceptions without logging. If SslProvider.valueOf() throws an exception due to an invalid configuration, this silent failure makes debugging difficult. Consider logging at debug level to help trace configuration issues while still letting the existing code handle the error.
| // If config is invalid, let existing code fail where it parses/builds the context. | |
| // If config is invalid, let existing code fail where it parses/builds the context. | |
| LOGGER.debug("TLS config for '{}' has invalid sslProvider value '{}'; skipping provider warning. " | |
| + "The TLS context builder will handle this configuration error.", contextName, configured, e); |
| } | ||
| String providerName = sslContext.getProvider() != null ? sslContext.getProvider().getName() : "null"; | ||
| String protocol = sslContext.getProtocol(); | ||
| String key = contextName + "|" + providerName + "|" + protocol + "|" + configuredSslProvider + "|" + insecure; |
There was a problem hiding this comment.
Using string concatenation with hardcoded delimiters to create a composite key is fragile and can lead to collisions if any component contains the delimiter character. Consider using a record class or a composite key object to ensure uniqueness and improve maintainability.
Log once at runtime which JSSE provider/protocol and enabled protocols are active for HTTPS and gRPC TLS contexts. Also warn when non-JDK SSL providers are configured, which can conflict with Platform-FIPS-JDK deployments.
xiangfu0
force-pushed
the
fips-grpc-tls-diagnostics
branch
from
3c876c2 to
1e0d7f6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17559 +/- ##
============================================
- Coverage 63.18% 63.15% -0.03%
+ Complexity 1477 1476 -1
============================================
Files 3172 3172
Lines 189773 189826 +53
Branches 29041 29052 +11
============================================
- Hits 119913 119894 -19
- Misses 60547 60613 +66
- Partials 9313 9319 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Log once at runtime which JSSE provider/protocol and enabled protocols are active for HTTPS and gRPC TLS contexts.
Also warn when non-JDK SSL providers are configured, which can conflict with Platform-FIPS-JDK deployments.