Repository navigation
fix: enable Redis Cluster clients with a single seed - #337
Merged
Merged
Conversation
With cluster_type=cluster and a single seed, Session, Notifier, and OAuth storage currently construct a standalone client. PING can succeed while key operations return MOVED. Use one internal Redis client constructor that selects standalone, Sentinel failover, or Cluster explicitly from cluster_type. Preserve authentication, existing configuration keys, and the existing Cluster behavior of ignoring db. Close clients when startup connection checks fail and report the component and selected client type. The constructor rejects unknown modes, missing addresses, multiple standalone addresses, and Sentinel without master_name. An omitted mode defaults to single; deployments relying on multiple addresses to implicitly select Cluster must explicitly configure cluster_type=cluster. Validation: - make lint passed (the existing target runs gofmt/goimports). - Full make test with TEST_FLAGS='-v -race' passed, with the opt-in Cluster test enabled. - A disposable three-primary Redis 7.2.5 Cluster passed the single-seed cross-shard test: a plain seed client receives MOVED, while Session registration/read/event delivery, Notifier delivery, and OAuth client CRUD succeed. Discovery also succeeds with an unreachable seed in the configured list. - go build passed for cmd/mcp-gateway and cmd/apiserver. The regular suite covers mode selection and all three single-seed constructors with miniredis. The real Cluster test is opt-in via UNLA_TEST_REDIS_CLUSTER_ADDR; execution instructions and compatibility behavior are in docs/redis-client-modes.md. Live Sentinel failover and OAuth all-shard token scans are outside this validation scope. No dependency upgrade or Redis key migration is required.
Contributor
Reviewer's GuideThe PR fixes single-seed Redis Cluster handling by introducing one validated, explicit mode-aware client constructor and migrating Session, Notifier, and OAuth storage to it. It preserves existing credentials and configuration keys, closes clients on failed startup checks, adds targeted unit and optional real-Cluster coverage, and documents the resulting compatibility contract. Sequence diagram for single-seed Redis Cluster initializationsequenceDiagram
participant Storage as Storage consumer
participant Factory as redisclient.New
participant Cluster as Redis Cluster client
participant Redis as Redis Cluster
Storage->>Factory: New(cluster, UniversalOptions)
Factory->>Cluster: NewClusterClient(options.Cluster())
Factory-->>Storage: UniversalClient
Storage->>Cluster: Ping()
Cluster->>Redis: Discover cluster topology
Redis-->>Cluster: Slot ownership
Cluster-->>Storage: Ping result
Storage->>Cluster: Close() on failed Ping
Flow diagram for Redis mode validationflowchart TD
Start[redisclient.New] --> Parse[Parse and normalize addresses]
Parse --> Valid{Mode and addresses valid?}
Valid -->|No| Error[Return configuration error]
Valid -->|Yes| Mode{Selected mode}
Mode -->|single| Standalone[Create standalone client]
Mode -->|sentinel| Failover[Create Sentinel failover client]
Mode -->|cluster| Cluster[Create Cluster client]
Failover --> Return[Return client to owner]
Standalone --> Return
Cluster --> Return
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/auth/storage/redis.go" line_range="22" />
<code_context>
- if clusterType != cnst.RedisClusterTypeCluster {
- // can not set db in cluster mode
- redisOptions.DB = db
+ client, err := redisclient.New(clusterType, redis.UniversalOptions{
+ Addrs: []string{addr},
+ MasterName: masterName,
</code_context>
<issue_to_address>
**Revoked tokens remain usable**
When OAuth storage is configured for Cluster mode and a client's tokens are stored on shards the scan does not visit, `DeleteTokensByClientID` scans for matching keys without covering every cluster primary, then reports successful cleanup while tokens on unscanned shards remain stored and usable.
Make token cleanup scan every cluster primary before reporting success.
Also at `internal/redisclient/client.go:55`.
</issue_to_address>
### Comment 2
<location path="internal/redisclient/client.go" line_range="20" />
<code_context>
+// +link=component://docs/redis-client-modes.md
+func New(mode string, options redis.UniversalOptions) (redis.UniversalClient, error) {
+ if mode == "" {
+ mode = cnst.RedisClusterTypeSingle
+ }
+ switch mode {
</code_context>
<issue_to_address>
**Helm chart cannot select Cluster**
When a Helm chart user configures Redis Cluster through the chart, `New` defaults an omitted mode to `single`, so Helm-generated Redis configs select standalone for a single Cluster seed and reject multiple seeds instead of using Cluster; shard operations fail or initialization stops.
Add `cluster_type` configuration and value/environment-variable plumbing for the Redis sections generated by the Helm chart.
</issue_to_address>
### Comment 3
<location path="internal/redisclient/client_test.go" line_range="104" />
<code_context>
+ require.Equal(t, tt.options.Username, c.Options().Username)
+ require.Equal(t, tt.options.Password, c.Options().Password)
+ } else {
+ c, ok := client.(*redis.Client)
+ require.True(t, ok, "single and sentinel modes must not create ClusterClient")
+ require.Equal(t, tt.db, c.Options().DB)
</code_context>
<issue_to_address>
**Sentinel failover regression passes**
When the Sentinel branch returns a standalone client instead of a failover client, `TestNewDeploymentMode` only requires a non-Cluster `*redis.Client` and checks fields a standalone client also has, so returning `redis.NewClient` for Sentinel mode still passes both Sentinel cases and leaves failover untested.
Assert the Sentinel failover configuration on the returned client, including the configured master name and Sentinel addresses.
</issue_to_address>
### Comment 4
<location path="internal/auth/storage/redis.go" line_range="22" />
<code_context>
- if clusterType != cnst.RedisClusterTypeCluster {
- // can not set db in cluster mode
- redisOptions.DB = db
+ client, err := redisclient.New(clusterType, redis.UniversalOptions{
+ Addrs: []string{addr},
+ MasterName: masterName,
</code_context>
<issue_to_address>
**Selected client type is not logged**
When OAuth Redis storage initializes successfully, `NewRedisStorage` returns without logging the selected Redis client type, so operators cannot verify whether OAuth storage selected standalone, Sentinel, or Cluster at startup.
Log the selected Redis client type when initializing OAuth storage.
</issue_to_address>
qiankunli
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
cluster_type=clusteris configured with only one seed address, Session, Notifier, and OAuth storage currently create a standalone Redis client.PINGmay succeed, but commands for keys owned by another node fail withMOVED.Set
UniversalOptions.IsClusterModefrom the existingcluster_typevalue in each of the three constructors. The existing go-redis v9.9.0 dependency already supports this option.The production change is three added lines. Three focused miniredis tests assert that each constructor returns a
ClusterClientfor a single seed. Existing configuration parsing, validation, client lifecycle, and other deployment modes remain unchanged.Validation on the final implementation:
make lintpassed.make testwithTEST_FLAGS='-v -race'passed.The committed tests verify client selection; they do not claim full Redis Cluster compatibility for every storage operation.
Summary by Sourcery
Enable Redis cluster client selection consistently across session, notifier, and OAuth storage constructors.
Bug Fixes:
Enhancements:
Tests: