feat(redis): support redis sentinel configuration#41568
feat(redis): support redis sentinel configuration#41568akantcheff wants to merge 4 commits intoappsmithorg:releasefrom
Conversation
Add `redis-sentinel://` scheme support in both `reactiveRedisConnectionFactory()` and `redisClient()` beans. Extract credentials parsing into a shared `extractCredentials()` method and add a `fillAuthentication()` overload for `RedisURI.Builder`. Sentinel master name is configured via `appsmith.redis.sentinel.master` property.
Add `log.info()` statements to each scheme branch (`redis`, `rediss`, `redis-sentinel`, `redis-cluster`) in `reactiveRedisConnectionFactory()` for better observability at startup.
Cover all scheme branches (standalone, SSL, sentinel, cluster), authentication variants (no auth, user+password, password-only), error cases (invalid scheme, missing sentinel master as null and empty string), and custom port.
Cover all scheme branches (standalone, SSL, sentinel, cluster), authentication variants, sentinel master validation (null and empty string), client option verification, and scheme conversion for cluster. Add `@AfterEach` cleanup for Lettuce client resources.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@NilanshBansal are you interesting by this feature request? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
|
@vivek-appsmith @manish535 @subrata71 @salevine can you please check this? |
subrata71
left a comment
subrata71
left a comment
There was a problem hiding this comment.
Thanks @akantcheff for this contribution! Redis Sentinel support has been a long-standing community ask. Appreciate the effort.
That said, there are a few items that need to be addressed before we can proceed with this.
| validateSentinelMaster(); | ||
| final RedisSentinelConfiguration sentinelConfig = new RedisSentinelConfiguration() | ||
| .master(redisSentinelMaster) | ||
| .sentinel(redisUri.getHost(), redisUri.getPort()); |
There was a problem hiding this comment.
The current implementation only supports a single sentinel node. In production, Redis Sentinel deployments typically have 3+ sentinel nodes for quorum. A single sentinel node defeats the purpose of Sentinel (if that one sentinel goes down, the client can't discover the master).
| isCluster = true; | ||
| // java clients do not support redis-cluster scheme | ||
| if (redisurl.startsWith("redis-cluster://")) { | ||
| redisurl = "redis://" + redisurl.substring("redis-cluster://".length()); |
There was a problem hiding this comment.
The existing code has rediss:// for standalone SSL connections. There's no equivalent rediss-sentinel:// or TLS option for the sentinel connection. In cloud environments (AWS ElastiCache, Azure Cache), sentinel with TLS is common.
| void testReactiveRedisConnectionFactory_WithStandaloneRedis_CreatesStandaloneConfiguration() { | ||
| // Given | ||
| String redisUrl = "redis://localhost:6379"; | ||
| ReflectionTestUtils.setField(redisConfig, "redisURL", redisUrl); |
There was a problem hiding this comment.
Nit: Tests use ReflectionTestUtils.setField() heavily, which is brittle if field names change.
| # Redis Properties | ||
| appsmith.redis.url=${APPSMITH_REDIS_URL} | ||
| appsmith.redis.git.url=${APPSMITH_REDIS_GIT_URL:${APPSMITH_REDIS_URL}} | ||
| appsmith.redis.sentinel.master=${APPSMITH_REDIS_SENTINEL_MASTER:} |
There was a problem hiding this comment.
No Helm/Docker documentation update: I thinkdeploy/helm/values.yaml and deploy/helm/templates/configMap.yaml need updates to support the new APPSMITH_REDIS_SENTINEL_MASTER env var. @wyattwalter Could you please confirm?
| @@ -67,6 +67,7 @@ sentry.environment=${APPSMITH_SERVER_SENTRY_ENVIRONMENT:} | |||
| # Redis Properties | |||
| appsmith.redis.url=${APPSMITH_REDIS_URL} | |||
| appsmith.redis.git.url=${APPSMITH_REDIS_GIT_URL:${APPSMITH_REDIS_URL}} | |||
There was a problem hiding this comment.
Concern: If a user sets APPSMITH_REDIS_GIT_URL to a sentinel URL, it will be passed to bash/RTS code that likely doesn't understand the redis-sentinel:// scheme. @sondermanish @wyattwalter What do you think?
| fillAuthentication(redisUri, sentinelConfig); | ||
| final LettuceClientConfiguration clientConfig = | ||
| LettuceClientConfiguration.builder().build(); | ||
| return new LettuceConnectionFactory(sentinelConfig, clientConfig); |
There was a problem hiding this comment.
QQ: Does it provide connection pooling out of the box? Or is it not covered in the current implementation?
3 participants
Description
Add Redis Sentinel support to the Appsmith server.
redis-sentinel://scheme support in bothreactiveRedisConnectionFactory()andredisClient()beansextractCredentials()method with proper validation for bothnulland empty sentinel master nameAPPSMITH_REDIS_SENTINEL_MASTERenvironment variable mapping inapplication-ce.propertiesfor consistency with otherAPPSMITH_*env varslog.info()to each scheme branch inreactiveRedisConnectionFactory()for startup observabilityConfiguration
APPSMITH_REDIS_URLredis-sentinel://sentinel-host:26379APPSMITH_REDIS_SENTINEL_MASTERmymasterAuthentication is supported via URL:
redis-sentinel://username:password@sentinel-host:26379Fixes #11976
cc @appsmithorg/query-widgets-pod
Thanks to contributors: @m-k8s @benjaminParisel @abirembaut
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?