Skip to content

Commit 4d56f7c

Browse files
kezhuwyang-wei
authored andcommitted
CURATOR-728: Not issue ZooKeeper::create if possible in ZkPaths::mkdirs (apache#518)
`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".
1 parent a8b4dc3 commit 4d56f7c

File tree

5 files changed

+51
-14
lines changed

5 files changed

+51
-14
lines changed

curator-client/src/main/java/org/apache/curator/utils/ZKPaths.java

+12-11
Original file line numberDiff line numberDiff line change
@@ -282,28 +282,29 @@ public static void mkdirs(ZooKeeper zookeeper, String path, boolean makeLastNode
282282
*/
283283
public static void mkdirs(
284284
ZooKeeper zookeeper,
285-
String path,
285+
final String path,
286286
boolean makeLastNode,
287287
InternalACLProvider aclProvider,
288288
boolean asContainers)
289289
throws InterruptedException, KeeperException {
290290
PathUtils.validatePath(path);
291291

292-
int pos = path.length();
293-
String subPath;
294-
295-
// This first loop locates the first parent that doesn't exist from leaf nodes towards root
292+
// This first loop locates the first node that doesn't exist from leaf nodes towards root
296293
// this way, it is not required to have read access on all parents.
297294
// This is relevant after https://issues.apache.org/jira/browse/ZOOKEEPER-2590.
298295

296+
int pos = path.length();
299297
do {
298+
String subPath = path.substring(0, pos);
299+
if (zookeeper.exists(subPath, false) != null) {
300+
break;
301+
}
300302
pos = path.lastIndexOf(PATH_SEPARATOR_CHAR, pos - 1);
301-
subPath = path.substring(0, pos);
302-
} while (pos > 0 && zookeeper.exists(subPath, false) == null);
303+
} while (pos > 0);
303304

304305
// Start creating the subtree after the longest path that exists, assuming root always exists.
305306

306-
do {
307+
while (pos < path.length()) {
307308
pos = path.indexOf(PATH_SEPARATOR_CHAR, pos + 1);
308309

309310
if (pos == -1) {
@@ -314,7 +315,7 @@ public static void mkdirs(
314315
}
315316
}
316317

317-
subPath = path.substring(0, pos);
318+
String subPath = path.substring(0, pos);
318319

319320
// All the paths from the initial `pos` do not exist.
320321
try {
@@ -332,8 +333,8 @@ public static void mkdirs(
332333
} catch (KeeperException.NodeExistsException ignore) {
333334
// ignore... someone else has created it since we checked
334335
}
335-
336-
} while (pos < path.length());
336+
}
337+
;
337338
}
338339

339340
/**

curator-framework/src/test/java/org/apache/curator/framework/imps/TestCreate.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import org.apache.zookeeper.ZooDefs;
4343
import org.apache.zookeeper.data.ACL;
4444
import org.apache.zookeeper.data.Stat;
45+
import org.junit.jupiter.api.Tag;
4546
import org.junit.jupiter.api.Test;
4647

4748
public class TestCreate extends BaseClassForTests {
@@ -601,6 +602,7 @@ public void testProtectedUtils() throws Exception {
601602
* but just to the first ancestor parent. (See https://issues.apache.org/jira/browse/ZOOKEEPER-2590)
602603
*/
603604
@Test
605+
@Tag("ZOOKEEPER-2590")
604606
public void testForbiddenAncestors() throws Exception {
605607
CuratorFramework client = createClient(testACLProvider);
606608
try {
@@ -613,9 +615,10 @@ public void testForbiddenAncestors() throws Exception {
613615

614616
// In creation attempts where the parent ("/bat") has ACL that restricts read, creation request fails.
615617
try {
616-
client.create().creatingParentsIfNeeded().forPath("/bat/bost");
618+
client.create().creatingParentsIfNeeded().forPath("/bat/foo/bost");
617619
fail("Expected NoAuthException when not authorized to read new node grandparent");
618620
} catch (KeeperException.NoAuthException noAuthException) {
621+
assertEquals(noAuthException.getPath(), "/bat");
619622
}
620623

621624
// But creating a node in the same subtree where its grandparent has read access is allowed and

curator-framework/src/test/java/org/apache/curator/framework/imps/TestEnsureContainers.java

+33
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,20 @@
1919

2020
package org.apache.curator.framework.imps;
2121

22+
import static org.apache.zookeeper.ZooDefs.Ids.ANYONE_ID_UNSAFE;
2223
import static org.junit.jupiter.api.Assertions.assertNotNull;
2324
import static org.junit.jupiter.api.Assertions.assertNull;
25+
import static org.junit.jupiter.api.Assertions.assertThrows;
26+
import java.util.Collections;
2427
import org.apache.curator.framework.CuratorFramework;
2528
import org.apache.curator.framework.CuratorFrameworkFactory;
2629
import org.apache.curator.framework.EnsureContainers;
2730
import org.apache.curator.retry.RetryOneTime;
2831
import org.apache.curator.test.BaseClassForTests;
2932
import org.apache.curator.utils.CloseableUtils;
33+
import org.apache.zookeeper.KeeperException;
34+
import org.apache.zookeeper.ZooDefs;
35+
import org.apache.zookeeper.data.ACL;
3036
import org.junit.jupiter.api.Test;
3137

3238
public class TestEnsureContainers extends BaseClassForTests {
@@ -63,4 +69,31 @@ public void testSingleExecution() throws Exception {
6369
CloseableUtils.closeQuietly(client);
6470
}
6571
}
72+
73+
@Test
74+
public void testNodeExistsButNoCreatePermission() throws Exception {
75+
CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1));
76+
try {
77+
client.start();
78+
79+
// given: "/bar/foo" created
80+
client.create().creatingParentsIfNeeded().forPath("/bar/foo");
81+
// given: only permission read to "/bar"
82+
client.setACL()
83+
.withACL(Collections.singletonList(new ACL(ZooDefs.Perms.READ, ANYONE_ID_UNSAFE)))
84+
.forPath("/bar");
85+
86+
// check: create "/bar/foo" will fail with NoAuth
87+
assertThrows(KeeperException.NoAuthException.class, () -> {
88+
client.create().forPath("/bar/foo");
89+
});
90+
91+
// when: mkdirs("/bar/foo")
92+
// then: everything fine as "/bar/foo" exists, and we have READ permission
93+
EnsureContainers ensureContainers = new EnsureContainers(client, "/bar/foo");
94+
ensureContainers.ensure();
95+
} finally {
96+
CloseableUtils.closeQuietly(client);
97+
}
98+
}
6699
}

curator-test-zk37/pom.xml

+1-1
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@
224224
<dependency>org.apache.curator:curator-recipes</dependency>
225225
<dependency>org.apache.curator:curator-client</dependency>
226226
</dependenciesToScan>
227-
<excludedGroups>master</excludedGroups>
227+
<excludedGroups>master,ZOOKEEPER-2590</excludedGroups>
228228
</configuration>
229229
</plugin>
230230
</plugins>

curator-test-zk38/pom.xml

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
<artifactId>curator-test-zk38</artifactId>
3232

3333
<properties>
34-
<zookeeper-38-version>3.8.3</zookeeper-38-version>
34+
<zookeeper-38-version>3.8.4</zookeeper-38-version>
3535
</properties>
3636

3737
<dependencies>

0 commit comments

Comments
 (0)