Copilot commented on code in PR #16450:
URL: https://github.com/apache/pinot/pull/16450#discussion_r2255617984
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/StartZookeeperCommand.java:
##########
@@ -41,9 +41,20 @@ public class StartZookeeperCommand extends
AbstractBaseAdminCommand implements C
@CommandLine.Option(names = {"-zkPort"}, required = false, description =
"Port to start zookeeper server on.")
private int _zkPort = 2181;
- @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeper data.")
+ @CommandLine.Option(names = {"-dataDir"}, required = false, description =
"Directory for zookeeper data.")
Review Comment:
Fixed typo in description from 'zookeper' to 'zookeeper'.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -178,6 +376,8 @@ public void run() {
}
}.start();
+ ZkSSLUtils.clearSystemPropertiesForSSL();
Review Comment:
Clearing SSL system properties after starting an SSL-enabled ZooKeeper
server could interfere with the server's operation or with clients trying to
connect. This should only be cleared when stopping the server or when
appropriate.
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/QuickstartRunner.java:
##########
@@ -76,6 +79,8 @@ public class QuickstartRunner {
private final Map<String, Object> _configOverrides;
private final Map<String, String> _clusterConfigOverrides;
private final boolean _deleteExistingData;
+ private final boolean _zookeeperSslEnabled = RANDOM.nextBoolean();
Review Comment:
[nitpick] Making SSL enabled/disabled random for every instance could make
debugging difficult. Consider making this configurable or at least logging the
decision prominently for troubleshooting purposes.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ZkStarter.java:
##########
@@ -154,15 +269,98 @@ public static String getDefaultZkStr() {
*/
public static ZookeeperInstance startLocalZkServer(final int port) {
return startLocalZkServer(port,
- org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis());
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ null);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL() {
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT));
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param port The port to listen on
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(final int port) {
+ // Create temporary directories for SSL certificates
+ File tempDir = new File(FileUtils.getTempDirectory(), "zkData" +
System.currentTimeMillis());
+ tempDir.mkdirs();
+ String keystorePath = new File(tempDir, "keystore.jks").getAbsolutePath();
+ String truststorePath = new File(tempDir,
"truststore.jks").getAbsolutePath();
+ String password = "testpassword";
+ SSLConfig sslConfig = createTestSSLConfig(keystorePath, truststorePath,
password);
+ return startLocalZkServerWithSSL(port, sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(SSLConfig
sslConfig) {
+ return
startLocalZkServerWithSSL(NetUtils.findOpenPort(DEFAULT_ZK_TEST_PORT),
sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled on a specific port
+ * @param port The port to listen on
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public static ZookeeperInstance startLocalZkServerWithSSL(final int port,
SSLConfig sslConfig) {
+ return startLocalZkServerWithSSL(port,
+ org.apache.commons.io.FileUtils.getTempDirectoryPath() +
File.separator + "test-" + System.currentTimeMillis(),
+ sslConfig);
+ }
+
+ /**
+ * Starts a local ZooKeeper instance with SSL enabled
+ * @param port The port to listen on
+ * @param dataDirPath The path for the Zk data directory
+ * @param sslConfig SSL configuration containing keystore and truststore
paths
+ * @return A ZookeeperInstance with SSL configuration
+ */
+ public synchronized static ZookeeperInstance startLocalZkServerWithSSL(final
int port, final String dataDirPath,
+ SSLConfig sslConfig) {
+ // Configure ZooKeeper SSL system properties for server
+ System.setProperty("zookeeper.serverCnxnFactory",
"org.apache.zookeeper.server.NettyServerCnxnFactory");
+ System.setProperty("zookeeper.ssl.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.keyStore.type", "JKS");
+ System.setProperty("zookeeper.ssl.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.trustStore.password",
sslConfig.getTrustStorePassword());
+ System.setProperty("zookeeper.ssl.trustStore.type", "JKS");
+
+ // Configure secure port for SSL ZooKeeper
+ System.setProperty("zookeeper.secureClientPort", String.valueOf(port));
+ System.setProperty("zookeeper.secureClientPortAddress", "0.0.0.0");
+
+ // Enable SSL for server
+ System.setProperty("zookeeper.sslQuorum", "true");
+ System.setProperty("zookeeper.ssl.quorum.keyStore.location",
sslConfig.getKeyStorePath());
+ System.setProperty("zookeeper.ssl.quorum.keyStore.password",
sslConfig.getKeyStorePassword());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.location",
sslConfig.getTrustStorePath());
+ System.setProperty("zookeeper.ssl.quorum.trustStore.password",
sslConfig.getTrustStorePassword());
+
+ // Intentionally set clientPort to -1 to disable the regular (non-SSL)
client port for SSL-only mode.
+ // This is not an error value; it is required to prevent ZooKeeper from
listening on the non-SSL port.
+ System.setProperty("zookeeper.clientPort", "-1");
Review Comment:
[nitpick] While there is a comment explaining why clientPort is set to -1,
this magic number could be confusing. Consider using a named constant like
`DISABLE_NON_SSL_PORT = -1` to make the intent clearer.
```suggestion
System.setProperty("zookeeper.clientPort",
String.valueOf(DISABLE_NON_SSL_PORT));
```
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -877,9 +892,9 @@ protected List<ValidDocIdsMetadataInfo>
getValidDocIdsMetadata(String tableNameW
ValidDocIdsType validDocIdsType)
throws Exception {
- StringBuilder urlBuilder = new StringBuilder(
+ String responseString = sendGetRequest(
_controllerRequestURLBuilder.forValidDocIdsMetadata(tableNameWithType,
validDocIdsType.toString()));
Review Comment:
[nitpick] This appears to be an unrelated refactoring change that removes
the StringBuilder and directly calls sendGetRequest. While not incorrect, this
change is unrelated to the SSL functionality and should be separated into its
own commit.
```suggestion
StringBuilder sb = new StringBuilder();
sb.append(_controllerRequestURLBuilder.forValidDocIdsMetadata(tableNameWithType,
validDocIdsType.toString()));
String responseString = sendGetRequest(sb.toString());
```
--
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]
