From ba387f2b3a8698d075346fb518ab7bdd21c42e22 Mon Sep 17 00:00:00 2001
From: Kezhu Wang <kezhuw@apache.org>
Date: Sat, 4 Jan 2025 00:54:59 +0800
Subject: [PATCH] CURATOR-728: Not issue ZooKeeper::create if possible in
ZkPaths::mkdirs
`ZkPaths::mkdir("/bar/foo")` will not run into `NoAuthException` if
"/bar/foo" exists, and we have `READ` permission to "/bar/foo" but not
`CREATE` permission to "/bar".
---
.../org/apache/curator/utils/ZKPaths.java | 23 ++++++-------
.../curator/framework/imps/TestCreate.java | 5 ++-
.../framework/imps/TestEnsureContainers.java | 33 +++++++++++++++++++
curator-test-zk37/pom.xml | 2 +-
curator-test-zk38/pom.xml | 2 +-
5 files changed, 51 insertions(+), 14 deletions(-)
diff --git a/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java b/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java
index 2bc47544e..e251a356e 100644
--- a/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java
+++ b/curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java
@@ -282,28 +282,29 @@ public static void mkdirs(ZooKeeper zookeeper, String path, boolean makeLastNode
*/
public static void mkdirs(
ZooKeeper zookeeper,
- String path,
+ final String path,
boolean makeLastNode,
InternalACLProvider aclProvider,
boolean asContainers)
throws InterruptedException, KeeperException {
PathUtils.validatePath(path);
- int pos = path.length();
- String subPath;
-
- // This first loop locates the first parent that doesn't exist from leaf nodes towards root
+ // This first loop locates the first node that doesn't exist from leaf nodes towards root
// this way, it is not required to have read access on all parents.
// This is relevant after https://issues.apache.org/jira/browse/ZOOKEEPER-2590.
+ int pos = path.length();
do {
+ String subPath = path.substring(0, pos);
+ if (zookeeper.exists(subPath, false) != null) {
+ break;
+ }
pos = path.lastIndexOf(PATH_SEPARATOR_CHAR, pos - 1);
- subPath = path.substring(0, pos);
- } while (pos > 0 && zookeeper.exists(subPath, false) == null);
+ } while (pos > 0);
// Start creating the subtree after the longest path that exists, assuming root always exists.
- do {
+ while (pos < path.length()) {
pos = path.indexOf(PATH_SEPARATOR_CHAR, pos + 1);
if (pos == -1) {
@@ -314,7 +315,7 @@ public static void mkdirs(
}
}
- subPath = path.substring(0, pos);
+ String subPath = path.substring(0, pos);
// All the paths from the initial `pos` do not exist.
try {
@@ -332,8 +333,8 @@ public static void mkdirs(
} catch (KeeperException.NodeExistsException ignore) {
// ignore... someone else has created it since we checked
}
-
- } while (pos < path.length());
+ }
+ ;
}
/**
diff --git a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestCreate.java b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestCreate.java
index 4fccd4c6e..9f4076e66 100644
--- a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestCreate.java
+++ b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestCreate.java
@@ -47,6 +47,7 @@
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
+import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
public class TestCreate extends BaseClassForTests {
@@ -684,6 +685,7 @@ public void testProtectedUtils() throws Exception {
* but just to the first ancestor parent. (See https://issues.apache.org/jira/browse/ZOOKEEPER-2590)
*/
@Test
+ @Tag("ZOOKEEPER-2590")
public void testForbiddenAncestors() throws Exception {
CuratorFramework client = createClient(testACLProvider);
try {
@@ -696,9 +698,10 @@ public void testForbiddenAncestors() throws Exception {
// In creation attempts where the parent ("/bat") has ACL that restricts read, creation request fails.
try {
- client.create().creatingParentsIfNeeded().forPath("/bat/bost");
+ client.create().creatingParentsIfNeeded().forPath("/bat/foo/bost");
fail("Expected NoAuthException when not authorized to read new node grandparent");
} catch (KeeperException.NoAuthException noAuthException) {
+ assertEquals(noAuthException.getPath(), "/bat");
}
// But creating a node in the same subtree where its grandparent has read access is allowed and
diff --git a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestEnsureContainers.java b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestEnsureContainers.java
index 1c1101ae2..bcfed5a21 100644
--- a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestEnsureContainers.java
+++ b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestEnsureContainers.java
@@ -19,14 +19,20 @@
package org.apache.curator.framework.imps;
+import static org.apache.zookeeper.ZooDefs.Ids.ANYONE_ID_UNSAFE;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.util.Collections;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.framework.CuratorFrameworkFactory;
import org.apache.curator.framework.EnsureContainers;
import org.apache.curator.retry.RetryOneTime;
import org.apache.curator.test.BaseClassForTests;
import org.apache.curator.utils.CloseableUtils;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.ZooDefs;
+import org.apache.zookeeper.data.ACL;
import org.junit.jupiter.api.Test;
public class TestEnsureContainers extends BaseClassForTests {
@@ -63,4 +69,31 @@ public void testSingleExecution() throws Exception {
CloseableUtils.closeQuietly(client);
}
}
+
+ @Test
+ public void testNodeExistsButNoCreatePermission() throws Exception {
+ CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1));
+ try {
+ client.start();
+
+ // given: "/bar/foo" created
+ client.create().creatingParentsIfNeeded().forPath("/bar/foo");
+ // given: only permission read to "/bar"
+ client.setACL()
+ .withACL(Collections.singletonList(new ACL(ZooDefs.Perms.READ, ANYONE_ID_UNSAFE)))
+ .forPath("/bar");
+
+ // check: create "/bar/foo" will fail with NoAuth
+ assertThrows(KeeperException.NoAuthException.class, () -> {
+ client.create().forPath("/bar/foo");
+ });
+
+ // when: mkdirs("/bar/foo")
+ // then: everything fine as "/bar/foo" exists, and we have READ permission
+ EnsureContainers ensureContainers = new EnsureContainers(client, "/bar/foo");
+ ensureContainers.ensure();
+ } finally {
+ CloseableUtils.closeQuietly(client);
+ }
+ }
}
diff --git a/curator-test-zk37/pom.xml b/curator-test-zk37/pom.xml
index 59bdfca83..b49574d98 100644
--- a/curator-test-zk37/pom.xml
+++ b/curator-test-zk37/pom.xml
@@ -224,7 +224,7 @@
<dependency>org.apache.curator:curator-recipes</dependency>
<dependency>org.apache.curator:curator-client</dependency>
</dependenciesToScan>
- <excludedGroups>master</excludedGroups>
+ <excludedGroups>master,ZOOKEEPER-2590</excludedGroups>
</configuration>
</plugin>
</plugins>
diff --git a/curator-test-zk38/pom.xml b/curator-test-zk38/pom.xml
index 1772f5de1..374b5f087 100644
--- a/curator-test-zk38/pom.xml
+++ b/curator-test-zk38/pom.xml
@@ -31,7 +31,7 @@
<artifactId>curator-test-zk38</artifactId>
<properties>
- <zookeeper-38-version>3.8.3</zookeeper-38-version>
+ <zookeeper-38-version>3.8.4</zookeeper-38-version>
</properties>
<dependencies>