From 78d6ad75381b4da8599c9dc2670d4ddc7d444533 Mon Sep 17 00:00:00 2001 From: vazois <96085550+vazois@users.noreply.github.com> Date: Fri, 17 May 2024 12:33:43 -0700 Subject: [PATCH] Fix Config GET databases in cluster mode (#389) * return correct db count when cluster is enabled * expose ReadWriteSession flag * create skeleton for CONFIG GET * add CONFIG|GET tests * fix formatting --- libs/cluster/Server/ClusterConfig.cs | 10 ++ libs/cluster/Session/ClusterSession.cs | 2 + libs/host/Configuration/Options.cs | 2 - libs/server/Cluster/IClusterSession.cs | 5 + libs/server/Resp/AdminCommands.cs | 31 +++--- libs/server/Resp/CmdStrings.cs | 10 -- libs/server/Resp/RespServerSession.cs | 6 +- libs/server/ServerConfig.cs | 107 +++++++++++++++++++++ libs/server/ServerConfigType.cs | 17 ++++ test/Garnet.test/RespAdminCommandsTests.cs | 51 ++++++++++ 10 files changed, 210 insertions(+), 31 deletions(-) create mode 100644 libs/server/ServerConfig.cs create mode 100644 libs/server/ServerConfigType.cs diff --git a/libs/cluster/Server/ClusterConfig.cs b/libs/cluster/Server/ClusterConfig.cs index 4a617ae478..0efcd03b73 100644 --- a/libs/cluster/Server/ClusterConfig.cs +++ b/libs/cluster/Server/ClusterConfig.cs @@ -169,6 +169,16 @@ public bool IsKnown(string nodeid) return false; } + /// + /// Check if local node is a PRIMARY node + /// + public bool IsPrimary => LocalNodeRole == NodeRole.PRIMARY; + + /// + /// Check if local node is a REPLICA node + /// + public bool IsReplica => LocalNodeRole == NodeRole.REPLICA; + #region GetLocalNodeInfo /// /// Get local node ip diff --git a/libs/cluster/Session/ClusterSession.cs b/libs/cluster/Session/ClusterSession.cs index f3933afada..8d44681f96 100644 --- a/libs/cluster/Session/ClusterSession.cs +++ b/libs/cluster/Session/ClusterSession.cs @@ -45,6 +45,8 @@ internal sealed unsafe partial class ClusterSession : IClusterSession /// bool readWriteSession = false; + public bool ReadWriteSession => clusterProvider.clusterManager.CurrentConfig.IsPrimary || readWriteSession; + public void SetReadOnlySession() => readWriteSession = false; public void SetReadWriteSession() => readWriteSession = true; diff --git a/libs/host/Configuration/Options.cs b/libs/host/Configuration/Options.cs index cce0fa7e8b..942617a476 100644 --- a/libs/host/Configuration/Options.cs +++ b/libs/host/Configuration/Options.cs @@ -629,8 +629,6 @@ private IAuthenticationSettings GetAuthenticationSettings(ILogger logger = null) throw new Exception($"Authentication mode {AuthenticationMode} is not supported."); } } - - } /// diff --git a/libs/server/Cluster/IClusterSession.cs b/libs/server/Cluster/IClusterSession.cs index 2fe068c1a3..338946410b 100644 --- a/libs/server/Cluster/IClusterSession.cs +++ b/libs/server/Cluster/IClusterSession.cs @@ -11,6 +11,11 @@ namespace Garnet.server /// public interface IClusterSession { + /// + /// Type of session + /// + bool ReadWriteSession { get; } + /// /// Make this cluster session a read-only session /// diff --git a/libs/server/Resp/AdminCommands.cs b/libs/server/Resp/AdminCommands.cs index 0bdc137d80..780ad2e66d 100644 --- a/libs/server/Resp/AdminCommands.cs +++ b/libs/server/Resp/AdminCommands.cs @@ -96,30 +96,29 @@ private bool ProcessAdminCommands(RespCommand command, ReadOnlySpan< } else if (command == RespCommand.CONFIG) { - if (!CheckACLAdminPermissions(bufSpan, count, out bool success)) + if (!CheckACLAdminPermissions(bufSpan, count, out var success)) { return success; } - var param = GetCommand(bufSpan, out bool success1); + // Terminate early if command arguments are less than expected + if (count < 1) + { + while (!RespWriteUtils.WriteBulkString("ERR wrong number of arguments for 'config' command"u8, ref dcurr, dend)) + SendAndReset(); + return true; + } + + var param = GetCommand(bufSpan, out var success1); if (!success1) return false; + if (param.SequenceEqual(CmdStrings.GET) || param.SequenceEqual(CmdStrings.get)) { - if (count > 2) - { - if (!DrainCommands(bufSpan, count - 1)) - return false; - errorFlag = true; - errorCmd = Encoding.ASCII.GetString(param); - } - else - { - var key = GetCommand(bufSpan, out bool success2); - if (!success2) return false; + if (!NetworkConfigGet(bufSpan, count, out errorFlag)) + return false; - while (!RespWriteUtils.WriteDirect(CmdStrings.GetConfig(key), ref dcurr, dend)) - SendAndReset(); - } + if (errorFlag) + errorCmd = Encoding.ASCII.GetString(CmdStrings.CONFIG) + "|" + Encoding.ASCII.GetString(param); } else if (param.SequenceEqual(CmdStrings.REWRITE) || param.SequenceEqual(CmdStrings.rewrite)) { diff --git a/libs/server/Resp/CmdStrings.cs b/libs/server/Resp/CmdStrings.cs index d81514fd35..7fb56143c2 100644 --- a/libs/server/Resp/CmdStrings.cs +++ b/libs/server/Resp/CmdStrings.cs @@ -10,16 +10,6 @@ namespace Garnet.server /// static partial class CmdStrings { - public static ReadOnlySpan GetConfig(ReadOnlySpan key) - { - if (key.SequenceEqual("timeout"u8)) return "*2\r\n$7\r\ntimeout\r\n$1\r\n0\r\n"u8; - else if (key.SequenceEqual("save"u8)) return "*2\r\n$4\r\nsave\r\n$0\r\n\r\n"u8; - else if (key.SequenceEqual("appendonly"u8)) return "*2\r\n$10\r\nappendonly\r\n$2\r\nno\r\n"u8; - else if (key.SequenceEqual("slave-read-only"u8)) return "$3\r\nyes\r\n"u8; - else if (key.SequenceEqual("databases"u8)) return "$2\r\n16\r\n"u8; - else return RESP_EMPTYLIST; - } - /// /// Request strings /// diff --git a/libs/server/Resp/RespServerSession.cs b/libs/server/Resp/RespServerSession.cs index 10193e0969..a14c99226f 100644 --- a/libs/server/Resp/RespServerSession.cs +++ b/libs/server/Resp/RespServerSession.cs @@ -654,7 +654,7 @@ private bool ProcessOtherCommands(RespCommand command, byte subcmd, bool DrainCommands(ReadOnlySpan bufSpan, int count) { - for (int i = 0; i < count; i++) + for (var i = 0; i < count; i++) { GetCommand(bufSpan, out bool success1); if (!success1) return false; @@ -662,7 +662,7 @@ bool DrainCommands(ReadOnlySpan bufSpan, int count) return true; } - ReadOnlySpan GetCommand(ReadOnlySpan bufSpan, out bool success) + Span GetCommand(ReadOnlySpan bufSpan, out bool success) { var ptr = recvBufferPtr + readHead; var end = recvBufferPtr + bytesRead; @@ -689,7 +689,7 @@ ReadOnlySpan GetCommand(ReadOnlySpan bufSpan, out bool success) RespParsingException.ThrowUnexpectedToken(*ptr); } - var result = bufSpan.Slice(readHead, length); + var result = new Span(recvBufferPtr + readHead, length); readHead += length + 2; success = true; diff --git a/libs/server/ServerConfig.cs b/libs/server/ServerConfig.cs new file mode 100644 index 0000000000..54440184b3 --- /dev/null +++ b/libs/server/ServerConfig.cs @@ -0,0 +1,107 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using Garnet.common; + +namespace Garnet.server +{ + static class ServerConfig + { + public static readonly HashSet DefaultConfigType = Enum.GetValues(). + Where(e => e switch + { + ServerConfigType.NONE => false, + ServerConfigType.ALL => false, + _ => true + }).ToHashSet(); + + public static unsafe ServerConfigType GetConfig(Span parameter) + { + ConvertUtils.MakeUpperCase(parameter); + return parameter switch + { + _ when parameter.SequenceEqual("TIMEOUT"u8) => ServerConfigType.TIMEOUT, + _ when parameter.SequenceEqual("SAVE"u8) => ServerConfigType.SAVE, + _ when parameter.SequenceEqual("APPENDONLY"u8) => ServerConfigType.APPENDONLY, + _ when parameter.SequenceEqual("SLAVE-READ-ONLY"u8) => ServerConfigType.SLAVE_READ_ONLY, + _ when parameter.SequenceEqual("DATABASES"u8) => ServerConfigType.DATABASES, + _ when parameter.SequenceEqual("CLUSTER-NODE-TIMEOUT"u8) => ServerConfigType.CLUSTER_NODE_TIMEOUT, + _ when parameter.SequenceEqual("*"u8) => ServerConfigType.ALL, + _ => ServerConfigType.NONE, + }; + } + } + + internal sealed unsafe partial class RespServerSession : ServerSessionBase + { + private bool NetworkConfigGet(ReadOnlySpan bufSpan, int count, out bool errorFlag) + { + errorFlag = false; + if (count < 2) + { + if (!DrainCommands(bufSpan, count - 1)) + return false; + errorFlag = true; + return true; + } + + // Extract requested parameters + HashSet parameters = []; + var returnAll = false; + for (var i = 0; i < count - 1; i++) + { + var parameter = GetCommand(bufSpan, out bool success2); + if (!success2) return false; + var serverConfigType = ServerConfig.GetConfig(parameter); + + if (returnAll) continue; + if (serverConfigType == ServerConfigType.ALL) + { + parameters = ServerConfig.DefaultConfigType; + returnAll = true; + continue; + } + + if (serverConfigType != ServerConfigType.NONE) + _ = parameters.Add(serverConfigType); + } + + // Generate response for matching parameters + if (parameters.Count > 0) + { + while (!RespWriteUtils.WriteArrayLength(parameters.Count * 2, ref dcurr, dend)) + SendAndReset(); + + foreach (var parameter in parameters) + { + var parameterValue = parameter switch + { + ServerConfigType.TIMEOUT => "$7\r\ntimeout\r\n$1\r\n0\r\n"u8, + ServerConfigType.SAVE => "$4\r\nsave\r\n$0\r\n\r\n"u8, + ServerConfigType.APPENDONLY => storeWrapper.serverOptions.EnableAOF ? "$10\r\nappendonly\r\n$3\r\nyes\r\n"u8 : "$10\r\nappendonly\r\n$2\r\nno\r\n"u8, + ServerConfigType.SLAVE_READ_ONLY => clusterSession == null || clusterSession.ReadWriteSession ? "$15\r\nslave-read-only\r\n$2\r\nno\r\n"u8 : "$15\r\nslave-read-only\r\n$3\r\nyes\r\n"u8, + ServerConfigType.DATABASES => storeWrapper.serverOptions.EnableCluster ? "$9\r\ndatabases\r\n$1\r\n1\r\n"u8 : "$9\r\ndatabases\r\n$2\r\n16\r\n"u8, + ServerConfigType.CLUSTER_NODE_TIMEOUT => Encoding.ASCII.GetBytes($"$20\r\ncluster-node-timeout\r\n${storeWrapper.serverOptions.ClusterTimeout.ToString().Length}\r\n{storeWrapper.serverOptions.ClusterTimeout}\r\n"), + ServerConfigType.NONE => throw new NotImplementedException(), + ServerConfigType.ALL => throw new NotImplementedException(), + _ => throw new NotImplementedException() + }; + + while (!RespWriteUtils.WriteDirect(parameterValue, ref dcurr, dend)) + SendAndReset(); + } + } + else + { + while (!RespWriteUtils.WriteDirect(CmdStrings.RESP_EMPTYLIST, ref dcurr, dend)) + SendAndReset(); + } + + return true; + } + } +} \ No newline at end of file diff --git a/libs/server/ServerConfigType.cs b/libs/server/ServerConfigType.cs new file mode 100644 index 0000000000..6372394d52 --- /dev/null +++ b/libs/server/ServerConfigType.cs @@ -0,0 +1,17 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace Garnet.server +{ + internal enum ServerConfigType : int + { + NONE, + ALL, + TIMEOUT, + SAVE, + APPENDONLY, + SLAVE_READ_ONLY, + DATABASES, + CLUSTER_NODE_TIMEOUT + } +} \ No newline at end of file diff --git a/test/Garnet.test/RespAdminCommandsTests.cs b/test/Garnet.test/RespAdminCommandsTests.cs index 23695cbb2f..4d8644a4b8 100644 --- a/test/Garnet.test/RespAdminCommandsTests.cs +++ b/test/Garnet.test/RespAdminCommandsTests.cs @@ -487,6 +487,57 @@ public async Task SeFlushDBTest([Values] bool async, [Values] bool unsafetruncat Assert.IsTrue(_value.IsNull); } + [Test] + [TestCase("timeout", "0")] + [TestCase("save", "")] + [TestCase("appendonly", "no")] + [TestCase("slave-read-only", "no")] + [TestCase("databases", "16")] + [TestCase("cluster-node-timeout", "60")] + public void SimpleConfigGet(string parameter, string parameterValue) + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + + var result = (string[])db.Execute("CONFIG", "GET", parameter); + + Assert.AreEqual(parameter, result[0]); + Assert.AreEqual(parameterValue, result[1]); + } + + #endregion + + #region NegativeTests + + [Test] + public void ConfigWrongNumberOfArguments() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + try + { + db.Execute("CONFIG"); + } + catch (Exception ex) + { + Assert.AreEqual("ERR wrong number of arguments for 'CONFIG' command", ex.Message); + } + } + + [Test] + public void ConfigGetWrongNumberOfArguments() + { + using var redis = ConnectionMultiplexer.Connect(TestUtils.GetConfig()); + var db = redis.GetDatabase(0); + try + { + db.Execute("CONFIG", "GET"); + } + catch (Exception ex) + { + Assert.AreEqual("ERR wrong number of arguments for 'CONFIG|GET' command", ex.Message); + } + } #endregion } } \ No newline at end of file