chore: add acl categories to all commands #1711

kostasrim · 2023-08-17T16:22:44Z

Add acl categories to each command
Extend facade::CommandId to include acl category
Add dragonfly extensions to acl categories for modules like search, json etc (since modules in redis do not have acl categories by default but in dragonfly these are not implemented as modules)

royjacobson · 2023-08-17T16:48:54Z

Ok, this is a LOT to review :)

Quick comment before I review the permissions for the actual commands - there are some 'old style' attributes like CO::WRITE that are redundant now, no? Do you plan to get rid of them in the future?

romange · 2023-08-17T18:21:34Z

src/server/acl/acl_commands_def.h

+#include "absl/container/flat_hash_map.h"
+
+namespace dfly {
+namespace AclCategory {


I am not a big fan of introducing lots of namespaces because it eats up horizontal space and hurts eyes.

if you want to add a namespace, then it's better to make it short and consistent with the folder name (i.e. acl).

everything in the folder should be under the same namespace.

classes inside the namespace should not have Acl prefix - i.e. either with namespace and without Acl or without the namespace with prefix.

+1 . note that I introduced the acl namespace here just so the enum constants in enum AclCat don't leak (since it's an unscoped enum). A class enum would not do either, because each time you assign one of its constants to a variable you need to static_cast. The last one was constexpr variables (which was originally what I did) but later Vlad pointed out that grouping with the enum is nice and I ended up opting for namespace + enum.

I will put everything in acl subfolder under the same namespace acl.

Just to clarify, the acl should be a top level namespace, that is ::acl::some_def and not dfly::acl::some_def is that correct?

everything is dfly under src/server, therefore I think it makes sense to introduce dfly::acl.
facade is the only other top-level namespace we introduced in dragonfly repo, imho.

romange · 2023-08-17T18:22:00Z

src/server/acl/acl_commands_def.h

+};
+
+// Special flag/mask for all
+inline constexpr uint32_t NONE = 0;


what inline gives us in this context?

nothing, muscle memory because I like to be explicit, constexpr variables are implicitly inline. I will remove this :)

and they are not implicitly inline! http://eel.is/c++draft/dcl.constexpr#1.sentence-3 . But constexpr implies const which enforces internal linkage (implies static unless declared as extern). However, I like inline because the compiler folds it, meaning that if-ever we take its address, all the TU will see the same address instead of a copy. Simply put, it's redundant to have multiple copies of the same constant (ok this will elided anyway since we don't use its address but I am being pedantic here)

romange · 2023-08-17T18:26:02Z

src/server/cluster/cluster_family.cc

@@ -598,12 +599,24 @@ inline CommandId::Handler HandlerFunc(ClusterFamily* se, EngineFunc f) {

 #define HFUNC(x) SetHandler(HandlerFunc(this, &ClusterFamily::x))

+namespace {
+namespace Acl {


namespaces should be all lowercase, and for consistency, I suggest using acl everywhere.
and you probably won't need namespace Cat = AclCategory; then.

Honestly, I really like the change, it's so much simpler now without the nested namespaces. Moreover, I removed the unnamed namespaces since constexpr implies const which implies static which has internal linkage

romange · 2023-08-17T18:32:55Z

src/server/hll_family.cc

+namespace {
+namespace Acl {
+namespace Cat = AclCategory;
+constexpr uint32_t kPFAdd = Cat::WRITE | Cat::HYPERLOGLOG | Cat::FAST;


Maybe introduce a macro to reduce the boilerplate? i.e. ACL_CATEGORY(WRITE, FAST, HYPERLOGLOG)

though it does not look like it will reduce much

This will change the syntax but not really reduce the boilerplate. I really don't like macros (unless there is a very good reason to use them) and since this won't add much here I rather not do it atm.

Sure! most comments I had - are purely subjective. Regarding macros - surprisingly I actually find them very useful for the production-grade code (i.e debug checks etc). (this case is irrelevant of course). Macros are one of the bigger reasons why I do not enjoy coding in golang or Java. Though I must say, the Rust "macro" language is 10 times better than C/C++.

Regarding macros - surprisingly I actually find them very useful for the production-grade code (i.e debug checks etc)

100% macros have good use cases and that's why are a necessary evil :D IMO, I have seen a lot of macro wonkery over the years (in places where templates would have been a better option) and my despise towards them comes from that.

p.s. one of the cases that macros are infamous but very very useful, is when you want to implement reflection in C++ with templates -- that is a library feature not a language feature. You need macros for that + SFINAE (since we don't have reflection implemented as a language feature -- soon though hopefully in 26 or 29)

for java and golang

Not a big fun of GC based languages especially when you want raw control. If you want to fine tune something that relies on GC you pretty much screwed -- meaning that you end up fighting the GC instead of solving the actual issue

Though I must say, the Rust "macro" language is 10 times better

I never tried Rust macros. I played with rust a little bit but IMO I don't buy its safety guarantees mostly because:

One way or another you will write rust that interferes with C/C++ code. You need unsafe for that so it kinda beats the purpose of the language

Compile times are SLOW AF. I have seen small codebases requiring hours to compile. People who complain about TMP and SFINAE compilation times would panic (pan intended here 🤣 ) when they see the compile times of Rust in big codebases.

You want to leverage RUST without a performance hit? You still need to write unsafe code :/

There are some TMP tricks that you just can't do with rust no matter how you try

The quality of the libraries found in rust are subpar compared to C++. One example is p2300 aka senders and receivers. It's a very complicated async model but it has 0 overhead compared to other async abstractions like futures and promises. The former does not require dynamic memory allocation for the shared state neither syncronization primitives for attaching continuations while the later does. You won't find high calibre libraries like that in Rust... Yes there is a learning curve here which is expensive but very rightfully so for certain cases and problems.

Plz don't get me wrong, my mental model is that every language and feature is tool -- I would program in any of them if it solves my problem elegantly. In my view, it's always a matter of asking the correct questions when you are implementing something (a good example here is your decision with boost fibers and helio -- can they be optimized further ? YES! Is it worth it ? NO! Why? Because there are other parts that bring more value!)

Btw I am not ranting here, I just love to think out loud and discuss these kind of things. I can do that for hours. Save it for when I am in Israel. I WOULD LOOOOOVE to discuss this!

Aaaaaalso one more thing. The goto async abstraction for Rust is https://tokio.rs/ which is AWESOME to play with and implement/try out new things. It just works!

However, I could write a lib that outperforms tokio (both in terms of performance and memory) any day -- it would be just like running to a park -- cardio 😄

romange · 2023-08-17T18:36:06Z

src/server/hset_family.cc

-            << CI{"HSETNX", CO::WRITE | CO::DENYOOM | CO::FAST, 4, 1, 1, 1}.HFUNC(HSetNx)
-            << CI{"HSTRLEN", CO::READONLY | CO::FAST, 3, 1, 1, 1}.HFUNC(HStrLen)
-            << CI{"HVALS", CO::READONLY, 2, 1, 1, 1}.HFUNC(HVals);
+  *registry


another idea - to introduce manipulators:

*registry << EnableCat(HASH) << .... << DisableCat(HASH)

That's a nice one and I was thinking of it for acl commands/subcommands but for this case it will add a lot of verbosity. At least with the constexpr variables we have everything in one place and we just pass a small (meaning in letters small) argument to the constructor.

src/server/hset_family.cc

kostasrim · 2023-08-18T07:03:17Z

@royjacobson

Ok, this is a LOT to review :)

Sorry, this one could not be made smaller :(

Quick comment before I review the permissions for the actual commands - there are some 'old style' attributes like CO::WRITE that are redundant now, no? Do you plan to get rid of them in the future?

Good question, I was thinking of removing them as well. However, there are also some CO:: variants that do not map 1-1 with the new Categories introduced (and from the redis command json files do not include anything regarding the ACL) and I will need to check this. Since this PR is already large, I would rather leave it as is for now and check back in the future.

dranikpg

I'm gonna approve here but I really hope those giant blocks will diminish sometime in the future 😀

kostasrim · 2023-08-19T09:28:54Z

I'm gonna approve here but I really hope those giant blocks will diminish sometime in the future 😀

I am not a huge fan of this either, once we get a prototype working I will see how we can clean up(I don't like tech debt neither do I like long blocks of literals). The constexpr transformations you mentioned are really nice -- but as I said I want to keep things decoupled for now to avoid any large change because of something that we missed. It's better to have something that you can reduce and simplify than the opposite way around (this is just my opinion -- take it with a grain of salt)

kostasrim added 2 commits August 17, 2023 19:19

chore: add acl categories to all commands

92db0e1

Merge branch 'main' into acl_part_2_add_acl_commands

5830749

kostasrim requested review from dranikpg and royjacobson August 17, 2023 16:23

romange reviewed Aug 17, 2023

View reviewed changes

dranikpg reviewed Aug 17, 2023

View reviewed changes

src/server/hset_family.cc Outdated Show resolved Hide resolved

kostasrim self-assigned this Aug 18, 2023

kostasrim added 2 commits August 18, 2023 19:22

chore: address gh comments

3c4c8bb

Merge branch 'main' into acl_part_2_add_acl_commands

7ddfe43

kostasrim requested review from romange and dranikpg August 18, 2023 16:29

dranikpg approved these changes Aug 18, 2023

View reviewed changes

romange approved these changes Aug 19, 2023

View reviewed changes

kostasrim merged commit cfc04cf into main Aug 20, 2023

kostasrim deleted the acl_part_2_add_acl_commands branch August 20, 2023 10:27

arukiidou mentioned this pull request Aug 31, 2023

Support ACL[API-6][API-7] #447

Closed

chore: add acl categories to all commands #1711

chore: add acl categories to all commands #1711

Uh oh!

Conversation

kostasrim commented Aug 17, 2023

Uh oh!

royjacobson commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kostasrim commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dranikpg left a comment

Choose a reason for hiding this comment

Uh oh!

kostasrim commented Aug 19, 2023

Uh oh!

Uh oh!

kostasrim Aug 18, 2023 •

edited

Loading

kostasrim Aug 18, 2023 •

edited

Loading

kostasrim Aug 18, 2023 •

edited

Loading

kostasrim commented Aug 18, 2023 •

edited

Loading