fix: use CLUSTER NODES hostname over raw ip for cluster discovery#6180
fix: use CLUSTER NODES hostname over raw ip for cluster discovery#6180MickeyShnaiderman-RecoLabs wants to merge 5 commits into
Conversation
Cluster auto-discovery failed with "failed to refresh slots cache" / "not all slots covered" for clusters that announce a client-facing hostname per node (Redis 7+ cluster-announce-hostname), e.g. AWS ElastiCache/OCI Cache sharded clusters and per-node load balancers / NAT setups where the node's raw ip is not routable to the client. Root cause: parseNodesFromClusterInfoReply() (reply.util.ts), used by discoverClusterNodes() (cluster.util.ts) to seed cluster root nodes for both the ioredis and node-redis connection strategies, parsed only the "ip:port@cport" prefix of each CLUSTER NODES line and silently discarded everything after the first comma - which is exactly where Redis 7+ appends "[,hostname[,aux_field=value]*]". discoverClusterNodes() then replaced the single (working) user- supplied entrypoint with a full list of unreachable internal ips before ioredis.Cluster / node-redis ever got a chance to use its own hostname-aware CLUSTER SLOTS-based discovery, so every bootstrap connection attempt failed. Note ClusterShardsInfoStrategy (CLUSTER SHARDS, used for the Redis 7+ cluster monitor UI) already preferred hostname over ip - only the CLUSTER NODES parsing path used for establishing the actual cluster connection had the bug. Fix: parse the optional hostname out of the endpoint field (ignoring any trailing aux fields such as shard-id) and prefer it over the raw ip when building root node addresses, falling back to the ip when no hostname is announced so standard clusters are unaffected. Addresses: - redis#5393 - redis#3416 - redis#3429 Signed-off-by: MickeyShnaiderman-RecoLabs <mickeys@reco.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed276c6a88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Addresses review feedback on redis#6180: `discoverClusterNodes` previously did `node.hostname || node.host`, unconditionally preferring a node's announced hostname over its ip whenever one was present. That ignores `cluster-preferred-endpoint-type`: a node can announce a hostname as metadata while the server is actually configured to prefer the ip (the default), in which case the hostname may be internal/non-resolvable from RedisInsight while the ip is the one that is actually reachable - regressing exactly the kind of cluster this PR was meant to fix. `CLUSTER NODES` has no way to express this correctly: the hostname it exposes is unconditional metadata, not the server's resolved preference. That preference is only available via `CLUSTER SLOTS` (or the newer `CLUSTER SHARDS`, 7.0+ only), whose first per-node field is already resolved server-side according to `cluster-preferred-endpoint-type`. `discoverClusterNodes` now calls `CLUSTER SLOTS` instead of `CLUSTER NODES` and uses that field as-is, fully delegating the ip-vs-hostname decision to the server rather than re-implementing it client-side. CLUSTER SLOTS is used over CLUSTER SHARDS for broader compatibility (available since Redis 3.0.0 vs 7.0+). ioredis/node-redis do not expose this as a reusable parser (ioredis parses CLUSTER SLOTS inline in `Cluster.prototype.getInfoFromNode`, not as a public API), so `parseNodesFromClusterSlotsReply` is added alongside a shared `resolvePreferredEndpoint` helper implementing the endpoint field's documented abnormal values (see https://redis.io/docs/latest/commands/cluster-slots/): - `null`/`''` (unknown endpoint): resolves to the host used to send the command. - `'?'` (misconfigured node - preferred type is hostname but none is announced): the spec warns this may not be the same node that served the command, so the node is skipped as a root-node candidate rather than guessed at. The now fully-unused `CLUSTER NODES` endpoint/hostname parsing (`parseNodesFromClusterInfoReply`, `IRedisClusterNode`, `RedisClusterNodeLinkState`) is removed rather than left dead - it has no remaining callers and, per the above, could never be made correct for this purpose regardless. CLUSTER SLOTS also makes the old "connected" link-state filter unnecessary: failed nodes are not returned by CLUSTER SLOTS in the first place. Also fixes `ClusterShardsInfoStrategy` (Redis 7+ cluster monitor UI), which had the same latent issue - `nodeObj.hostname || nodeObj.ip` instead of the shard's own `endpoint` field (CLUSTER SHARDS' equivalent of CLUSTER SLOTS' preferred endpoint, same abnormal-value semantics). This is a display-only path (not used to establish connections), so a misconfigured/unknown ('?') endpoint falls back to the node's own `ip` for a best-effort label instead of being skipped. Updated/added unit tests in reply.util.spec.ts, cluster.util.spec.ts and cluster-shards.info.strategy.spec.ts covering preferred-endpoint- type=hostname, =ip (including a hostname being announced but not preferred - the exact scenario flagged in review), and the null/''/'?' abnormal endpoint values. Signed-off-by: MickeyShnaiderman-RecoLabs <mickeys@reco.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29ce958c7d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Addresses review feedback on redis#6180: node ids were only added to CLUSTER SLOTS in Redis 4.0.0 (see the "Behavior change history" on https://redis.io/docs/latest/commands/cluster-slots/); pre-4.0 clusters return just [ip, port] per node with no id. The previous `if (!id) continue` guard in parseNodesFromClusterSlotsReply treated that as invalid and skipped every node, so discoverClusterNodes returned an empty root-node list for any pre-4.0 cluster - a regression relative to the old CLUSTER NODES parser, and a direct contradiction of this change's own "broader compatibility" rationale for choosing CLUSTER SLOTS over CLUSTER SHARDS. Fall back to a "host:port" dedup key when no node id is present so those nodes are still discovered, instead of dropping them. Added regression tests in reply.util.spec.ts and cluster.util.spec.ts for [ip, port]-only node tuples, including deduplication across non-contiguous slot ranges. Signed-off-by: MickeyShnaiderman-RecoLabs <mickeys@reco.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@valkirilov, I apologize if I’m tagging you, but I noticed your active involvement in the repository. Could you please tag the appropriate team member to review this change? Our team has been using a patched version with this fix extensively. |
|
Sorry for the delay @MickeyShnaiderman-RecoLabs, we saw it, but forgot to take care of it. We'll take a look and let you know whether it's good to go or if it needs some minor tweaks. Anyway, thanks for the time spent on it and for your contribution! |
pawelangelow
left a comment
pawelangelow
left a comment
There was a problem hiding this comment.
The title says use CLUSTER NODES hostname over raw ip, but the PR moves away from CLUSTER NODES to CLUSTER SLOTS and deliberately doesn't prefer hostname - it uses the server's resolved preferred endpoint. Suggest updating to something like fix: use CLUSTER SLOTS preferred endpoint for cluster discovery so the history reflects the final approach.
…red slots - Add back an IPv6 preferred-endpoint test in parseNodesFromClusterSlotsReply (reply.util.spec.ts) that was dropped along with the removed CLUSTER NODES parser. - discoverClusterNodes now falls back to the connection entrypoint (client.options.host/port) when CLUSTER SLOTS resolves to zero nodes (e.g. every node reports the '?' misconfigured-endpoint marker), instead of returning an empty root-node list. Addresses PR review comments from pawelangelow on redis#6180. Signed-off-by: MickeyShnaiderman-RecoLabs <mickeys@reco.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit a9cc0c2. Configure here.
resolvePreferredEndpoint was called with fallbackHost, so any node with a null/empty endpoint resolved straight to the shared connection host - before nodeObj.ip was ever considered. Every such node in the cluster monitor UI ended up labeled with the same host instead of its own address. Reorder the fallback chain to endpoint -> ip -> fallbackHost, matching the inline comment's stated intent (ip for a display-only unknown endpoint; fallbackHost only as a last resort when ip is absent too). Addresses BugBot review comment on redis#6180 (discussion_r3632382937). Signed-off-by: MickeyShnaiderman-RecoLabs <mickeys@reco.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks so much for all the iterations on this @MickeyShnaiderman-RecoLabs - really appreciate how thoroughly you've handled the feedback, the IPv6 test and the entrypoint fallback are exactly what I had in mind, and the spec coverage is great. 🙏 Just one thing left before I can approve: the type-check / tsc baselines job is failing because the changes introduce a couple of new TypeScript errors that trip our baseline gate. Please fix the types directly rather than refreshing the baselines - we don't want to bake these into .tscheck.rec.json. You can reproduce locally with Once that's green I'll resolve the two threads and approve. Thanks again for sticking with this - it's a solid fix and I'm keen to get it merged! 🚀 |
4 participants
What
Cluster auto-discovery fails with
failed to refresh slots cache/not all slots coveredfor any Redis Cluster that announces a client-facing hostname per node (cluster-announce-hostname, Redis 7+) instead of a directly-routable IP — e.g. AWS ElastiCache/OCI Cache sharded clusters, or any per-node load balancer / NAT setup where the raw pod/node IP isn't reachable from the client. Related to #5393, #3416, #3429.Root cause:
discoverClusterNodes()(cluster.util.ts), used to seed cluster root nodes for both theioredisandnode-redisconnection strategies, read node addresses fromCLUSTER NODES, which only exposes an announced hostname as unconditional metadata — it has no way to express whether the server is actually configured to prefer that hostname over the IP (cluster-preferred-endpoint-type).discoverClusterNodes()then replaced the single, working, user-supplied entrypoint with a full list of these raw addresses beforeioredis.Cluster/node-redisever got a chance to run their own preferred-endpoint-awareCLUSTER SLOTS-based discovery, so every bootstrap connection attempt failed.Fix:
discoverClusterNodes()now callsCLUSTER SLOTSinstead ofCLUSTER NODESand uses each node's preferred endpoint field as-is — that field is already resolved server-side according tocluster-preferred-endpoint-type(ip/hostname/unknown-endpoint), so the client fully delegates the ip-vs-hostname decision to the server instead of re-implementing it.CLUSTER SLOTSis used over the newerCLUSTER SHARDSfor broader compatibility (available since Redis 3.0.0 vs 7.0+). The endpoint field's documented abnormal values are handled per spec:null/''(unknown endpoint): resolves to the host used to send the command.'?'(misconfigured node — preferred type is hostname but none is announced): the spec warns this may not be the same node that served the command, so it's skipped as a root-node candidate rather than guessed at.The now fully-unused
CLUSTER NODESendpoint parsing is removed rather than left dead, since it has no remaining callers and could never correctly express the server's preference regardless of how it's parsed.Also fixes
ClusterShardsInfoStrategy(the Redis 7+ cluster monitor UI), which had the exact same latent issue — it read the announcedhostnamefield directly instead of the shard's ownendpointfield (CLUSTER SHARDS' equivalent ofCLUSTER SLOTS' preferred endpoint, same abnormal-value semantics). This path is display-only (not used to establish connections), so an unknown/misconfigured endpoint there falls back to the node's ownipfor a best-effort label instead of being skipped.Testing
reply.util.spec.ts,cluster.util.spec.tsandcluster-shards.info.strategy.spec.tscovering: preferred-endpoint-type=hostname, preferred-endpoint-type=ip (including a node that announces a hostname but isn't configured to prefer it), and thenull/''/'?'abnormal endpoint values.cluster-preferred-endpoint-type: hostname) that previously failed with the exactfailed to refresh slots cacheerror reported in the linked issues — cluster auto-discovery now succeeds and the app connects normally.Note
High Risk
Changes the cluster bootstrap path used before ioredis/node-redis connect; wrong addresses would break or misroute production cluster connections, though behavior is aligned with Redis spec and heavily unit-tested.
Overview
Fixes cluster auto-discovery for deployments where nodes expose a client-facing hostname (NLB/NAT, ElastiCache-style) by trusting Redis’s server-resolved preferred address instead of inferring it from
CLUSTER NODESmetadata.Connection discovery (
discoverClusterNodes) now runsCLUSTER SLOTSand maps each node from the reply’s preferred-endpoint field via newresolvePreferredEndpoint/parseNodesFromClusterSlotsReply(dedupe by node id orhost:port, pre-4.0[ip, port]only). Abnormal endpoints:null/''→ connection host;?skipped; if every node is skipped, seed from the user’s connection host.CLUSTER NODESparsing and relatedIRedisClusterNodetypes/mocks are removed.Cluster monitor UI (
ClusterShardsInfoStrategy) uses each shard node’sendpoint(not barehostname) with the same resolver, thenip, then connection host only when needed for display labels.Tests and factories updated for ip vs hostname preference, TLS ports, and edge cases.
Reviewed by Cursor Bugbot for commit 823dd01. Bugbot is set up for automated code reviews on this repo. Configure here.