fix: use CLUSTER NODES hostname over raw ip for cluster discovery#6180
Open
MickeyShnaiderman-RecoLabs wants to merge 3 commits into
Open
fix: use CLUSTER NODES hostname over raw ip for cluster discovery#6180MickeyShnaiderman-RecoLabs wants to merge 3 commits into
MickeyShnaiderman-RecoLabs wants to merge 3 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>
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
2 participants
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.
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
Medium Risk
Changes the cluster connection bootstrap path used by ioredis/node-redis strategies; incorrect parsing could still break discovery, but behavior aligns with Redis spec and is heavily unit-tested.
Overview
Fixes cluster auto-discovery for deployments where nodes announce hostnames behind load balancers/NAT (e.g. ElastiCache, OCI) by delegating address choice to Redis instead of parsing
CLUSTER NODES.discoverClusterNodesnow runsCLUSTER SLOTSand builds root node lists from each entry’s server-resolved preferred endpoint (cluster-preferred-endpoint-type).CLUSTER NODESparsing (parseNodesFromClusterInfoReply) and relatedIRedisClusterNodetypes are removed.Adds
resolvePreferredEndpointandparseNodesFromClusterSlotsReply(dedup by node id orhost:port, handlesnull/''via connect host, skips'?'misconfigured markers, supports pre-4.0[ip, port]replies).ClusterShardsInfoStrategy(monitor UI) now labels nodes fromendpoint(via the same helper) rather thanhostname, with display fallbacks toipwhen needed.Tests and factories updated for ip vs hostname preference and abnormal endpoint values.
Reviewed by Cursor Bugbot for commit 6319f30. Bugbot is set up for automated code reviews on this repo. Configure here.