Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-2623: [ADDENDUM] Forbid OpCode.check outside OpCode.multi #2104

Merged
merged 1 commit into from
Aug 5, 2024
Merged

ZOOKEEPER-2623: [ADDENDUM] Forbid OpCode.check outside OpCode.multi #2104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import org.apache.zookeeper.data.Id;
import org.apache.zookeeper.data.Stat;
import org.apache.zookeeper.proto.AddWatchRequest;
import org.apache.zookeeper.proto.CheckVersionRequest;
import org.apache.zookeeper.proto.CheckWatchesRequest;
import org.apache.zookeeper.proto.Create2Response;
import org.apache.zookeeper.proto.CreateResponse;
Expand Down Expand Up @@ -355,10 +354,8 @@ public void processRequest(Request request) {
}
case OpCode.check: {
lastOp = "CHEC";
CheckVersionRequest checkVersionRequest = request.readRequestRecord(CheckVersionRequest::new);
path = checkVersionRequest.getPath();
handleCheckVersionRequest(checkVersionRequest, cnxn, request.authInfo);
requestPathMetricsCollector.registerRequest(request.type, path);
rsp = new SetDataResponse(rc.stat);
err = Code.get(rc.err);
break;
}
case OpCode.exists: {
Expand Down Expand Up @@ -655,19 +652,6 @@ private Record handleGetDataRequest(Record request, ServerCnxn cnxn, List<Id> au
return new GetDataResponse(b, stat);
}

private void handleCheckVersionRequest(CheckVersionRequest request, ServerCnxn cnxn, List<Id> authInfo) throws KeeperException {
String path = request.getPath();
DataNode n = zks.getZKDatabase().getNode(path);
if (n == null) {
throw new KeeperException.NoNodeException();
}
zks.checkACL(cnxn, zks.getZKDatabase().aclForNode(n), ZooDefs.Perms.READ, authInfo, path, null);
int version = request.getVersion();
if (version != -1 && version != n.stat.getVersion()) {
throw new KeeperException.BadVersionException(path);
}
}

private boolean closeSession(ServerCnxnFactory serverCnxnFactory, long sessionId) {
if (serverCnxnFactory == null) {
return false;
Expand Down
4 changes: 4 additions & 0 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,10 @@ private void pRequestHelper(Request request) {
SetACLRequest setAclRequest = request.readRequestRecord(SetACLRequest::new);
pRequest2Txn(request.type, zks.getNextZxid(), request, setAclRequest);
break;
case OpCode.check:
CheckVersionRequest checkRequest = request.readRequestRecord(CheckVersionRequest::new);
pRequest2Txn(request.type, zks.getNextZxid(), request, checkRequest);
break;
case OpCode.multi:
MultiOperationRecord multiRequest;
try {
Expand Down
3 changes: 2 additions & 1 deletion zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ static boolean isValid(int type) {
// make sure this is always synchronized with Zoodefs!!
switch (type) {
case OpCode.notification:
return false;
case OpCode.check:
return false;
Comment on lines 298 to +299
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the effective change beyond the revert. According to https://lists.apache.org/thread/vl816jfrklvqz29coz5qnwpom9q41pcg.

case OpCode.closeSession:
case OpCode.create:
case OpCode.create2:
Expand Down Expand Up @@ -352,6 +352,7 @@ public boolean isQuorum() {
case OpCode.deleteContainer:
case OpCode.setACL:
case OpCode.setData:
case OpCode.check:
case OpCode.multi:
case OpCode.reconfig:
return true;
Expand Down
1 change: 1 addition & 0 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/CommitProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ protected boolean needCommit(Request request) {
case OpCode.reconfig:
case OpCode.multi:
case OpCode.setACL:
case OpCode.check:
return true;
case OpCode.sync:
return matchSyncs;
Expand Down
1 change: 1 addition & 0 deletions ...per-server/src/main/java/org/apache/zookeeper/server/quorum/FollowerRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public void run() {
case OpCode.reconfig:
case OpCode.setACL:
case OpCode.multi:
case OpCode.check:
zks.getFollower().request(request);
break;
case OpCode.createSession:
Expand Down
1 change: 1 addition & 0 deletions ...per-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public void run() {
case OpCode.reconfig:
case OpCode.setACL:
case OpCode.multi:
case OpCode.check:
zks.getObserver().request(request);
break;
case OpCode.createSession:
Expand Down
1 change: 1 addition & 0 deletions ...per-server/src/main/java/org/apache/zookeeper/server/quorum/ReadOnlyRequestProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public void run() {
case OpCode.reconfig:
case OpCode.setACL:
case OpCode.multi:
case OpCode.check:
sendErrorResponse(request);
continue;
case OpCode.closeSession:
Expand Down
1 change: 1 addition & 0 deletions ...er-server/src/main/java/org/apache/zookeeper/server/util/RequestPathMetricsCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ static boolean isWriteOp(int requestType) {
case ZooDefs.OpCode.reconfig:
case ZooDefs.OpCode.setACL:
case ZooDefs.OpCode.multi:
case ZooDefs.OpCode.check:
return true;
}
return false;
Expand Down
13 changes: 3 additions & 10 deletions zookeeper-server/src/test/java/org/apache/zookeeper/test/CheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,7 @@ private static void checkVersion(TestableZooKeeper zk, String path, int version)
private void testOperations(TestableZooKeeper zk) throws Exception {
Stat stat = new Stat();
zk.getData("/", false, stat);
checkVersion(zk, "/", -1);
checkVersion(zk, "/", stat.getVersion());
assertThrows(KeeperException.BadVersionException.class, () -> {
checkVersion(zk, "/", stat.getVersion() + 1);
});
assertThrows(KeeperException.NoNodeException.class, () -> {
checkVersion(zk, "/no-node", Integer.MAX_VALUE);
});
assertThrows(KeeperException.UnimplementedException.class, () -> checkVersion(zk, "/", -1));
}

@Test
Expand All @@ -96,7 +89,7 @@ public void testStandalone() throws Exception {
public void testStandaloneDatabaseReloadAfterCheck() throws Exception {
try {
testOperations(createClient());
} catch (Exception ignored) {
} catch (Throwable ignored) {
// Ignore to test database reload after check
}
stopServer();
Expand Down Expand Up @@ -131,7 +124,7 @@ public void testClusterDatabaseReloadAfterCheck() throws Exception {

try {
testOperations(qb.createClient(new CountdownWatcher(), QuorumPeer.ServerState.LEADING));
} catch (Exception ignored) {
} catch (Throwable ignored) {
// Ignore to test database reload after check
}
qb.shutdown(leader);
Expand Down
Loading