Skip to content

Commit 65b91d2

Browse files
anmolnarphuntztzg
committed
ZOOKEEPER-4799: Refactor ACL check in 'addWatch' command
As of today, it is impossible to diagnose which watch events are dropped because of ACLs. Let's centralize, systematize, and log the checks at the 'process()' site in the Netty and NIO connections. (These 'process()' methods contain some duplicated code, and should also be refactored at some point. This series does not change them.) This patch also adds a substantial number of tests in order to avoid unexpected regressions. Co-authored-by: Patrick Hunt <[email protected]> Co-authored-by: Damien Diederen <[email protected]>
1 parent b8eb6a3 commit 65b91d2

File tree

14 files changed

+763
-35
lines changed

14 files changed

+763
-35
lines changed

‎zookeeper-it/src/main/java/org/apache/zookeeper/server/watch/WatchBench.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ void prepare() {
191191
@Measurement(iterations = 3, time = 10, timeUnit = TimeUnit.SECONDS)
192192
public void testTriggerConcentrateWatch(InvocationState state) throws Exception {
193193
for (String path : state.paths) {
194-
state.watchManager.triggerWatch(path, event);
194+
state.watchManager.triggerWatch(path, event, null);
195195
}
196196
}
197197

@@ -225,7 +225,7 @@ public void tearDown() {
225225

226226
// clear all the watches
227227
for (String path : paths) {
228-
watchManager.triggerWatch(path, event);
228+
watchManager.triggerWatch(path, event, null);
229229
}
230230
}
231231
}
@@ -294,7 +294,7 @@ public void prepare() {
294294
@Measurement(iterations = 3, time = 10, timeUnit = TimeUnit.SECONDS)
295295
public void testTriggerSparseWatch(TriggerSparseWatchState state) throws Exception {
296296
for (String path : state.paths) {
297-
state.watchManager.triggerWatch(path, event);
297+
state.watchManager.triggerWatch(path, event, null);
298298
}
299299
}
300300
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,10 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
450450
if (parent == null) {
451451
throw new KeeperException.NoNodeException();
452452
}
453+
List<ACL> parentAcl;
453454
synchronized (parent) {
455+
parentAcl = getACL(parent);
456+
454457
// Add the ACL to ACL cache first, to avoid the ACL not being
455458
// created race condition during fuzzy snapshot sync.
456459
//
@@ -527,8 +530,9 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
527530
updateQuotaStat(lastPrefix, bytes, 1);
528531
}
529532
updateWriteStat(path, bytes);
530-
dataWatches.triggerWatch(path, Event.EventType.NodeCreated);
531-
childWatches.triggerWatch(parentName.equals("") ? "/" : parentName, Event.EventType.NodeChildrenChanged);
533+
dataWatches.triggerWatch(path, Event.EventType.NodeCreated, acl);
534+
childWatches.triggerWatch(parentName.equals("") ? "/" : parentName,
535+
Event.EventType.NodeChildrenChanged, parentAcl);
532536
}
533537

534538
/**
@@ -568,16 +572,20 @@ public void deleteNode(String path, long zxid) throws KeeperException.NoNodeExce
568572
if (node == null) {
569573
throw new KeeperException.NoNodeException();
570574
}
575+
List<ACL> acl;
571576
nodes.remove(path);
572577
synchronized (node) {
578+
acl = getACL(node);
573579
aclCache.removeUsage(node.acl);
574580
nodeDataSize.addAndGet(-getNodeSize(path, node.data));
575581
}
576582

577583
// Synchronized to sync the containers and ttls change, probably
578584
// only need to sync on containers and ttls, will update it in a
579585
// separate patch.
586+
List<ACL> parentAcl;
580587
synchronized (parent) {
588+
parentAcl = getACL(parent);
581589
long eowner = node.stat.getEphemeralOwner();
582590
EphemeralType ephemeralType = EphemeralType.get(eowner);
583591
if (ephemeralType == EphemeralType.CONTAINER) {
@@ -624,9 +632,10 @@ public void deleteNode(String path, long zxid) throws KeeperException.NoNodeExce
624632
"childWatches.triggerWatch " + parentName);
625633
}
626634

627-
WatcherOrBitSet processed = dataWatches.triggerWatch(path, EventType.NodeDeleted);
628-
childWatches.triggerWatch(path, EventType.NodeDeleted, processed);
629-
childWatches.triggerWatch("".equals(parentName) ? "/" : parentName, EventType.NodeChildrenChanged);
635+
WatcherOrBitSet processed = dataWatches.triggerWatch(path, EventType.NodeDeleted, acl);
636+
childWatches.triggerWatch(path, EventType.NodeDeleted, acl, processed);
637+
childWatches.triggerWatch("".equals(parentName) ? "/" : parentName,
638+
EventType.NodeChildrenChanged, parentAcl);
630639
}
631640

632641
public Stat setData(String path, byte[] data, int version, long zxid, long time) throws KeeperException.NoNodeException {
@@ -635,8 +644,10 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time)
635644
if (n == null) {
636645
throw new KeeperException.NoNodeException();
637646
}
647+
List<ACL> acl;
638648
byte[] lastdata = null;
639649
synchronized (n) {
650+
acl = getACL(n);
640651
lastdata = n.data;
641652
nodes.preChange(path, n);
642653
n.data = data;
@@ -658,7 +669,7 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time)
658669
nodeDataSize.addAndGet(getNodeSize(path, data) - getNodeSize(path, lastdata));
659670

660671
updateWriteStat(path, dataBytes);
661-
dataWatches.triggerWatch(path, EventType.NodeDataChanged);
672+
dataWatches.triggerWatch(path, EventType.NodeDataChanged, acl);
662673
return s;
663674
}
664675

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/DumbWatcher.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,10 @@
2222
import java.net.InetSocketAddress;
2323
import java.nio.ByteBuffer;
2424
import java.security.cert.Certificate;
25+
import java.util.List;
2526
import org.apache.jute.Record;
2627
import org.apache.zookeeper.WatchedEvent;
28+
import org.apache.zookeeper.data.ACL;
2729
import org.apache.zookeeper.data.Stat;
2830
import org.apache.zookeeper.proto.ReplyHeader;
2931

@@ -48,7 +50,7 @@ void setSessionTimeout(int sessionTimeout) {
4850
}
4951

5052
@Override
51-
public void process(WatchedEvent event) {
53+
public void process(WatchedEvent event, List<ACL> znodeAcl) {
5254
}
5355

5456
@Override

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/NIOServerCnxn.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,17 @@
3030
import java.nio.channels.SelectionKey;
3131
import java.nio.channels.SocketChannel;
3232
import java.security.cert.Certificate;
33+
import java.util.List;
3334
import java.util.Queue;
3435
import java.util.concurrent.LinkedBlockingQueue;
3536
import java.util.concurrent.atomic.AtomicBoolean;
3637
import org.apache.jute.BinaryInputArchive;
3738
import org.apache.jute.Record;
3839
import org.apache.zookeeper.ClientCnxn;
40+
import org.apache.zookeeper.KeeperException;
3941
import org.apache.zookeeper.WatchedEvent;
4042
import org.apache.zookeeper.ZooDefs;
43+
import org.apache.zookeeper.data.ACL;
4144
import org.apache.zookeeper.data.Id;
4245
import org.apache.zookeeper.data.Stat;
4346
import org.apache.zookeeper.proto.ReplyHeader;
@@ -697,7 +700,18 @@ public int sendResponse(ReplyHeader h, Record r, String tag, String cacheKey, St
697700
* @see org.apache.zookeeper.server.ServerCnxnIface#process(org.apache.zookeeper.proto.WatcherEvent)
698701
*/
699702
@Override
700-
public void process(WatchedEvent event) {
703+
public void process(WatchedEvent event, List<ACL> znodeAcl) {
704+
try {
705+
zkServer.checkACL(this, znodeAcl, ZooDefs.Perms.READ, getAuthInfo(), event.getPath(), null);
706+
} catch (KeeperException.NoAuthException e) {
707+
if (LOG.isTraceEnabled()) {
708+
ZooTrace.logTraceMessage(
709+
LOG,
710+
ZooTrace.EVENT_DELIVERY_TRACE_MASK,
711+
"Not delivering event " + event + " to 0x" + Long.toHexString(this.sessionId) + " (filtered by ACL)");
712+
}
713+
return;
714+
}
701715
ReplyHeader h = new ReplyHeader(ClientCnxn.NOTIFICATION_XID, -1L, 0);
702716
if (LOG.isTraceEnabled()) {
703717
ZooTrace.logTraceMessage(

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxn.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,15 @@
3838
import java.nio.channels.SelectionKey;
3939
import java.security.cert.Certificate;
4040
import java.util.Arrays;
41+
import java.util.List;
4142
import java.util.concurrent.atomic.AtomicBoolean;
4243
import org.apache.jute.BinaryInputArchive;
4344
import org.apache.jute.Record;
4445
import org.apache.zookeeper.ClientCnxn;
46+
import org.apache.zookeeper.KeeperException;
4547
import org.apache.zookeeper.WatchedEvent;
48+
import org.apache.zookeeper.ZooDefs;
49+
import org.apache.zookeeper.data.ACL;
4650
import org.apache.zookeeper.data.Id;
4751
import org.apache.zookeeper.data.Stat;
4852
import org.apache.zookeeper.proto.ReplyHeader;
@@ -159,7 +163,18 @@ public int getSessionTimeout() {
159163
}
160164

161165
@Override
162-
public void process(WatchedEvent event) {
166+
public void process(WatchedEvent event, List<ACL> znodeAcl) {
167+
try {
168+
zkServer.checkACL(this, znodeAcl, ZooDefs.Perms.READ, getAuthInfo(), event.getPath(), null);
169+
} catch (KeeperException.NoAuthException e) {
170+
if (LOG.isTraceEnabled()) {
171+
ZooTrace.logTraceMessage(
172+
LOG,
173+
ZooTrace.EVENT_DELIVERY_TRACE_MASK,
174+
"Not delivering event " + event + " to 0x" + Long.toHexString(this.sessionId) + " (filtered by ACL)");
175+
}
176+
return;
177+
}
163178
ReplyHeader h = new ReplyHeader(ClientCnxn.NOTIFICATION_XID, -1L, 0);
164179
if (LOG.isTraceEnabled()) {
165180
ZooTrace.logTraceMessage(

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerCnxn.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@
3939
import org.apache.jute.Record;
4040
import org.apache.zookeeper.Quotas;
4141
import org.apache.zookeeper.WatchedEvent;
42-
import org.apache.zookeeper.Watcher;
4342
import org.apache.zookeeper.ZooDefs.OpCode;
43+
import org.apache.zookeeper.data.ACL;
4444
import org.apache.zookeeper.data.Id;
4545
import org.apache.zookeeper.data.Stat;
4646
import org.apache.zookeeper.metrics.Counter;
@@ -53,7 +53,7 @@
5353
* Interface to a Server connection - represents a connection from a client
5454
* to the server.
5555
*/
56-
public abstract class ServerCnxn implements Stats, Watcher {
56+
public abstract class ServerCnxn implements Stats, ServerWatcher {
5757

5858
// This is just an arbitrary object to represent requests issued by
5959
// (aka owned by) this class
@@ -264,7 +264,11 @@ protected ByteBuffer[] serialize(ReplyHeader h, Record r, String tag,
264264
/* notify the client the session is closing and close/cleanup socket */
265265
public abstract void sendCloseSession();
266266

267-
public abstract void process(WatchedEvent event);
267+
public void process(WatchedEvent event) {
268+
process(event, null);
269+
}
270+
271+
public abstract void process(WatchedEvent event, List<ACL> znodeAcl);
268272

269273
public abstract long getSessionId();
270274

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/ServerWatcher.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.apache.zookeeper.server;
19+
20+
import java.util.List;
21+
import org.apache.zookeeper.WatchedEvent;
22+
import org.apache.zookeeper.Watcher;
23+
import org.apache.zookeeper.data.ACL;
24+
25+
public interface ServerWatcher extends Watcher {
26+
27+
void process(WatchedEvent event, List<ACL> znodeAcl);
28+
29+
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/IWatchManager.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,10 @@
1919
package org.apache.zookeeper.server.watch;
2020

2121
import java.io.PrintWriter;
22+
import java.util.List;
2223
import org.apache.zookeeper.Watcher;
2324
import org.apache.zookeeper.Watcher.Event.EventType;
25+
import org.apache.zookeeper.data.ACL;
2426

2527
public interface IWatchManager {
2628

@@ -82,10 +84,11 @@ default boolean addWatch(String path, Watcher watcher, WatcherMode watcherMode)
8284
*
8385
* @param path znode path
8486
* @param type the watch event type
87+
* @param acl ACL of the znode in path
8588
*
8689
* @return the watchers have been notified
8790
*/
88-
WatcherOrBitSet triggerWatch(String path, EventType type);
91+
WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl);
8992

9093
/**
9194
* Distribute the watch event for the given path, but ignore those
@@ -97,7 +100,7 @@ default boolean addWatch(String path, Watcher watcher, WatcherMode watcherMode)
97100
*
98101
* @return the watchers have been notified
99102
*/
100-
WatcherOrBitSet triggerWatch(String path, EventType type, WatcherOrBitSet suppress);
103+
WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl, WatcherOrBitSet suppress);
101104

102105
/**
103106
* Get the size of watchers.

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchManager.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,18 @@
2222
import java.util.HashMap;
2323
import java.util.HashSet;
2424
import java.util.Iterator;
25+
import java.util.List;
2526
import java.util.Map;
2627
import java.util.Map.Entry;
2728
import java.util.Set;
2829
import org.apache.zookeeper.WatchedEvent;
2930
import org.apache.zookeeper.Watcher;
3031
import org.apache.zookeeper.Watcher.Event.EventType;
3132
import org.apache.zookeeper.Watcher.Event.KeeperState;
33+
import org.apache.zookeeper.data.ACL;
3234
import org.apache.zookeeper.server.ServerCnxn;
3335
import org.apache.zookeeper.server.ServerMetrics;
36+
import org.apache.zookeeper.server.ServerWatcher;
3437
import org.apache.zookeeper.server.ZooTrace;
3538
import org.slf4j.Logger;
3639
import org.slf4j.LoggerFactory;
@@ -115,12 +118,12 @@ public synchronized void removeWatcher(Watcher watcher) {
115118
}
116119

117120
@Override
118-
public WatcherOrBitSet triggerWatch(String path, EventType type) {
119-
return triggerWatch(path, type, null);
121+
public WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl) {
122+
return triggerWatch(path, type, acl, null);
120123
}
121124

122125
@Override
123-
public WatcherOrBitSet triggerWatch(String path, EventType type, WatcherOrBitSet supress) {
126+
public WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl, WatcherOrBitSet supress) {
124127
WatchedEvent e = new WatchedEvent(type, KeeperState.SyncConnected, path);
125128
Set<Watcher> watchers = new HashSet<>();
126129
PathParentIterator pathParentIterator = getPathParentIterator(path);
@@ -165,7 +168,11 @@ public WatcherOrBitSet triggerWatch(String path, EventType type, WatcherOrBitSet
165168
if (supress != null && supress.contains(w)) {
166169
continue;
167170
}
168-
w.process(e);
171+
if (w instanceof ServerWatcher) {
172+
((ServerWatcher) w).process(e, acl);
173+
} else {
174+
w.process(e);
175+
}
169176
}
170177

171178
switch (type) {

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/watch/WatchManagerOptimized.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.BitSet;
2323
import java.util.HashMap;
2424
import java.util.HashSet;
25+
import java.util.List;
2526
import java.util.Map;
2627
import java.util.Map.Entry;
2728
import java.util.Set;
@@ -31,8 +32,10 @@
3132
import org.apache.zookeeper.Watcher;
3233
import org.apache.zookeeper.Watcher.Event.EventType;
3334
import org.apache.zookeeper.Watcher.Event.KeeperState;
35+
import org.apache.zookeeper.data.ACL;
3436
import org.apache.zookeeper.server.ServerCnxn;
3537
import org.apache.zookeeper.server.ServerMetrics;
38+
import org.apache.zookeeper.server.ServerWatcher;
3639
import org.apache.zookeeper.server.util.BitHashSet;
3740
import org.apache.zookeeper.server.util.BitMap;
3841
import org.slf4j.Logger;
@@ -202,12 +205,12 @@ public void processDeadWatchers(Set<Integer> deadWatchers) {
202205
}
203206

204207
@Override
205-
public WatcherOrBitSet triggerWatch(String path, EventType type) {
206-
return triggerWatch(path, type, null);
208+
public WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl) {
209+
return triggerWatch(path, type, acl, null);
207210
}
208211

209212
@Override
210-
public WatcherOrBitSet triggerWatch(String path, EventType type, WatcherOrBitSet suppress) {
213+
public WatcherOrBitSet triggerWatch(String path, EventType type, List<ACL> acl, WatcherOrBitSet suppress) {
211214
WatchedEvent e = new WatchedEvent(type, KeeperState.SyncConnected, path);
212215

213216
BitHashSet watchers = remove(path);
@@ -232,7 +235,11 @@ public WatcherOrBitSet triggerWatch(String path, EventType type, WatcherOrBitSet
232235
continue;
233236
}
234237

235-
w.process(e);
238+
if (w instanceof ServerWatcher) {
239+
((ServerWatcher) w).process(e, acl);
240+
} else {
241+
w.process(e);
242+
}
236243
triggeredWatches++;
237244
}
238245
}

0 commit comments

Comments
 (0)