From c6802ba2472a9476bb5a91f190ad884e0104709f Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 12 Nov 2019 15:21:24 +0100 Subject: [PATCH] DATAREDIS-1060 - Polishing. Remove data node password reuse for sentinel as we favor explicit configuration over implicit. Use RedisURI builder and not sentinel URI builder to configure the SentinelURI correctly. Associate masterId with outermost RedisURI. Original pull request: #495. --- .../redis/connection/RedisConfiguration.java | 43 +++++-------------- .../RedisSentinelConfiguration.java | 33 ++------------ .../connection/lettuce/LettuceConverters.java | 12 +++--- .../RedisSentinelConfigurationUnitTests.java | 38 +--------------- .../LettuceConnectionFactoryUnitTests.java | 24 ----------- 5 files changed, 21 insertions(+), 129 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java index e04f118ab..90a8a2712 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisConfiguration.java @@ -339,20 +339,20 @@ public interface RedisConfiguration { Set getSentinels(); /** - * Get the {@link RedisPassword} used when authenticating with a Redis Server. - * + * Get the {@link RedisPassword} used when authenticating with a Redis Server.. + * * @return never {@literal null}. + * @since 2.2.2 */ default RedisPassword getDataNodePassword() { return getPassword(); } /** - * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given {@link String} + * Create and set a {@link RedisPassword} to be used when authenticating with Redis Sentinel from the given + * {@link String}. * * @param password can be {@literal null}. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ default void setSentinelPassword(@Nullable String password) { @@ -360,12 +360,10 @@ public interface RedisConfiguration { } /** - * Create and set a {@link RedisPassword} to be used when authenticating with Sentinel from the given + * Create and set a {@link RedisPassword} to be used when authenticating with Redis Sentinel from the given * {@link Character} sequence. * * @param password can be {@literal null}. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ default void setSentinelPassword(@Nullable char[] password) { @@ -373,43 +371,22 @@ public interface RedisConfiguration { } /** - * Set a {@link RedisPassword} to be used when authenticating with Sentinel. + * Set a {@link RedisPassword} to be used when authenticating with Redis Sentinel. * * @param password must not be {@literal null} use {@link RedisPassword#none()} instead. - * @throws IllegalStateException if the {@link #useDataNodeAuthenticationForSentinel(boolean) Data Node Password} - * should be used for authenticating with Redis Sentinel. * @since 2.2.2 */ void setSentinelPassword(RedisPassword password); /** - * Get the {@link RedisPassword} to use when connecting to a Redis Sentinel.
- * This can be the password explicitly set via {@link #setSentinelPassword(RedisPassword)}, or the - * {@link #getDataNodePassword() Data Node password} if it should be also used for - * {@link #getUseDataNodeAuthenticationForSentinel() sentinel}, or {@link RedisPassword#none()} if no password has + * Returns the {@link RedisPassword} to use when connecting to a Redis Sentinel.
+ * Can be set via {@link #setSentinelPassword(RedisPassword)} or {@link RedisPassword#none()} if no password has * been set. * - * @return the {@link RedisPassword} for authenticating with Sentinel. + * @return the {@link RedisPassword} for authenticating with Redis Sentinel. * @since 2.2.2 */ RedisPassword getSentinelPassword(); - - /** - * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. - * - * @param useDataNodeAuthenticationForSentinel - * @throws IllegalStateException if a {@link #getSentinelPassword() Sentinel Password} is already set. - * @since 2.2.2 - */ - void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel); - - /** - * Use the {@link #getDataNodePassword() RedisPassword} also for authentication with Redis Sentinel. - * - * @return - * @since 2.2.2 - */ - boolean getUseDataNodeAuthenticationForSentinel(); } /** diff --git a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java index e29b6f1c9..a28b4d212 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisSentinelConfiguration.java @@ -52,7 +52,6 @@ public class RedisSentinelConfiguration implements RedisConfiguration, SentinelC private RedisPassword dataNodePassword = RedisPassword.none(); private RedisPassword sentinelPassword = RedisPassword.none(); - private boolean useDataNodePasswordForSentinel = false; /** * Creates new {@link RedisSentinelConfiguration}. @@ -253,47 +252,21 @@ public class RedisSentinelConfiguration implements RedisConfiguration, SentinelC /* * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword(org.springframework.data.redis.connection.RedisPassword) + * @see org.springframework.data.redis.connection.RedisConfiguration.SentinelConfiguration#setSentinelPassword(org.springframework.data.redis.connection.RedisPassword) */ public void setSentinelPassword(RedisPassword sentinelPassword) { - Assert.state(!useDataNodePasswordForSentinel, - "Configuration uses Redis Data Node password for authenticating with Sentinel. Please set 'RedisSentinelConfiguration.useDataNodeAuthenticationForSentinel(false)' before using this option."); Assert.notNull(sentinelPassword, "SentinelPassword must not be null!"); this.sentinelPassword = sentinelPassword; } /* * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#setSentinelPassword() + * @see org.springframework.data.redis.connection.RedisConfiguration.SentinelConfiguration#setSentinelPassword() */ @Override public RedisPassword getSentinelPassword() { - return getUseDataNodeAuthenticationForSentinel() ? this.dataNodePassword : sentinelPassword; - } - - /* - * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#useDataNodeAuthenticationForSentinel(boolean) - */ - @Override - public void useDataNodeAuthenticationForSentinel(boolean useDataNodeAuthenticationForSentinel) { - - if (useDataNodeAuthenticationForSentinel) { - Assert.state(!this.sentinelPassword.isPresent(), - "Configuration already defines a password for authenticating with Sentinel. Please use 'RedisSentinelConfiguration.setSentinelPassword(RedisPassword.none())' remove the password."); - } - - this.useDataNodePasswordForSentinel = useDataNodeAuthenticationForSentinel; - } - - /* - * (non-Javadoc) - * @see org.springframework.data.redis.connection.RedisConfiguration.WithPassword#getUseDataNodeAuthenticationForSentinel() - */ - @Override - public boolean getUseDataNodeAuthenticationForSentinel() { - return this.useDataNodePasswordForSentinel; + return sentinelPassword; } private RedisNode readHostAndPortFromString(String hostAndPort) { diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java index b7b8c22cc..7ff1a37b2 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConverters.java @@ -640,16 +640,16 @@ abstract public class LettuceConverters extends Converters { Assert.notNull(sentinelConfiguration, "RedisSentinelConfiguration is required"); Set sentinels = sentinelConfiguration.getSentinels(); + RedisPassword sentinelPassword = sentinelConfiguration.getSentinelPassword(); RedisURI.Builder builder = RedisURI.builder(); for (RedisNode sentinel : sentinels) { - RedisURI.Builder uri = RedisURI.Builder.sentinel(sentinel.getHost(), sentinel.getPort(), - sentinelConfiguration.getMaster().getName()); + RedisURI.Builder sentinelBuilder = RedisURI.Builder.redis(sentinel.getHost(), sentinel.getPort()); - if (sentinelConfiguration.getSentinelPassword().isPresent()) { - uri.withPassword(sentinelConfiguration.getSentinelPassword().get()); + if (sentinelPassword.isPresent()) { + sentinelBuilder.withPassword(sentinelPassword.get()); } - builder.withSentinel(uri.build()); + builder.withSentinel(sentinelBuilder.build()); } RedisPassword password = sentinelConfiguration.getPassword(); @@ -657,6 +657,8 @@ abstract public class LettuceConverters extends Converters { builder.withPassword(password.get()); } + builder.withSentinelMasterId(sentinelConfiguration.getMaster().getName()); + return builder.build(); } diff --git a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java index db2848bab..35100ea25 100644 --- a/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/RedisSentinelConfigurationUnitTests.java @@ -120,41 +120,6 @@ public class RedisSentinelConfigurationUnitTests { new RedisNode("localhost", 789)); } - @Test // DATAREDIS-1060 - public void throwsExceptionOnSentinelPasswordAlreadySetWhenTryingToReuseDataNodePassword() { - - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setSentinelPassword(RedisPassword.of("so-secret-you'll-never-guess-123")); - - assertThatExceptionOfType(IllegalStateException.class) - .isThrownBy(() -> configuration.useDataNodeAuthenticationForSentinel(true)); - } - - @Test // DATAREDIS-1060 - public void throwsExceptionOnSettingSentinelPasswordWhenAlreadyReusingDataNodePassword() { - - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setPassword(RedisPassword.of("qwerty")); - configuration.useDataNodeAuthenticationForSentinel(true); - - assertThatExceptionOfType(IllegalStateException.class) - .isThrownBy(() -> configuration.setSentinelPassword(RedisPassword.of("who-needs-security-anyway"))); - } - - @Test // DATAREDIS-1060 - public void settingSentinelPasswordReturnsDataNodePasswordIfUseDataNodeAuthIsTrue() { - - RedisPassword password = RedisPassword.of("monkey-dragon->yeah-getting-better-combining-trivial-ones"); - RedisSentinelConfiguration configuration = new RedisSentinelConfiguration("myMaster", - Collections.singleton(HOST_AND_PORT_1)); - configuration.setPassword(password); - configuration.useDataNodeAuthenticationForSentinel(true); - - assertThat(configuration.getSentinelPassword()).isEqualTo(password); - } - @Test // DATAREDIS-1060 public void dataNodePasswordDoesNotAffectSentinelPassword() { @@ -177,7 +142,6 @@ public class RedisSentinelConfigurationUnitTests { RedisSentinelConfiguration config = new RedisSentinelConfiguration(propertySource); assertThat(config.getSentinelPassword()).isEqualTo(RedisPassword.of("computer-says-no")); - assertThat(config.getSentinels().size()).isEqualTo(1); - assertThat(config.getSentinels()).contains(new RedisNode("127.0.0.1", 123)); + assertThat(config.getSentinels()).hasSize(1).contains(new RedisNode("127.0.0.1", 123)); } } diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java index 6e25b8ce0..04e7ef0ca 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java @@ -224,30 +224,6 @@ public class LettuceConnectionFactoryUnitTests { } } - @Test // DATAREDIS-1060 - public void redisPasswordShouldBeSetOnSentinelClientIfItShouldBeReused() { - - RedisSentinelConfiguration config = new RedisSentinelConfiguration("mymaster", Collections.singleton("host:1234")); - config.useDataNodeAuthenticationForSentinel(true); - - LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(config); - connectionFactory.setClientResources(getSharedClientResources()); - connectionFactory.setPassword("o_O"); - connectionFactory.afterPropertiesSet(); - ConnectionFactoryTracker.add(connectionFactory); - - AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client"); - assertThat(client).isInstanceOf(RedisClient.class); - - RedisURI redisUri = (RedisURI) getField(client, "redisURI"); - - assertThat(redisUri.getPassword()).isEqualTo(connectionFactory.getPassword().toCharArray()); - - for (RedisURI sentinel : redisUri.getSentinels()) { - assertThat(sentinel.getPassword()).isEqualTo("o_O".toCharArray()); - } - } - @Test // DATAREDIS-1060 public void sentinelPasswordShouldNotLeakIntoDataNodeClient() {