feat(router): add sentinel mode support to RedisCloser#2267
feat(router): add sentinel mode support to RedisCloser#2267biubiubiuboomboomboom wants to merge 4 commits intowundergraph:mainfrom
Conversation
biubiubiuboomboomboom
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughAdds Redis Sentinel support to the rate-limiter: new sentinel fields in public config and RedisCloserOptions; implements mode dispatch (sentinel/cluster/direct) with validation and client constructors; updates tests, error messages, and JSON schema; relaxes required Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/internal/rediscloser/rediscloser.go (1)
127-151: Same leak pattern for direct client; close and return nil on failure.Avoid returning a half-initialized client.
Apply this diff:
if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { - return rdb, fmt.Errorf("failed to create a functioning Redis direct client: %w", err) + _ = rdb.Close() + return nil, fmt.Errorf("failed to create a functioning Redis direct client: %w", err) }Optional: move the isClusterClient() warning after a successful ping to reduce noise when the endpoint is unreachable.
🧹 Nitpick comments (1)
router/internal/rediscloser/rediscloser.go (1)
24-30: Sentinel config lacks parity (username/DB/TLS/timeouts).Direct/cluster modes can carry username/DB/TLS/timeouts via URL; sentinel mode cannot. Add optional fields (e.g., Username, DB, TLSConfig, Dial/Read/Write timeouts) to RedisCloserOptions and map them into redis.FailoverOptions to reach feature parity.
Would ACL username/DB/TLS be required by your deployments?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
router/core/router.go(1 hunks)router/internal/rediscloser/rediscloser.go(1 hunks)router/internal/rediscloser/rediscloser_test.go(2 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/internal/rediscloser/rediscloser_test.go (1)
router/internal/rediscloser/rediscloser.go (2)
RedisCloserOptions(19-30)NewRedisCloser(32-45)
router/internal/rediscloser/rediscloser.go (1)
router/pkg/mcpserver/util.go (1)
Logger(6-9)
🔇 Additional comments (1)
router/internal/rediscloser/rediscloser.go (1)
47-70: Validation logic looks solid.Mode exclusivity and required fields per mode are enforced correctly.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/config/config.schema.json (1)
1916-1948: Schema must enforce sentinel-specific requirementsRuntime validation already rejects
sentinel_enabled: truewithoutmaster_name/sentinel_addrs, but the schema still allows it, so configs pass static validation and only fail at runtime. Add conditional requirements (and aminItemsguard) so users get immediate feedback."default": "cosmo_rate_limit" }, "sentinel_enabled": { "type": "boolean", "description": "Enable Redis Sentinel mode for high availability. Cannot be used together with cluster_enabled.", "default": false }, "master_name": { "type": "string", "description": "The name of the Redis master as configured in Sentinel. Required when sentinel_enabled is true." }, "sentinel_addrs": { "type": "array", "description": "List of Redis Sentinel addresses in the format 'host:port'. Required when sentinel_enabled is true.", + "minItems": 1, "items": { "type": "string" } }, "sentinel_password": { "type": "string", "description": "Password for authenticating with Redis Sentinel nodes, if required." } + }, + "allOf": [ + { + "if": { "properties": { "sentinel_enabled": { "const": true } } }, + "then": { "required": ["master_name", "sentinel_addrs"] } + }, + { + "if": { "properties": { "cluster_enabled": { "const": true } } }, + "then": { "required": ["urls"] } + } + ] } } }
♻️ Duplicate comments (2)
router/internal/rediscloser/rediscloser.go (2)
91-94: Return nil client when sentinel ping failsWe still return the
redis.Clientafter closing it, so callers can unknowingly hold an unusable client. Returnnilonce ping fails.if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { _ = rdb.Close() - return rdb, fmt.Errorf("failed to create a functioning Redis sentinel client: %w", err) + return nil, fmt.Errorf("failed to create a functioning Redis sentinel client: %w", err) }
127-129: Cluster ping failure must return nilSame issue here: after closing we hand back the client. Return
nilto avoid leaking a dead handle.if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { _ = rdb.Close() - return rdb, fmt.Errorf("failed to create a functioning Redis cluster client: %w", err) + return nil, fmt.Errorf("failed to create a functioning Redis cluster client: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/internal/rediscloser/rediscloser.go(1 hunks)router/pkg/config/config.schema.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/internal/rediscloser/rediscloser.go (2)
router/pkg/mcpserver/util.go (1)
Logger(6-9)router/internal/persistedoperation/operationstorage/cdn/client.go (1)
NewClient(130-158)
Docstrings generation was requested by @biubiubiuboomboomboom. * #2267 (comment) The following files were modified: * `router/internal/rediscloser/rediscloser.go`
|
Note Generated docstrings for this pull request at #2268 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
router/internal/rediscloser/rediscloser.go (3)
91-93: Return nil instead of closed client on failure.Line 93 returns the closed
rdbpointer after callingClose(). Callers expect a nil client on error to avoid misuse of the closed connection.Apply this diff:
if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { _ = rdb.Close() - return rdb, fmt.Errorf("failed to create a functioning Redis sentinel client: %w", err) + return nil, fmt.Errorf("failed to create a functioning Redis sentinel client: %w", err) }
127-129: Return nil instead of closed client on failure.Line 129 returns the closed
rdbpointer after callingClose(). Callers expect a nil client on error to avoid misuse of the closed connection.Apply this diff:
if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { _ = rdb.Close() - return rdb, fmt.Errorf("failed to create a functioning Redis cluster client: %w", err) + return nil, fmt.Errorf("failed to create a functioning Redis cluster client: %w", err) }
154-156: Return nil instead of closed client on failure.Line 156 returns the closed
rdbpointer after callingClose(). Callers expect a nil client on error to avoid misuse of the closed connection.Apply this diff:
if isFunctioning, err := IsFunctioningClient(rdb); !isFunctioning { _ = rdb.Close() - return rdb, fmt.Errorf("failed to create a functioning Redis direct client: %w", err) + return nil, fmt.Errorf("failed to create a functioning Redis direct client: %w", err) }
🧹 Nitpick comments (1)
router/internal/rediscloser/rediscloser.go (1)
192-199: Add timeout to prevent indefinite blocking on Ping.Using
context.Background()without a timeout can cause indefinite blocking if the Redis server is unresponsive.Apply this diff:
+import ( + "time" +) + func IsFunctioningClient(rdb RDCloser) (bool, error) { if rdb == nil { return false, nil } - res, err := rdb.Ping(context.Background()).Result() + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + res, err := rdb.Ping(ctx).Result() return err == nil && res == "PONG", err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/internal/rediscloser/rediscloser.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/internal/rediscloser/rediscloser.go (2)
router/pkg/mcpserver/util.go (1)
Logger(6-9)router/internal/persistedoperation/operationstorage/cdn/client.go (1)
NewClient(130-158)
🔇 Additional comments (3)
router/internal/rediscloser/rediscloser.go (3)
24-30: LGTM! Sentinel configuration fields are well-structured.The new fields comprehensively support Redis Sentinel mode with appropriate types and clear naming conventions.
32-51: LGTM! Robust nil-safety and clean mode dispatch.The nil checks for
optsandopts.Loggerprevent panics, and the switch-based mode dispatch cleanly separates Sentinel, Cluster, and Direct client creation.
53-76: LGTM! Validation ensures mutual exclusivity and required fields.The validation correctly enforces that Sentinel and Cluster modes cannot be enabled simultaneously, and verifies that each mode has its required configuration fields.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Checklist
Motivation and Context
Currently, Redis in RateLimit only supports direct connection mode and cluster mode, but does not support sentinel mode. I hope we can help add this capability if it is a good requirement.
How to use it
Configuration example as follows :
Related Issue
#2057