That's true, but we'll lose that ACL either way, right? I mean before that recent patch mentioned by Damien, the situation was the same: we emitted a warning message about a znode which had a non- existing ACL reference and went on.
"Ignoring acl XYZ as it does not exist in the cache" at INFO level. We're talking about an ACL which never gets persisted and now with the patch, ZK is not willing to startup in the situation anymore. Andor On Thu, 2025-02-06 at 11:03 -0800, Chris Nauroth wrote: > In the scenario described in the pull request, it sounds like we > ultimately > are not serializing the ACL for inode /foo. IIUC, this proposal would > result in setting /foo to world readable, not the original intended > ACL > that was dropped from serialization. It would fix the failure, but it > could > open a security risk. (Let me know if I misunderstood.) > > The only other solution I can think of is to persist a full > representation > of the ACLs per every node. The ReferenceCountedACLCache would not be > serialized, and instead be reconstructed from the deserialized nodes, > becoming purely a runtime memory optimization. The HDFS > implementation for > ACLs is similar to this. Of course, this would be a much bigger > change. > > Chris Nauroth > > > On Wed, Feb 5, 2025 at 4:03 PM Andor Molnar <an...@apache.org> wrote: > > > I've a crazy idea for this which is super quick: > > > > Here we add usage of ACL to the cache: > > > > https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L1358 > > > > What if we do this...? > > > > In the cache, when we realize that ACL is missing, return false: > > > > https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java#L175 > > > > In DataTree we'll modify the Znode ACL reference to "-1" which is > > "world readable", essentially removing the ACL from the znode and > > continue: > > > > synchronized (node) { > > if (!aclCache.addUsage(node.acl)) { > > // Fix missing ACL > > node.acl = OPEN_UNSAFE_ACL_ID; > > LOG.warn("Missing ACL has been removed from znode, > > proceeding."); > > } > > } > > > > Txn's processing will be fine, next snapshot will be "fixed". > > > > Andor > > > > > > > > On Wed, 2025-02-05 at 15:45 -0600, Andor Molnar wrote: > > > Hi ZK folks, > > > > > > Let me draw your attention to this ticket. We've seen this > > > happening > > > in > > > production and I would like to work on a fix. > > > > > > Damien already created a draft PR here: > > > https://github.com/apache/zookeeper/pull/2183 > > > > > > Let's take a closer look and work on a strategic solution. > > > > > > Thanks, > > > Andor > > > > > > > > > >
