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 preserves the previous behavior exactly.

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 — applying it only to the root would block adding/removing top-level znodes while leaving all cluster state readable, writable and deletable by any session.

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
    // exists/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. If the supplied ACL does not grant the caller CREATE on the root, cluster creation fails partway through and leaves an incomplete cluster behind.
• Existing clusters are not re-ACLed. With recreateIfExists=false, addCluster returns true for an existing cluster and the ACL is skipped; use setACL to change ACLs in place.
• 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

Added to TestZkHelixAdmin, all run against the embedded real ZooKeeper server (no mocks):

• testAddClusterWithAcl — the root and all 15 cluster metadata nodes carry the supplied ACL, and a node created after addCluster does not.
• testAddClusterWithoutAclKeepsDefaultAcl — the two-argument path is unchanged.
• 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.

mvn -pl helix-core test -Dtest=TestZkHelixAdmin → 25/25 pass.

[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.

[bellatrix007]

Fixed in fdedb6f — rawZooKeeper now takes HelixZkClient instead of Object, so both call sites are type checked. The cast to the concrete ZkClient/ZkConnection is still needed because getConnection()/getZookeeper() are not on the interface, but it is now isolated to this one helper.

[bellatrix007]

Good catch, fixed in fdedb6f. The default method now delegates to addCluster(String, boolean) when the ACL is null or empty, and only throws UnsupportedOperationException when a caller actually asks for custom ACLs. Javadoc updated with an @throws to match.

[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.