Skip to content

Commit 4d034f4

Browse files
ptupitsynptupitsyn
authored
IGNITE-28303 Clients: do not retry authentication errors (#7955)
Fix authn error handing in .NET and Java clients: - Propagate authn exceptions correctly - Do not invoke `RetryPolicy` for those errors
1 parent ca6afea commit 4d034f4

11 files changed

Lines changed: 121 additions & 39 deletions

File tree

‎modules/cli/src/main/java/org/apache/ignite/internal/cli/core/exception/handler/SqlExceptionHandler.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.apache.ignite.internal.logger.IgniteLogger;
3232
import org.apache.ignite.internal.logger.Loggers;
3333
import org.apache.ignite.internal.util.ExceptionUtils;
34+
import org.apache.ignite.lang.ErrorGroups.Authentication;
3435
import org.apache.ignite.lang.ErrorGroups.Client;
3536
import org.apache.ignite.lang.ErrorGroups.Sql;
3637
import org.apache.ignite.lang.IgniteCheckedException;
@@ -54,6 +55,7 @@ public class SqlExceptionHandler implements ExceptionHandler<SQLException> {
5455

5556
/** Default constructor. */
5657
private SqlExceptionHandler() {
58+
sqlExceptionMappers.put(Authentication.AUTHENTICATION_ERR_GROUP.groupCode(), SqlExceptionHandler::authnErrUiComponent);
5759
sqlExceptionMappers.put(Client.CLIENT_ERR_GROUP.groupCode(), SqlExceptionHandler::connectionErrUiComponent);
5860
sqlExceptionMappers.put(Sql.SQL_ERR_GROUP.groupCode(), SqlExceptionHandler::sqlErrUiComponent);
5961
}
@@ -66,15 +68,6 @@ private static ErrorUiComponent connectionErrUiComponent(IgniteException e) {
6668
if (e.getCause() instanceof IgniteClientConnectionException) {
6769
IgniteClientConnectionException cause = (IgniteClientConnectionException) e.getCause();
6870

69-
InvalidCredentialsException invalidCredentialsException = findCause(cause, InvalidCredentialsException.class);
70-
if (invalidCredentialsException != null) {
71-
return ErrorUiComponent.builder()
72-
.header("Could not connect to node. Check authentication configuration")
73-
.details(invalidCredentialsException.getMessage())
74-
.verbose(extractCauseMessage(cause.getMessage()))
75-
.build();
76-
}
77-
7871
SSLHandshakeException sslHandshakeException = findCause(cause, SSLHandshakeException.class);
7972
if (sslHandshakeException != null) {
8073
return ErrorUiComponent.builder()
@@ -90,6 +83,19 @@ private static ErrorUiComponent connectionErrUiComponent(IgniteException e) {
9083
return fromIgniteException(CLIENT_CONNECTION_FAILED_MESSAGE, e);
9184
}
9285

86+
private static ErrorUiComponent authnErrUiComponent(IgniteException e) {
87+
InvalidCredentialsException invalidCredentialsException = findCause(e, InvalidCredentialsException.class);
88+
if (invalidCredentialsException != null) {
89+
return ErrorUiComponent.builder()
90+
.header("Could not connect to node. Check authentication configuration")
91+
.details(invalidCredentialsException.getMessage())
92+
.verbose(extractCauseMessage(e.getMessage()))
93+
.build();
94+
}
95+
96+
return fromIgniteException("Could not connect to node. Check authentication configuration", e);
97+
}
98+
9399
@Nullable
94100
private static <T extends Throwable> T findCause(Throwable e, Class<T> type) {
95101
while (e != null) {

‎modules/client/src/integrationTest/java/org/apache/ignite/internal/client/ItThinClientAuthenticationTest.java‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.stream.Collectors;
3636
import org.apache.ignite.client.BasicAuthenticator;
3737
import org.apache.ignite.client.IgniteClient;
38+
import org.apache.ignite.client.IgniteClient.Builder;
3839
import org.apache.ignite.internal.app.IgniteImpl;
3940
import org.apache.ignite.internal.configuration.ConfigurationRegistry;
4041
import org.apache.ignite.internal.security.authentication.basic.BasicAuthenticationProviderChange;
@@ -98,12 +99,17 @@ void setUp() {
9899

99100
assertThat(enableAuthentication, willCompleteSuccessfully());
100101

101-
clientWithAuth = IgniteClient.builder()
102+
Builder builder = IgniteClient.builder()
102103
.authenticator(basicAuthenticator)
103-
.addresses(getClientAddresses().toArray(new String[0]))
104-
.build();
104+
.addresses(getClientAddresses().toArray(new String[0]));
105+
106+
await().untilAsserted(() -> {
107+
try (IgniteClient c = builder.build()) {
108+
assertThat(checkConnection(c), willCompleteSuccessfully());
109+
}
110+
});
105111

106-
await().untilAsserted(() -> checkConnection(clientWithAuth));
112+
clientWithAuth = builder.build();
107113
}
108114

109115
@AfterEach
@@ -174,31 +180,33 @@ void connectionIsClosedIfUserRemoved() {
174180
}
175181

176182
@Test
177-
void renameBasicProviderAndThenChangeUserPassword() {
183+
void renameBasicProviderAndThenChangeUserPassword() throws InterruptedException {
178184
updateClusterConfiguration("ignite {\n"
179185
+ "security.authentication.providers.basic={\n"
180186
+ "type=basic,\n"
181187
+ "users=[{username=newuser,password=newpassword}]},"
182188
+ "security.authentication.providers.default=null\n"
183189
+ "}");
184190

185-
try (IgniteClient client = IgniteClient.builder()
186-
.authenticator(BasicAuthenticator.builder().username("newuser").password("newpassword").build())
187-
.addresses(getClientAddresses().toArray(new String[0]))
188-
.build()) {
191+
await().untilAsserted(() -> {
192+
try (IgniteClient client = IgniteClient.builder()
193+
.authenticator(BasicAuthenticator.builder().username("newuser").password("newpassword").build())
194+
.addresses(getClientAddresses().toArray(new String[0]))
195+
.build()) {
189196

190-
checkConnection(client);
197+
checkConnection(client);
191198

192-
securityConfiguration.authentication().providers()
193-
.get("basic")
194-
.change(change -> {
195-
change.convert(BasicAuthenticationProviderChange.class)
196-
.changeUsers()
197-
.update("newuser", user -> user.changePassword("newpassword-changed"));
198-
}).join();
199+
securityConfiguration.authentication().providers()
200+
.get("basic")
201+
.change(change -> {
202+
change.convert(BasicAuthenticationProviderChange.class)
203+
.changeUsers()
204+
.update("newuser", user -> user.changePassword("newpassword-changed"));
205+
}).join();
199206

200-
await().until(() -> checkConnection(client), willThrowWithCauseOrSuppressed(InvalidCredentialsException.class));
201-
}
207+
await().until(() -> checkConnection(client), willThrowWithCauseOrSuppressed(InvalidCredentialsException.class));
208+
}
209+
});
202210
}
203211

204212
/**

‎modules/client/src/integrationTest/java/org/apache/ignite/internal/client/ItThinClientSqlTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,7 @@ void testResultSetMappingColumnNameMismatch() {
626626
IgniteException.class,
627627
() -> client().sql().execute((Transaction) null, Mapper.of(Pojo.class), query));
628628

629-
assertEquals("Failed to deserialize server response for op 50: No mapped object field found for column 'FOO'", e.getMessage());
629+
assertEquals("No mapped object field found for column 'FOO'", e.getMessage());
630630
}
631631

632632
@Test

‎modules/client/src/main/java/org/apache/ignite/internal/client/ReliableChannel.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import org.apache.ignite.internal.logger.IgniteLogger;
7474
import org.apache.ignite.internal.thread.NamedThreadFactory;
7575
import org.apache.ignite.internal.util.IgniteUtils;
76+
import org.apache.ignite.lang.ErrorGroups.Authentication;
7677
import org.apache.ignite.lang.IgniteException;
7778
import org.apache.ignite.network.ClusterNode;
7879
import org.jetbrains.annotations.Nullable;
@@ -779,7 +780,8 @@ private boolean shouldRetry(
779780
return false;
780781
}
781782

782-
if (exception.code() == CLUSTER_ID_MISMATCH_ERR) {
783+
if (exception.code() == CLUSTER_ID_MISMATCH_ERR
784+
|| exception.groupCode() == Authentication.AUTHENTICATION_ERR_GROUP.groupCode()) {
783785
return false;
784786
}
785787

‎modules/client/src/main/java/org/apache/ignite/internal/client/TcpClientChannel.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@
7777
import org.apache.ignite.internal.util.ViewUtils;
7878
import org.apache.ignite.lang.ErrorGroups.Table;
7979
import org.apache.ignite.lang.IgniteException;
80+
import org.apache.ignite.lang.TraceableException;
8081
import org.apache.ignite.network.NetworkAddress;
8182
import org.apache.ignite.sql.SqlBatchException;
8283
import org.apache.ignite.tx.TransactionException;
@@ -533,6 +534,10 @@ private <T> void completeAsync(
533534

534535
return null;
535536
} catch (Throwable e) {
537+
if (e instanceof TraceableException) {
538+
throw sneakyThrow(e);
539+
}
540+
536541
log.error("Failed to deserialize server response [remoteAddress=" + cfg.getAddress() + ", opCode=" + opCode + "]: "
537542
+ e.getMessage(), e);
538543

@@ -754,7 +759,13 @@ private CompletableFuture<Object> handshakeAsync(ProtocolVersion ver) throws Ign
754759
metrics.handshakesFailedTimeoutIncrement();
755760
throw new IgniteClientConnectionException(CONNECTION_ERR, "Handshake timeout", endpoint(), err);
756761
}
762+
757763
metrics.handshakesFailedIncrement();
764+
765+
if (err instanceof TraceableException) {
766+
throw new IgniteClientConnectionException(((TraceableException) err).code(), "Handshake error", endpoint(), err);
767+
}
768+
758769
throw new IgniteClientConnectionException(CONNECTION_ERR, "Handshake error", endpoint(), err);
759770
});
760771
}

‎modules/client/src/test/java/org/apache/ignite/client/ClientAuthenticationTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
2424
import static org.apache.ignite.internal.util.IgniteUtils.closeAll;
2525
import static org.hamcrest.MatcherAssert.assertThat;
26+
import static org.junit.jupiter.api.Assertions.assertEquals;
2627

2728
import java.util.UUID;
2829
import org.apache.ignite.client.fakes.FakeIgnite;
@@ -93,6 +94,23 @@ public void testAuthnOnClientAuthnOnServer() {
9394
client = startClient(BasicAuthenticator.builder().username(DEFAULT_USERNAME).password(DEFAULT_PASSWORD).build());
9495
}
9596

97+
@Test
98+
public void testBadCredentialsAreNotRetried() {
99+
server = startServer(true);
100+
101+
BasicAuthenticator authenticator = BasicAuthenticator.builder().username("u").password("wrong").build();
102+
103+
var builder = IgniteClient.builder()
104+
.addresses("127.0.0.1:" + server.port())
105+
.authenticator(authenticator)
106+
.retryPolicy(new RetryLimitPolicy().retryLimit(5));
107+
108+
IgniteTestUtils.assertThrowsWithCause(builder::build, InvalidCredentialsException.class, "Authentication failed");
109+
110+
// The server should have received exactly one connection attempt, no retries.
111+
assertEquals(1, server.metrics().sessionsRejected());
112+
}
113+
96114
private IgniteClient startClient(@Nullable IgniteClientAuthenticator authenticator) {
97115
return IgniteClient.builder()
98116
.addresses("127.0.0.1:" + server.port())

‎modules/client/src/test/java/org/apache/ignite/client/ClientKeyValueViewTest.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -579,32 +579,31 @@ public void testNonNullableColumnWithDefaultValueSetNullThrowsException() {
579579

580580
@Test
581581
public void testGetNullValueThrows() {
582-
testNullValueThrows(view -> view.get(null, DEFAULT_ID), "getNullable", 12);
582+
testNullValueThrows(view -> view.get(null, DEFAULT_ID), "getNullable");
583583
}
584584

585585
@Test
586586
public void testGetAndPutNullValueThrows() {
587-
testNullValueThrows(view -> view.getAndPut(null, DEFAULT_ID, DEFAULT_NAME), "getNullableAndPut", 16);
587+
testNullValueThrows(view -> view.getAndPut(null, DEFAULT_ID, DEFAULT_NAME), "getNullableAndPut");
588588
}
589589

590590
@Test
591591
public void testGetAndRemoveNullValueThrows() {
592-
testNullValueThrows(view -> view.getAndRemove(null, DEFAULT_ID), "getNullableAndRemove", 32);
592+
testNullValueThrows(view -> view.getAndRemove(null, DEFAULT_ID), "getNullableAndRemove");
593593
}
594594

595595
@Test
596596
public void testGetAndReplaceNullValueThrows() {
597-
testNullValueThrows(view -> view.getAndReplace(null, DEFAULT_ID, DEFAULT_NAME), "getNullableAndReplace", 26);
597+
testNullValueThrows(view -> view.getAndReplace(null, DEFAULT_ID, DEFAULT_NAME), "getNullableAndReplace");
598598
}
599599

600-
private void testNullValueThrows(Consumer<KeyValueView<Long, String>> run, String methodName, int op) {
600+
private void testNullValueThrows(Consumer<KeyValueView<Long, String>> run, String methodName) {
601601
KeyValueView<Long, String> primitiveView = defaultTable().keyValueView(Mapper.of(Long.class), Mapper.of(String.class));
602602
primitiveView.put(null, DEFAULT_ID, null);
603603

604604
var ex = assertThrowsWithCause(() -> run.accept(primitiveView), UnexpectedNullValueException.class);
605605
assertEquals(
606-
format("Failed to deserialize server response for op {}: Got unexpected null value: use `{}` sibling method instead.",
607-
op, methodName),
606+
format("Got unexpected null value: use `{}` sibling method instead.", methodName),
608607
ex.getMessage());
609608
}
610609

‎modules/platforms/dotnet/Apache.Ignite.Tests/BasicAuthenticatorTests.cs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,27 @@ public async Task TestAuthnOnServerNoAuthnOnClient()
6464
Assert.AreEqual(ErrorGroups.Authentication.InvalidCredentials, invalidCredentialsException.Code);
6565
}
6666

67+
[Test]
68+
public async Task TestBadCredentialsAreNotRetried()
69+
{
70+
await EnableAuthn(true);
71+
72+
var retryPolicy = new TestRetryPolicy();
73+
var cfg = new IgniteClientConfiguration(GetConfig())
74+
{
75+
RetryPolicy = retryPolicy,
76+
Authenticator = new BasicAuthenticator
77+
{
78+
Username = "user-1",
79+
Password = "wrong"
80+
}
81+
};
82+
83+
var ex = Assert.ThrowsAsync<IgniteClientConnectionException>(async () => await IgniteClient.StartAsync(cfg));
84+
Assert.IsInstanceOf<InvalidCredentialsException>(ex.InnerException);
85+
Assert.AreEqual(0, retryPolicy.InvokeCount, "Retry policy should not be invoked");
86+
}
87+
6788
[Test]
6889
public async Task TestAuthnOnClientAndServer()
6990
{
@@ -150,4 +171,15 @@ await TestUtils.WaitForConditionAsync(async () =>
150171

151172
_authnEnabled = enable;
152173
}
174+
175+
private class TestRetryPolicy : RetryLimitPolicy
176+
{
177+
public int InvokeCount { get; private set; }
178+
179+
public override bool ShouldRetry(IRetryPolicyContext context)
180+
{
181+
InvokeCount++;
182+
return base.ShouldRetry(context);
183+
}
184+
}
153185
}

‎modules/platforms/dotnet/Apache.Ignite/IgniteException.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public class IgniteException : Exception
4040
public IgniteException(Guid traceId, int code, string? message, Exception? innerException = null)
4141
: base(message, innerException)
4242
{
43-
TraceId = traceId;
43+
TraceId = (innerException as IgniteException)?.TraceId ?? traceId;
4444
Code = code;
4545
}
4646

‎modules/platforms/dotnet/Apache.Ignite/Internal/ClientFailoverSocket.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -881,6 +881,12 @@ private bool ShouldRetry(Exception exception, ClientOp op, int attempt, IRetryPo
881881
return false;
882882
}
883883

884+
if (e is IgniteException { GroupCode: ErrorGroups.Authentication.GroupCode })
885+
{
886+
// Authentication errors should not be retried.
887+
return false;
888+
}
889+
884890
if (retryPolicy is null or RetryNonePolicy)
885891
{
886892
return false;

0 commit comments

Comments
 (0)