chore: remove locks from acl validation in the hot path #1765

kostasrim · 2023-08-29T19:08:22Z

Currently df acquires a ReadLock over the UserRegistry to validate ACL categories, therefore any changes to the UserRegistry are immediately available when promt. This is not very performant for the hot path since the atomic increments on the lock force the cores to synchronize (caches + penalty of accessing memory that is outside of the core for NUMA).

This behavior is replaced by deep copying the acl categories in the ConnectionContext. When there is an update to that user's ACL categories, it is streamlined via the proactor to the already opened connections.

Therefore this PR:

removes locking the registry when Validating users
deep copies the acl categories in the connection context
streamline updates to the acl categories via the proactor pool

kostasrim · 2023-08-30T12:48:21Z

this is tested via the already existing acl_family_test.py which passes with the latest changes

adiholden · 2023-08-30T14:40:15Z

src/server/acl/acl_family.cc

@@ -193,4 +225,8 @@ void AclFamily::Register(dfly::CommandRegistry* registry) {

 #undef HFUNC

+void AclFamily::Init(std::vector<facade::Listener*> listeners) {


why not const& and than copy here?

I like to delegate this to the callers. An example is:

std::vector<...> tmp;
Init(std::move(tmp)); //init moves internally so in total 2 moves

If we do what you suggest we end up with 1 move(fine const& will work) and then a deep copy.

Generally, I like to:

Accept by value an delegate the choice to the user.

Accept by && only if the function is supposed to be used as a sink.

adiholden · 2023-08-30T14:45:57Z

src/server/acl/acl_family.cc

    ServerState::tlocal()->user_registry->MaybeAddAndUpdate(username, std::move(req));
+    auto cred = ServerState::tlocal()->user_registry->GetCredentials(username);
+    StreamUpdatesToAllProactorConnections(username, cred.acl_categories);


maybe you want to do this only if this is not a new user?

do we want to have a lock here so no 2 set user commands are done at the same time because if we have 2 set commands for the same user at the same time what will be set in the connections?

maybe you want to do this only if this is not a new user?
That's true we can do this

That's a good catch! I already have a pattern for this 😜

dranikpg · 2023-08-30T20:38:49Z

src/server/acl/acl_family.cc

+void AclFamily::StreamUpdatesToAllProactorConnections(std::string_view user, uint32_t update_cat) {
+  auto update_cb = [user, update_cat]([[maybe_unused]] size_t id, util::Connection* conn) {
+    DCHECK(conn);
+    auto connection = static_cast<facade::Connection*>(conn);
+    auto ctx = static_cast<ConnectionContext*>(connection->cntx());
+    if (ctx && user == ctx->authed_username) {
+      ctx->acl_categories = update_cat;
+    }
+  };
+


SetUser is called so rarely that we can neglect the cost of traversing all connections? (of which there are also just a few usually?)

Damn... What if the context is performing squashing? 😭

We can copy the value (instead of referencing the parent), but then how do we propagate changes? On each batch? 😕

If you want you can drop support for squashing and I will think how to enable support for your features

Btw, because Redis is single-threaded it's impossible for ACL rules to change while its executing a script / a multi transaction, right?

SetUser is called so rarely that we can neglect the cost of traversing all connections? (of which there are also just a few usually?)

Yes that's true but it doesn't hurt to not traverse if the user was just created :)

Damn... What if the context is performing squashing? 😭 We can copy the value (instead of referencing the parent), but then how do we propagate changes? On each batch? 😕

I already thought of this 👨‍🍳 ! It won't affect us. Why? Because, sure we update the "stub" contexts as well BUT validation is done on the parent and not on the stub context. Therefore, we don't really care, since the update done to stub contexts won't affect anything(think of it a stub update -- we mocked the mock 🔪 ). That is before a batch executes we call ValidateCommandExecution which will see the updated changes since it promts the parent and not the stub context

If you want you can drop support for squashing and I will think how to enable support for your features

It's all fine and if it wasn't we would fix it 🚀 No rush here

Btw, because Redis is single-threaded it's impossible for ACL rules to change while its executing a script / a multi transaction, right?

Redis is single threaded indeed but connections can be multiplexed just like on a single thread you can run multiple tasks interleaved with each other (to the user this looks like multihreading, in reality it's just a single thread that interleaves task execution). That being said, this example works on redis:

time client 1 client 2 t1 redis-cli t2 auth user user t3 multi t4 set key 23 t5 redis-cli t6 setuser kostas -@string t7 exec --> ERROR, categories changed between `multi/exec`

t1 to t7 is monotonically increasing

I was referring to the case when acl rules change while exec is running

BUT validation is done on the parent and not on the stub context

Exactly. When you change a connection from its own thread, you assume no other thread has access to it. So it is safe to mutate variables. BUT with squashing, the parent connection is accessed from multiple threads for validation, so its not possible to mutate it (because ValidateCommandExecution might be running on another thread)

romange · 2023-08-31T13:01:59Z

src/server/main_service.cc

@@ -665,6 +665,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>

  shard_set->Init(shard_num, !opts.disable_time_update);

+  acl_family_.Init(listeners);


do we enforce ACL on all listeners? I think we should only do it for the main listener (and not uds and admin).

fixed, you are right.

romange · 2023-08-31T16:29:52Z

src/server/main_service.cc

@@ -665,7 +665,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>

  shard_set->Init(shard_num, !opts.disable_time_update);
  const auto tcp_disabled = GetFlag(FLAGS_port) == 0u;
-  if (!tcp_disabled) {
+  if (!tcp_disabled && !listeners.empty()) {


please add a comment that we assume here that the main interface comes first. Also, please CHECK that it's not an admin interface.

I did add a comment, in dfly_main but I will add one here as well

romange · 2023-08-31T16:33:37Z

src/server/acl/acl_family.cc

+
+  pp_.AwaitFiberOnAll([this, update_cb](util::ProactorBase* pb) {
+    if (main_listener_) {
+      main_listener_->TraverseConnections(update_cb);


// traverses all client connections in all threads. cb must be thread safe. // cb should not keep Connection* pointers beyond the run of this function because // Connection* are valid only during the call to cb. void TraverseConnections(TraverseCB cb);

dranikpg

Gonna approve this but see my comment, I'm not sure the issue is resolved fully

dranikpg · 2023-08-31T18:59:21Z

src/server/acl/acl_family.cc

+void AclFamily::StreamUpdatesToAllProactorConnections(std::string_view user, uint32_t update_cat) {
+  auto update_cb = [user, update_cat]([[maybe_unused]] size_t id, util::Connection* conn) {
+    DCHECK(conn);
+    auto connection = static_cast<facade::Connection*>(conn);
+    auto ctx = static_cast<ConnectionContext*>(connection->cntx());
+    if (ctx && user == ctx->authed_username) {
+      ctx->acl_categories = update_cat;
+    }
+  };
+


I was referring to the case when acl rules change while exec is running

BUT validation is done on the parent and not on the stub context

Exactly. When you change a connection from its own thread, you assume no other thread has access to it. So it is safe to mutate variables. BUT with squashing, the parent connection is accessed from multiple threads for validation, so its not possible to mutate it (because ValidateCommandExecution might be running on another thread)

dranikpg · 2023-08-31T19:00:06Z

I didn't see you have auto merge 😄

kostasrim added 2 commits August 30, 2023 15:46

chore: remove locks from acl validation in the hot path

90b623f

Merge branch 'main' into stream_acl_user_updates

627581f

kostasrim force-pushed the stream_acl_user_updates branch from 0a16135 to 627581f Compare August 30, 2023 12:46

kostasrim changed the title ~~(WIP - Do not review) chore: remove locks from acl validation in the hot path~~ chore: remove locks from acl validation in the hot path Aug 30, 2023

kostasrim marked this pull request as ready for review August 30, 2023 12:47

kostasrim self-assigned this Aug 30, 2023

kostasrim requested review from dranikpg and adiholden August 30, 2023 12:47

adiholden reviewed Aug 30, 2023

View reviewed changes

dranikpg reviewed Aug 30, 2023

View reviewed changes

romange reviewed Aug 31, 2023

View reviewed changes

chore: address gh comments

a3bfb88

kostasrim requested review from dranikpg, adiholden and romange August 31, 2023 15:33

kostasrim added 2 commits August 31, 2023 18:33

Merge branch 'main' into stream_acl_user_updates

d0b8752

fix: broken check causing segfault

ed4484c

kostasrim enabled auto-merge (squash) August 31, 2023 16:25

romange reviewed Aug 31, 2023

View reviewed changes

kostasrim added 2 commits August 31, 2023 20:15

chore: address gh comments

01efc46

Merge branch 'main' into stream_acl_user_updates

739581e

kostasrim requested a review from romange August 31, 2023 17:25

dranikpg approved these changes Aug 31, 2023

View reviewed changes

kostasrim merged commit 866b9af into main Aug 31, 2023

kostasrim deleted the stream_acl_user_updates branch August 31, 2023 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove locks from acl validation in the hot path #1765

chore: remove locks from acl validation in the hot path #1765

kostasrim commented Aug 29, 2023 •

edited

Loading

kostasrim commented Aug 30, 2023

adiholden Aug 30, 2023

kostasrim Aug 30, 2023

adiholden Aug 30, 2023

kostasrim Aug 30, 2023

dranikpg Aug 30, 2023

dranikpg Aug 30, 2023

dranikpg Aug 30, 2023

dranikpg Aug 30, 2023

kostasrim Aug 31, 2023 •

edited

Loading

kostasrim Aug 31, 2023

dranikpg Aug 31, 2023

romange Aug 31, 2023

kostasrim Aug 31, 2023

romange Aug 31, 2023

kostasrim Aug 31, 2023

romange Aug 31, 2023

dranikpg left a comment

dranikpg Aug 31, 2023

dranikpg commented Aug 31, 2023

		@@ -193,4 +225,8 @@ void AclFamily::Register(dfly::CommandRegistry* registry) {

		#undef HFUNC

		void AclFamily::Init(std::vector<facade::Listener*> listeners) {

		@@ -665,6 +665,7 @@ void Service::Init(util::AcceptServer* acceptor, std::vector<facade::Listener*>

		shard_set->Init(shard_num, !opts.disable_time_update);

		acl_family_.Init(listeners);

chore: remove locks from acl validation in the hot path #1765

chore: remove locks from acl validation in the hot path #1765

Conversation

kostasrim commented Aug 29, 2023 • edited Loading

kostasrim commented Aug 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Aug 31, 2023 • edited Loading

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

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

dranikpg commented Aug 31, 2023

kostasrim commented Aug 29, 2023 •

edited

Loading

kostasrim Aug 31, 2023 •

edited

Loading