Skip to content

Commit d12aba5

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 803c485 commit d12aba5

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

+3-3
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, WatchedEvent.NO_ZXID);
194+
state.watchManager.triggerWatch(path, event, WatchedEvent.NO_ZXID, 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, WatchedEvent.NO_ZXID);
228+
watchManager.triggerWatch(path, event, WatchedEvent.NO_ZXID, 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, WatchedEvent.NO_ZXID);
297+
state.watchManager.triggerWatch(path, event, WatchedEvent.NO_ZXID, null);
298298
}
299299
}
300300
}

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

+17-6
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,10 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
445445
if (parent == null) {
446446
throw new NoNodeException();
447447
}
448+
List<ACL> parentAcl;
448449
synchronized (parent) {
450+
parentAcl = getACL(parent);
451+
449452
// Add the ACL to ACL cache first, to avoid the ACL not being
450453
// created race condition during fuzzy snapshot sync.
451454
//
@@ -518,8 +521,9 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
518521
updateQuotaStat(lastPrefix, bytes, 1);
519522
}
520523
updateWriteStat(path, bytes);
521-
dataWatches.triggerWatch(path, Event.EventType.NodeCreated, zxid);
522-
childWatches.triggerWatch(parentName.equals("") ? "/" : parentName, Event.EventType.NodeChildrenChanged, zxid);
524+
dataWatches.triggerWatch(path, Event.EventType.NodeCreated, zxid, acl);
525+
childWatches.triggerWatch(parentName.equals("") ? "/" : parentName,
526+
Event.EventType.NodeChildrenChanged, zxid, parentAcl);
523527
}
524528

525529
/**
@@ -559,16 +563,20 @@ public void deleteNode(String path, long zxid) throws NoNodeException {
559563
if (node == null) {
560564
throw new NoNodeException();
561565
}
566+
List<ACL> acl;
562567
nodes.remove(path);
563568
synchronized (node) {
569+
acl = getACL(node);
564570
aclCache.removeUsage(node.acl);
565571
nodeDataSize.addAndGet(-getNodeSize(path, node.data));
566572
}
567573

568574
// Synchronized to sync the containers and ttls change, probably
569575
// only need to sync on containers and ttls, will update it in a
570576
// separate patch.
577+
List<ACL> parentAcl;
571578
synchronized (parent) {
579+
parentAcl = getACL(parent);
572580
long owner = node.stat.getEphemeralOwner();
573581
EphemeralType ephemeralType = EphemeralType.get(owner);
574582
if (ephemeralType == EphemeralType.CONTAINER) {
@@ -615,9 +623,10 @@ public void deleteNode(String path, long zxid) throws NoNodeException {
615623
"childWatches.triggerWatch " + parentName);
616624
}
617625

618-
WatcherOrBitSet processed = dataWatches.triggerWatch(path, EventType.NodeDeleted, zxid);
619-
childWatches.triggerWatch(path, EventType.NodeDeleted, zxid, processed);
620-
childWatches.triggerWatch("".equals(parentName) ? "/" : parentName, EventType.NodeChildrenChanged, zxid);
626+
WatcherOrBitSet processed = dataWatches.triggerWatch(path, EventType.NodeDeleted, zxid, acl);
627+
childWatches.triggerWatch(path, EventType.NodeDeleted, zxid, acl, processed);
628+
childWatches.triggerWatch("".equals(parentName) ? "/" : parentName,
629+
EventType.NodeChildrenChanged, zxid, parentAcl);
621630
}
622631

623632
public Stat setData(String path, byte[] data, int version, long zxid, long time) throws NoNodeException {
@@ -626,8 +635,10 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time)
626635
if (n == null) {
627636
throw new NoNodeException();
628637
}
638+
List<ACL> acl;
629639
byte[] lastData;
630640
synchronized (n) {
641+
acl = getACL(n);
631642
lastData = n.data;
632643
nodes.preChange(path, n);
633644
n.data = data;
@@ -649,7 +660,7 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time)
649660
nodeDataSize.addAndGet(getNodeSize(path, data) - getNodeSize(path, lastData));
650661

651662
updateWriteStat(path, dataBytes);
652-
dataWatches.triggerWatch(path, EventType.NodeDataChanged, zxid);
663+
dataWatches.triggerWatch(path, EventType.NodeDataChanged, zxid, acl);
653664
return s;
654665
}
655666

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

+3-1
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

@@ -51,7 +53,7 @@ void setSessionTimeout(int sessionTimeout) {
5153
}
5254

5355
@Override
54-
public void process(WatchedEvent event) {
56+
public void process(WatchedEvent event, List<ACL> znodeAcl) {
5557
mostRecentEventType = event.getType();
5658
mostRecentZxid = event.getZxid();
5759
mostRecentPath = event.getPath();

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

+15-1
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.ConnectRequest;
@@ -704,7 +707,18 @@ public int sendResponse(ReplyHeader h, Record r, String tag, String cacheKey, St
704707
* @see org.apache.zookeeper.server.ServerCnxnIface#process(org.apache.zookeeper.proto.WatcherEvent)
705708
*/
706709
@Override
707-
public void process(WatchedEvent event) {
710+
public void process(WatchedEvent event, List<ACL> znodeAcl) {
711+
try {
712+
zkServer.checkACL(this, znodeAcl, ZooDefs.Perms.READ, getAuthInfo(), event.getPath(), null);
713+
} catch (KeeperException.NoAuthException e) {
714+
if (LOG.isTraceEnabled()) {
715+
ZooTrace.logTraceMessage(
716+
LOG,
717+
ZooTrace.EVENT_DELIVERY_TRACE_MASK,
718+
"Not delivering event " + event + " to 0x" + Long.toHexString(this.sessionId) + " (filtered by ACL)");
719+
}
720+
return;
721+
}
708722
ReplyHeader h = new ReplyHeader(ClientCnxn.NOTIFICATION_XID, event.getZxid(), 0);
709723
if (LOG.isTraceEnabled()) {
710724
ZooTrace.logTraceMessage(

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

+16-1
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.ConnectRequest;
@@ -161,7 +165,18 @@ public int getSessionTimeout() {
161165
}
162166

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

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

+7-3
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@
3737
import org.apache.jute.Record;
3838
import org.apache.zookeeper.Quotas;
3939
import org.apache.zookeeper.WatchedEvent;
40-
import org.apache.zookeeper.Watcher;
4140
import org.apache.zookeeper.ZooDefs.OpCode;
4241
import org.apache.zookeeper.compat.ProtocolManager;
42+
import org.apache.zookeeper.data.ACL;
4343
import org.apache.zookeeper.data.Id;
4444
import org.apache.zookeeper.data.Stat;
4545
import org.apache.zookeeper.metrics.Counter;
@@ -52,7 +52,7 @@
5252
* Interface to a Server connection - represents a connection from a client
5353
* to the server.
5454
*/
55-
public abstract class ServerCnxn implements Stats, Watcher {
55+
public abstract class ServerCnxn implements Stats, ServerWatcher {
5656

5757
// This is just an arbitrary object to represent requests issued by
5858
// (aka owned by) this class
@@ -254,7 +254,11 @@ protected ByteBuffer[] serialize(ReplyHeader h, Record r, String cacheKey, Stat
254254
/* notify the client the session is closing and close/cleanup socket */
255255
public abstract void sendCloseSession();
256256

257-
public abstract void process(WatchedEvent event);
257+
public void process(WatchedEvent event) {
258+
process(event, null);
259+
}
260+
261+
public abstract void process(WatchedEvent event, List<ACL> znodeAcl);
258262

259263
public abstract long getSessionId();
260264

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

+29
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

+5-2
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@
1919
package org.apache.zookeeper.server.watch;
2020

2121
import java.io.PrintWriter;
22+
import java.util.List;
2223
import javax.annotation.Nullable;
2324
import org.apache.zookeeper.Watcher;
2425
import org.apache.zookeeper.Watcher.Event.EventType;
26+
import org.apache.zookeeper.data.ACL;
2527

2628
public interface IWatchManager {
2729

@@ -114,10 +116,11 @@ default boolean removeWatcher(String path, Watcher watcher, WatcherMode watcherM
114116
* @param path znode path
115117
* @param type the watch event type
116118
* @param zxid the zxid for the corresponding change that triggered this event
119+
* @param acl ACL of the znode in path
117120
*
118121
* @return the watchers have been notified
119122
*/
120-
WatcherOrBitSet triggerWatch(String path, EventType type, long zxid);
123+
WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, List<ACL> acl);
121124

122125
/**
123126
* Distribute the watch event for the given path, but ignore those
@@ -130,7 +133,7 @@ default boolean removeWatcher(String path, Watcher watcher, WatcherMode watcherM
130133
*
131134
* @return the watchers have been notified
132135
*/
133-
WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, WatcherOrBitSet suppress);
136+
WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, List<ACL> acl, WatcherOrBitSet suppress);
134137

135138
/**
136139
* Get the size of watchers.

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

+11-4
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,18 @@
2323
import java.util.HashMap;
2424
import java.util.HashSet;
2525
import java.util.Iterator;
26+
import java.util.List;
2627
import java.util.Map;
2728
import java.util.Map.Entry;
2829
import java.util.Set;
2930
import org.apache.zookeeper.WatchedEvent;
3031
import org.apache.zookeeper.Watcher;
3132
import org.apache.zookeeper.Watcher.Event.EventType;
3233
import org.apache.zookeeper.Watcher.Event.KeeperState;
34+
import org.apache.zookeeper.data.ACL;
3335
import org.apache.zookeeper.server.ServerCnxn;
3436
import org.apache.zookeeper.server.ServerMetrics;
37+
import org.apache.zookeeper.server.ServerWatcher;
3538
import org.apache.zookeeper.server.ZooTrace;
3639
import org.slf4j.Logger;
3740
import org.slf4j.LoggerFactory;
@@ -129,12 +132,12 @@ public synchronized void removeWatcher(Watcher watcher) {
129132
}
130133

131134
@Override
132-
public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid) {
133-
return triggerWatch(path, type, zxid, null);
135+
public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, List<ACL> acl) {
136+
return triggerWatch(path, type, zxid, acl, null);
134137
}
135138

136139
@Override
137-
public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, WatcherOrBitSet supress) {
140+
public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, List<ACL> acl, WatcherOrBitSet supress) {
138141
WatchedEvent e = new WatchedEvent(type, KeeperState.SyncConnected, path, zxid);
139142
Set<Watcher> watchers = new HashSet<>();
140143
synchronized (this) {
@@ -182,7 +185,11 @@ public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, Watc
182185
if (supress != null && supress.contains(w)) {
183186
continue;
184187
}
185-
w.process(e);
188+
if (w instanceof ServerWatcher) {
189+
((ServerWatcher) w).process(e, acl);
190+
} else {
191+
w.process(e);
192+
}
186193
}
187194

188195
switch (type) {

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

+11-4
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, long zxid) {
206-
return triggerWatch(path, type, zxid, null);
208+
public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, List<ACL> acl) {
209+
return triggerWatch(path, type, zxid, acl, null);
207210
}
208211

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

213216
BitHashSet watchers = remove(path);
@@ -232,7 +235,11 @@ public WatcherOrBitSet triggerWatch(String path, EventType type, long zxid, Watc
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)