feat(AclFamily): add acl commands #1844

kostasrim · 2023-09-11T16:20:32Z

Add acl commands
Add tests
Extend validation to work with acl commands
Extend acl load/save to work with acl commands
Extend acl list to work with acl commands
Move helper functions used for parsing in acl family to a separate file

resolves #1809

kostasrim · 2023-09-12T10:55:12Z

regression tests run https://github.com/dragonflydb/dragonfly/actions/runs/6159488030

kostasrim · 2023-09-12T10:57:18Z

@dranikpg Do not hate me, this is the only large PR that was left. It's mostly boilerplate, so you don't have to worry 😛

Maybe I could have move the utility functions in AclFamily.cc in another PR but it was such a pain to have them in the same file so I moved them here

src/facade/dragonfly_connection.cc

kostasrim · 2023-09-12T11:00:16Z

src/server/bitops_family.cc

@@ -850,6 +850,8 @@ void BitOpsFamily::Register(CommandRegistry* registry) {
      << CI{"BITOP", CO::WRITE | CO::NO_AUTOJOURNAL, -4, 2, -1, 1, acl::kBitOp}.SetHandler(&BitOp)
      << CI{"GETBIT", CO::READONLY | CO::FAST, 3, 1, 1, 1, acl::kGetBit}.SetHandler(&GetBit)
      << CI{"SETBIT", CO::WRITE | CO::DENYOOM, 4, 1, 1, 1, acl::kSetBit}.SetHandler(&SetBit);
+
+  builder | "BITPOS" | "BITCOUNT" | "BITFIELD" | "BITFIELD_RO" | "BITOP" | "GETBIT" | "SETBIT";


@dranikpg I Know what your thoughts are here. Probably you would comment that why don't you move this to the Command constructor such that we get the strings naturally. The reason is:

It requires passing the builder to the construction of the CI objects. That means that we make the same amount of line changes

I think it's better this way because it decouples it. If we push everything in the constructor of the CI it will be painfully to work with all these parameters

The only downside is that I might misspelled a command name. I trust my copy pasting ninja skills so I would say that this won't happen. If it happens we will get a bug report and fix this quickly. Either way not a big deal.

You guessed correctly. Duplicating all command names yet a third time is hilarious

I'd argue there is a muuuch better approach. The CommandRegistry is not a relict of the past - change it as you like. Lets add the notion of families there. Lets change it to a CommandRegistryBuilder if it becomes too much.

In short, I'd like to keep existing code as simple as possible. It is possible, without changing all the familes and passing the acl stuff around to keep track of families.

Let then your builder access that information from the command registry. Also if you have it already prepared, you don't need a full blown builder for it

src/server/acl/acl_family.cc

kostasrim · 2023-09-12T11:07:14Z

src/server/acl/acl_commands_def.h

+using RevCommandField = std::vector<std::string>;
+using RevCommandsIndexStore = std::vector<RevCommandField>;
+
+class CommandTableBuilder {


Bread and butter of this PR. This allows us to build adhoc the index table for commands, since it's so large and I didn't even bother hardcoding the index structure.

You forgot the letter i in Pair

Ah you have a typedef one line above, matched the wrong line

kostasrim · 2023-09-12T11:09:26Z

I am also planning some small refactoring of the common logic found in helpers.hpp/cpp but that will be done in a separate PR. This is too large, I don't want to make it even harder for reviewing

src/facade/dragonfly_connection.cc

dranikpg · 2023-09-13T10:45:40Z

src/server/acl/acl_commands_def.h

+using RevCommandField = std::vector<std::string>;
+using RevCommandsIndexStore = std::vector<RevCommandField>;
+
+class CommandTableBuilder {


You forgot the letter i in Pair

src/server/acl/acl_family.cc

dranikpg · 2023-09-13T11:00:57Z

src/server/acl/user.cc

+void User::SetAclCommands(size_t index, uint64_t bit_index, bool all) {
+  if (all) {
+    for (auto& family : commands_) {
+      family = ALL_COMMANDS;
+    }
+    return;
+  }
+  commands_[index] |= bit_index;
+}


its kinda confusing that with the all argument your index & bit_index have no meaning

I agree on this, let me see what I can do

I ended up using the index, if it's numeric_limits::max() then it denotes an all. I also added a comment

dranikpg · 2023-09-13T11:07:43Z

src/server/bitops_family.cc

@@ -850,6 +850,8 @@ void BitOpsFamily::Register(CommandRegistry* registry) {
      << CI{"BITOP", CO::WRITE | CO::NO_AUTOJOURNAL, -4, 2, -1, 1, acl::kBitOp}.SetHandler(&BitOp)
      << CI{"GETBIT", CO::READONLY | CO::FAST, 3, 1, 1, 1, acl::kGetBit}.SetHandler(&GetBit)
      << CI{"SETBIT", CO::WRITE | CO::DENYOOM, 4, 1, 1, 1, acl::kSetBit}.SetHandler(&SetBit);
+
+  builder | "BITPOS" | "BITCOUNT" | "BITFIELD" | "BITFIELD_RO" | "BITOP" | "GETBIT" | "SETBIT";


You guessed correctly. Duplicating all command names yet a third time is hilarious

I'd argue there is a muuuch better approach. The CommandRegistry is not a relict of the past - change it as you like. Lets add the notion of families there. Lets change it to a CommandRegistryBuilder if it becomes too much.

In short, I'd like to keep existing code as simple as possible. It is possible, without changing all the familes and passing the acl stuff around to keep track of families.

Let then your builder access that information from the command registry. Also if you have it already prepared, you don't need a full blown builder for it

dranikpg · 2023-09-13T11:11:14Z

src/server/acl/acl_commands_def.h

+  friend CommandTableBuilder& operator|(CommandTableBuilder& builder, std::string name) {
+    (*builder.index_)[name] = {builder.pos_, 1ULL << (builder.bit_number_++)};
+    (*builder.rindex_)[builder.pos_].push_back(std::move(name));
+    // We should crash if somehow we end up feeding more than the bitfield size
+    // This won't happen in production, since our tests will fail and we will never
+    // merge something that violates this
+    CHECK_LT(++builder.bits_limit, size_t(64));
+    return builder;
+  }


Whats the difference between the builder and a single function that accepts an absl::Span<string_view> 😆

But as I suggested above, lets not duplicate everything a third time. Lets finally make the command registry itself usable and then you don't need 800 lines of changes to do this all

src/server/acl/validator.cc

dranikpg · 2023-09-13T16:23:56Z

Does redis actually do the same with bitfields?

What if I persist search settings and now a new command is added in-between? All the bit indices become displaced and the settings are wrong noq

kostasrim · 2023-09-14T08:15:59Z

Does redis actually do the same with bitfields?

Persistence does not write the index structure on the file but rather the full command that was used to create the user. Therefore, we won't ever have to deal with such problem

dranikpg

I'll approve to not delay this PR any longer, but please see my comments. Briefly, I suggest fully removing the CommandsIndexer and store family id / command it in the commands itself. I don't see any downsides and it makes it much simpler

src/server/acl/acl_commands_def.h

src/server/acl/acl_family.cc

src/server/conn_context.cc

dranikpg · 2023-09-14T14:03:55Z

src/server/acl/validator.cc

+  const auto& commands = acl::CommandsIndexer();
+  DCHECK(commands.contains(id.name()));
+  auto [index, command_mask] = commands.find(id.name())->second;
+  DCHECK_LT(index, cntx.acl_commands.size());


Actually, on a second thought 😅 we don't need any of the global stores if we store the family index & command index in the CommandId itself and as a bonus we save a hashmap lookup... I don't see any downsides in storing this info there

I think this is a good idea, I will get back on this. I won't merge this PR. Let's make it as good as possible.

dranikpg

Smallest nit

dranikpg · 2023-09-15T09:46:14Z

src/server/command_registry.cc

+
+  family_of_commands_.back().push_back(std::string(k));
+  cmd.SetFamily(family_of_commands_.size() - 1);
+  cmd.SetBitIndex(1ULL << bit_index_++);


Then its a bit mask 🙂 actually you can store only the index and return 1<<i in get bit mask

Yes it's a bit mask! Don't hate me, too lazy to make this change and I don't want to do a shift every time I access it :P (not that it matters though)

kostasrim added 4 commits September 12, 2023 12:27

feat(AclFamily): add acl commands

fea06b5

Merge branch 'main' into acl_part_10_add_commands

6d1e91e

fix: make all tests pass

2c2ca25

chore: add tests for commands

bb220fa

kostasrim force-pushed the acl_part_10_add_commands branch from 47a0884 to bb220fa Compare September 12, 2023 10:54

kostasrim marked this pull request as ready for review September 12, 2023 10:54

kostasrim requested a review from dranikpg September 12, 2023 10:55

kostasrim assigned adiholden and unassigned adiholden Sep 12, 2023

kostasrim requested a review from adiholden September 12, 2023 10:56

kostasrim self-assigned this Sep 12, 2023

kostasrim commented Sep 12, 2023

View reviewed changes

src/facade/dragonfly_connection.cc Show resolved Hide resolved

kostasrim commented Sep 12, 2023

View reviewed changes

src/server/acl/acl_family.cc Show resolved Hide resolved

kostasrim commented Sep 12, 2023

View reviewed changes

kostasrim added 2 commits September 12, 2023 14:44

chore: add missing commands to the index tables

3462da3

fix underflow

337fb75

kostasrim changed the title ~~[WIP - do not review] feat(AclFamily): add acl commands~~ feat(AclFamily): add acl commands Sep 12, 2023

fix: parse +all properly

cfcb57d

dranikpg reviewed Sep 13, 2023

View reviewed changes

src/server/acl/validator.cc Outdated Show resolved Hide resolved

kostasrim added 2 commits September 13, 2023 17:32

Merge branch 'main' into acl_part_10_add_commands

e2447a5

Move builder to command registry

869787a

kostasrim added 2 commits September 14, 2023 11:53

chore: cleanup

702e49e

Merge branch 'main' into acl_part_10_add_commands

160a5e9

kostasrim requested a review from dranikpg September 14, 2023 09:04

dranikpg previously approved these changes Sep 14, 2023

View reviewed changes

chore: address gh comments

68d26e5

kostasrim dismissed dranikpg’s stale review via 68d26e5 September 15, 2023 09:21

kostasrim requested a review from dranikpg September 15, 2023 09:21

Merge branch 'main' into acl_part_10_add_commands

888c320

dranikpg approved these changes Sep 15, 2023

View reviewed changes

kostasrim merged commit bbd4c6b into main Sep 15, 2023
10 checks passed

kostasrim deleted the acl_part_10_add_commands branch September 15, 2023 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AclFamily): add acl commands #1844

feat(AclFamily): add acl commands #1844

kostasrim commented Sep 11, 2023 •

edited

kostasrim commented Sep 12, 2023 •

edited

kostasrim commented Sep 12, 2023

kostasrim Sep 12, 2023

dranikpg Sep 13, 2023

kostasrim Sep 12, 2023

dranikpg Sep 13, 2023

dranikpg Sep 13, 2023

kostasrim commented Sep 12, 2023

dranikpg Sep 13, 2023

dranikpg Sep 13, 2023

kostasrim Sep 14, 2023

kostasrim Sep 14, 2023

dranikpg Sep 13, 2023

dranikpg Sep 13, 2023

dranikpg commented Sep 13, 2023

kostasrim commented Sep 14, 2023

dranikpg left a comment

dranikpg Sep 14, 2023

kostasrim Sep 14, 2023

dranikpg left a comment

dranikpg Sep 15, 2023

kostasrim Sep 15, 2023

feat(AclFamily): add acl commands #1844

feat(AclFamily): add acl commands #1844

Conversation

kostasrim commented Sep 11, 2023 • edited

kostasrim commented Sep 12, 2023 • edited

kostasrim commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg commented Sep 13, 2023

kostasrim commented Sep 14, 2023

dranikpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Sep 11, 2023 •

edited

kostasrim commented Sep 12, 2023 •

edited