Copilot commented on code in PR #17776: URL: https://github.com/apache/pinot/pull/17776#discussion_r2867326940
########## pinot-common/src/test/java/org/apache/pinot/common/utils/tls/JvmDefaultSslContextTest.java: ########## @@ -0,0 +1,60 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.utils.tls; + +import java.lang.reflect.Field; +import org.testng.Assert; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.Test; + +public class JvmDefaultSslContextTest { + private static final String JVM_TRUST_STORE = "javax.net.ssl.trustStore"; + + @AfterMethod + public void cleanup() throws Exception { + System.clearProperty(JVM_TRUST_STORE); + setInitialized(false); Review Comment: This test clears `javax.net.ssl.trustStore` and forces `JvmDefaultSslContext._initialized=false` after each method. If the test JVM was started with an existing trustStore system property (or if other tests in the same JVM rely on it), this can cause cross-test interference. Consider capturing the previous property value / initialized state in `@BeforeMethod` and restoring them in `@AfterMethod` instead of always clearing/resetting. ```suggestion import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; public class JvmDefaultSslContextTest { private static final String JVM_TRUST_STORE = "javax.net.ssl.trustStore"; private String _originalTrustStore; private boolean _originalInitialized; @BeforeMethod public void setUp() throws Exception { _originalTrustStore = System.getProperty(JVM_TRUST_STORE); _originalInitialized = isInitialized(); } @AfterMethod public void cleanup() throws Exception { if (_originalTrustStore != null) { System.setProperty(JVM_TRUST_STORE, _originalTrustStore); } else { System.clearProperty(JVM_TRUST_STORE); } setInitialized(_originalInitialized); ``` ########## pinot-common/src/main/java/org/apache/pinot/common/utils/tls/JvmDefaultSslContext.java: ########## @@ -101,8 +122,9 @@ public static synchronized void initDefaultSslContext() { .map(String::trim).filter(StringUtils::isNotBlank).orElse(null); RenewableTlsUtils.enableAutoRenewalFromFileStoreForSSLFactory(jvmSslFactory, jvmKeystoreType, jvmKeyStorePath, jvmKeystorePassword, jvmTrustStoreType, jvmTrustStorePath, jvmTrustStorePassword, null, null, () -> false); Review Comment: The `RenewableTlsUtils.enableAutoRenewalFromFileStoreForSSLFactory(...)` call uses `jvmKeystoreType`, but `jvmKeystoreType` is currently derived from `javax.net.ssl.trustStoreType` (same property used for `jvmTrustStoreType`) instead of `javax.net.ssl.keyStoreType`. This can ignore an explicitly configured keystore type and cause SSLFactory identity material loading to fail when keystore/truststore types differ. ########## pinot-plugins/pinot-stream-ingestion/pinot-kafka-base/src/main/java/org/apache/pinot/plugin/stream/kafka/KafkaSSLUtils.java: ########## @@ -59,7 +59,8 @@ private KafkaSSLUtils() { private static final String DEFAULT_KEY_ALGORITHM = "RSA"; private static final String DEFAULT_KEYSTORE_TYPE = "PKCS12"; private static final String DEFAULT_SECURITY_PROTOCOL = "SSL"; - private static final String DEFAULT_TRUSTSTORE_TYPE = "jks"; + // Follow the JVM default keystore type (typically "jks") unless explicitly configured. Review Comment: The comment says the JVM default keystore type is "typically \"jks\"", but on modern JDKs (including Java 11) `KeyStore.getDefaultType()` is commonly "pkcs12". To avoid misleading operators, consider rewording this comment to be version-agnostic (e.g., "depends on JVM 'keystore.type'"), or mention both common defaults. ```suggestion // Follow the JVM default keystore type (from 'keystore.type', commonly "jks" or "pkcs12") unless explicitly configured. ``` -- 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]
