chore(acl): Implicit categories #3411
Conversation
|
@dranikpg acl_fam unit failing |
Signed-off-by: Vladislav Oleshko <[email protected]>
| if (mask & CO::BLOCKING) | ||
| out |= acl::BLOCKING; | ||
|
|
||
| if ((out & acl::FAST) == 0) |
There was a problem hiding this comment.
A counter example to this is HGETALL which we mark as:
<< CI{"HGETALL", CO::FAST | CO::READONLY, 2, 1, 1, acl::kHGetAll}.HFUNC(HGetAll)
which according to the rules above would mark it as FAST instead of SLOW.
My concern here is that we have to manually check all of these commands and the CO:: variants actually do map to the rules implemented in ImplicitAclCategories (and if the above mapping has no mistakes as well)
Another concern I have is that if we have bugs in our CO:: variants the Implicit rules describe above will insert a bugs in our ACL rules.
There was a problem hiding this comment.
Another concern I have is that if we have bugs
You can apply that concern to any code, I've added an option to return acl categories from COMMAND, so we can compare before and after and see the differences
A counter example to this is HGETALL which we mark as:
It's not a counter example, we just remove CO::FAST. Again, the CO tags don't mean anything now
There was a problem hiding this comment.
Nice, in fact a diff on this will also hinter any errors. I like that
| << CI{"SETRANGE", CO::WRITE | CO::FAST | CO::DENYOOM, 4, 1, 1, acl::kSetRange}.HFUNC(SetRange) | ||
| << CI{"CL.THROTTLE", CO::WRITE | CO::DENYOOM | CO::FAST, -5, 1, 1, acl::kClThrottle}.HFUNC( | ||
| ClThrottle); | ||
| registry->StartFamily(acl::STRING); |
There was a problem hiding this comment.
Maybe we can create a class template that is implicitly convertible to CommandId and during that conversion it injects the acl family like acl::STRING here which will move away the logic from registry. But IMO I am mostly worried about correctness than syntactic sugar
My weekly cleanup idea. Most of our CO:: categories became meaningless with the introduction of acl. For example, CO::FAST literally doesn't mean anything, it's never read or used.
The two options are either removing outdated categories or using the rules for acl category deduction:
https://github.com/redis/redis/blob/93fb83b4cb5090b7b48f5466924237558685b811/src/server.c#L3008