From b21a4fdb563af18a3be69ce32d9556628960424d Mon Sep 17 00:00:00 2001 From: Kevin Risden Date: Thu, 5 Oct 2023 12:24:22 -0400 Subject: [PATCH 1/4] HADOOP-18922: Race condition in ZKDelegationTokenSecretManager creating znode --- .../token/delegation/ZKDelegationTokenSecretManager.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java index 2524929853cc7..b8b031877e0ab 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java @@ -270,10 +270,9 @@ public void startThreads() throws IOException { CuratorFramework nullNsFw = zkClient.usingNamespace(null); try { String nameSpace = "/" + zkClient.getNamespace(); - Stat stat = nullNsFw.checkExists().forPath(nameSpace); - if (stat == null) { - nullNsFw.create().creatingParentContainersIfNeeded().forPath(nameSpace); - } + nullNsFw.create().creatingParentContainersIfNeeded().forPath(nameSpace); + } catch (KeeperException.NodeExistsException ignore) { + // We don't care if the znode already exists } catch (Exception e) { throw new IOException("Could not create namespace", e); } From 35288f9413378dba8d911cfe4155b11f96f62c92 Mon Sep 17 00:00:00 2001 From: Kevin Risden Date: Thu, 5 Oct 2023 19:20:41 -0400 Subject: [PATCH 2/4] Remove unused import --- .../token/delegation/ZKDelegationTokenSecretManager.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java index b8b031877e0ab..925bc030c2cd8 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java @@ -60,7 +60,6 @@ import org.apache.zookeeper.client.ZKClientConfig; import org.apache.zookeeper.data.ACL; import org.apache.zookeeper.data.Id; -import org.apache.zookeeper.data.Stat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; From 8c39d70455a075b934b83956dc1b34a302951b64 Mon Sep 17 00:00:00 2001 From: Kevin Risden Date: Thu, 5 Oct 2023 19:20:52 -0400 Subject: [PATCH 3/4] Add test --- .../TestZKDelegationTokenSecretManager.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java index 469d87ab30c6f..66b07cefbab00 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java @@ -20,8 +20,13 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; import org.apache.curator.RetryPolicy; @@ -585,4 +590,52 @@ public void testCreateNameSpaceRepeatedly() throws Exception { "KeeperErrorCode = NodeExists for "+workingPath, () -> createModeStat.forPath(workingPath)); } + + @Test + public void testMultipleInit() throws Exception { + + String connectString = zkServer.getConnectString(); + RetryPolicy retryPolicy = new ExponentialBackoffRetry(1000, 3); + Configuration conf = getSecretConf(connectString); + CuratorFramework curatorFramework = + CuratorFrameworkFactory.builder() + .connectString(connectString) + .retryPolicy(retryPolicy) + .build(); + curatorFramework.start(); + ZKDelegationTokenSecretManager.setCurator(curatorFramework); + + DelegationTokenManager tm1 = new DelegationTokenManager(conf, new Text("foo")); + DelegationTokenManager tm2 = new DelegationTokenManager(conf, new Text("bar")); + // When the init method is called, + // the ZKDelegationTokenSecretManager#startThread method will be called, + // and the creatingParentContainersIfNeeded will be called to create the nameSpace. + ExecutorService executorService = Executors.newFixedThreadPool(2); + + Callable tm1Callable = () -> { + tm1.init(); + return true; + }; + Callable tm2Callable = () -> { + tm2.init(); + return true; + }; + List> futures = executorService.invokeAll(Arrays.asList(tm1Callable, tm2Callable)); + for(Future future : futures) { + Assert.assertTrue(future.get()); + } + executorService.shutdownNow(); + Assert.assertTrue(executorService.awaitTermination(1, TimeUnit.SECONDS)); + tm1.destroy(); + tm2.destroy(); + + String workingPath = "/" + conf.get(ZKDelegationTokenSecretManager.ZK_DTSM_ZNODE_WORKING_PATH, + ZKDelegationTokenSecretManager.ZK_DTSM_ZNODE_WORKING_PATH_DEAFULT) + "/ZKDTSMRoot"; + + // Check if the created NameSpace exists. + Stat stat = curatorFramework.checkExists().forPath(workingPath); + Assert.assertNotNull(stat); + + curatorFramework.close(); + } } From 26c2333e3d950fb425b7ef843990bd48b7569884 Mon Sep 17 00:00:00 2001 From: Kevin Risden Date: Fri, 6 Oct 2023 08:32:38 -0400 Subject: [PATCH 4/4] Fix line length checkstyle --- .../token/delegation/TestZKDelegationTokenSecretManager.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java index 66b07cefbab00..2312af3c79dfa 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java @@ -620,7 +620,8 @@ public void testMultipleInit() throws Exception { tm2.init(); return true; }; - List> futures = executorService.invokeAll(Arrays.asList(tm1Callable, tm2Callable)); + List> futures = executorService.invokeAll( + Arrays.asList(tm1Callable, tm2Callable)); for(Future future : futures) { Assert.assertTrue(future.get()); }