Skip to content

Commit

Permalink
Update tests
Browse files Browse the repository at this point in the history
Signed-off-by: Andrew Carbonetto <[email protected]>
  • Loading branch information
acarbonetto committed Nov 19, 2024
1 parent 58ffca6 commit e2af4fa
Show file tree
Hide file tree
Showing 10 changed files with 195 additions and 67 deletions.
6 changes: 6 additions & 0 deletions glide-core/redis-rs/redis/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@ pub enum ErrorKind {

/// Not all slots are covered by the cluster
NotAllSlotsCovered,

/// Used when an error occurs on when user perform wrong usage of management operation.
/// E.g. not allowed configuration change.
UserOperationError,
}

#[derive(PartialEq, Debug)]
Expand Down Expand Up @@ -900,6 +904,7 @@ impl RedisError {
ErrorKind::RESP3NotSupported => "resp3 is not supported by server",
ErrorKind::ParseError => "parse error",
ErrorKind::NotAllSlotsCovered => "not all slots are covered",
ErrorKind::UserOperationError => "Wrong usage of management operation",
}
}

Expand Down Expand Up @@ -1095,6 +1100,7 @@ impl RedisError {
ErrorKind::NotAllSlotsCovered => RetryMethod::NoRetry,
ErrorKind::FatalReceiveError => RetryMethod::Reconnect,
ErrorKind::FatalSendError => RetryMethod::ReconnectAndRetry,
ErrorKind::UserOperationError => RetryMethod::NoRetry,
}
}
}
Expand Down
67 changes: 51 additions & 16 deletions glide-core/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,28 +479,63 @@ impl Client {

/// Update the password used to authenticate with the servers.
/// If None is passed, the password will be removed.
/// If `re_auth` is true, the new password will be used to re-authenticate with all of the nodes.
/// If `immediate_auth` is true, the password will be used to authenticate with the servers immediately using the `AUTH` command.
/// The default behavior is to update the password without authenticating immediately.
/// If the password is empty or None, and `immediate_auth` is true, the password will be updated and an error will be returned.
pub async fn update_connection_password(
&mut self,
password: Option<String>,
re_auth: bool,
immediate_auth: bool,
) -> RedisResult<Value> {
if re_auth {
let routing = RoutingInfo::MultiNode((
MultipleNodeRoutingInfo::AllNodes,
Some(ResponsePolicy::AllSucceeded),
));
let mut cmd = redis::cmd("AUTH");
cmd.arg(&password);
self.send_command(&cmd, Some(routing)).await?;
let timeout = self.request_timeout;
// The password update operation is wrapped in a timeout to prevent it from blocking indefinitely.
// If the operation times out, an error is returned.
// Since the password update operation is not a command that go through the regular command pipeline,
// it is not have the regular timeout handling, as such we need to handle it separately.
match tokio::time::timeout(timeout, async {
match self.internal_client {
ClientWrapper::Standalone(ref mut client) => {
client.update_connection_password(password.clone()).await
}
ClientWrapper::Cluster { ref mut client } => {
client.update_connection_password(password.clone()).await
}
}
})
.await
{
Ok(result) => {
if immediate_auth {
self.send_immediate_auth(password).await
} else {
result
}
}
Err(_elapsed) => Err(RedisError::from((
ErrorKind::IoError,
"Password update operation timed out, please check the connection",
))),
}
}

match self.internal_client {
ClientWrapper::Standalone(ref mut client) => {
client.update_connection_password(password).await
}
ClientWrapper::Cluster { ref mut client } => {
client.update_connection_password(password).await
async fn send_immediate_auth(&mut self, password: Option<String>) -> RedisResult<Value> {
match &password {
Some(pw) if pw.is_empty() => Err(RedisError::from((
ErrorKind::UserOperationError,
"Empty password provided for authentication",
))),
None => Err(RedisError::from((
ErrorKind::UserOperationError,
"No password provided for authentication",
))),
Some(password) => {
let routing = RoutingInfo::MultiNode((
MultipleNodeRoutingInfo::AllNodes,
Some(ResponsePolicy::AllSucceeded),
));
let mut cmd = redis::cmd("AUTH");
cmd.arg(password);
self.send_command(&cmd, Some(routing)).await
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion glide-core/src/protobuf/command_request.proto
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ message ClusterScan {

message UpdateConnectionPassword {
optional string password = 1;
bool re_auth = 2;
bool immediate_auth = 2;
}

message CommandRequest {
Expand Down
2 changes: 1 addition & 1 deletion glide-core/src/socket_listener.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ fn handle_request(request: CommandRequest, mut client: Client, writer: Rc<Writer
update_connection_password_command
.password
.map(|chars| chars.to_string()),
update_connection_password_command.re_auth,
update_connection_password_command.immediate_auth,
)
.await
.map_err(|err| err.into()),
Expand Down
2 changes: 1 addition & 1 deletion java/client/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ repositories {
}

dependencies {
implementation group: 'com.google.protobuf', name: 'protobuf-java', version: '4.27.1'
implementation group: 'com.google.protobuf', name: 'protobuf-java', version: '4.28.2'
implementation group: 'org.apache.commons', name: 'commons-lang3', version: '3.13.0'

implementation group: 'io.netty', name: 'netty-handler', version: '4.1.100.Final'
Expand Down
26 changes: 18 additions & 8 deletions java/client/src/main/java/glide/api/BaseClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,6 @@
import glide.api.models.commands.LInsertOptions.InsertPosition;
import glide.api.models.commands.LPosOptions;
import glide.api.models.commands.ListDirection;
import glide.api.models.commands.PasswordUpdateMode;
import glide.api.models.commands.RangeOptions;
import glide.api.models.commands.RangeOptions.LexRange;
import glide.api.models.commands.RangeOptions.RangeQuery;
Expand Down Expand Up @@ -787,10 +786,15 @@ protected Map<String, Object> handleLcsIdxResponse(Map<String, Object> response)
* that the internal reconnection mechanism can handle reconnection seamlessly, preventing the
* loss of in-flight commands.
*
* @param immediateAuth A <code>boolean</code> flag. If <code>true</code>, the client will
* authenticate immediately with the new password against all connections, Using <code>AUTH
* </code> command. <br>
* If password supplied is an empty string, the client will not perform auth and a warning
* will be returned. <br>
* The default is `false`.
* @apiNote This method updates the client's internal password configuration and does not perform
* password rotation on the server side.
* @param password A new password to set.
* @param mode Password update mode, see {@link PasswordUpdateMode}.
* @return <code>"OK"</code>.
* @example
* <pre>{@code
Expand All @@ -799,9 +803,9 @@ protected Map<String, Object> handleLcsIdxResponse(Map<String, Object> response)
* }</pre>
*/
public CompletableFuture<String> updateConnectionPassword(
@NonNull String password, @NonNull PasswordUpdateMode mode) {
@NonNull String password, boolean immediateAuth) {
return commandManager.submitPasswordUpdate(
Optional.of(password), mode, this::handleStringResponse);
Optional.of(password), immediateAuth, this::handleStringResponse);
}

/**
Expand All @@ -815,16 +819,22 @@ public CompletableFuture<String> updateConnectionPassword(
*
* @apiNote This method updates the client's internal password configuration and does not perform
* password rotation on the server side.
* @param mode Password update mode, see {@link PasswordUpdateMode}.
* @param immediateAuth A <code>boolean</code> flag. If <code>true</code>, the client will
* authenticate immediately with the new password against all connections, Using <code>AUTH
* </code> command. <br>
* If password supplied is an empty string, the client will not perform auth and a warning
* will be returned. <br>
* The default is `false`.
* @return <code>"OK"</code>.
* @example
* <pre>{@code
* String response = client.resetConnectionPassword(RE_AUTHENTICATE).get();
* String response = client.resetConnectionPassword(true).get();
* assert response.equals("OK");
* }</pre>
*/
public CompletableFuture<String> updateConnectionPassword(@NonNull PasswordUpdateMode mode) {
return commandManager.submitPasswordUpdate(Optional.empty(), mode, this::handleStringResponse);
public CompletableFuture<String> updateConnectionPassword(boolean immediateAuth) {
return commandManager.submitPasswordUpdate(
Optional.empty(), immediateAuth, this::handleStringResponse);
}

@Override
Expand Down
7 changes: 3 additions & 4 deletions java/client/src/main/java/glide/managers/CommandManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import glide.api.models.GlideString;
import glide.api.models.Script;
import glide.api.models.Transaction;
import glide.api.models.commands.PasswordUpdateMode;
import glide.api.models.commands.scan.ClusterScanCursor;
import glide.api.models.commands.scan.ScanOptions;
import glide.api.models.configuration.RequestRoutingConfiguration.ByAddressRoute;
Expand Down Expand Up @@ -224,16 +223,16 @@ public <T> CompletableFuture<T> submitClusterScan(
* Submit a password update request to GLIDE core.
*
* @param password A new password to set or empty value to remove the password.
* @param mode Password update mode.
* @param immediateAuth immediately perform auth.
* @param responseHandler A response handler.
* @return A request promise.
* @param <T> Type of the response.
*/
public <T> CompletableFuture<T> submitPasswordUpdate(
Optional<String> password,
PasswordUpdateMode mode,
boolean immediateAuth,
GlideExceptionCheckedFunction<Response, T> responseHandler) {
var builder = UpdateConnectionPassword.newBuilder().setReAuth(mode.getValue());
var builder = UpdateConnectionPassword.newBuilder().setImmediateAuth(immediateAuth);
password.ifPresent(builder::setPassword);

var command = CommandRequest.newBuilder().setUpdateConnectionPassword(builder.build());
Expand Down
3 changes: 2 additions & 1 deletion java/integTest/src/test/java/glide/SharedClientTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@
import net.bytebuddy.utility.RandomString;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

// @Timeout(35) // seconds
@Timeout(35) // seconds
public class SharedClientTests {

private static GlideClient standaloneClient = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
import static org.junit.jupiter.api.Assumptions.assumeTrue;

import glide.api.GlideClusterClient;
import glide.api.models.commands.PasswordUpdateMode;
import glide.api.models.configuration.ServerCredentials;
import glide.api.models.exceptions.ClosingException;
import glide.api.models.exceptions.ConnectionException;
import glide.api.models.exceptions.RequestException;
import java.util.Map;
import java.util.UUID;
Expand All @@ -24,7 +24,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.ValueSource;

@Timeout(10) // seconds
public class ClusterClientTests {
Expand Down Expand Up @@ -168,8 +168,8 @@ public void closed_client_throws_ExecutionException_with_ClosingException_as_cau

@SneakyThrows
@ParameterizedTest
@EnumSource(PasswordUpdateMode.class)
public void password_update(PasswordUpdateMode mode) {
@ValueSource(booleans = {true, false})
public void password_update(boolean immediateAuth) {
GlideClusterClient client =
GlideClusterClient.createClient(commonClusterClientConfig().build()).get();

Expand All @@ -185,16 +185,15 @@ public void password_update(PasswordUpdateMode mode) {

// set the password and forcefully drop connection for the second client
assertEquals("OK", client.configSet(Map.of("requirepass", pwd)).get());
if (mode == PasswordUpdateMode.RE_AUTHENTICATE)
testClient.customCommand(new String[] {"RESET"}, ALL_NODES).get();
if (immediateAuth) testClient.customCommand(new String[] {"RESET"}, ALL_NODES).get();
else client.customCommand(new String[] {"CLIENT", "KILL", "TYPE", "NORMAL"}, ALL_NODES).get();

// client should reconnect, but will receive NOAUTH error
var exception = assertThrows(ExecutionException.class, () -> testClient.get(key).get());
assertInstanceOf(RequestException.class, exception.getCause());
assertInstanceOf(ConnectionException.class, exception.getCause());
assertTrue(exception.getMessage().toLowerCase().contains("noauth"));

assertEquals("OK", testClient.updateConnectionPassword(pwd, mode).get());
assertEquals("OK", testClient.updateConnectionPassword(pwd, immediateAuth).get());

// after setting new password we should be able to work with the server
assertEquals("meow meow", testClient.get(key).get());
Expand Down
Loading

0 comments on commit e2af4fa

Please sign in to comment.