Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit d7d38d7

Browse files
committedApr 19, 2023
[improve][broker] ServerCnx: go to Failed state when auth fails (apache#19312)
PIP: apache#12105 When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. This is not a breaking change. - [x] `doc-not-needed` PR in forked repository: #18 (cherry picked from commit 8049690)
1 parent a347f7f commit d7d38d7

File tree

3 files changed

+3
-4
lines changed

3 files changed

+3
-4
lines changed
 

‎pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -966,7 +966,7 @@ protected void handleAuthResponse(CommandAuthResponse authResponse) {
966966
service.getPulsarStats().recordConnectionCreateFail();
967967
state = State.Failed;
968968
log.warn("[{}] Authentication failed: {} ", remoteAddress, e.getMessage());
969-
ctx.writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, e.getMessage()));
969+
ctx.writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, "Unable to authenticate"));
970970
close();
971971
} catch (Exception e) {
972972
service.getPulsarStats().recordConnectionCreateFail();

‎pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -728,7 +728,7 @@ public void testAuthResponseWithFailingAuthData() throws Exception {
728728

729729
Object response3 = getResponse();
730730
assertTrue(response3 instanceof CommandError);
731-
assertEquals(((CommandError) response3).getMessage(), "Do not pass");
731+
assertEquals(((CommandError) response3).getMessage(), "Unable to authenticate");
732732
assertEquals(serverCnx.getState(), State.Failed);
733733
assertFalse(serverCnx.isActive());
734734
channel.finish();

‎pulsar-broker/src/test/java/org/apache/pulsar/broker/service/utils/ClientChannelHelper.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,8 @@
1919
package org.apache.pulsar.broker.service.utils;
2020

2121
import java.util.Queue;
22-
23-
import org.apache.pulsar.common.api.proto.CommandPartitionedTopicMetadataResponse;
2422
import org.apache.pulsar.common.api.proto.CommandAuthChallenge;
23+
import org.apache.pulsar.common.api.proto.CommandPartitionedTopicMetadataResponse;
2524
import org.apache.pulsar.common.protocol.PulsarDecoder;
2625
import org.apache.pulsar.common.api.proto.CommandAck;
2726
import org.apache.pulsar.common.api.proto.CommandCloseConsumer;

0 commit comments

Comments
 (0)
Please sign in to comment.