Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add redis sentinel password property #21353

Closed
mcanessa opened this issue May 7, 2020 · 7 comments
Closed

Add redis sentinel password property #21353

mcanessa opened this issue May 7, 2020 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@mcanessa
Copy link

mcanessa commented May 7, 2020

Spring boot version: 2.2.6.RELEASE
I get the RedisCommandExecutionException: NOAUTH Authentication required. After setting the configuration like this:

spring:
  cache:
    cache-names: mycache
    type: redis
  redis:
    password: mypassword
    sentinel:
      master: mymaster
      password: mypassword
      nodes: myredis01:26379,myredis02:26379,myredis03:2637{code}

I tried setting only spring.redis.password, only spring.redis.sentinel.password and both at the same time and it's not working.

Digging through the code I found that the sentinel password is never set on the configuration:

1- org.springframework.boot.autoconfigure.data.redis.RedisProperties.Sentinel doesn't have the 'password' property.
2- org.springframework.boot.autoconfigure.data.redis.RedisConnectionConfiguration#getSentinelConfig() never sets the existing property 'sentinelPassword' in RedisSentinelConfiguration.
3 - Resulting in the org.springframework.data.redis.connection.lettuce.LettuceConverters setting always a null password which is the default when not set in the sentinelConfigurationToRedisURI method.

So the problem comes when the sentinel is also password protected. I mistakenly filed the bug on the Spring redis data and they sent me here.

regards,

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2020
@mbhave
Copy link
Contributor

mbhave commented May 8, 2020

This is the Spring Data issue for reference.

Based on some comments from #18942, we didn't surface a property for spring.redis.sentinel.password because Sentinel authentication was only supported by Lettuce and not by Jedis.

@mp911de What are your thoughts on surfacing a property that applies to one driver and not the other?

@mcanessa
Copy link
Author

mcanessa commented May 8, 2020

Hello, i found this Jedis merge.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label May 8, 2020
@mp911de
Copy link
Member

mp911de commented May 11, 2020

Back then, Jedis didn't provide an option to configure the Sentinel password. In the meantime, Jedis provided the required functionality so I filed DATAREDIS-1145 to support Sentinel passwords with Jedis in Spring Data Redis.

That being said, it makes sense to surface the password property in the spring.redis.sentinel namespace.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 11, 2020
@mbhave mbhave added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels May 11, 2020
@mbhave mbhave added this to the 2.3.x milestone May 11, 2020
@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label May 12, 2020
@philwebb philwebb added status: blocked An issue that's blocked on an external project change and removed status: blocked An issue that's blocked on an external project change for: team-attention An issue we'd like other members of the team to review labels May 13, 2020
@philwebb philwebb changed the title Redis auto configuration fails to set sentinel password Add Redis sentinel password property May 13, 2020
@philwebb philwebb changed the title Add Redis sentinel password property Add redis sentinel password property May 13, 2020
@philwebb

This comment has been minimized.

@philwebb philwebb assigned philwebb and mbhave and unassigned philwebb May 13, 2020
@mp911de
Copy link
Member

mp911de commented May 13, 2020

We've tried a similar approach in Spring Data Redis and learned that we should not couple the Redis password to the Sentinel password. The Sentinel password was added in a later Redis version so earlier Redis setups will fail when the Sentinel password is set to the Redis password.

@philwebb
Copy link
Member

@mp911de Interesting. We currently have code that sets the sentinel password to the redis one and we were worried about back compatibility. After discussing it some more, we'll just add a new property and make the user set it if they want to use the same value.

@mbhave mbhave added status: noteworthy A noteworthy issue to call out in the release notes and removed status: noteworthy A noteworthy issue to call out in the release notes labels May 13, 2020
@mbhave
Copy link
Contributor

mbhave commented May 13, 2020

Looking at the code further, I don't think there's a back compatibility issue as we've never set the sentinelPassword. Even in the RedisSentinelConfiguration, it was the dataNodePassword that we configured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants