Skip to content

Commit

Permalink
Fix ACL and Txn unit test
Browse files Browse the repository at this point in the history
  • Loading branch information
hamdaankhalidmsft committed Jan 6, 2025
1 parent fa5941b commit dab924c
Show file tree
Hide file tree
Showing 11 changed files with 65 additions and 35 deletions.
69 changes: 46 additions & 23 deletions libs/resources/RespCommandsInfo.json
Original file line number Diff line number Diff line change
Expand Up @@ -1506,9 +1506,21 @@
"LastKey": 1,
"Step": 1,
"AclCategories": "Fast, String, Read",
"Tips": null,
"KeySpecifications": null,
"SubCommands": null
"KeySpecifications": [
{
"BeginSearch": {
"TypeDiscriminator": "BeginSearchIndex",
"Index": 1
},
"FindKeys": {
"TypeDiscriminator": "FindKeysRange",
"LastKey": 0,
"KeyStep": 1,
"Limit": 0
},
"Flags": "RO, Access"
}
]
},
{
"Command": "GETRANGE",
Expand Down Expand Up @@ -1570,9 +1582,21 @@
"LastKey": 1,
"Step": 1,
"AclCategories": "Fast, String, Read",
"Tips": null,
"KeySpecifications": null,
"SubCommands": null
"KeySpecifications": [
{
"BeginSearch": {
"TypeDiscriminator": "BeginSearchIndex",
"Index": 1
},
"FindKeys": {
"TypeDiscriminator": "FindKeysRange",
"LastKey": 0,
"KeyStep": 1,
"Limit": 0
},
"Flags": "RO, Access"
}
]
},
{
"Command": "HDEL",
Expand Down Expand Up @@ -3503,19 +3527,6 @@
}
]
},
{
"Command": "SETEXNX",
"Name": "SETEXNX",
"Arity": -3,
"Flags": "NONE",
"FirstKey": 1,
"LastKey": 1,
"Step": 1,
"AclCategories": "Fast, String, Write, Transaction",
"Tips": null,
"KeySpecifications": null,
"SubCommands": null
},
{
"Command": "SETIFMATCH",
"Name": "SETIFMATCH",
Expand All @@ -3525,10 +3536,22 @@
"FirstKey": 1,
"LastKey": 1,
"Step": 1,
"AclCategories": "Fast, String, Write",
"Tips": null,
"KeySpecifications": null,
"SubCommands": null
"AclCategories": "Slow, String, Write",
"KeySpecifications": [
{
"BeginSearch": {
"TypeDiscriminator": "BeginSearchIndex",
"Index": 1
},
"FindKeys": {
"TypeDiscriminator": "FindKeysRange",
"LastKey": 0,
"KeyStep": 1,
"Limit": 0
},
"Flags": "RW, Access, Update, VariableFlags"
}
]
},
{
"Command": "SETRANGE",
Expand Down
2 changes: 1 addition & 1 deletion libs/server/ACL/ACLParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ static bool IsValidParse(RespCommand command, ReadOnlySpan<char> fromStr)

// Some commands aren't really commands, so ACLs shouldn't accept their names
static bool IsInvalidCommandToAcl(RespCommand command)
=> command == RespCommand.INVALID || command == RespCommand.NONE || command.NormalizeForACLs() != command;
=> command == RespCommand.INVALID || command == RespCommand.NONE || command.Normalize() != command;
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions libs/server/ACL/CommandPermissionSet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public CommandPermissionSet Copy()
/// </summary>
public void AddCommand(RespCommand command)
{
Debug.Assert(command.NormalizeForACLs() == command, "Cannot control access to this command, it's an implementation detail");
Debug.Assert(command.Normalize() == command, "Cannot control access to this command, it's an implementation detail");

int index = (int)command;
int ulongIndex = index / 64;
Expand All @@ -118,7 +118,7 @@ public void AddCommand(RespCommand command)
/// </summary>
public void RemoveCommand(RespCommand command)
{
Debug.Assert(command.NormalizeForACLs() == command, "Cannot control access to this command, it's an implementation detail");
Debug.Assert(command.Normalize() == command, "Cannot control access to this command, it's an implementation detail");

// Can't remove access to these commands
if (command.IsNoAuth())
Expand Down
6 changes: 4 additions & 2 deletions libs/server/InputHeader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ namespace Garnet.server
/// Flags used by append-only file (AOF/WAL)
/// The byte representation only use the last 3 bits of the byte since the lower 5 bits of the field used to store the flag stores other data in the case of Object types.
/// In the case of a Rawstring, the last 4 bits are used for flags, and the other 4 bits are unused of the byte.
/// NOTE: This will soon be expanded as a part of a breaking change to make WithEtag bit compatible with object store as well.
/// </summary>
[Flags]
public enum RespInputFlags : byte
{
/// <summary>
/// Flag indicating an operation intending to add an etag for a RAWSTRING command
/// Flag indicating an operation intending to add an etag for a RAWSTRING command.
/// </summary>
WithEtag = 16,

Expand Down Expand Up @@ -323,7 +324,8 @@ public struct RawStringInput : IStoreInput
{
/// <summary>
/// Mutable state we keep around for efficient EtagOffsetManagement, this will be removed when ETag is stored at the record level separately and does not require offset management.
/// NOTE: We do not serialize this to disk or read it from disk, it is only used in memory, and a temporary solution to keep track of Etag offsets across method calls.
/// NOTE: We do not serialize this to disk or read it from disk, it is only kept in volatile memory. The WITHETAG flag that may or may not be stored in the header is used to conditionally
/// initialize the values for this field.
/// </summary>
public EtagOffsetManagementContext etagOffsetManagementContext;

Expand Down
6 changes: 3 additions & 3 deletions libs/server/Resp/Parser/RespCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,10 @@ public static bool IsAofIndependent(this RespCommand cmd)

/// <summary>
/// Turns any not-quite-a-real-command entries in <see cref="RespCommand"/> into the equivalent command
/// for ACL'ing purposes.
/// for ACL'ing purposes and reading command info purposes
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static RespCommand NormalizeForACLs(this RespCommand cmd)
public static RespCommand Normalize(this RespCommand cmd)
{
return
cmd switch
Expand All @@ -452,7 +452,7 @@ public static RespCommand NormalizeForACLs(this RespCommand cmd)
}

/// <summary>
/// Reverses <see cref="NormalizeForACLs(RespCommand)"/>, producing all the equivalent <see cref="RespCommand"/>s which are covered by <paramref name="cmd"/>.
/// Reverses <see cref="Normalize(RespCommand)"/>, producing all the equivalent <see cref="RespCommand"/>s which are covered by <paramref name="cmd"/>.
/// </summary>
public static ReadOnlySpan<RespCommand> ExpandForACLs(this RespCommand cmd)
{
Expand Down
2 changes: 1 addition & 1 deletion libs/server/Resp/RespServerSessionSlotVerify.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ bool CanServeSlot(RespCommand cmd)
if (!cmd.IsDataCommand())
return true;

cmd = cmd.NormalizeForACLs();
cmd = cmd.Normalize();
if (!RespCommandsInfo.TryFastGetRespCommandInfo(cmd, out var commandInfo))
// This only happens if we failed to parse the json file
return false;
Expand Down
4 changes: 4 additions & 0 deletions libs/server/Storage/EtagOffsetManagement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

namespace Garnet.server
{
/// <summary>
/// Offset acounting done to prevent the need for recalculation at different methods. This is passed as context along with RawStringInput.
/// Making it a struct makes sure the values are embedded as a part of RawStringInput.
/// </summary>
public struct EtagOffsetManagementContext
{
public int EtagIgnoredOffset { get; private set; }
Expand Down
2 changes: 1 addition & 1 deletion libs/server/Storage/Functions/MainStore/RMWMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public bool NeedInitialUpdate(ref SpanByte key, ref RawStringInput input, ref Sp
{
switch (input.header.cmd)
{
case RespCommand.SETIFMATCH:
case RespCommand.SETKEEPTTLXX:
case RespCommand.PERSIST:
case RespCommand.EXPIRE:
Expand All @@ -29,6 +28,7 @@ public bool NeedInitialUpdate(ref SpanByte key, ref RawStringInput input, ref Sp
case RespCommand.GETDEL:
case RespCommand.GETEX:
return false;
case RespCommand.SETIFMATCH:
case RespCommand.SETEXXX:
// when called withetag all output needs to be placed on the buffer
if (input.header.CheckWithEtagFlag())
Expand Down
2 changes: 2 additions & 0 deletions libs/server/Transaction/TxnRespCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ private bool NetworkEXEC()
private bool NetworkSKIP(RespCommand cmd)
{
// Retrieve the meta-data for the command to do basic sanity checking for command arguments
// Normalize will turn internal "not-real commands" such as SETEXNX, and SETEXXX to the command info parent
cmd = cmd.Normalize();
if (!RespCommandsInfo.TryGetRespCommandInfo(cmd, out var commandInfo, txnOnly: true, logger))
{
while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_GENERIC_UNK_CMD, ref dcurr, dend))
Expand Down
2 changes: 1 addition & 1 deletion test/Garnet.test/Resp/ACL/RespCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public void AllCommandsCovered()

// Check tests against RespCommand
{
IEnumerable<RespCommand> allValues = Enum.GetValues<RespCommand>().Select(static x => x.NormalizeForACLs()).Distinct();
IEnumerable<RespCommand> allValues = Enum.GetValues<RespCommand>().Select(static x => x.Normalize()).Distinct();
IEnumerable<RespCommand> testableValues =
allValues
.Except([RespCommand.NONE, RespCommand.INVALID])
Expand Down
1 change: 0 additions & 1 deletion test/Garnet.test/Resp/RespReadUtilsTests.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;
using System.Text;
using Garnet.common;
using Garnet.common.Parsing;
Expand Down

0 comments on commit dab924c

Please sign in to comment.