[adityaanikam]

Fixes #3184

ZooKeeper always reports SyncConnected first. When a SASL client is used, it then reports SaslAuthenticated, which replaces the stored connection state, so the state is no longer SyncConnected after authentication. waitUntilConnected therefore waits for SaslAuthenticated when SASL is configured. That check was added in #3102 to fix #3101 (it was then named isKerberosAuthEnabled) and it only recognized Krb5LoginModule. A DIGEST-MD5 client against a server that requires SASL client authentication (sessionRequireClientSASLAuth=true) behaves the same way, but waitUntilConnected kept waiting for SyncConnected. Any call made after the state was replaced, for example a reconnect, retryUntilConnected after a connection loss, or a race on the initial connect, then times out although the authentication itself succeeded.

The check no longer matches login module class names. isSaslAuthEnabled (renamed from isKerberosAuthEnabled) now returns true when the SASL client is enabled and the JAAS config has an entry for the client context. ZooKeeper builds its SASL client whenever that entry exists, whatever login module it names, so this covers Kerberos, DIGEST and custom modules.

Because the check now applies to every SASL module, waitUntilConnected also accepts SyncConnected when ZooKeeper connected without SASL: its SASL client could not be created, for example because the login failed (missing keytab, expired ticket cache, a login module that cannot be loaded), or it failed. ZooKeeper then continues to connect without SASL if the server allows it, so the session is usable but never reaches SaslAuthenticated. Before #3102 that connection worked, and this keeps it working instead of timing out.

Testing: TestZkClientKerberos is renamed to TestZkClientSasl, as it covers more than Kerberos now.

• testConnectsToDigestSaslServer connects to an in-process ZooKeeper server that requires DIGEST SASL (sessionRequireClientSASLAuth=true) and checks waitUntilConnected after authentication.
• testConnectsWhenSaslLoginFails uses a login module that always fails against the non-SASL test server. It fails without the change with ZkTimeoutException: Unable to connect to zookeeper server within timeout and passes with it.
• The login module predicate tests for DIGEST and a custom module share one data provider.

TestZkClientSasl runs 6 tests with no failures, and the full zookeeper-api module run passes (106 tests, 0 failures).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[ReiValera]

Should this method name change now? Same with the Javadoc for the return statement

[adityaanikam]

Good point, the method covers DIGEST as well now, so I renamed it to isSaslAuthEnabled and updated its javadoc (including the @return), the log message, and the tests that reach it by reflection. Pushed as a separate commit.

[junkaixue]

This logic hard-codes only Krb5LoginModule and DigestLoginModule as SASL authentication. ZooKeeper clients can use a custom or wrapped JAAS LoginModule; in that case, SASL may still complete and emit SaslAuthenticated, but this method returns false and waitUntilConnected() waits for SyncConnected instead. That can reintroduce the connection timeout this change is trying to fix.

Can we determine the expected connection state from whether client SASL is enabled/configured, rather than from a substring match on two specific login-module class names? At minimum, please add coverage for a non-built-in/custom login-module configuration.

[adityaanikam]

Thanks, that makes sense. I checked how ZooKeeper decides this: ZooKeeperSaslClient builds its SASL client whenever the JAAS config has an entry for the client context, whatever login module that entry names, and clientTunneledAuthenticationInProgress() uses the same check. So the module class name is not what decides it.

I removed the Krb5LoginModule/DigestLoginModule match. isSaslAuthEnabled() now returns true when the SASL client is enabled and the JAAS config has an entry for the client context. I also replaced the predicate test with tests for a DIGEST module and a custom module. They enable SASL after the client is built, because with SASL expected a live connection to the non-SASL test server would wait for a SaslAuthenticated event that never arrives.

[junkaixue]

Could we add an integration test that configures a DIGEST-enabled server and client before building ZkClient, then asserts that the client connects successfully? This helper deliberately connects to the non-SASL server first and enables SASL afterward. The current tests therefore verify isSaslAuthEnabled() by reflection, but do not exercise the waitUntilConnected() path this PR is intended to fix. A regression that still times out during a real DIGEST connection would pass these tests.

[adityaanikam]

Thanks, I added the integration test in 4b63a5a.

It starts a ZooKeeper server that requires DIGEST SASL, builds the client with SASL already configured, and checks that the client connects. It then waits for SaslAuthenticated and checks that waitUntilConnected() returns.

With the fix, the five tests in TestZkClientKerberos pass. With waitUntilConnected() expecting SyncConnected again, as before the fix, the new test fails. On a local server the client sees SyncConnected before SaslAuthenticated replaces it, so building the client alone passes either way. The waitUntilConnected() call after SaslAuthenticated is what makes the old behavior fail.

[junkaixue]

Thanks for the integration test and for grounding the check in how ZooKeeperSaslClient itself decides to attempt SASL. That resolves my earlier concern about matching on module class names. A couple of follow-ups before I approve:

1. Behavior when SASL is configured but login fails (blocking)

ClientCnxn.startConnect catches LoginException from building the SASL client (missing keytab, expired ticket cache, unloadable custom module), queues AuthFailed, and continues connecting without SASL "if the server allows it". In that case the session still reaches SyncConnected and is usable, but _currentState never becomes SaslAuthenticated, so connect() waits for the full timeout and throws ZkTimeoutException. Before #3102 that degraded connection worked. #3102 changed it for Kerberos only. This PR extends the fail-fast to every SASL module.

Failing fast may well be the right choice, but it is a behavior change beyond the bug fix. Have waitUntilConnected() also accept SyncConnected when the underlying ZooKeeper.getSaslClient() reports isFailed(). Helix relies a lot on ZK behaviors.

2. PR description does not match the actual mechanism

The description says SyncConnected "never follows". ZooKeeper always emits SyncConnected first on connect; SaslAuthenticated then overwrites _currentState. The bug is that any later waitUntilConnected() call (reconnect, retryUntilConnected after a connection loss, or a race on the initial connect) compares against a state that has already been replaced. Your comment on the integration test describes this correctly. Please update the description, since it becomes the squash commit message.

3. Test nits

• testConnectsToDigestSaslServer: System.setProperty("zookeeper.authProvider.1", ...) and ProviderRegistry.reset() run before the try, so if new ZooKeeperServer(...) throws, the JVM-global registry and property leak into later tests. Move them inside the try.
• Consider setting zookeeper.sessionRequireClientSASLAuth=true on the test server so it matches the scenario in Helix ZkClient times out after successful ZooKeeper SASL DIGEST-MD5 authentication #3184. The test is still a valid regression test without it.
• Use TestHelper.getRandomPort() instead of the ServerSocket(0) block, consistent with TestRawZkClient.
• testIsSaslAuthEnabled_WhenSaslDisabled: zookeeper.sasl.client defaults to true, so SASL is not disabled here; the method returns false because there is no Client JAAS entry. Either set the property to false explicitly or rename the test.
• The DIGEST and custom-module predicate tests differ only by module name; a @DataProvider would collapse them.
• The class name TestZkClientKerberos now covers DIGEST and custom modules too. TestZkClientSasl would be more accurate.

[adityaanikam]

Thanks for the detailed review. I pushed ca9d35b.

1. waitUntilConnected now also accepts SyncConnected when ZooKeeper has no SASL client or its client failed. In ZooKeeper 3.6.3 a failed login leaves getSaslClient() null and the client connects without SASL, so I check for null as well as isFailed(). A new test uses a login module that always fails: it times out without the change and passes with it.
2. I rewrote the description to match the mechanism you described.
3. The test points are done: the property and registry reset are inside the try, the server sets sessionRequireClientSASLAuth, getRandomPort() is used, the disabled test sets zookeeper.sasl.client=false, the module tests share a data provider, and the class is now TestZkClientSasl.

The full zookeeper-api module passes (106 tests).

[junkaixue]

Thanks, ca9d35b covers everything from the last round, and I verified TestZkClientSasl passes when merged onto current master (ZooKeeper 3.9.3).

One last change, and it corrects my own earlier suggestion: please drop the || saslClient.isFailed() half of isSaslSkipped() and keep only the null check you added. When the SASL client is in the failed state, ClientCnxn moves the session to AUTH_FAILED, so that session is never usable. Accepting SyncConnected there only lets connect() return a dead client in the window before the AuthFailed event is processed. I reproduced that with a Client JAAS entry whose login succeeds but produces no SASL client: build() returns with zkState=AUTH_FAILED. With only the null check it times out with ZkTimeoutException, and your tests still pass. Please also trim "or it failed" from the Javadoc and the PR description.

LGTM once that is in.

Also, I triggered the approval workflows.