Support custom ZooKeeper ACLs when creating a cluster

[bellatrix007]

Description

Adds HelixAdmin#addCluster(String clusterName, boolean recreateIfExists, List<ACL> acl) so callers can create a cluster whose metadata nodes are owned by a specific ZooKeeper identity instead of the client default ACL.

• The overload is a default method, so existing HelixAdmin implementations keep compiling. It delegates to addCluster(String, boolean) for a null/empty ACL and otherwise throws UnsupportedOperationException.
• ZKHelixAdmin implements it and routes the existing two-argument version through it with a null ACL.
• A null or empty ACL uses the client-default ACL and retains the original create-then-write cluster-config initialization, including initial data version 1. Custom ACLs create the config with its initial data at version 0, without requiring a separate WRITE.
• During initialization, both paths may reuse existing config parents, but fail if the config leaf already exists instead of overwriting that record.
• Existing-root checks in ZKHelixAdmin read only the root's data. A successful read, including null data, establishes existence and READ access consistently on ZooKeeper 3.6.3 and 3.8.6. Authorization failures are reported as HelixException; child metadata and its ACLs are not validated. This root-read requirement is an intentional change from the older existence-only check, not an exact preservation of all prior error behavior.

The ACL is applied to the cluster root and to every cluster metadata node created by addCluster (IDEALSTATES, CONFIGS/*, PROPERTYSTORE, LIVEINSTANCES, INSTANCES, EXTERNALVIEW, STATEMODELDEFS, CONTROLLER/*). ZooKeeper has no ACL inheritance, so each node has to be created with the ACL explicitly. For nested cluster paths, missing namespace ancestors above the cluster root use the client-default ACL; existing ancestor ACLs are not changed.

Usage

The Id scheme and format are determined by the authentication provider the ZooKeeper ensemble is running, and the client that calls addCluster must itself authenticate as a principal the ACL grants CREATE to.

Service and group identities (x509)

import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Id;

List<ACL> acl = Arrays.asList(
    // the service that owns the cluster
    new ACL(ZooDefs.Perms.ALL, new Id("x509", serviceId)),
    // an operator group, so ownership is not tied to a single service identity
    new ACL(ZooDefs.Perms.ALL, new Id("x509", adminGroupId)),
    // keep the cluster readable, otherwise unauthenticated sessions get NoAuth on
    // getData/getChildren and generic tooling breaks
    new ACL(ZooDefs.Perms.READ, ZooDefs.Ids.ANYONE_ID_UNSAFE));

admin.addCluster(clusterName, false, acl);

With the stock X509AuthenticationProvider, the id is the certificate subject DN as returned by X500Principal#getName (for example CN=my-service,OU=my-team,O=my-org,C=US); ids that are not valid DNs are rejected with InvalidACLException. Deployments that run a custom provider use whatever identity that provider extracts from the certificate, so take the exact string from your ZooKeeper operators rather than assuming a format.

Digest identities

List<ACL> acl = Collections.singletonList(new ACL(ZooDefs.Perms.ALL,
    new Id("digest", DigestAuthenticationProvider.generateDigest("user:password"))));

((ZkClient) zkClient).addAuthInfo("digest", "user:password".getBytes(StandardCharsets.UTF_8));

Prefer sourcing ACLs from configuration over hardcoding them, so identities can be rotated without a code change.

Caveats

• The ACL only covers what addCluster creates. Nodes written afterwards (resources, instances, live instances, ...) go through other code paths, and ZkClient.createPersistent(path, createParents) hardcodes Ids.OPEN_ACL_UNSAFE. Covering those means an ACL on the ZkClient itself, which is out of scope here.
• The calling client must satisfy the ACL. Creating the root requires CREATE on its parent, and creating metadata requires CREATE under the supplied ACL. NoAuth failures are wrapped in HelixException. Recreation still requires the permissions needed to traverse and delete the existing subtree.
• Failed creation is not rolled back. Initialization is not transactional and can leave partial metadata. Automatic path-based deletion could remove a replacement created concurrently by another client; even a version check cannot identify a replacement whose data version resets to zero. Inspect the remaining state before explicitly cleaning up or recreating a failed cluster.
• Existing clusters are not re-ACLed or validated for completeness. With recreateIfExists=false, addCluster returns true when the root exists and its data is readable; the supplied ACL is not applied to that existing root. It does not inspect child metadata or child permissions. A readable but incomplete cluster can therefore still return true, including after failed initialization. Use setACL with an identity granted ADMIN on the node for in-place ACL changes.
• Some deployments assign ACLs server side. A ZooKeeper ensemble running an authentication provider that stamps ACLs on create (rather than honoring the client-supplied list) will ignore this argument for ordinary clients. Likewise, ACLs are inert on an ensemble started with skipACL=yes. Check with whoever operates the ensemble before relying on this.

Tests

Coverage in TestZkHelixAdmin uses the embedded real ZooKeeper server for ACL behavior and mocks/spies for selected failures and concurrent-creation cases:

• testAddClusterWithAcl — the root and all 15 cluster metadata nodes carry the supplied ACL, and a node created after addCluster does not.

• testAddClusterWithoutAclKeepsDefaultAcl — two-argument, null-ACL, and empty-ACL calls retain the default ACL and initial config data version 1.

• testAddClusterDoesNotOverwriteConcurrentConfig — a second client creates the config during initialization; two-argument, null-ACL, empty-ACL, and custom-ACL calls fail without changing its fields, data version, or ACL.

• testAddClusterAclEnforcement — creates the cluster under a digest ACL, then opens a second unauthenticated ZooKeeper session and verifies it cannot read the root ACL, delete the root or its top-level children, read or overwrite the cluster config, inject a resource, or rewrite a child ACL to grant itself access.

• testAddClusterWithoutWritePermission — a custom ACL without WRITE initializes config data at version 0, while a later write is denied.

• Root-only cases cover an empty readable root, unreadable child metadata that is intentionally not inspected, unauthenticated digest-only callers, retries, and recreation.

• Namespace cases cover distinct owners creating sibling clusters and preserving existing ancestor ACLs.

• Failure cases cover partial initialization, unchanged authorization exceptions, concurrent child and parent creation, and preservation of both a replaced root and replaced metadata. A removed root is not recreated implicitly during metadata initialization.

The 22 targeted cluster-creation cases passed on both ZooKeeper 3.8.6 and 3.6.3. The 3.8.6 run used mvn -o -pl helix-core '-Dtest=TestZkHelixAdmin#testAddCluster*+testZkHelixAdmin' test; the 3.6.3 run used the same compiled cases through TestNG with the cached ZooKeeper and Jute 3.6.3 runtime jars.

[Copilot]

Pull request overview

Adds an ACL-aware cluster creation API so callers can create the cluster root znode with a caller-specified ZooKeeper ACL (while leaving child znodes on the existing default behavior), and verifies the behavior with new integration tests against the embedded ZooKeeper.

Changes:

• Added HelixAdmin#addCluster(String, boolean, List<ACL>) as a new overload for ACL-aware cluster creation.
• Implemented the new overload in ZKHelixAdmin, routing the existing 2-arg overload through it.
• Added test coverage in TestZkHelixAdmin to validate ACL application and enforcement boundaries.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
helix-core/src/main/java/org/apache/helix/HelixAdmin.java	Introduces the new addCluster(..., List<ACL>) API contract (default method) for ACL-aware cluster creation.
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java	Implements the ACL-aware overload and routes the existing overload through it with null.
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java	Adds integration tests for applying ACLs on the cluster root and validating enforcement behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java:447

• Avoid relying on the platform default charset when generating digest auth credentials; the default can vary across environments and make this test brittle. Use an explicit charset (UTF-8) when calling getBytes().

    final byte[] credentials = (owner + ":" + password).getBytes();

helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java:484

• Minor grammar: use “non-empty” instead of “non empty” in the error message for consistency with standard usage.

        Assert.fail("Expected the delete of a non empty cluster root to be rejected");

helix-core/src/main/java/org/apache/helix/HelixAdmin.java:125

• Minor grammar in Javadoc: use “non-empty” instead of “non empty”.

   * @throws UnsupportedOperationException if a non empty ACL is supplied and the implementation

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java:1684

• If createZKPaths(clusterName, acl) throws (e.g., because the supplied ACL doesn’t grant CREATE/WRITE on some path), addCluster currently returns false but leaves a partially-created cluster root behind. That can make subsequent addCluster(..., recreateIfExists=false) calls incorrectly return true just because the root exists, and it leaves garbage znodes in ZooKeeper. Consider best-effort cleanup of the root on failure so the operation is closer to atomic.

    try {
      createZKPaths(clusterName, acl);
    } catch (Exception e) {
      logger.error("Error creating cluster:" + clusterName, e);
      return false;
    }

helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java:570

• This test only deletes the digest-protected cluster inside the try block. If any assertion fails before the deleteRecursively call, the finally block closes the authorized client without cleaning up, leaving a cluster that the suite’s global unauthenticated _gZkClient may not be able to remove later. Please do best-effort cleanup in finally (before closing the authorized client) to keep the test isolated and avoid leaking protected znodes across failures.

    } finally {
      if (unauthorizedClient != null) {
        unauthorizedClient.close();
      }
      authorizedClient.close();

helix-core/src/main/java/org/apache/helix/HelixAdmin.java:125

• The PR description’s caveat says only the cluster root znode carries the supplied ACL and children keep the default open ACL, but the API Javadoc here (and the ZKHelixAdmin implementation/tests) indicate the ACL is applied to the root and all metadata nodes created by addCluster. Please align the PR description (or, if root-only was intended, adjust the implementation/Javadoc) so the documented security boundary is consistent.

   * @param acl ACLs applied to the cluster root node ("/{clusterName}") and to every cluster
   *            metadata node created underneath it by this call. If null or empty, the default ACL
   *            of the underlying metadata store client is used, making this equivalent to
   *            {@link #addCluster(String, boolean)}. ZooKeeper does not propagate ACLs to children,
   *            so nodes created after this call (resources, instances, live instances, ...) are
   *            NOT covered and keep the client default ACL. The ACL is only applied when the nodes
   *            are created by this call; the ACL of a pre-existing cluster is left untouched unless
   *            recreateIfExists is true.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

helix-core/src/main/java/org/apache/helix/HelixAdmin.java:125

• The PR description states that ZooKeeper ACLs do not propagate so “only the cluster root carries the supplied ACL” and that deeper ACLs would require a ZK client change. However, this new API contract/Javadoc (and ZKHelixAdmin implementation) applies the provided ACL to all metadata nodes created by this call (and their parents when createParents=true). Please reconcile the PR description/scope notes with the actual behavior so callers aren’t misled about what is and isn’t protected.

   * Add a cluster whose metadata store nodes are created with the given ACLs
   * @param clusterName
   * @param recreateIfExists If the cluster already exists, it will delete it and recreate
   * @param acl ACLs applied to the cluster root node ("/{clusterName}") and to every cluster
   *            metadata node created underneath it by this call. If null or empty, the default ACL
   *            of the underlying metadata store client is used, making this equivalent to
   *            {@link #addCluster(String, boolean)}. ZooKeeper does not propagate ACLs to children,
   *            so nodes created after this call (resources, instances, live instances, ...) are
   *            NOT covered and keep the client default ACL. The ACL is only applied when the nodes
   *            are created by this call; the ACL of a pre-existing cluster is left untouched unless
   *            recreateIfExists is true.

[LZD-PratyushBhatt]

discussed this with @bellatrix007 offline

on the ensembles this would deploy against, acls are assigned server side today, so a client supplied list is ignored. that behaviour would have to change for this argument to have any effect, and that is the part we are still working through.

two things I do not have a clear answer on yet.

sequencing: what lands when. if this merges first it is inert until the server side changes, and if the server side changes first then everything created in the meantime falls back to the client default. either order leaves a window, and it spans every deployment on that ensemble rather than just helix.

acl transfer: zk writes the acl at create time and never revisits it, so a server side change does not touch anything that already exists. existing nodes keep what they were created with, new ones get whatever the client passed, and that split is permanent unless something walks the tree and re-acls it. the ephemeral nodes make it uneven too, liveinstances and the leader node are recreated on every restart so they would turn over quickly while the rest stays as is.

is there a migration plan for that, or is the idea to let it converge as nodes get recreated?

[bellatrix007]

on the ensembles this would deploy against, acls are assigned server side today, so a client supplied list is ignored. that behaviour would have to change for this argument to have any effect, and that is the part we are still working through.

setX509ClientIdAsAcl will set client acls and ignore the supplied acls. For most of our real customers, this flag is false.

Sequencing wont matter since the required flag is already false wherever required for now.

acl transfer: zk writes the acl at create time and never revisits it, so a server side change does not touch anything that already exists. existing nodes keep what they were created with, new ones get whatever the client passed, and that split is permanent unless something walks the tree and re-acls it. the ephemeral nodes make it uneven too, liveinstances and the leader node are recreated on every restart so they would turn over quickly while the rest stays as is.

We will manually (with some automation) backfill the acls required in the existing clusters. The paths which are missed should be fine since the resources and instances' deletion / addition is governed by parent znode which will have acls.

is there a migration plan for that, or is the idea to let it converge as nodes get recreated?

Will backfill the data for now.

[LZD-PratyushBhatt]

Tested with zk 3.6.3 and 3.8.6, Few doubts and questions below

[LZD-PratyushBhatt]

hmmm, this is accurate but i think it reads like more is covered than it actually is, and this is the line someone checks before deciding a cluster is safe.

cluster created with an acl, then one instance and one resource added the normal way. same numbers on 3.6.3 and 3.8.6:

total 29, acl protected 18, world open 11
/INSTANCES/{id}/MESSAGES        31,s{'world,'anyone}
/INSTANCES/{id}/CURRENTSTATES   31,s{'world,'anyone}
/IDEALSTATES/myDB               31,s{'world,'anyone}

with MESSAGES open i read the participant session id off CURRENTSTATES, wrote a state transition into MESSAGES from a client that never authenticated, and the participant ran it:

EXECUTED MASTER->SLAVE  src=not-the-controller

can we name MESSAGES and CURRENTSTATES here? "resources, instances, live instances" sounds like data, but the control plane is in that list too.

[bellatrix007]

Updated in c44c601. The Javadoc now explicitly names INSTANCES/{instance}/MESSAGES and CURRENTSTATES as uncovered control-plane nodes and calls out the state-transition risk of an open MESSAGES node. It no longer suggests the ACL argument protects the whole cluster or nodes created later.

[LZD-PratyushBhatt]

CREATE on its own isn't enough. createZKPaths does a writeData on the cluster config right after creating it -> the acl needs WRITE too, and DELETE as well if anyone passes recreateIfExists.

CREATE|READ|DELETE gives:

KeeperErrorCode = NoAuth for /{cluster}/CONFIGS/CLUSTER/{cluster}
  at ZKHelixAdmin.createZKPaths(ZKHelixAdmin.java:1697)

and 1 more, ADMIN. setACL needs it, so if the acl gives it to nobody then the acl can never be changed again, not even by the client that made the cluster:

acl = world CREATE|READ|WRITE|DELETE
  setACL by creator      = NoAuthException
  setACL by anon         = NoAuthException
acl = digest owner CREATE|READ|WRITE|DELETE
  setACL by digest owner = NoAuthException

that is the acl testAddClusterWithAcl uses -> that cluster's acl is fixed at create time, and rotating or tightening it later means deleting the cluster and building it again.

can you verify if the server side provider on the ensemble stamps ADMIN for anyone, or does the acl we pass go in as is? if it goes in as is then can we list the full set here, getting it wrong puts you in the half built case below.

[bellatrix007]

This is intended, once the acls are set, we do not any actor to update acls unless it a ZK super user / helix admin.

can you verify if the server side provider on the ensemble stamps ADMIN for anyone, or does the acl we pass go in as is

It goes as is, but this is not the end of the world and can be edited.

[bellatrix007]

Updated the permission documentation in c44c601. Initial config data is now supplied during creation, removing the separate WRITE requirement for initialization; later updates still need WRITE. Recreation requires READ for traversal and DELETE on the relevant parents under their existing ACLs. The Javadoc also states that setACL requires ADMIN and the creator has no implicit ADMIN privilege. As noted above, omitting ADMIN is intentional in the ACL-distinguishing fixture; this change does not broaden that ACL.

[LZD-PratyushBhatt]

1 question, is there a follow up planned for ClusterSetup and the rest ClusterAccessor? both still call the two arg addCluster, and ClusterAccessor goes through ClusterSetup -> right now this only does anything if you hold a ZKHelixAdmin yourself.

clusters made through the cli or the rest server still come out with the open acl, and those are the paths most clusters actually get created on. not sure if that is intentional for a first cut.

[bellatrix007]

True, till now, the use cases that I require it for are library based, I can create a follow up for API but not a blocker for now.

[LZD-PratyushBhatt]

hmmm, if the acl doesn't include the calling client this goes two different ways on 3.6.3 and 3.8.6, and on 3.6.3 it fails quietly.

on 3.6.3 exists() isn't acl checked, so the exists() at the top of the method returns true on the retry even though the caller can't read the node. acl naming a different principal:

attempt 1: addCluster=false
attempt 2: addCluster=true
attempt 3: addCluster=true
top level znodes present: 0 of 8
recreateIfExists=true: THREW NoAuthException

so addCluster says true for a cluster that has nothing at all under the root, and the only loud failure is recreateIfExists, where deleteRecursively calls getChildren and NoAuth comes out of a method that returns boolean and declares nothing.

on 3.8.6 exists() does need READ -> the same input throws on attempt 2 instead of returning true. so this path changes behaviour when we upgrade.

can we catch NoAuth here and throw a HelixException saying the caller isn't in the supplied acl, so it fails the same way on both?

[bellatrix007]

READ is given to everyone (world), so 3.6.3 will not have false positives, but wrapped the no auth as helix exception

[LZD-PratyushBhatt]

ummm, if this throws part way through, the root is already there -> the exists() at the top of the method sees it and returns true at line 1662. acl of CREATE|READ|DELETE, caller is in it but WRITE is missing, on 3.6.3:

attempt 1: addCluster=false   isClusterSetup=false
attempt 2: addCluster=true    isClusterSetup=false
attempt 3: addCluster=true    isClusterSetup=false
top level znodes present: 2 of 8
missing: LIVEINSTANCES INSTANCES EXTERNALVIEW STATEMODELDEFS CONTROLLER PROPERTYSTORE

3.8.6 does the same. so every call after the first says true for a cluster that will never work. the description does mention leaving an incomplete cluster behind, but not that it then reports success forever, and i think that part is going to be painful to debug.

can we delete the root when createZKPaths fails, or have the early return check isClusterSetup instead of just exists?

[bellatrix007]

Final scope clarification in 4051917, superseding my earlier replies promising best-effort cleanup: the custom-ACL config WRITE failure is fixed by supplying its data at creation, and authorization failures consistently throw HelixException. However, automatic rollback has now been removed because even individually recorded paths can be deleted/recreated by another client, so cleanup can delete a replacement owned by that client. We are retaining the deliberately narrow root-read check rather than isClusterSetup/child traversal. Thus failed initialization may leave partial metadata, and a later recreateIfExists=false call can return true if that root is readable. The Javadoc and PR description explicitly state this remaining limitation and require inspecting the state before explicit cleanup or recreation. Regression coverage preserves concurrent children, parents, and replacement znodes without weakening the NoAuth checks.

[LZD-PratyushBhatt]

yeah this is the writeData i mentioned above, and it is why CREATE alone isn't enough. the createPersistent on the line before works and then this one fails with NoAuth when the acl has no WRITE.

controllerHistory at 1722 already does it the other way, createPersistent(path, emptyHistory, acl), and the helper for that is right there at 1746. can we do the same here so the acl only needs CREATE?

[bellatrix007]

Changed in c44c601 to use the existing create-with-data helper for the initial cluster config. The CONFIGS/CLUSTER parents are created first with the supplied ACL, since the data-taking overload does not create parents. The separate writeData call is removed. Added a regression case using CREATE|READ|DELETE that verifies initialization succeeds with the expected config data and ACLs while a subsequent config write is still denied.

[LZD-PratyushBhatt]

this cast is the only reason the test can authenticate. addAuthInfo is only on the concrete ZkClient, it isn't on RealmAwareZkClient, and nothing in helix-core or helix-rest calls it -> not sure how a controller or participant is meant to pass these credentials.

started a controller against a cluster created with this exact digest acl, on 3.6.3:

connect() RETURNED, isConnected=true
LIVEINSTANCES : []
CONTROLLER    : [ERRORS, STATUSUPDATES, MESSAGES, HISTORY]   (no LEADER)

so it comes up, says it is connected, and then never wins leadership or does any work. on 3.8.6 the same setup fails loudly instead, connect() throws Cluster structure is not set up, because isClusterSetup is built on exists() and that is acl checked there but not on 3.6.3.

x509 and sasl are fine either way, auth happens on the connection. can you check if there is a path here i am missing? otherwise can we use one of those in the test, or say in a comment that digest only works for an admin client?

[bellatrix007]

Added the scope clarification in c44c601 immediately above addAuthInfo: digest authentication is explicitly configured for this admin session, and the test verifies ACL enforcement rather than digest authentication support for controller/participant sessions created by ZKHelixManager. This does not add credential propagation or claim that those other sessions can use this digest ACL without their own authentication setup.

[LZD-PratyushBhatt]

hmmm, i think this comment says more than the test checks. idealStatePath is PropertyPathBuilder.idealState(clusterName), so it is the /IDEALSTATES container that addCluster creates, not a resource under it. the real state, /IDEALSTATES/myDB and the per instance nodes, gets created later by addResource and addInstance and is still world writable -> "leaving every piece of cluster state world writable" is still true after this change.

can we trim it to what the test actually asserts?

[bellatrix007]

Narrowed the comment in c44c601 to the actual assertions: unauthorized sessions cannot read/write the cluster config or create children under the IDEALSTATES container. It now explicitly says resource and per-instance nodes created later do not inherit these ACLs, rather than implying all cluster state is protected.

[LZD-PratyushBhatt]

nit: this is inside the try, so if an assertion above fails the cluster stays behind with an acl only the digest user can touch. this test re-auths and deletes first so it recovers on the next run, but nothing else on the same test zk can clean it up. can we move it into the finally?

[bellatrix007]

Added guarded cleanup in finally in c44c601, using the authenticated client before closing it. Nested finally blocks ensure both client-close calls are attempted even if cleanup fails. The successful-path delete/assertion is retained to verify the owner can delete the cluster; the finally cleanup also handles failures earlier in the test.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Preserve the existing default-ACL create-then-write behavior to avoid changing znode versions and data-change events.

Review effort: Lite
Findings: None

Resolved since last review (2)

• The new default overload always throws UnsupportedOperationException, even when acl is null/empty.…
• rawZooKeeper takes Object and relies on hard casts to a specific ZkClient/connection…

[LZD-PratyushBhatt]

javadoc looks much better now and the write fix also looks right.
btw one thing, i can't find the noauth wrap in your latest patch, addCluster looks unchanged to me. Did you forgot to push?

[LZD-PratyushBhatt]

this still returns true when the root exists but nothing is under it. with the acl from testAddClusterAclEnforcement, on 3.6.3 i get false then true with 0 of 8 znodes. can we check isClusterSetup here instead of exists?

[bellatrix007]

Fixed in e6e0d50. Both the existing-root fast path and the concurrent NodeExists path now validate the required metadata before returning success. The validator reuses the path list from ZKUtil but uses permission-checked reads, so missing metadata returns false and NoAuth becomes a HelixException on both ZooKeeper versions. Failed initialization also attempts cleanup only after this invocation created the root; if cleanup fails, a later retry cannot report success merely because that partial root remains.

[bellatrix007]

Scope update in ab9109b, superseding the completeness-validation part of my previous reply: we narrowed this to reading only the root data, with no child metadata or child ACL checks. An unauthenticated caller using the digest-only ACL still gets HelixException with the NoAuth cause on the initial failure and subsequent retries on both ZooKeeper versions. However, a readable incomplete root is intentionally accepted as existing; this is not a full isClusterSetup validation. Best-effort cleanup remains, and the PR description/Javadoc now explicitly document that remaining completeness limitation.

[LZD-PratyushBhatt]

this acl has no world entry, so read isn't open to anyone but the digest user. can we add a case that calls addCluster with it from a client that hasn't authed? on 3.6.3 that returns true on an empty root.

[bellatrix007]

Added testAddClusterUnauthorizedCaller in e6e0d50. It uses a digest-only ACL with no world entry and calls addCluster from an unauthenticated client. The initial attempt, a retry with recreateIfExists=false, and a recreation attempt all throw HelixException with the original NoAuth cause rather than reporting success for an empty root. A separate authenticated owner verifies the root is empty and cleans it up. The case passes with both ZooKeeper 3.6.3 and 3.8.6.

[bellatrix007]

@LZD-PratyushBhatt Following up on your latest review: the NoAuth/completeness changes were still a separate local follow-up when you reviewed c44c601. They are now pushed in e6e0d50. addCluster normalizes wrapped NoAuth failures from lookup, creation, and recreation into HelixException; validates required readable metadata instead of accepting root existence alone; and preserves the original error if best-effort cleanup fails. The unauthenticated digest-ACL regression you requested is included. All 12 targeted cluster-creation cases passed on ZooKeeper 3.6.3 and 3.8.6. I have also replied on both inline threads.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical ACL compatibility and namespace-isolation issues, plus a moderate default-path regression, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Two-argument overload breaks existing-root compatibility · New
• Nested ancestors incorrectly receive the cluster ACL · New
• Default ACL path changes initial ZooKeeper data version · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Address the cleanup race and root existence-check issue before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Cleanup can delete concurrently created child nodes · New

Resolved since last review (3)

• Nested ancestors incorrectly receive the cluster ACL
• Two-argument overload breaks existing-root compatibility
• Default ACL path changes initial ZooKeeper data version

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical defects can reject normal cluster creation and delete a concurrently recreated root.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Cleanup can delete a concurrently recreated znode · New
• Parent comparison rejects normal CONFIGS creation · New

Resolved since last review (1)

• Cleanup can delete concurrently created child nodes

[Copilot]

Copilot review overview

🟡 Changes recommended

Two critical cluster-creation race and initialization defects remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Default ACL path can overwrite concurrently created config · New
• Root-parent guard prevents creating new cluster configuration · New

Resolved since last review (2)

• Parent comparison rejects normal CONFIGS creation
• Cleanup can delete a concurrently recreated znode

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (2)

• Root-parent guard prevents creating new cluster configuration
• Default ACL path can overwrite concurrently created config

[LZD-PratyushBhatt]

LGTM, thanks for addressing the comments!