You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "kezhuw (via GitHub)" <gi...@apache.org> on 2023/09/15 07:21:50 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
kezhuw opened a new pull request, #2062:
URL: https://github.com/apache/zookeeper/pull/2062
Currently, `ClientCnxn` will strip path "/zookeeper/config" to "keeper/config" for chroot "/zoo". This causes exception and finally connection loss.
This is a leftover of ZOOKEEPER-4565.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#discussion_r1327083090
##########
zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java:
##########
@@ -65,7 +65,9 @@ public boolean matches() throws InterruptedException {
@Test
public void testChrootWithZooKeeperPathWatcher() throws Exception {
- ZooKeeper zk1 = createClient(hostPort + "/chroot");
+ // "/zoo" is short enough and a prefix of "/zookeeper/config".
Review Comment:
New test added.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#issuecomment-1721003134
@tisonkun Thank you for reviwing. I have assigned ZOOKEEPER-838 to myself and try to work out a solution for evaluation.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on a diff in pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#discussion_r1327013852
##########
zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java:
##########
@@ -65,7 +65,9 @@ public boolean matches() throws InterruptedException {
@Test
public void testChrootWithZooKeeperPathWatcher() throws Exception {
- ZooKeeper zk1 = createClient(hostPort + "/chroot");
+ // "/zoo" is short enough and a prefix of "/zookeeper/config".
Review Comment:
Please try to write a test case.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] tisonkun commented on pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#issuecomment-1720932722
Here is a patch:
```diff
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
index 727d97daa..a2be07deb 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
@@ -856,13 +856,13 @@ class SendThread extends ZooKeeperThread {
private volatile ZooKeeperSaslClient zooKeeperSaslClient;
private String stripChroot(String serverPath) {
- if (serverPath.startsWith(chrootPath)) {
+ if (serverPath.startsWith(ZooDefs.ZOOKEEPER_NODE_SUBTREE)) {
+ return serverPath;
+ } else if (serverPath.startsWith(chrootPath)) {
if (serverPath.length() == chrootPath.length()) {
return "/";
}
return serverPath.substring(chrootPath.length());
- } else if (serverPath.startsWith(ZooDefs.ZOOKEEPER_NODE_SUBTREE)) {
- return serverPath;
}
LOG.warn("Got server path {} which is not descendant of chroot path {}.", serverPath, chrootPath);
return serverPath;
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java
index fca48c751..54f0f6560 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/ChrootTest.java
@@ -82,6 +82,25 @@ public void testChrootWithZooKeeperPathWatcher() throws Exception {
assertEquals(ZooDefs.CONFIG_NODE, event.getPath());
}
+ @Test
+ public void testChrootWithZooKeeperPathWatcherAndOverlapChrootPath() throws Exception {
+ ZooKeeper zk1 = createClient(hostPort + "/zoo");
+ BlockingQueue<WatchedEvent> events = new LinkedBlockingQueue<>();
+ byte[] config = zk1.getConfig(events::add, null);
+
+ ZooKeeper zk2 = createClient();
+ zk2.addAuthInfo("digest", "super:test".getBytes());
+ zk2.setData(ZooDefs.CONFIG_NODE, config, -1);
+
+ waitFor("config watcher receive no event", () -> !events.isEmpty(), 10);
+
+ WatchedEvent event = events.poll();
+ assertNotNull(event);
+ assertEquals(Watcher.Event.KeeperState.SyncConnected, event.getState());
+ assertEquals(Watcher.Event.EventType.NodeDataChanged, event.getType());
+ assertEquals(ZooDefs.CONFIG_NODE, event.getPath());
+ }
+
@Test
public void testChrootSynchronous() throws IOException, InterruptedException, KeeperException {
ZooKeeper zk1 = createClient();
```
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#issuecomment-1720812292
Hi @tisonkun, I split this from #1996, I think this will make both pr more concentrated. And also, we once considered ZOOKEEPER-4601 as a [breaking change due to misinformation from my side](https://lists.apache.org/thread/460mvno660xxm4cqd23xtkmcrrpgofw4). So splitting should also be good for backport.
@eolivelli @symat Would you mind take a look ? This is a leftover of #1899.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw closed pull request #2062: ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path
URL: https://github.com/apache/zookeeper/pull/2062
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4742: Fix connection loss due to abnormal `getConfig` watcher path [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2062:
URL: https://github.com/apache/zookeeper/pull/2062#issuecomment-2870897648
Superceded by #2065.
--
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org