diff --git a/docs/Configuration.md b/docs/Configuration.md index ac044e4dc..1df31a2ae 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -422,6 +422,8 @@ The library determines whether to use RESP3 by: - Otherwise: RESP3 is attempted if `defaultVersion` (6.0 unless overridden) is v6 or above Note that `HELLO` is issued either way: `HELLO 2` when staying on RESP2. The reply tells us the server version, replication role, mode (standalone/sentinel/cluster) and connection -identifier, none of which we would otherwise know without `INFO` or `CONFIG GET` - both of which are in the `@dangerous` ACL category, and are commonly restricted. `HELLO` itself is +identifier, none of which we would otherwise know without `INFO` or `CONFIG GET` - both of which are in the `@dangerous` ACL category, and are commonly restricted. The role and mode +are only taken from the interactive connection (the subscription connection does no discovery), and a reported mode of `cluster` only leads to cluster *routing* once a slot map has +been obtained from `CLUSTER SLOTS`/`CLUSTER NODES`; a proxy that announces a cluster but refuses those commands is routed as a single standalone server. `HELLO` itself is in the `@connection` category. If you need to prevent it (for example when talking to a proxy that does not understand it), disable it at the command-map level with `$hello=` in the configuration string - or specify a `defaultVersion` below 6.0, since `HELLO` did not exist before then. diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.cs b/src/StackExchange.Redis/ConnectionMultiplexer.cs index a764e574d..14b2815b0 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.cs @@ -1518,7 +1518,7 @@ public IDatabase GetDatabase(int db = -1, object? asyncState = null) for (int i = 0; i < len; i++) { var server = tmp[(int)(((uint)i + startOffset) % len)]; - if (server != null && server.ServerType == serverType && server.IsSelectable(command, allowDisconnected)) + if (server != null && server.ServerType.IsRoutableAs(serverType) && server.IsSelectable(command, allowDisconnected)) { if (server.IsReplica) { @@ -2046,7 +2046,19 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog } } - if (clusterCount == 0) + // A server *saying* it is a cluster is not enough to route as one: that needs a slot map, and + // proxied deployments can announce "cluster" while refusing every CLUSTER subcommand (AMR with + // the Enterprise clustering policy, #3284). With no map there is nothing to route by and the + // only thing cluster mode would add is rejecting cross-slot commands client-side, which the + // proxy would have accepted. The map is never cleared once populated, so an established + // cluster whose refresh is refused (#3275) keeps routing as one. + bool noSlotMap = ServerSelectionStrategy.IsSlotMapEmpty; + if (clusterCount > 0 && noSlotMap) + { + log?.LogInformationClusterWithoutSlotMap(clusterCount); + } + + if (clusterCount == 0 || noSlotMap) { // Set the serverSelectionStrategy if (RawConfig.Proxy == Proxy.Twemproxy) @@ -2068,7 +2080,7 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog InitializeSentinel(log); } } - else if (standaloneCount > 0) + else if (standaloneCount > 0 || clusterCount > 0) { ServerSelectionStrategy.ServerType = ServerType.Standalone; } @@ -2097,8 +2109,14 @@ internal async Task ReconfigureAsync(bool first, bool reconfigureAll, ILog else { ServerSelectionStrategy.ServerType = ServerType.Cluster; - long coveredSlots = ServerSelectionStrategy.CountCoveredSlots(); - log?.LogInformationClusterSlotsCovered(coveredSlots, ServerSelectionStrategy.TotalSlots); + log?.LogInformationClusterSlotsCovered(ServerSelectionStrategy.CountCoveredSlots(), ServerSelectionStrategy.TotalSlots); + + // a cluster has many primaries by design; if we routed as standalone before the map + // arrived, the nomination above may have marked all but one of them redundant + foreach (var primary in primaries) + { + primary.ClearUnselectable(UnselectableFlags.RedundantPrimary); + } } if (!first) { diff --git a/src/StackExchange.Redis/Enums/ServerType.cs b/src/StackExchange.Redis/Enums/ServerType.cs index e7a84ad0a..d7dcccd7b 100644 --- a/src/StackExchange.Redis/Enums/ServerType.cs +++ b/src/StackExchange.Redis/Enums/ServerType.cs @@ -78,5 +78,16 @@ internal static class ServerTypeExtensions ServerType.Envoyproxy => false, _ => true, }; + + /// + /// Whether a server of this type can be sent commands when the multiplexer is routing as . + /// + /// + /// Usually the types match, and the check keeps data traffic away from sentinels; but a server that calls itself + /// a cluster while giving us no slot map is routed as a single standalone server (see #3284), so it must remain a + /// target in that mode. + /// + internal static bool IsRoutableAs(this ServerType server, ServerType routing) + => server == routing || (routing == ServerType.Standalone && server == ServerType.Cluster); } } diff --git a/src/StackExchange.Redis/LoggerExtensions.cs b/src/StackExchange.Redis/LoggerExtensions.cs index df661e32e..8916f25de 100644 --- a/src/StackExchange.Redis/LoggerExtensions.cs +++ b/src/StackExchange.Redis/LoggerExtensions.cs @@ -280,6 +280,12 @@ internal static void LogWithThreadPoolStats(this ILogger? log, string message) Message = "Cluster: {CoveredSlots} of {TotalSlots} slots covered")] internal static partial void LogInformationClusterSlotsCovered(this ILogger logger, long coveredSlots, int totalSlots); + [LoggerMessage( + Level = LogLevel.Information, + EventId = 133, + Message = "{ClusterCount} server(s) identify as a cluster but no slot map is available; routing as standalone")] + internal static partial void LogInformationClusterWithoutSlotMap(this ILogger logger, int clusterCount); + [LoggerMessage( Level = LogLevel.Information, EventId = 35, diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index cd6992312..e985b10f2 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/src/StackExchange.Redis/ResultProcessor.cs @@ -1157,6 +1157,11 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes // version ("server" => "valkey", "version" => "9.1.2", while INFO still says redis_version:7.2.4), // so we can only decide what to do with "version" once the whole map has been read (#3278). bool serverIsRedis = true; // absent means redis (older servers) + // "mode" and "role" describe the *server*, and only the interactive connection does discovery: + // the subscription connection's handshake is HELLO alone, with no INFO to follow, so a proxy + // whose HELLO disagrees with its INFO (AMR says "cluster" to one and "standalone" to the other, + // #3284) would otherwise have the last word every time pub/sub reconnects on its own + bool helloFromInteractive = connection.BridgeCouldBeNull?.ConnectionType == ConnectionType.Interactive; ProductVariant? helloVariant = null, vendorVariant = null; string? vendorVersion = null; // a snapshot of the "version" value (IsScalar when present), so we can read it as a Version or a @@ -1226,15 +1231,17 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes // note: "mode" and "role" describe a *server*, so we only trust them when we're // talking to one directly; a proxy may answer HELLO from an arbitrary backend node // (envoy 1.39 does), and taking that at face value would flip us into cluster mode - // or mark the proxy as a replica + // or mark the proxy as a replica - and only from the interactive connection (above) case HelloField.Mode - when server.ServerType.SupportsAutoConfigure() + when helloFromInteractive + && server.ServerType.SupportsAutoConfigure() && iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType): server.ServerType = serverType; Log?.LogInformationAutoConfiguredHelloServerType(new(server), serverType); break; case HelloField.Role - when server.ServerType.SupportsAutoConfigure() + when helloFromInteractive + && server.ServerType.SupportsAutoConfigure() && iter.Value.TryParseScalar(&KnownRoleMetadata.TryParse, out bool isReplica): server.IsReplica = isReplica; server.RoleKnownFromHello = true; // so we don't need the key-based fallback probe diff --git a/src/StackExchange.Redis/ServerSelectionStrategy.cs b/src/StackExchange.Redis/ServerSelectionStrategy.cs index 2c79aa804..a3ea96c2d 100644 --- a/src/StackExchange.Redis/ServerSelectionStrategy.cs +++ b/src/StackExchange.Redis/ServerSelectionStrategy.cs @@ -322,6 +322,18 @@ internal bool OwnsAnySlot(ServerEndPoint server) return false; } + /// + /// Whether no slot has an owner, i.e. we hold no cluster topology to route by. + /// + /// + /// Derived from the map rather than tracked separately, which is sound because the map is allocated only by + /// , immediately before a write; the writers never store null and refuse + /// empty ranges (see ); and nothing ever clears an entry - + /// retirement declines to drop a server the map still references (). So a non-null + /// map always covers at least one slot. O(1); is the scan, for logging. + /// + internal bool IsSlotMapEmpty => map is null; + internal int CountCoveredSlots() { var arr = map; @@ -334,6 +346,11 @@ internal int CountCoveredSlots() internal void UpdateClusterRange(int fromInclusive, int toInclusive, ServerEndPoint server) { + // never allocate without writing (see IsSlotMapEmpty): SlotRange does not validate ordering, and the + // reply-driven paths build ranges straight from what the server sent - an inverted or out-of-range + // range is garbage, so ignore it rather than guess at what was meant (or index out of bounds) + if (fromInclusive < 0 || toInclusive >= RedisClusterSlotCount || fromInclusive > toInclusive) return; + var arr = MapForMutation(); for (int i = fromInclusive; i <= toInclusive; i++) { diff --git a/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs b/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs index 1f6e34de5..1932824dc 100644 --- a/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/ClusterCommandsRefusedUnitTests.cs @@ -24,6 +24,9 @@ protected override TypedRedisValue ClusterSlots(RedisClient client, in RedisRequ protected override TypedRedisValue ClusterNodes(RedisClient client, in RedisRequest request) => refuseNodes ? TypedRedisValue.Error("ERR command is not allowed") : base.ClusterNodes(client, request); + + // a proxy that hides the shards accepts cross-slot commands itself + public override bool CheckCrossSlot => false; } [Fact] @@ -71,5 +74,11 @@ public async Task AllClusterCommandsRefusedIsQuiet() // ...and a later explicit reconfigure stays quiet too Assert.True(await conn.ReconfigureAsync("test")); Assert.Empty(logger.Matching("Encountered error while updating cluster config")); + + // with no slot map there is nothing to route by, so we must not *route* as a cluster - in particular, + // cross-slot commands go to the proxy rather than being refused client-side (#3284) + Assert.NotEmpty(logger.Matching("no slot map is available; routing as standalone")); + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + await conn.GetDatabase().StringGetAsync(["a", "b"]); } } diff --git a/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs b/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs new file mode 100644 index 000000000..c5b827969 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/HelloClusterModeUnitTests.cs @@ -0,0 +1,99 @@ +using System; +using System.Threading.Tasks; +using StackExchange.Redis.Server; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// A proxied deployment can answer HELLO with mode: cluster while INFO says +/// redis_mode:standalone and every CLUSTER subcommand is refused (Azure Managed Redis with the +/// Enterprise clustering policy; see #3284). Such a server gives the client no slot map, so it must not be +/// treated as a cluster - in particular, cross-slot commands must keep working, including after a reconnect +/// of the subscription connection alone, whose handshake is HELLO without the INFO that follows +/// on the interactive connection. +/// +public class HelloClusterModeUnitTests(ITestOutputHelper log) +{ + private sealed class HelloSaysClusterServer(ITestOutputHelper log) : InProcessTestServer(log) + { + protected override string HelloModeValue => "cluster"; + + protected override TypedRedisValue ClusterSlots(RedisClient client, in RedisRequest request) + => TypedRedisValue.Error("ERR command is not allowed"); + + protected override TypedRedisValue ClusterNodes(RedisClient client, in RedisRequest request) + => TypedRedisValue.Error("ERR command is not allowed"); + } + + [Theory] + [InlineData(RedisProtocol.Resp2)] + [InlineData(RedisProtocol.Resp3)] + public async Task CrossSlotSurvivesSubscriptionReconnect(RedisProtocol protocol) + { + using var server = new HelloSaysClusterServer(log); // ServerType stays Standalone: INFO says so + + var config = server.GetClientConfig(defaultOnly: true); + config.Protocol = protocol; + config.AllowSimulateConnectionFailure = true; + config.AbortOnConnectFail = false; + var logger = new CapturingLogger(); + config.LoggerFactory = logger; + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + var db = conn.GetDatabase(); + + RedisKey[] crossSlot = ["a", "b"]; // different hash slots + await db.StringGetAsync(crossSlot); // sanity: fine on a fresh connection + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + + var endpoint = conn.GetServerSnapshot()[0]; + await WaitUntilAsync(() => endpoint.IsSubscriberConnected, "subscriber connected"); + + // drop *only* the subscription connection, as the server did in #3284; the reconnect is too quick + // to observe by polling, so watch for the restored event instead + var restored = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + conn.ConnectionRestored += (_, e) => + { + if (e.ConnectionType == ConnectionType.Subscription) restored.TrySetResult(true); + }; + log.WriteLine("dropping the subscription connection"); + endpoint.SimulateConnectionFailure(SimulatedFailureType.AllSubscription); + await restored.Task.WaitAsync(TimeSpan.FromSeconds(10)); + await WaitUntilAsync(() => endpoint.IsSubscriberConnected, "subscriber reconnected"); + + // the restored connection triggers a fire-and-forget reconfigure (and so can a server-type change); + // ReconfigureAsync declines while one is in flight, so insist on one that actually ran + var deadline = DateTime.UtcNow.AddSeconds(10); + while (!await conn.ReconfigureAsync("after subscription reconnect")) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Timed out waiting for a reconfigure to run"); + await Task.Delay(50); + } + + try + { + // the user-visible symptom from #3284: cross-slot commands refused client-side + for (int i = 0; i < 5; i++) + { + await db.StringGetAsync(crossSlot); + await Task.Delay(100); + } + Assert.Equal(ServerType.Standalone, endpoint.ServerType); + Assert.Equal(ServerType.Standalone, conn.ServerSelectionStrategy.ServerType); + } + finally + { + log.WriteLine(logger.All); + } + } + + private static async Task WaitUntilAsync(Func condition, string what) + { + var deadline = DateTime.UtcNow.AddSeconds(10); + while (!condition()) + { + if (DateTime.UtcNow > deadline) throw new TimeoutException("Timed out waiting for: " + what); + await Task.Delay(50); + } + } +} diff --git a/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs b/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs new file mode 100644 index 000000000..3ab329e0d --- /dev/null +++ b/tests/StackExchange.Redis.Tests/SlotMapUnitTests.cs @@ -0,0 +1,33 @@ +using System.Threading.Tasks; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// is derived from whether the map has been allocated, which is +/// only sound if nothing can allocate it without writing an owner into it (see #3284/#3285). +/// +public class SlotMapUnitTests(ITestOutputHelper log) +{ + [Fact] + public async Task RejectedRangesDoNotAllocateTheMap() + { + using var server = new InProcessTestServer(log); + await using var conn = await ConnectionMultiplexer.ConnectAsync(server.GetClientConfig(withPubSub: false, defaultOnly: true)); + var strategy = conn.ServerSelectionStrategy; + var endpoint = conn.GetServerSnapshot()[0]; + + Assert.True(strategy.IsSlotMapEmpty); // standalone: nothing has been applied + Assert.Equal(0, strategy.CountCoveredSlots()); + + strategy.UpdateClusterRange(5, 3, endpoint); // inverted + strategy.UpdateClusterRange(-1, 3, endpoint); // below range + strategy.UpdateClusterRange(16380, 16384, endpoint); // above range + Assert.True(strategy.IsSlotMapEmpty); + Assert.Equal(0, strategy.CountCoveredSlots()); + + strategy.UpdateClusterRange(3, 5, endpoint); + Assert.False(strategy.IsSlotMapEmpty); + Assert.Equal(3, strategy.CountCoveredSlots()); + } +} diff --git a/toys/StackExchange.Redis.Server/RedisServer.cs b/toys/StackExchange.Redis.Server/RedisServer.cs index a0862ff86..5289b8791 100644 --- a/toys/StackExchange.Redis.Server/RedisServer.cs +++ b/toys/StackExchange.Redis.Server/RedisServer.cs @@ -675,7 +675,7 @@ protected virtual TypedRedisValue Hello(RedisClient client, in RedisRequest requ span[index++] = TypedRedisValue.BulkString("id"); span[index++] = TypedRedisValue.Integer(client.Id); span[index++] = TypedRedisValue.BulkString("mode"); - span[index++] = TypedRedisValue.BulkString(ServerModeValue); + span[index++] = TypedRedisValue.BulkString(HelloModeValue); span[index++] = TypedRedisValue.BulkString("role"); span[index++] = TypedRedisValue.BulkString(GetPrimaryOf(client?.Node) is null ? "master" : "replica"); span[index++] = TypedRedisValue.BulkString("modules"); @@ -699,6 +699,12 @@ protected virtual TypedRedisValue Hello(RedisClient client, in RedisRequest requ /// protected virtual IReadOnlyList> HelloExtraFields => []; + /// + /// The mode reported by HELLO; by default the same as INFO's redis_mode, but a + /// proxy can disagree with itself (see #3284), so tests can override this independently. + /// + protected virtual string HelloModeValue => ServerModeValue; + [RedisCommand(-3)] protected virtual TypedRedisValue Sadd(RedisClient client, in RedisRequest request) {