From 99cfbdb2da1dfe6db088be98dd9014ee381217d6 Mon Sep 17 00:00:00 2001 From: Igor Malinovskiy Date: Tue, 21 May 2024 11:59:16 +0200 Subject: [PATCH] Address review suggestions - Add support for endpoints without username - Improve endpoints naming in AutomaticFailoverTest - Remove more hardcoded settings from Migrate tests - Fix comments in SSLACL tests --- .../redis/clients/jedis/EndpointConfig.java | 17 +++++--- .../clients/jedis/MigratePipeliningTest.java | 36 ++++++++++------ .../clients/jedis/SSLACLJedisClusterTest.java | 1 + .../redis/clients/jedis/SSLACLJedisTest.java | 3 +- .../jedis/commands/jedis/MigrateTest.java | 43 +++++++++++-------- .../jedis/misc/AutomaticFailoverTest.java | 21 +++++---- src/test/resources/endpoints.json | 1 - 7 files changed, 72 insertions(+), 50 deletions(-) diff --git a/src/test/java/redis/clients/jedis/EndpointConfig.java b/src/test/java/redis/clients/jedis/EndpointConfig.java index 0f72500316..5cb322224d 100644 --- a/src/test/java/redis/clients/jedis/EndpointConfig.java +++ b/src/test/java/redis/clients/jedis/EndpointConfig.java @@ -34,7 +34,7 @@ public String getPassword() { } public String getUsername() { - return username; + return username == null? "default" : username; } public String getHost() { @@ -68,8 +68,8 @@ public EndpointURIBuilder() { } public EndpointURIBuilder defaultCredentials() { - this.username = EndpointConfig.this.username; - this.password = EndpointConfig.this.password; + this.username = EndpointConfig.this.username == null ? "" : getUsername(); + this.password = EndpointConfig.this.getPassword(); return this; } @@ -91,7 +91,7 @@ public EndpointURIBuilder credentials(String u, String p) { public URI build() { String userInfo = !(this.username.isEmpty() && this.password.isEmpty()) ? - this.username + ":" + this.password + "@" : + this.username + ':' + this.password + '@' : ""; return URI.create( getURISchema(this.tls) + userInfo + getHost() + ":" + getPort() + this.path); @@ -103,7 +103,14 @@ public EndpointURIBuilder getURIBuilder() { } public DefaultJedisClientConfig.Builder getClientConfigBuilder() { - return DefaultJedisClientConfig.builder().user(username).password(password).ssl(tls); + DefaultJedisClientConfig.Builder builder = DefaultJedisClientConfig.builder() + .password(password).ssl(tls); + + if (username != null) { + return builder.user(username); + } + + return builder; } protected String getURISchema(boolean tls) { diff --git a/src/test/java/redis/clients/jedis/MigratePipeliningTest.java b/src/test/java/redis/clients/jedis/MigratePipeliningTest.java index 35f96611ed..8cac62c1f0 100644 --- a/src/test/java/redis/clients/jedis/MigratePipeliningTest.java +++ b/src/test/java/redis/clients/jedis/MigratePipeliningTest.java @@ -33,9 +33,17 @@ public class MigratePipeliningTest extends JedisCommandsTestBase { private static final byte[] bfoo3 = { 0x07, 0x08, 0x03 }; private static final byte[] bbar3 = { 0x09, 0x00, 0x03 }; - private static final String host = endpoint.getHost(); - private static final int port = 6386; - private static final int portAuth = endpoint.getPort() + 1; + private static final EndpointConfig endpoint = HostAndPorts.getRedisEndpoint("standalone0-acl"); + + private static final EndpointConfig destEndpoint = HostAndPorts.getRedisEndpoint( + "standalone7-with-lfu-policy"); + + private static final EndpointConfig destEndpointWithAuth = HostAndPorts.getRedisEndpoint( + "standalone1"); + + private static final String host = destEndpoint.getHost(); + private static final int port = destEndpoint.getPort(); + private static final int portAuth = destEndpointWithAuth.getPort(); private static final int db = 2; private static final int dbAuth = 3; private static final int timeout = Protocol.DEFAULT_TIMEOUT; @@ -56,8 +64,8 @@ public void setUp() throws Exception { dest.flushAll(); dest.select(db); - destAuth = new Jedis(host, portAuth, 500); - destAuth.auth(endpoint.getPassword()); + destAuth = new Jedis(destEndpointWithAuth.getHostAndPort(), + destEndpointWithAuth.getClientConfigBuilder().build()); destAuth.flushAll(); destAuth.select(dbAuth); } @@ -258,7 +266,8 @@ public void migrateAuth() { Pipeline p = jedis.pipelined(); p.set("foo", "bar"); - p.migrate(host, portAuth, dbAuth, timeout, new MigrateParams().auth(endpoint.getPassword()), "foo"); + p.migrate(host, portAuth, dbAuth, timeout, + new MigrateParams().auth(destEndpointWithAuth.getPassword()), "foo"); p.get("foo"); assertThat(p.syncAndReturnAll(), @@ -274,7 +283,8 @@ public void migrateAuthBinary() { Pipeline p = jedis.pipelined(); p.set(bfoo, bbar); - p.migrate(host, portAuth, dbAuth, timeout, new MigrateParams().auth(endpoint.getPassword()), bfoo); + p.migrate(host, portAuth, dbAuth, timeout, + new MigrateParams().auth(destEndpointWithAuth.getPassword()), bfoo); p.get(bfoo); assertThat(p.syncAndReturnAll(), @@ -287,13 +297,12 @@ public void migrateAuthBinary() { public void migrateAuth2() { assertNull(jedis.get("foo")); - - Pipeline p = destAuth.pipelined(); p.set("foo", "bar"); - p.migrate(host, endpoint.getPort(), 0, timeout, - new MigrateParams().auth2("acljedis", "fizzbuzz"), "foo"); + p.migrate(endpoint.getHost(), endpoint.getPort(), 0, timeout, + new MigrateParams().auth2(endpoint.getUsername(), endpoint.getPassword()), + "foo"); p.get("foo"); assertThat(p.syncAndReturnAll(), @@ -309,8 +318,9 @@ public void migrateAuth2Binary() { Pipeline p = dest.pipelined(); p.set(bfoo, bbar); - p.migrate(host, endpoint.getPort(), 0, timeout, - new MigrateParams().auth2("acljedis", "fizzbuzz"), bfoo); + p.migrate(endpoint.getHost(), endpoint.getPort(), 0, timeout, + new MigrateParams().auth2(endpoint.getUsername(), endpoint.getPassword()), + bfoo); p.get(bfoo); assertThat(p.syncAndReturnAll(), diff --git a/src/test/java/redis/clients/jedis/SSLACLJedisClusterTest.java b/src/test/java/redis/clients/jedis/SSLACLJedisClusterTest.java index 3a3ddf9700..16eb63c0e0 100644 --- a/src/test/java/redis/clients/jedis/SSLACLJedisClusterTest.java +++ b/src/test/java/redis/clients/jedis/SSLACLJedisClusterTest.java @@ -43,6 +43,7 @@ public class SSLACLJedisClusterTest extends JedisClusterTestBase { @BeforeClass public static void prepare() { + // We need to set up certificates first before connecting to the endpoint with enabled TLS SSLJedisTest.setupTrustStore(); // TODO(imalinovskyi): Remove hardcoded connection settings diff --git a/src/test/java/redis/clients/jedis/SSLACLJedisTest.java b/src/test/java/redis/clients/jedis/SSLACLJedisTest.java index 1c9f58cc2f..8151729e6e 100644 --- a/src/test/java/redis/clients/jedis/SSLACLJedisTest.java +++ b/src/test/java/redis/clients/jedis/SSLACLJedisTest.java @@ -28,8 +28,9 @@ public class SSLACLJedisTest { @BeforeClass public static void prepare() { - // Use to check if the ACL test should be ran. ACL are available only in 6.0 and later + // We need to set up certificates first before connecting to the endpoint with enabled TLS SSLJedisTest.setupTrustStore(); + // Use to check if the ACL test should be ran. ACL are available only in 6.0 and later org.junit.Assume.assumeTrue("Not running ACL test on this version of Redis", RedisVersionUtil.checkRedisMajorVersionNumber(6, endpoint)); } diff --git a/src/test/java/redis/clients/jedis/commands/jedis/MigrateTest.java b/src/test/java/redis/clients/jedis/commands/jedis/MigrateTest.java index ab6a1a89f3..9d44302e37 100644 --- a/src/test/java/redis/clients/jedis/commands/jedis/MigrateTest.java +++ b/src/test/java/redis/clients/jedis/commands/jedis/MigrateTest.java @@ -12,9 +12,7 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; -import redis.clients.jedis.Jedis; -import redis.clients.jedis.Protocol; -import redis.clients.jedis.RedisProtocol; +import redis.clients.jedis.*; import redis.clients.jedis.exceptions.JedisDataException; import redis.clients.jedis.params.MigrateParams; @@ -32,9 +30,18 @@ public class MigrateTest extends JedisCommandsTestBase { private Jedis dest; private Jedis destAuth; - private static final String host = endpoint.getHost(); - private static final int port = 6386; - private static final int portAuth = endpoint.getPort() + 1; + + private static final EndpointConfig endpoint = HostAndPorts.getRedisEndpoint("standalone0-acl"); + + private static final EndpointConfig destEndpoint = HostAndPorts.getRedisEndpoint( + "standalone7-with-lfu-policy"); + + private static final EndpointConfig destEndpointWithAuth = HostAndPorts.getRedisEndpoint( + "standalone1"); + + private static final String host = destEndpoint.getHost(); + private static final int port = destEndpoint.getPort(); + private static final int portAuth = destEndpointWithAuth.getPort(); private static final int db = 2; private static final int dbAuth = 3; private static final int timeout = Protocol.DEFAULT_TIMEOUT; @@ -52,8 +59,8 @@ public void setUp() throws Exception { dest.flushAll(); dest.select(db); - destAuth = new Jedis(host, portAuth, 500); - destAuth.auth(endpoint.getPassword()); + destAuth = new Jedis(destEndpointWithAuth.getHostAndPort(), + destEndpointWithAuth.getClientConfigBuilder().build()); destAuth.flushAll(); destAuth.select(dbAuth); } @@ -150,14 +157,14 @@ public void migrateCopyReplace() { @Test public void migrateAuth() { jedis.set("foo", "bar"); - assertEquals("OK", - jedis.migrate(host, portAuth, dbAuth, timeout, new MigrateParams().auth(endpoint.getPassword()), "foo")); + assertEquals("OK", jedis.migrate(host, portAuth, dbAuth, timeout, + new MigrateParams().auth(destEndpointWithAuth.getPassword()), "foo")); assertEquals("bar", destAuth.get("foo")); assertNull(jedis.get("foo")); jedis.set(bfoo, bbar); - assertEquals("OK", - jedis.migrate(host, portAuth, dbAuth, timeout, new MigrateParams().auth(endpoint.getPassword()), bfoo)); + assertEquals("OK", jedis.migrate(host, portAuth, dbAuth, timeout, + new MigrateParams().auth(destEndpointWithAuth.getPassword()), bfoo)); assertArrayEquals(bbar, destAuth.get(bfoo)); assertNull(jedis.get(bfoo)); } @@ -166,14 +173,14 @@ public void migrateAuth() { public void migrateAuth2() { destAuth.set("foo", "bar"); assertEquals("OK", destAuth.migrate(host, endpoint.getPort(), 0, timeout, - new MigrateParams().auth2("acljedis", "fizzbuzz"), "foo")); + new MigrateParams().auth2(endpoint.getUsername(), endpoint.getPassword()), "foo")); assertEquals("bar", jedis.get("foo")); assertNull(destAuth.get("foo")); // binary dest.set(bfoo1, bbar1); assertEquals("OK", dest.migrate(host, endpoint.getPort(), 0, timeout, - new MigrateParams().auth2("acljedis", "fizzbuzz"), bfoo1)); + new MigrateParams().auth2(endpoint.getUsername(), endpoint.getPassword()), bfoo1)); assertArrayEquals(bbar1, jedis.get(bfoo1)); assertNull(dest.get(bfoo1)); } @@ -182,10 +189,8 @@ public void migrateAuth2() { public void migrateCopyReplaceAuth() { jedis.set("foo", "bar1"); destAuth.set("foo", "bar2"); - assertEquals( - "OK", - jedis.migrate(host, portAuth, dbAuth, timeout, - new MigrateParams().copy().replace().auth(endpoint.getPassword()), "foo")); + assertEquals("OK", jedis.migrate(host, portAuth, dbAuth, timeout, + new MigrateParams().copy().replace().auth(destEndpointWithAuth.getPassword()), "foo")); assertEquals("bar1", destAuth.get("foo")); assertEquals("bar1", jedis.get("foo")); @@ -194,7 +199,7 @@ public void migrateCopyReplaceAuth() { assertEquals( "OK", jedis.migrate(host, portAuth, dbAuth, timeout, - new MigrateParams().copy().replace().auth(endpoint.getPassword()), bfoo)); + new MigrateParams().copy().replace().auth(destEndpointWithAuth.getPassword()), bfoo)); assertArrayEquals(bbar1, destAuth.get(bfoo)); assertArrayEquals(bbar1, jedis.get(bfoo)); } diff --git a/src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java b/src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java index 0df7457f80..e12c0f65f9 100644 --- a/src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java +++ b/src/test/java/redis/clients/jedis/misc/AutomaticFailoverTest.java @@ -26,10 +26,9 @@ public class AutomaticFailoverTest { private static final Logger log = LoggerFactory.getLogger(AutomaticFailoverTest.class); - // TODO(imalinovskyi): Figure out how we deploy this endpoint - private final HostAndPort hostPort_0 = new HostAndPort(HostAndPorts.getRedisEndpoint("standalone0").getHost(), 6378); - private final EndpointConfig endpointStandalone0 = HostAndPorts.getRedisEndpoint("standalone0"); - private final EndpointConfig endpointStandalone7 = HostAndPorts.getRedisEndpoint("standalone7-with-lfu-policy"); + private final HostAndPort hostPortWithFailure = new HostAndPort(HostAndPorts.getRedisEndpoint("standalone0").getHost(), 6378); + private final EndpointConfig workingEndpointWithPriority1 = HostAndPorts.getRedisEndpoint("standalone0"); + private final EndpointConfig workingEndpointWithPriority2 = HostAndPorts.getRedisEndpoint("standalone7-with-lfu-policy"); private final JedisClientConfig clientConfig = DefaultJedisClientConfig.builder().build(); @@ -44,8 +43,8 @@ private List getClusterConfigs( @Before public void setUp() { - jedis2 = new Jedis(endpointStandalone7.getHostAndPort(), - endpointStandalone7.getClientConfigBuilder().build()); + jedis2 = new Jedis(workingEndpointWithPriority2.getHostAndPort(), + workingEndpointWithPriority2.getClientConfigBuilder().build()); jedis2.flushAll(); } @@ -57,7 +56,7 @@ public void cleanUp() { @Test public void pipelineWithSwitch() { MultiClusterPooledConnectionProvider provider = new MultiClusterPooledConnectionProvider( - new MultiClusterClientConfig.Builder(getClusterConfigs(clientConfig, hostPort_0, endpointStandalone7.getHostAndPort())).build()); + new MultiClusterClientConfig.Builder(getClusterConfigs(clientConfig, hostPortWithFailure, workingEndpointWithPriority2.getHostAndPort())).build()); try (UnifiedJedis client = new UnifiedJedis(provider)) { AbstractPipeline pipe = client.pipelined(); @@ -74,7 +73,7 @@ public void pipelineWithSwitch() { @Test public void transactionWithSwitch() { MultiClusterPooledConnectionProvider provider = new MultiClusterPooledConnectionProvider( - new MultiClusterClientConfig.Builder(getClusterConfigs(clientConfig, hostPort_0, endpointStandalone7.getHostAndPort())).build()); + new MultiClusterClientConfig.Builder(getClusterConfigs(clientConfig, hostPortWithFailure, workingEndpointWithPriority2.getHostAndPort())).build()); try (UnifiedJedis client = new UnifiedJedis(provider)) { AbstractTransaction tx = client.multi(); @@ -94,7 +93,7 @@ public void commandFailover() { int slidingWindowSize = 10; MultiClusterClientConfig.Builder builder = new MultiClusterClientConfig.Builder( - getClusterConfigs(clientConfig, hostPort_0, endpointStandalone7.getHostAndPort())) + getClusterConfigs(clientConfig, hostPortWithFailure, workingEndpointWithPriority2.getHostAndPort())) .circuitBreakerSlidingWindowMinCalls(slidingWindowMinCalls) .circuitBreakerSlidingWindowSize(slidingWindowSize); @@ -132,7 +131,7 @@ public void pipelineFailover() { int slidingWindowSize = 10; MultiClusterClientConfig.Builder builder = new MultiClusterClientConfig.Builder( - getClusterConfigs(clientConfig, hostPort_0, endpointStandalone7.getHostAndPort())) + getClusterConfigs(clientConfig, hostPortWithFailure, workingEndpointWithPriority2.getHostAndPort())) .circuitBreakerSlidingWindowMinCalls(slidingWindowMinCalls) .circuitBreakerSlidingWindowSize(slidingWindowSize) .fallbackExceptionList(Arrays.asList(JedisConnectionException.class)); @@ -165,7 +164,7 @@ public void failoverFromAuthError() { int slidingWindowSize = 10; MultiClusterClientConfig.Builder builder = new MultiClusterClientConfig.Builder( - getClusterConfigs(clientConfig, endpointStandalone0.getHostAndPort(), endpointStandalone7.getHostAndPort())) + getClusterConfigs(clientConfig, workingEndpointWithPriority1.getHostAndPort(), workingEndpointWithPriority2.getHostAndPort())) .circuitBreakerSlidingWindowMinCalls(slidingWindowMinCalls) .circuitBreakerSlidingWindowSize(slidingWindowSize) .fallbackExceptionList(Arrays.asList(JedisAccessControlException.class)); diff --git a/src/test/resources/endpoints.json b/src/test/resources/endpoints.json index eedc38f137..c1d905bfb8 100644 --- a/src/test/resources/endpoints.json +++ b/src/test/resources/endpoints.json @@ -1,6 +1,5 @@ { "standalone0": { - "username": "default", "password": "foobared", "tls": false, "endpoints": [