Uh oh!
There was an error while loading. Please reload this page.
TEZ-4749: Support SSL/TLS connections to ZooKeeper in ZkAMRegistry and ZkAMRegistryClient - #532
Open
g3rg0 wants to merge 8 commits into
Open
TEZ-4749: Support SSL/TLS connections to ZooKeeper in ZkAMRegistry and ZkAMRegistryClient#532g3rg0 wants to merge 8 commits into
g3rg0 wants to merge 8 commits into
Conversation
…d ZkAMRegistryClient Add explicit per-connection SSL/TLS configuration for Tez's ZooKeeper connections, removing the need to rely on JVM-wide system properties. - New configuration properties: tez.am.zookeeper.ssl.enable, keystore/truststore location and password - New SSLZookeeperFactory using Netty-based secure ZK client connection - When ssl.enable is "true", CuratorFramework uses SSLZookeeperFactory; when "false", JVM-level secure properties are overridden to force insecure; when unset, existing JVM default behavior is preserved - Integration tests for secure/insecure ZK connections
tez-yetus
commented
Aug 10, 2026
💔 -1 overall
This message was automatically generated. |
added 3 commits
August 10, 2026 16:37
- Expand single-line getter methods to multi-line (LeftCurly) - Rename parameter in isValidSslEnabledValue to avoid field shadowing (HiddenField)
- Rename TEST_ROOT_DIR to testRootDir (StaticVariableName) - Fix indentation of ImmutableMap.of arguments (Indentation) - Break long AMRecord constructor line (LineLength > 120) - Add period to Javadoc first sentence (JavadocStyle)
…indbugs check The method intentionally returns null to indicate "not configured", distinguishing it from explicit true/false.
tez-yetus
commented
Aug 10, 2026
🎊 +1 overall
This message was automatically generated. |
| ); | ||
| if (isSslEnabled() == null) { | ||
| return CuratorFrameworkFactory.newClient( | ||
| getZkQuorum(), |
Contributor
There was a problem hiding this comment.
Why not use the built-in .zkClientConfig() method provided by Curator 5.x? It provides native support for managing these SSL properties. SSLZookeeperFactory.java can we entirely removed and just configure it inline inside ZkConfig.java like this:
ZKClientConfigzkClientConfig = newZKClientConfig();
zkClientConfig.setProperty(ZKClientConfig.SECURE_CLIENT, "true");
.....
.....
returnCuratorFrameworkFactory.builder()
.connectString(getZkQuorum())
// ... other settings ...
.zkClientConfig(zkClientConfig) // Built-in Curator support
.build();Author
There was a problem hiding this comment.
Makes sense. I've added a commit that removed SSLZookeeperFactory.java and uses the build-in curator config.
| <!-- TEZ-4749 --> | ||
| <Match> | ||
| <Class name="org.apache.tez.client.registry.zookeeper.ZkConfig" /> | ||
| <Method name="isSslEnabled" /> |
Contributor
There was a problem hiding this comment.
For NULL check , updating spotbugs configs seems overkill, can't we use java OPTIONAL ?
Author
There was a problem hiding this comment.
It works with Optional, too. I've added a commit with this change.
added 2 commits
August 12, 2026 11:24
…f nullable Boolean Replace nullable Boolean return with Optional<Boolean> to avoid the NP_BOOLEAN_RETURN_NULL spotbugs warning without needing a findbugs exclusion. Revert the findbugs-exclude.xml entry added previously.
…ookeeperFactory Replace SSLZookeeperFactory with Curator's built-in .zkClientConfig() builder method. The ZKClientConfig with SSL properties is now configured inline in ZkConfig.createCuratorFramework(), eliminating the need for a separate factory class.
tez-yetus
commented
Aug 12, 2026
🎊 +1 overall
This message was automatically generated. |
| <dependency> | ||
| <groupId>org.apache.curator</groupId> | ||
| <artifactId>curator-test</artifactId> | ||
| <version>${curator.version}</version> |
Contributor
There was a problem hiding this comment.
nit: No need for version tag here as dependencyManagement handles that.
| if (StringUtils.isEmpty(getZookeeperTrustStoreLocation())) { | ||
| LOG.warn("Missing trustStoreLocation parameter"); | ||
| } | ||
| zkClientConfig.setProperty(x509Util.getSslKeystoreLocationProperty(), getZookeeperKeyStoreLocation()); |
Contributor
There was a problem hiding this comment.
The above 2 if statements feel wrong. We are LOGGING the warning that parameters are missing and then continuing the use them in 2nd arg in setProperty. that can lead to excpetions like NPE or IllegalArgs.
Instead of lgging we should throw the exception there itself. Maybe IllegalArgException.
Author
There was a problem hiding this comment.
If we throw an exception here, these parameters will become mandatory instead of optional.
The keystore is only needed if zookeeper is configured for mTLS authentication, so I think it should remain optional. The truststore, on the other hand, is also optional, because if the system truststore trusts the zookeeper TLS cert, then this configuration would be redundant.
How about something like this?
if (isSslEnabled().get()) {
try (ClientX509Util x509Util = new ClientX509Util()) {
if (StringUtils.isNotEmpty(getZookeeperKeyStoreLocation())) {
zkClientConfig.setProperty(x509Util.getSslKeystoreLocationProperty(), getZookeeperKeyStoreLocation());
zkClientConfig.setProperty(x509Util.getSslKeystorePasswdProperty(), getZookeeperKeyStorePassword());
} else {
LOG.info("No keystore location configured, using ZooKeeper client defaults");
}
if (StringUtils.isNotEmpty(getZookeeperTrustStoreLocation())) {
zkClientConfig.setProperty(x509Util.getSslTruststoreLocationProperty(), getZookeeperTrustStoreLocation());
zkClientConfig.setProperty(x509Util.getSslTruststorePasswdProperty(), getZookeeperTrustStorePassword());
} else {
LOG.info("No truststore location configured, using ZooKeeper client defaults");
}
}
}
Contributor
There was a problem hiding this comment.
these parameters will become mandatory instead of optional
thats correct, I missed that part. What you are proposing looks good, wondering if we need to put isNotEmpty check on password as well. Making it a too much if nested code :-) . As I'm not a committer to this project, I would request @abstractdog , for suggestions here. Based on that we can push again.
rest all changes are good.
Contributor
There was a problem hiding this comment.
Your above solution, just small refactor
From e6ca2489dc5179e0b0cc9e3bc90385e78c31af62 Mon Sep 17 00:00:00 2001
From: Raghav Aggarwal <raghavaggarwal03.ra@gmail.com>
Date: Wed, 12 Aug 2026 23:56:47 +0530
Subject: [PATCH] TEZ-4749: Refactor
---
.../client/registry/zookeeper/ZkConfig.java | 31 ++++++++++++-------
tez-tests/pom.xml | 1 -
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/tez-api/src/main/java/org/apache/tez/client/registry/zookeeper/ZkConfig.java b/tez-api/src/main/java/org/apache/tez/client/registry/zookeeper/ZkConfig.java
index 7c3807c7c..cb8380e51 100644
--- a/tez-api/src/main/java/org/apache/tez/client/registry/zookeeper/ZkConfig.java+++ b/tez-api/src/main/java/org/apache/tez/client/registry/zookeeper/ZkConfig.java@@ -165,7 +165,7 @@ public class ZkConfig {
}
public CuratorFramework createCuratorFramework() {
- if (!isSslEnabled().isPresent()) {+ if (isSslEnabled().isEmpty()) {
return CuratorFrameworkFactory.newClient(
getZkQuorum(),
getSessionTimeoutMs(),
@@ -179,17 +179,13 @@ public class ZkConfig {
zkClientConfig.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET,
"org.apache.zookeeper.ClientCnxnSocketNetty");
if (isSslEnabled().get()) {
- ClientX509Util x509Util = new ClientX509Util();- if (StringUtils.isEmpty(getZookeeperKeyStoreLocation())) {- LOG.warn("Missing keystoreLocation parameter");+ try (ClientX509Util x509Util = new ClientX509Util()) {+ setStoreConfig(zkClientConfig, x509Util.getSslKeystoreLocationProperty(), getZookeeperKeyStoreLocation(),+ x509Util.getSslKeystorePasswdProperty(), getZookeeperKeyStorePassword(), "keystore");++ setStoreConfig(zkClientConfig, x509Util.getSslTruststoreLocationProperty(), getZookeeperTrustStoreLocation(),+ x509Util.getSslTruststorePasswdProperty(), getZookeeperTrustStorePassword(), "truststore");
}
- if (StringUtils.isEmpty(getZookeeperTrustStoreLocation())) {- LOG.warn("Missing trustStoreLocation parameter");- }- zkClientConfig.setProperty(x509Util.getSslKeystoreLocationProperty(), getZookeeperKeyStoreLocation());- zkClientConfig.setProperty(x509Util.getSslKeystorePasswdProperty(), getZookeeperKeyStorePassword());- zkClientConfig.setProperty(x509Util.getSslTruststoreLocationProperty(), getZookeeperTrustStoreLocation());- zkClientConfig.setProperty(x509Util.getSslTruststorePasswdProperty(), getZookeeperTrustStorePassword());
}
return CuratorFrameworkFactory.builder()
@@ -201,6 +197,19 @@ public class ZkConfig {
.build();
}
+ private void setStoreConfig(ZKClientConfig config, String locationProp, String locationVal, String passwordProp,+ String passwordVal, String storeName) {+ if (StringUtils.isEmpty(locationVal)) {+ LOG.info("No {} location configured, using ZooKeeper client defaults", storeName);+ return;+ }++ config.setProperty(locationProp, locationVal);+ if (StringUtils.isNotEmpty(passwordVal)) {+ config.setProperty(passwordProp, passwordVal);+ }+ }+
private boolean isValidSslEnabledValue(String value) {
return value == null || value.isEmpty()
|| value.trim().equalsIgnoreCase("true")
diff --git a/tez-tests/pom.xml b/tez-tests/pom.xml
index 3b76ed090..8920c17e6 100644
--- a/tez-tests/pom.xml+++ b/tez-tests/pom.xml@@ -136,7 +136,6 @@
<dependency>
<groupId>org.apache.curator</groupId>
<artifactId>curator-test</artifactId>
- <version>${curator.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
--
2.55.0
added 2 commits
August 12, 2026 19:42
Version is already managed by dependencyManagement in the parent POM.
Extract setStoreConfig() helper to conditionally set keystore/truststore properties only when a location is configured. Use try-with-resources for ClientX509Util and Optional.isEmpty() for readability.
Aggarwal-Raghav
commented
Aug 12, 2026
Contributor
LGTM +1, pending tests |
tez-yetus
commented
Aug 12, 2026
🎊 +1 overall
This message was automatically generated. |
abstractdog
requested changes
Aug 17, 2026
abstractdog
left a comment
Contributor
There was a problem hiding this comment.
left a few comments, basically nitpicking :)
| public void testZkConfigSslDisabled() { | ||
| Configuration conf = new Configuration(); | ||
| conf.set(TezConfiguration.TEZ_AM_ZOOKEEPER_QUORUM, "dummyZkQuorum"); | ||
| conf.set(TezConfiguration.TEZ_AM_ZOOKEEPER_SSL_ENABLE, "False"); |
Contributor
There was a problem hiding this comment.
"False" looks strange, even if it's by design, I would keep using "false", and create a separate test case to show valid values
| zkServer.stop(); | ||
| } | ||
| public void enableZookeeperSecureClientWithJVMProperties() { |
Contributor
There was a problem hiding this comment.
this can be private I think
| // information registered in registry eventually reaches the registry client | ||
| AMRecord amRecordFetched = registryClient.getRecord(appId); | ||
| while (amRecordFetched == null) { | ||
| Thread.sleep(1000); |
Contributor
There was a problem hiding this comment.
this can be tightened to 500ms I believe for this scenario to lower the worst-case lost time for each test case
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add explicit per-connection SSL/TLS configuration for Tez's ZooKeeper connections, removing the need to rely on JVM-wide system properties.