Copilot commented on code in PR #17559:
URL: https://github.com/apache/pinot/pull/17559#discussion_r2719804797
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/TlsUtils.java:
##########
@@ -369,4 +377,92 @@ static boolean
isKeyOrTrustStorePathNullOrHasFileScheme(String keyOrTrustStorePa
throw new RuntimeException(e);
}
}
+
+ private static void warnIfNonJdkProviderConfiguredInternal(String
contextName, TlsConfig tlsConfig) {
+ // In Platform-FIPS-JDK deployments, you typically want to stay on the JDK
TLS stack
+ // (JSSE provider selection happens via java.security / JVM configuration).
+ String configured = tlsConfig != null ? tlsConfig.getSslProvider() : null;
+ if (configured != null) {
+ try {
+ SslProvider sslProvider = SslProvider.valueOf(configured);
+ if (sslProvider != SslProvider.JDK) {
+ LOGGER.warn("TLS config for '{}' sets sslProvider='{}'.
Platform-FIPS-JDK deployments typically require "
+ + "sslProvider='JDK' (avoid OpenSSL).", contextName, configured);
+ }
+ } catch (Exception e) {
+ // If config is invalid, let existing code fail where it parses/builds
the context.
Review 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.
```suggestion
// 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);
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/TlsUtils.java:
##########
@@ -369,4 +377,92 @@ static boolean
isKeyOrTrustStorePathNullOrHasFileScheme(String keyOrTrustStorePa
throw new RuntimeException(e);
}
}
+
+ private static void warnIfNonJdkProviderConfiguredInternal(String
contextName, TlsConfig tlsConfig) {
+ // In Platform-FIPS-JDK deployments, you typically want to stay on the JDK
TLS stack
+ // (JSSE provider selection happens via java.security / JVM configuration).
+ String configured = tlsConfig != null ? tlsConfig.getSslProvider() : null;
+ if (configured != null) {
+ try {
+ SslProvider sslProvider = SslProvider.valueOf(configured);
+ if (sslProvider != SslProvider.JDK) {
+ LOGGER.warn("TLS config for '{}' sets sslProvider='{}'.
Platform-FIPS-JDK deployments typically require "
+ + "sslProvider='JDK' (avoid OpenSSL).", contextName, configured);
+ }
+ } catch (Exception e) {
+ // If config is invalid, let existing code fail where it parses/builds
the context.
+ }
+ }
+ }
+
+ /**
+ * Log (once) the JSSE provider/protocol actually used at runtime for the
given TLS config.
+ * <p>
+ * This is intended as a lightweight runtime self-check for
Platform-FIPS-JDK deployments: Pinot generally uses the
+ * JDK TLS stack (JSSE), and the platform/JDK decides which provider is
active via {@code java.security} and other JVM
+ * settings. This method helps surface misconfiguration early without
enforcing behavior.
+ */
+ public static void logJsseDiagnosticsOnce(String contextName, SSLFactory
sslFactory, TlsConfig tlsConfig) {
+ if (sslFactory == null) {
+ return;
+ }
+ try {
+ SSLContext sslContext = sslFactory.getSslContext();
+ String configuredSslProvider = tlsConfig != null ?
tlsConfig.getSslProvider() : null;
+ boolean insecure = tlsConfig != null && tlsConfig.isInsecure();
+ logTlsDiagnosticsOnce(contextName, sslContext, configuredSslProvider,
insecure);
+ } catch (Exception e) {
+ LOGGER.warn("TLS diagnostics ({}): failed to obtain SSLContext for
diagnostics", contextName, e);
+ }
+ }
+
+ /**
+ * Emit a warning when a non-JDK TLS stack is configured.
+ */
+ public static void warnIfNonJdkProviderConfigured(String contextName,
TlsConfig tlsConfig) {
+ warnIfNonJdkProviderConfiguredInternal(contextName, tlsConfig);
+ }
+
+ private static void logTlsDiagnosticsOnce(String contextName, SSLContext
sslContext, String configuredSslProvider,
+ boolean insecure) {
+ if (sslContext == null) {
+ return;
+ }
+ String providerName = sslContext.getProvider() != null ?
sslContext.getProvider().getName() : "null";
+ String protocol = sslContext.getProtocol();
+ String key = contextName + "|" + providerName + "|" + protocol + "|" +
configuredSslProvider + "|" + insecure;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
