From ed276c6a8881905f198ebc85420e748a4138d751 Mon Sep 17 00:00:00 2001 From: MickeyShnaiderman-RecoLabs Date: Wed, 8 Jul 2026 18:34:21 +0300 Subject: [PATCH 1/3] be/bugfix: use CLUSTER NODES hostname over raw ip 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: - https://github.com/redis/RedisInsight/issues/5393 - https://github.com/redis/RedisInsight/issues/3416 - https://github.com/redis/RedisInsight/issues/3429 Signed-off-by: MickeyShnaiderman-RecoLabs Co-authored-by: Cursor --- redisinsight/api/src/__mocks__/redis-info.ts | 8 +++++ redisinsight/api/src/models/redis-cluster.ts | 3 ++ .../modules/redis/utils/cluster.util.spec.ts | 23 ++++++++++++ .../src/modules/redis/utils/cluster.util.ts | 7 +++- .../modules/redis/utils/reply.util.spec.ts | 35 +++++++++++++++++++ .../api/src/modules/redis/utils/reply.util.ts | 18 +++++++++- 6 files changed, 92 insertions(+), 2 deletions(-) diff --git a/redisinsight/api/src/__mocks__/redis-info.ts b/redisinsight/api/src/__mocks__/redis-info.ts index 8a24d99ecb..ea04168c9f 100644 --- a/redisinsight/api/src/__mocks__/redis-info.ts +++ b/redisinsight/api/src/__mocks__/redis-info.ts @@ -114,6 +114,14 @@ export const mockRedisClusterNodesResponseIPv6: string = '07c37dfeb235213a872192d90877d0cd55635b91 2001:db8::1:7001@17001 slave e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 0 1426238317239 4 connected\n' + 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 2001:db8::2:7002@17002 myself,master - 0 0 1 connected 0-16383'; +// Redis 7+ nodes announcing a hostname via `cluster-announce-hostname`, e.g. behind +// per-node load balancers/NAT (mirrors OCI Cache Sharded / AWS-style deployments, +// see https://github.com/redis/RedisInsight/issues/5393) +// eslint-disable-next-line max-len +export const mockRedisClusterNodesResponseWithHostname: string = + '07c37dfeb235213a872192d90877d0cd55635b91 10.0.161.40:7379@16379,node-1.redis.example.com slave e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 0 1426238317239 4 connected\n' + + 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 10.0.146.93:7379@16379,node-2.redis.example.com,shard-id=abc123 myself,master - 0 0 1 connected 0-16383'; + export const mockStandaloneRedisInfoReply: string = `${ mockRedisServerInfoResponse }\r\n${mockRedisClientsInfoResponse}\r\n${mockRedisMemoryInfoResponse}\r\n${ diff --git a/redisinsight/api/src/models/redis-cluster.ts b/redisinsight/api/src/models/redis-cluster.ts index 924a708b93..10a06f813a 100644 --- a/redisinsight/api/src/models/redis-cluster.ts +++ b/redisinsight/api/src/models/redis-cluster.ts @@ -21,6 +21,9 @@ export interface IRedisClusterNode extends IRedisClusterNodeAddress { replicaOf: string; linkState: RedisClusterNodeLinkState; slot: string; + // Client-facing hostname announced by the node (Redis 7+, `cluster-announce-hostname`). + // Undefined when the node has no hostname configured. + hostname?: string; } export enum RedisClusterNodeLinkState { diff --git a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts index 2ea3d1fb4d..eb11732198 100644 --- a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts @@ -1,6 +1,7 @@ import { mockRedisClusterFailInfoResponse, mockRedisClusterNodesResponse, + mockRedisClusterNodesResponseWithHostname, mockRedisClusterOkInfoResponse, mockStandaloneRedisClient, } from 'src/__mocks__'; @@ -66,4 +67,26 @@ describe('discoverClusterNodes', () => { expect(err).toEqual(replyError); } }); + + it('should use the announced hostname instead of the raw ip when present (Redis 7+)', async () => { + // Reproduces https://github.com/redis/RedisInsight/issues/5393, + // https://github.com/redis/RedisInsight/issues/3416 and + // https://github.com/redis/RedisInsight/issues/3429: nodes behind + // per-node load balancers / NAT announce a client-facing hostname + // because their raw ip is not routable to RedisInsight. + mockStandaloneRedisClient.sendCommand.mockResolvedValue( + mockRedisClusterNodesResponseWithHostname, + ); + + expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual([ + { + host: 'node-1.redis.example.com', + port: 7379, + }, + { + host: 'node-2.redis.example.com', + port: 7379, + }, + ]); + }); }); diff --git a/redisinsight/api/src/modules/redis/utils/cluster.util.ts b/redisinsight/api/src/modules/redis/utils/cluster.util.ts index ac43e7a2c5..c30ed94106 100644 --- a/redisinsight/api/src/modules/redis/utils/cluster.util.ts +++ b/redisinsight/api/src/modules/redis/utils/cluster.util.ts @@ -31,6 +31,11 @@ export const isCluster = async (client: RedisClient): Promise => { /** * Discover all cluster nodes for current connection + * Prefers each node's announced hostname (Redis 7+, `cluster-announce-hostname`) + * over its raw IP so that clusters behind per-node load balancers or NAT + * (where the raw IP is not routable to clients) remain reachable. Falls back + * to the IP when no hostname is announced, preserving behavior for standard + * clusters. * @param client */ export const discoverClusterNodes = async ( @@ -43,7 +48,7 @@ export const discoverClusterNodes = async ( ).filter((node) => node.linkState === RedisClusterNodeLinkState.Connected); return nodes.map((node) => ({ - host: node.host, + host: node.hostname || node.host, port: node.port, })); }; diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts index bc9e78f7e2..6c732ccbeb 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts @@ -1,6 +1,7 @@ import { mockRedisClusterNodesResponse, mockRedisClusterNodesResponseIPv6, + mockRedisClusterNodesResponseWithHostname, mockRedisServerInfoResponse, } from 'src/__mocks__'; import { flatMap } from 'lodash'; @@ -114,4 +115,38 @@ describe('parseNodesFromClusterInfoReply', () => { expect(result).toEqual(mockRedisClusterNodesIPv6); }); + it('should parse announced hostname (Redis 7+) alongside the ip, ignoring trailing aux fields', async () => { + const result = parseNodesFromClusterInfoReply( + mockRedisClusterNodesResponseWithHostname, + ); + + expect(result).toEqual([ + { + id: '07c37dfeb235213a872192d90877d0cd55635b91', + host: '10.0.161.40', + hostname: 'node-1.redis.example.com', + port: 7379, + replicaOf: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', + linkState: RedisClusterNodeLinkState.Connected, + slot: undefined, + }, + { + id: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', + host: '10.0.146.93', + // hostname must still be parsed correctly when followed by aux fields (e.g. shard-id) + hostname: 'node-2.redis.example.com', + port: 7379, + replicaOf: undefined, + linkState: RedisClusterNodeLinkState.Connected, + slot: '0-16383', + }, + ]); + }); + it('should leave hostname undefined when the node has none announced', async () => { + const result = parseNodesFromClusterInfoReply( + mockRedisClusterNodesResponse, + ); + + expect(result.every((node) => node.hostname === undefined)).toEqual(true); + }); }); diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.ts b/redisinsight/api/src/modules/redis/utils/reply.util.ts index 399c68bc61..c09ae503df 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.ts @@ -84,11 +84,18 @@ export const convertMultilineReplyToObject = ( /** * Parse and return all endpoints from the nodes list returned by "cluster info" command + * Since Redis 7.0 the endpoint field may also carry a client-facing hostname + * (`ip:port@cport[,hostname[,aux_field=aux_value]*]`), announced via + * `cluster-announce-hostname`. Managed/hosted clusters commonly sit behind + * per-node load balancers or NAT where the raw IP is not routable to clients, + * so the hostname (when present) is parsed out separately and should be + * preferred over the raw IP when establishing connections. * @Input * ``` * 08418e3514990489e48fa05d642efc33e205f5 172.31.100.211:6379@16379 myself,master - 0 1698694904000 1 connected 0-5460 * d2dee846c715a917ec9a4963e8885b06130f9f 172.31.100.212:6379@16379 master - 0 1698694905285 2 connected 5461-10922 * 3e92457ab813ad7a62dacf768ec7309210feaf [2001:db8::1]:7001@17001 master - 0 1698694906000 3 connected 10923-16383 + * 41ae438fc3ba52a13f40ed67841d463f8bd1ec 10.0.161.40:7379@16379,node-1.example.com master - 0 1698694907000 4 connected 10923-16383 * ``` * @Output * ``` @@ -104,6 +111,11 @@ export const convertMultilineReplyToObject = ( * { * host: "2001:db8::1", * port: 7001 + * }, + * { + * host: "10.0.161.40", + * hostname: "node-1.example.com", + * port: 7379 * } * ] * ``` @@ -121,7 +133,10 @@ export const parseNodesFromClusterInfoReply = ( const fields = line.split(' '); const [id, endpoint, , master, , , , linkState, slot] = fields; - const hostAndPort = endpoint.split('@')[0]; + // endpoint = "ip:port@cport[,hostname[,aux_field=aux_value]*]" + const [ipPortCport, hostname] = endpoint.split(','); + + const hostAndPort = ipPortCport.split('@')[0]; const lastColonIndex = hostAndPort.lastIndexOf(':'); const host = hostAndPort.substring(0, lastColonIndex); @@ -129,6 +144,7 @@ export const parseNodesFromClusterInfoReply = ( nodes.push({ id, host, + hostname: hostname || undefined, port: parseInt(port, 10), replicaOf: master !== '-' ? master : undefined, linkState, From 29ce958c7db45e854cd661903dca9df6f1a9dc16 Mon Sep 17 00:00:00 2001 From: MickeyShnaiderman-RecoLabs Date: Thu, 9 Jul 2026 13:55:36 +0300 Subject: [PATCH 2/3] be/bugfix: derive root nodes from CLUSTER SLOTS preferred endpoint Addresses review feedback on #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 Co-authored-by: Cursor --- redisinsight/api/src/__mocks__/redis-info.ts | 18 -- redisinsight/api/src/models/redis-cluster.ts | 15 -- .../__tests__/cluster-shards.factory.ts | 29 ++- .../cluster-shards.info.strategy.spec.ts | 98 +++++++- .../cluster-shards.info.strategy.ts | 21 +- .../modules/redis/utils/cluster.util.spec.ts | 96 +++++--- .../src/modules/redis/utils/cluster.util.ts | 38 ++-- .../modules/redis/utils/reply.util.spec.ts | 214 ++++++++++-------- .../api/src/modules/redis/utils/reply.util.ts | 157 ++++++++----- 9 files changed, 438 insertions(+), 248 deletions(-) diff --git a/redisinsight/api/src/__mocks__/redis-info.ts b/redisinsight/api/src/__mocks__/redis-info.ts index ea04168c9f..a45da683df 100644 --- a/redisinsight/api/src/__mocks__/redis-info.ts +++ b/redisinsight/api/src/__mocks__/redis-info.ts @@ -104,24 +104,6 @@ export const mockRedisSentinelMasterResponse: Array = [ mockSentinelMasterInOkState, ]; -// eslint-disable-next-line max-len -export const mockRedisClusterNodesResponse: string = - '07c37dfeb235213a872192d90877d0cd55635b91 127.0.0.1:30004@31004 slave e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 0 1426238317239 4 connected\n' + - 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 127.0.0.1:30001@31001 myself,master - 0 0 1 connected 0-16383'; - -// eslint-disable-next-line max-len -export const mockRedisClusterNodesResponseIPv6: string = - '07c37dfeb235213a872192d90877d0cd55635b91 2001:db8::1:7001@17001 slave e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 0 1426238317239 4 connected\n' + - 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 2001:db8::2:7002@17002 myself,master - 0 0 1 connected 0-16383'; - -// Redis 7+ nodes announcing a hostname via `cluster-announce-hostname`, e.g. behind -// per-node load balancers/NAT (mirrors OCI Cache Sharded / AWS-style deployments, -// see https://github.com/redis/RedisInsight/issues/5393) -// eslint-disable-next-line max-len -export const mockRedisClusterNodesResponseWithHostname: string = - '07c37dfeb235213a872192d90877d0cd55635b91 10.0.161.40:7379@16379,node-1.redis.example.com slave e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 0 1426238317239 4 connected\n' + - 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca 10.0.146.93:7379@16379,node-2.redis.example.com,shard-id=abc123 myself,master - 0 0 1 connected 0-16383'; - export const mockStandaloneRedisInfoReply: string = `${ mockRedisServerInfoResponse }\r\n${mockRedisClientsInfoResponse}\r\n${mockRedisMemoryInfoResponse}\r\n${ diff --git a/redisinsight/api/src/models/redis-cluster.ts b/redisinsight/api/src/models/redis-cluster.ts index 10a06f813a..7582cf433d 100644 --- a/redisinsight/api/src/models/redis-cluster.ts +++ b/redisinsight/api/src/models/redis-cluster.ts @@ -15,18 +15,3 @@ export interface IRedisClusterNodeAddress { host: string; port: number; } - -export interface IRedisClusterNode extends IRedisClusterNodeAddress { - id: string; - replicaOf: string; - linkState: RedisClusterNodeLinkState; - slot: string; - // Client-facing hostname announced by the node (Redis 7+, `cluster-announce-hostname`). - // Undefined when the node has no hostname configured. - hostname?: string; -} - -export enum RedisClusterNodeLinkState { - Connected = 'connected', - Disconnected = 'disconnected', -} diff --git a/redisinsight/api/src/modules/cluster-monitor/strategies/__tests__/cluster-shards.factory.ts b/redisinsight/api/src/modules/cluster-monitor/strategies/__tests__/cluster-shards.factory.ts index a019eff88e..28a1a2f139 100644 --- a/redisinsight/api/src/modules/cluster-monitor/strategies/__tests__/cluster-shards.factory.ts +++ b/redisinsight/api/src/modules/cluster-monitor/strategies/__tests__/cluster-shards.factory.ts @@ -34,17 +34,24 @@ export const toNodeReplyArray = (obj: Record): any[] => { }; export const clusterShardNodeRawFactory = Factory.define( - () => ({ - id: faker.string.hexadecimal({ length: 40, prefix: '' }), - port: faker.number.int({ min: 6379, max: 6400 }), - ip: faker.internet.ipv4(), - endpoint: faker.internet.ipv4(), - hostname: '', - role: 'master', - 'replication-offset': faker.number.int({ min: 0, max: 200000 }), - health: faker.helpers.arrayElement(Object.values(HealthStatus)), - 'tls-port': undefined, - }), + () => { + const ip = faker.internet.ipv4(); + + return { + id: faker.string.hexadecimal({ length: 40, prefix: '' }), + port: faker.number.int({ min: 6379, max: 6400 }), + ip, + // `cluster-preferred-endpoint-type ip` is Redis's own default, so the + // preferred endpoint equals the ip unless a test explicitly overrides + // it to simulate a `hostname` or unknown-endpoint configuration. + endpoint: ip, + hostname: '', + role: 'master', + 'replication-offset': faker.number.int({ min: 0, max: 200000 }), + health: faker.helpers.arrayElement(Object.values(HealthStatus)), + 'tls-port': undefined, + }; + }, ); export const clusterShardRawFactory = Factory.define( diff --git a/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.spec.ts b/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.spec.ts index 8e674e5d36..bd9a9a1b3b 100644 --- a/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.spec.ts +++ b/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.spec.ts @@ -26,7 +26,7 @@ describe('ClusterShardsInfoStrategy', () => { }); describe('getClusterNodesFromRedis', () => { - it('should return cluster info with ip when hostname is empty', async () => { + it('should use the ip when it is the preferred endpoint (cluster-preferred-endpoint-type ip, the default)', async () => { const node1 = clusterShardNodeRawFactory.build(); const node2 = clusterShardNodeRawFactory.build(); const shard1 = clusterShardRawFactory.build( @@ -51,18 +51,21 @@ describe('ClusterShardsInfoStrategy', () => { expect(info[1].slots).toEqual([formatSlotRange(5001, 16383)]); }); - it('should use hostname when cluster-announce-hostname is configured', async () => { + it('should use the endpoint when the preferred endpoint type is hostname', async () => { const hostname1 = 'redis-node-1.example.com'; const hostname2 = 'redis-node-2.example.com'; const master1 = clusterShardNodeRawFactory.build({ hostname: hostname1, + endpoint: hostname1, }); const replica1 = clusterShardNodeRawFactory.build({ hostname: hostname1, + endpoint: hostname1, role: 'slave', }); const master2 = clusterShardNodeRawFactory.build({ hostname: hostname2, + endpoint: hostname2, }); const shard1 = clusterShardRawFactory.build( @@ -89,10 +92,31 @@ describe('ClusterShardsInfoStrategy', () => { expect(info[1].host).toBe(hostname2); }); + it('should use the ip, not an announced hostname, when the preferred endpoint type is ip', async () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3546234089: + // a node may announce a hostname purely as metadata while + // cluster-preferred-endpoint-type still resolves to ip. + const node = clusterShardNodeRawFactory.build({ + hostname: 'redis-node-1.example.com', + }); + const shard = clusterShardRawFactory.build( + {}, + { transient: { slotStart: 0, slotEnd: 16383, nodes: [node] } }, + ); + const reply = [shard].map(toClusterShardsReplyEntry); + when(clusterClient.sendCommand).mockResolvedValue(reply); + + const info = await service.getClusterNodesFromRedis(clusterClient); + + expect(info).toHaveLength(1); + expect(info[0].host).toBe(node.ip); + }); + it('should use tls-port when regular port is not available', async () => { const tlsPort = 6380; const node = clusterShardNodeRawFactory.build({ hostname: 'redis-tls.example.com', + endpoint: 'redis-tls.example.com', port: undefined, 'tls-port': tlsPort, }); @@ -114,7 +138,7 @@ describe('ClusterShardsInfoStrategy', () => { describe('processShardNodes', () => { const slots = ['0-5000']; - it('should use ip as host when hostname is empty string', () => { + it('should use ip as host when it is the preferred endpoint (default)', () => { const raw = clusterShardNodeRawFactory.build(); const result = ClusterShardsInfoStrategy.processShardNodes( @@ -125,9 +149,12 @@ describe('ClusterShardsInfoStrategy', () => { expect(result[0]).toHaveProperty('ip', raw.ip); }); - it('should prefer hostname over ip when hostname is set', () => { + it('should use the endpoint when the preferred endpoint type is hostname', () => { const hostname = 'redis.example.com'; - const raw = clusterShardNodeRawFactory.build({ hostname }); + const raw = clusterShardNodeRawFactory.build({ + hostname, + endpoint: hostname, + }); const result = ClusterShardsInfoStrategy.processShardNodes( [toNodeReplyArray(raw)], @@ -137,7 +164,19 @@ describe('ClusterShardsInfoStrategy', () => { expect(result[0]).toHaveProperty('ip', raw.ip); }); - it('should fall back to ip when hostname is not present in reply', () => { + it('should use ip, not an announced hostname, when the preferred endpoint type is ip', () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3546234089 + const hostname = 'redis.example.com'; + const raw = clusterShardNodeRawFactory.build({ hostname }); + + const result = ClusterShardsInfoStrategy.processShardNodes( + [toNodeReplyArray(raw)], + slots, + ); + expect(result[0].host).toBe(raw.ip); + }); + + it('should fall back to ip when endpoint is not present in reply', () => { const ip = '10.0.0.99'; const shardNodes = [ [ @@ -162,6 +201,51 @@ describe('ClusterShardsInfoStrategy', () => { expect(result[0]).toHaveProperty('ip', ip); }); + it('should fall back to ip when endpoint is an empty string', () => { + const raw = clusterShardNodeRawFactory.build({ endpoint: '' }); + + const result = ClusterShardsInfoStrategy.processShardNodes( + [toNodeReplyArray(raw)], + slots, + ); + expect(result[0].host).toBe(raw.ip); + }); + + it('should use fallbackHost when endpoint is null (unknown endpoint) and no ip is present', () => { + const shardNodes = [ + [ + 'id', + 'n1', + 'port', + 6379, + 'endpoint', + null, + 'role', + 'master', + 'health', + 'online', + ], + ]; + + const result = ClusterShardsInfoStrategy.processShardNodes( + shardNodes, + slots, + '203.0.113.10', + ); + expect(result[0].host).toBe('203.0.113.10'); + }); + + it('should fall back to ip when endpoint is the "?" misconfigured marker', () => { + const raw = clusterShardNodeRawFactory.build({ endpoint: '?' }); + + const result = ClusterShardsInfoStrategy.processShardNodes( + [toNodeReplyArray(raw)], + slots, + '203.0.113.10', // must be ignored - '?' is not necessarily this node + ); + expect(result[0].host).toBe(raw.ip); + }); + it('should use tls-port when port is missing', () => { const tlsPort = 6380; const raw = clusterShardNodeRawFactory.build({ @@ -230,11 +314,13 @@ describe('ClusterShardsInfoStrategy', () => { const tlsPort = 6390; const primary = clusterShardNodeRawFactory.build({ hostname, + endpoint: hostname, port: undefined, 'tls-port': tlsPort, }); const replica = clusterShardNodeRawFactory.build({ hostname, + endpoint: hostname, port: undefined, 'tls-port': tlsPort + 1, role: 'slave', diff --git a/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.ts b/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.ts index b73deeb714..0179023990 100644 --- a/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.ts +++ b/redisinsight/api/src/modules/cluster-monitor/strategies/cluster-shards.info.strategy.ts @@ -4,7 +4,10 @@ import { ClusterNodeDetails, NodeRole, } from 'src/modules/cluster-monitor/models'; -import { convertArrayReplyToObject } from 'src/modules/redis/utils'; +import { + convertArrayReplyToObject, + resolvePreferredEndpoint, +} from 'src/modules/redis/utils'; import { RedisClient } from 'src/modules/redis/client'; export class ClusterShardsInfoStrategy extends AbstractInfoStrategy { @@ -17,7 +20,11 @@ export class ClusterShardsInfoStrategy extends AbstractInfoStrategy { ...resp.map((shardArray) => { const shard = convertArrayReplyToObject(shardArray); const slots = ClusterShardsInfoStrategy.calculateSlots(shard.slots); - return ClusterShardsInfoStrategy.processShardNodes(shard.nodes, slots); + return ClusterShardsInfoStrategy.processShardNodes( + shard.nodes, + slots, + client.options?.host, + ); }), ); } @@ -35,13 +42,21 @@ export class ClusterShardsInfoStrategy extends AbstractInfoStrategy { static processShardNodes( shardNodes: any[], slots: string[], + fallbackHost?: string, ): Partial[] { let primary; const nodes = shardNodes.map((nodeArray) => { const nodeObj = convertArrayReplyToObject(nodeArray); const node = { id: nodeObj.id, - host: nodeObj.hostname || nodeObj.ip, + // `endpoint` is CLUSTER SHARDS' server-resolved preferred address + // (respects `cluster-preferred-endpoint-type`) - `hostname` alone is + // only supplementary metadata and must not drive this on its own. + // Fall back to `ip` for an unknown/misconfigured endpoint since this + // is a display-only value, not a connection target. + host: + resolvePreferredEndpoint(nodeObj.endpoint, fallbackHost) || + nodeObj.ip, ip: nodeObj.ip, port: nodeObj.port || nodeObj['tls-port'], tlsPort: nodeObj['tls-port'], diff --git a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts index eb11732198..076cbe6439 100644 --- a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts @@ -1,11 +1,11 @@ import { mockRedisClusterFailInfoResponse, - mockRedisClusterNodesResponse, - mockRedisClusterNodesResponseWithHostname, mockRedisClusterOkInfoResponse, mockStandaloneRedisClient, + generateMockRedisClient, } from 'src/__mocks__'; import { IRedisClusterNodeAddress, ReplyError } from 'src/models'; +import { RedisClusterSlotsReply } from 'src/modules/redis/utils/reply.util'; import { isCluster, discoverClusterNodes } from './cluster.util'; describe('isCluster', () => { @@ -33,25 +33,22 @@ describe('isCluster', () => { }); describe('discoverClusterNodes', () => { - const mockClusterNodeAddresses: IRedisClusterNodeAddress[] = [ - { - host: '127.0.0.1', - port: 30004, - }, - { - host: '127.0.0.1', - port: 30001, - }, - ]; + it('should return nodes in a defined format, using the ip when it is the preferred endpoint', async () => { + const slots: RedisClusterSlotsReply = [ + [0, 5460, ['127.0.0.1', 30004, 'node-1']], + [5461, 16383, ['127.0.0.1', 30001, 'node-2']], + ]; + mockStandaloneRedisClient.sendCommand.mockResolvedValue(slots); - it('should return nodes in a defined format', async () => { - mockStandaloneRedisClient.sendCommand.mockResolvedValue( - mockRedisClusterNodesResponse, - ); + const expected: IRedisClusterNodeAddress[] = [ + { host: '127.0.0.1', port: 30004 }, + { host: '127.0.0.1', port: 30001 }, + ]; expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual( - mockClusterNodeAddresses, + expected, ); }); + it('cluster not supported', async () => { const replyError: ReplyError = { name: 'ReplyError', @@ -68,25 +65,66 @@ describe('discoverClusterNodes', () => { } }); - it('should use the announced hostname instead of the raw ip when present (Redis 7+)', async () => { + it('should use the announced hostname as root node address when it is the resolved preferred endpoint', async () => { // Reproduces https://github.com/redis/RedisInsight/issues/5393, // https://github.com/redis/RedisInsight/issues/3416 and // https://github.com/redis/RedisInsight/issues/3429: nodes behind // per-node load balancers / NAT announce a client-facing hostname - // because their raw ip is not routable to RedisInsight. - mockStandaloneRedisClient.sendCommand.mockResolvedValue( - mockRedisClusterNodesResponseWithHostname, + // because their raw ip is not routable to RedisInsight, and the + // server resolves `cluster-preferred-endpoint-type hostname` into + // CLUSTER SLOTS' preferred-endpoint field. + const slots: RedisClusterSlotsReply = [ + [0, 16383, ['node-1.redis.example.com', 7379, 'node-1']], + ]; + mockStandaloneRedisClient.sendCommand.mockResolvedValue(slots); + + expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual([ + { host: 'node-1.redis.example.com', port: 7379 }, + ]); + }); + + it('should use the ip, not an announced hostname, when the preferred endpoint type is ip', async () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3546234089: + // a node can announce a hostname as metadata while + // `cluster-preferred-endpoint-type` still resolves to `ip` - the raw ip + // must be used in that case even though a hostname exists. + const slots: RedisClusterSlotsReply = [ + [0, 16383, ['10.0.161.40', 7379, 'node-1']], + ]; + mockStandaloneRedisClient.sendCommand.mockResolvedValue(slots); + + expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual([ + { host: '10.0.161.40', port: 7379 }, + ]); + }); + + it('should fall back to the host used to connect when a node has an unknown (null) endpoint', async () => { + const client = generateMockRedisClient( + { databaseId: 'unknown-endpoint-test' }, + undefined, + { host: '203.0.113.10', port: 6379 }, ); + const slots: RedisClusterSlotsReply = [[0, 16383, [null, 6379, 'node-1']]]; + client.sendCommand = jest.fn().mockResolvedValue(slots); + + expect(await discoverClusterNodes(client)).toEqual([ + { host: '203.0.113.10', port: 6379 }, + ]); + }); + + it('should skip a node whose endpoint is the "?" misconfigured marker', async () => { + const slots: RedisClusterSlotsReply = [ + [ + 0, + 16383, + ['?', 6379, 'node-1'], + ['node-2.redis.example.com', 6380, 'node-2'], + ], + ]; + mockStandaloneRedisClient.sendCommand.mockResolvedValue(slots); expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual([ - { - host: 'node-1.redis.example.com', - port: 7379, - }, - { - host: 'node-2.redis.example.com', - port: 7379, - }, + { host: 'node-2.redis.example.com', port: 6380 }, ]); }); }); diff --git a/redisinsight/api/src/modules/redis/utils/cluster.util.ts b/redisinsight/api/src/modules/redis/utils/cluster.util.ts index c30ed94106..750c81d976 100644 --- a/redisinsight/api/src/modules/redis/utils/cluster.util.ts +++ b/redisinsight/api/src/modules/redis/utils/cluster.util.ts @@ -1,12 +1,10 @@ import { RedisClient } from 'src/modules/redis/client'; import { convertMultilineReplyToObject, - parseNodesFromClusterInfoReply, + parseNodesFromClusterSlotsReply, + RedisClusterSlotsReply, } from 'src/modules/redis/utils/reply.util'; -import { - IRedisClusterNodeAddress, - RedisClusterNodeLinkState, -} from 'src/models'; +import { IRedisClusterNodeAddress } from 'src/models'; /** * Check weather database is a cluster @@ -30,25 +28,25 @@ export const isCluster = async (client: RedisClient): Promise => { }; /** - * Discover all cluster nodes for current connection - * Prefers each node's announced hostname (Redis 7+, `cluster-announce-hostname`) - * over its raw IP so that clusters behind per-node load balancers or NAT - * (where the raw IP is not routable to clients) remain reachable. Falls back - * to the IP when no hostname is announced, preserving behavior for standard - * clusters. + * Discover all cluster nodes for current connection. + * + * Uses "CLUSTER SLOTS" rather than "CLUSTER NODES": each node's preferred + * connection address there is already resolved server-side according to + * the `cluster-preferred-endpoint-type` config (ip / hostname / + * unknown-endpoint), so clusters behind per-node load balancers or NAT + * (where the raw ip is not routable to clients) remain reachable without + * the client re-deriving an ip-vs-hostname preference itself - which + * "CLUSTER NODES" has no way to express correctly, since it only exposes + * the announced hostname as unconditional metadata, not the server's + * actual preference. * @param client */ export const discoverClusterNodes = async ( client: RedisClient, ): Promise => { - const nodes = parseNodesFromClusterInfoReply( - (await client.sendCommand(['cluster', 'nodes'], { - replyEncoding: 'utf8', - })) as string, - ).filter((node) => node.linkState === RedisClusterNodeLinkState.Connected); + const slots = (await client.sendCommand(['cluster', 'slots'], { + replyEncoding: 'utf8', + })) as RedisClusterSlotsReply; - return nodes.map((node) => ({ - host: node.hostname || node.host, - port: node.port, - })); + return parseNodesFromClusterSlotsReply(slots, client.options?.host); }; diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts index 6c732ccbeb..b3a94c32aa 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts @@ -1,15 +1,12 @@ -import { - mockRedisClusterNodesResponse, - mockRedisClusterNodesResponseIPv6, - mockRedisClusterNodesResponseWithHostname, - mockRedisServerInfoResponse, -} from 'src/__mocks__'; +import { mockRedisServerInfoResponse } from 'src/__mocks__'; import { flatMap } from 'lodash'; -import { IRedisClusterNode, RedisClusterNodeLinkState } from 'src/models'; import { convertArrayReplyToObject, convertMultilineReplyToObject, - parseNodesFromClusterInfoReply, + parseNodesFromClusterSlotsReply, + RedisClusterSlotsReply, + resolvePreferredEndpoint, + UNKNOWN_ENDPOINT_MARKER, } from './reply.util'; const mockRedisServerInfo = { @@ -21,45 +18,6 @@ const mockRedisServerInfo = { uptime_in_seconds: '1000', }; -const mockRedisClusterNodes: IRedisClusterNode[] = [ - { - id: '07c37dfeb235213a872192d90877d0cd55635b91', - host: '127.0.0.1', - port: 30004, - replicaOf: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - linkState: RedisClusterNodeLinkState.Connected, - slot: undefined, - }, - { - id: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - host: '127.0.0.1', - port: 30001, - replicaOf: undefined, - linkState: RedisClusterNodeLinkState.Connected, - slot: '0-16383', - }, -]; - -// IPv6 expected results -const mockRedisClusterNodesIPv6: IRedisClusterNode[] = [ - { - id: '07c37dfeb235213a872192d90877d0cd55635b91', - host: '2001:db8::1', - port: 7001, - replicaOf: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - linkState: RedisClusterNodeLinkState.Connected, - slot: undefined, - }, - { - id: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - host: '2001:db8::2', - port: 7002, - replicaOf: undefined, - linkState: RedisClusterNodeLinkState.Connected, - slot: '0-16383', - }, -]; - const mockIncorrectString = '$6\r\nfoobar\r\n'; describe('convertArrayReplyToObject', () => { @@ -95,58 +53,136 @@ describe('convertMultilineReplyToObject', () => { }); }); -describe('parseNodesFromClusterInfoReply', () => { - it('should return array object in a defined format', async () => { - const result = parseNodesFromClusterInfoReply( - mockRedisClusterNodesResponse, +describe('resolvePreferredEndpoint', () => { + it('should use the preferred endpoint as-is when it is a normal ip', () => { + expect(resolvePreferredEndpoint('172.31.100.211', 'fallback')).toEqual( + '172.31.100.211', ); + }); + it('should use the preferred endpoint as-is when it is a normal hostname', () => { + expect( + resolvePreferredEndpoint('node-1.redis.example.com', 'fallback'), + ).toEqual('node-1.redis.example.com'); + }); + it('should fall back to the command host when endpoint is null (unknown endpoint)', () => { + expect(resolvePreferredEndpoint(null, '10.0.0.1')).toEqual('10.0.0.1'); + }); + it('should fall back to the command host when endpoint is an empty string', () => { + expect(resolvePreferredEndpoint('', '10.0.0.1')).toEqual('10.0.0.1'); + }); + it('should return undefined when endpoint is null and there is no fallback host', () => { + expect(resolvePreferredEndpoint(null, undefined)).toBeUndefined(); + }); + it('should return undefined for the "?" misconfigured-node marker, ignoring the fallback host', () => { + expect( + resolvePreferredEndpoint(UNKNOWN_ENDPOINT_MARKER, '10.0.0.1'), + ).toBeUndefined(); + }); +}); + +describe('parseNodesFromClusterSlotsReply', () => { + it('should use the ip as-is when cluster-preferred-endpoint-type is ip (default)', () => { + const slots: RedisClusterSlotsReply = [ + [0, 5460, ['172.31.100.211', 6379, 'node-1']], + [5461, 10922, ['172.31.100.212', 6379, 'node-2']], + ]; - expect(result).toEqual(mockRedisClusterNodes); + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: '172.31.100.211', port: 6379 }, + { host: '172.31.100.212', port: 6379 }, + ]); }); - it('should return empty array when incorrect string passed', async () => { - const result = parseNodesFromClusterInfoReply(mockIncorrectString); - expect(result).toEqual([]); + it('should use the announced hostname when it is the resolved preferred endpoint', () => { + const slots: RedisClusterSlotsReply = [ + [0, 16383, ['node-1.redis.example.com', 7379, 'node-1']], + ]; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: 'node-1.redis.example.com', port: 7379 }, + ]); }); - it('should parse IPv6 addresses correctly', async () => { - const result = parseNodesFromClusterInfoReply( - mockRedisClusterNodesResponseIPv6, - ); - expect(result).toEqual(mockRedisClusterNodesIPv6); + it('should use the ip even when the node also announces a hostname, when the preferred endpoint is the ip', () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3546234089: + // `cluster-announce-hostname` alone does not mean hostname is preferred - + // `cluster-preferred-endpoint-type` decides that, and CLUSTER SLOTS' + // first field already reflects the resolved decision. A node can + // announce a hostname purely as metadata while the preferred endpoint + // (what we must actually use) stays the ip. + const slots: RedisClusterSlotsReply = [ + [0, 16383, ['10.0.161.40', 7379, 'node-1']], + ]; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: '10.0.161.40', port: 7379 }, + ]); }); - it('should parse announced hostname (Redis 7+) alongside the ip, ignoring trailing aux fields', async () => { - const result = parseNodesFromClusterInfoReply( - mockRedisClusterNodesResponseWithHostname, - ); - expect(result).toEqual([ - { - id: '07c37dfeb235213a872192d90877d0cd55635b91', - host: '10.0.161.40', - hostname: 'node-1.redis.example.com', - port: 7379, - replicaOf: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - linkState: RedisClusterNodeLinkState.Connected, - slot: undefined, - }, - { - id: 'e7d1eecce10fd6bb5eb35b9f99a514335d9ba9ca', - host: '10.0.146.93', - // hostname must still be parsed correctly when followed by aux fields (e.g. shard-id) - hostname: 'node-2.redis.example.com', - port: 7379, - replicaOf: undefined, - linkState: RedisClusterNodeLinkState.Connected, - slot: '0-16383', - }, + it('should fall back to the command host for an unknown (null) endpoint', () => { + const slots: RedisClusterSlotsReply = [[0, 16383, [null, 6379, 'node-1']]]; + + expect(parseNodesFromClusterSlotsReply(slots, '203.0.113.10')).toEqual([ + { host: '203.0.113.10', port: 6379 }, + ]); + }); + + it('should fall back to the command host for an unknown (empty string) endpoint', () => { + const slots: RedisClusterSlotsReply = [[0, 16383, ['', 6379, 'node-1']]]; + + expect(parseNodesFromClusterSlotsReply(slots, '203.0.113.10')).toEqual([ + { host: '203.0.113.10', port: 6379 }, + ]); + }); + + it('should skip a node whose endpoint is the "?" misconfigured marker', () => { + const slots: RedisClusterSlotsReply = [ + [ + 0, + 16383, + [UNKNOWN_ENDPOINT_MARKER, 6379, 'node-1'], + ['node-2.redis.example.com', 6380, 'node-2'], + ], + ]; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: 'node-2.redis.example.com', port: 6380 }, + ]); + }); + + it('should deduplicate the same node id across non-contiguous slot ranges', () => { + const slots: RedisClusterSlotsReply = [ + [0, 400, ['node-1.redis.example.com', 7379, 'node-1']], + [900, 900, ['node-1.redis.example.com', 7379, 'node-1']], + [1800, 6000, ['node-1.redis.example.com', 7379, 'node-1']], + ]; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: 'node-1.redis.example.com', port: 7379 }, + ]); + }); + + it('should include replica nodes in addition to the master for each slot range', () => { + const slots: RedisClusterSlotsReply = [ + [ + 0, + 16383, + ['node-1.redis.example.com', 7379, 'master-1'], + ['node-2.redis.example.com', 7380, 'replica-1'], + ], + ]; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: 'node-1.redis.example.com', port: 7379 }, + { host: 'node-2.redis.example.com', port: 7380 }, ]); }); - it('should leave hostname undefined when the node has none announced', async () => { - const result = parseNodesFromClusterInfoReply( - mockRedisClusterNodesResponse, - ); - expect(result.every((node) => node.hostname === undefined)).toEqual(true); + it('should return empty array in case of an error', () => { + expect( + parseNodesFromClusterSlotsReply( + null as unknown as RedisClusterSlotsReply, + ), + ).toEqual([]); }); }); diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.ts b/redisinsight/api/src/modules/redis/utils/reply.util.ts index c09ae503df..974b34e3dd 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.ts @@ -1,5 +1,5 @@ import { chunk, isArray } from 'lodash'; -import { IRedisClusterNode } from 'src/models'; +import { IRedisClusterNodeAddress } from 'src/models'; /** * Converts array of strings to object when each even element is a key and odd is a value @@ -83,76 +83,119 @@ export const convertMultilineReplyToObject = ( }; /** - * Parse and return all endpoints from the nodes list returned by "cluster info" command - * Since Redis 7.0 the endpoint field may also carry a client-facing hostname - * (`ip:port@cport[,hostname[,aux_field=aux_value]*]`), announced via - * `cluster-announce-hostname`. Managed/hosted clusters commonly sit behind - * per-node load balancers or NAT where the raw IP is not routable to clients, - * so the hostname (when present) is parsed out separately and should be - * preferred over the raw IP when establishing connections. - * @Input + * The `?` endpoint marker Redis returns for a misconfigured node (preferred + * endpoint type is `hostname` but no `cluster-announce-hostname` is set). + * Per the spec this must NOT be treated the same as an unknown ("connect to + * the same host used to send the command") endpoint - the node may not be + * the one that served the command at all. + * See https://redis.io/docs/latest/commands/cluster-slots/ and + * https://redis.io/docs/latest/commands/cluster-shards/ + */ +export const UNKNOWN_ENDPOINT_MARKER = '?'; + +/** + * Resolve the address to use for a node from Redis's own "preferred + * endpoint" (`CLUSTER SLOTS` / `CLUSTER SHARDS`), which is already resolved + * server-side according to the `cluster-preferred-endpoint-type` config + * (`ip` | `hostname` | `unknown-endpoint`). Clients must use this value + * as-is rather than re-deriving an ip-vs-hostname preference themselves, + * since a node may announce a hostname purely as metadata while the server + * is actually configured to prefer the ip (or vice versa). + * + * Handles the endpoint field's documented abnormal values: + * - `null` / `''`: unknown endpoint - resolves to `fallbackHost` (the host + * used to send the command). + * - `'?'`: misconfigured node - the spec explicitly warns this may not be + * the same node that served the command, so `undefined` is returned + * instead of guessing; callers decide how to handle an unresolvable node. + * @param endpoint + * @param fallbackHost + */ +export const resolvePreferredEndpoint = ( + endpoint: string | null | undefined, + fallbackHost?: string, +): string | undefined => { + if (endpoint === UNKNOWN_ENDPOINT_MARKER) { + return undefined; + } + if (!endpoint) { + return fallbackHost || undefined; + } + return endpoint; +}; + +/** + * A single node entry within a "CLUSTER SLOTS" slot range: + * `[preferredEndpoint, port, nodeId, metadata?]`. + */ +export type RedisClusterSlotsNode = [string | null, number, string, unknown?]; + +/** + * Raw "CLUSTER SLOTS" reply shape once decoded from RESP into JS values: + * an array of slot ranges, each `[startSlot, endSlot, ...nodes]`. + * See https://redis.io/docs/latest/commands/cluster-slots/ + */ +export type RedisClusterSlotsReply = Array< + [number, number, ...RedisClusterSlotsNode[]] +>; + +/** + * Parse the reply of "CLUSTER SLOTS" into a deduplicated list of node + * addresses (one entry per unique node id across all slot ranges), using + * each node's server-resolved preferred endpoint (see + * `resolvePreferredEndpoint`) rather than any raw ip/hostname field. + * + * CLUSTER SLOTS is used over the newer CLUSTER SHARDS here for broader + * compatibility - it has been available since Redis 3.0.0, while CLUSTER + * SHARDS requires 7.0+. + * + * @Input (already parsed into nested arrays, see `RedisClusterSlotsReply`) * ``` - * 08418e3514990489e48fa05d642efc33e205f5 172.31.100.211:6379@16379 myself,master - 0 1698694904000 1 connected 0-5460 - * d2dee846c715a917ec9a4963e8885b06130f9f 172.31.100.212:6379@16379 master - 0 1698694905285 2 connected 5461-10922 - * 3e92457ab813ad7a62dacf768ec7309210feaf [2001:db8::1]:7001@17001 master - 0 1698694906000 3 connected 10923-16383 - * 41ae438fc3ba52a13f40ed67841d463f8bd1ec 10.0.161.40:7379@16379,node-1.example.com master - 0 1698694907000 4 connected 10923-16383 + * [ + * [0, 5460, ["10.0.161.40", 7379, "07c37dfe...", []], ["10.0.146.93", 7379, "e7d1eecc...", []]], + * ... + * ] * ``` * @Output * ``` * [ - * { - * host: "172.31.100.211", - * port: 6379 - * }, - * { - * host: "172.31.100.212", - * port: 6379 - * }, - * { - * host: "2001:db8::1", - * port: 7001 - * }, - * { - * host: "10.0.161.40", - * hostname: "node-1.example.com", - * port: 7379 - * } + * { host: "10.0.161.40", port: 7379 }, + * { host: "10.0.146.93", port: 7379 } * ] * ``` - * @param info + * @param slots + * @param fallbackHost host used to send the "CLUSTER SLOTS" command, used + * to resolve nodes with an unknown (`null`/`''`) preferred endpoint */ -export const parseNodesFromClusterInfoReply = ( - info: string, -): IRedisClusterNode[] => { +export const parseNodesFromClusterSlotsReply = ( + slots: RedisClusterSlotsReply, + fallbackHost?: string, +): IRedisClusterNodeAddress[] => { try { - const lines = info.split('\n'); - const nodes = []; - lines.forEach((line: string) => { - if (line && line.split) { - // fields = [id, endpoint, flags, master, pingSent, pongRecv, configEpoch, linkState, slot] - const fields = line.split(' '); - const [id, endpoint, , master, , , , linkState, slot] = fields; + const nodeById = new Map(); - // endpoint = "ip:port@cport[,hostname[,aux_field=aux_value]*]" - const [ipPortCport, hostname] = endpoint.split(','); + slots.forEach((slotRange) => { + // slotRange = [startSlot, endSlot, master, ...replicas] + for (let i = 2; i < slotRange.length; i++) { + const [endpoint, port, id] = slotRange[ + i + ] as unknown as RedisClusterSlotsNode; + if (!id || nodeById.has(id)) { + continue; + } - const hostAndPort = ipPortCport.split('@')[0]; - const lastColonIndex = hostAndPort.lastIndexOf(':'); + const host = resolvePreferredEndpoint(endpoint, fallbackHost); + if (!host) { + // '?' (misconfigured node) - spec says this may not be the same + // node used to send the command, so don't guess an address. + continue; + } - const host = hostAndPort.substring(0, lastColonIndex); - const port = hostAndPort.substring(lastColonIndex + 1); - nodes.push({ - id, - host, - hostname: hostname || undefined, - port: parseInt(port, 10), - replicaOf: master !== '-' ? master : undefined, - linkState, - slot, - }); + nodeById.set(id, { host, port }); } }); - return nodes; + + return [...nodeById.values()]; } catch (e) { return []; } From 6319f30fced69c486ac930410f88f810f1821583 Mon Sep 17 00:00:00 2001 From: MickeyShnaiderman-RecoLabs Date: Thu, 9 Jul 2026 14:11:15 +0300 Subject: [PATCH 3/3] fix(cluster-discovery): support CLUSTER SLOTS replies without node id Addresses review feedback on #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 Co-authored-by: Cursor --- .../modules/redis/utils/cluster.util.spec.ts | 14 ++++++++++ .../modules/redis/utils/reply.util.spec.ts | 28 +++++++++++++++++++ .../api/src/modules/redis/utils/reply.util.ts | 25 +++++++++++------ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts index 076cbe6439..e280826340 100644 --- a/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/cluster.util.spec.ts @@ -112,6 +112,20 @@ describe('discoverClusterNodes', () => { ]); }); + it('should still discover nodes from a pre-4.0.0 cluster reply with no node id', async () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3551046293 + const slots = [ + [0, 5460, ['127.0.0.1', 30001]], + [5461, 16383, ['127.0.0.1', 30002]], + ] as unknown as RedisClusterSlotsReply; + mockStandaloneRedisClient.sendCommand.mockResolvedValue(slots); + + expect(await discoverClusterNodes(mockStandaloneRedisClient)).toEqual([ + { host: '127.0.0.1', port: 30001 }, + { host: '127.0.0.1', port: 30002 }, + ]); + }); + it('should skip a node whose endpoint is the "?" misconfigured marker', async () => { const slots: RedisClusterSlotsReply = [ [ diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts index b3a94c32aa..14716ee9ad 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.spec.ts @@ -162,6 +162,34 @@ describe('parseNodesFromClusterSlotsReply', () => { ]); }); + it('should still discover nodes from a pre-4.0.0 cluster reply with no node id ([ip, port] only)', () => { + // Regression test for https://github.com/redis/RedisInsight/pull/6180#discussion_r3551046293: + // node ids were only added to CLUSTER SLOTS in Redis 4.0.0 - older + // clusters return just [ip, port] per node, so `id` is undefined and + // must not cause every node to be dropped. + const slots = [ + [0, 5460, ['127.0.0.1', 30001]], + [5461, 10922, ['127.0.0.1', 30002]], + ] as unknown as RedisClusterSlotsReply; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: '127.0.0.1', port: 30001 }, + { host: '127.0.0.1', port: 30002 }, + ]); + }); + + it('should deduplicate pre-4.0.0 nodes without an id by host:port across non-contiguous slot ranges', () => { + const slots = [ + [0, 400, ['127.0.0.1', 30001]], + [900, 900, ['127.0.0.1', 30001]], + [1800, 6000, ['127.0.0.1', 30001]], + ] as unknown as RedisClusterSlotsReply; + + expect(parseNodesFromClusterSlotsReply(slots)).toEqual([ + { host: '127.0.0.1', port: 30001 }, + ]); + }); + it('should include replica nodes in addition to the master for each slot range', () => { const slots: RedisClusterSlotsReply = [ [ diff --git a/redisinsight/api/src/modules/redis/utils/reply.util.ts b/redisinsight/api/src/modules/redis/utils/reply.util.ts index 974b34e3dd..33ca9880b4 100644 --- a/redisinsight/api/src/modules/redis/utils/reply.util.ts +++ b/redisinsight/api/src/modules/redis/utils/reply.util.ts @@ -126,9 +126,12 @@ export const resolvePreferredEndpoint = ( /** * A single node entry within a "CLUSTER SLOTS" slot range: - * `[preferredEndpoint, port, nodeId, metadata?]`. + * `[preferredEndpoint, port, nodeId?, metadata?]`. `nodeId` was only added + * 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]`. */ -export type RedisClusterSlotsNode = [string | null, number, string, unknown?]; +export type RedisClusterSlotsNode = [string | null, number, string?, unknown?]; /** * Raw "CLUSTER SLOTS" reply shape once decoded from RESP into JS values: @@ -141,8 +144,9 @@ export type RedisClusterSlotsReply = Array< /** * Parse the reply of "CLUSTER SLOTS" into a deduplicated list of node - * addresses (one entry per unique node id across all slot ranges), using - * each node's server-resolved preferred endpoint (see + * addresses (one entry per unique node across all slot ranges, keyed by + * node id when present or by "host:port" on pre-4.0.0 clusters that don't + * return one), using each node's server-resolved preferred endpoint (see * `resolvePreferredEndpoint`) rather than any raw ip/hostname field. * * CLUSTER SLOTS is used over the newer CLUSTER SHARDS here for broader @@ -180,9 +184,6 @@ export const parseNodesFromClusterSlotsReply = ( const [endpoint, port, id] = slotRange[ i ] as unknown as RedisClusterSlotsNode; - if (!id || nodeById.has(id)) { - continue; - } const host = resolvePreferredEndpoint(endpoint, fallbackHost); if (!host) { @@ -191,7 +192,15 @@ export const parseNodesFromClusterSlotsReply = ( continue; } - nodeById.set(id, { host, port }); + // Pre-4.0.0 clusters have no node id (just [ip, port]); fall back to + // "host:port" as a stable dedup key so those nodes are still + // discovered instead of being silently dropped. + const dedupeKey = id || `${host}:${port}`; + if (nodeById.has(dedupeKey)) { + continue; + } + + nodeById.set(dedupeKey, { host, port }); } });