feat(AclFamily): load/store aclfile #1820

kostasrim · 2023-09-07T17:31:21Z

add ACL LOAD
add ACL SAVE
add --aclfile command

I need to polish a little bit and fix the pytest. I am a little tired atm and I will do it in the morning, it's basically 3 small fixes.
Some parts of the PR are not very polished and can be simplified (I duplicate some parts). These will be fixed after the PRs tha add the filtering based on acl commands && and the acl logs since I expect there will be some changes there as well.

Feel free to review, but I would advice to do it tomorrow after I fix the tests.

Closes: #1807

kostasrim · 2023-09-07T17:32:46Z

src/facade/dragonfly_connection.cc

-    dispatch_q_.push_front(std::move(msg));
+    // We need to reorder the queue, since multiple updates might happen before we
+    // pop the message, invalidating the correct order since we always push at the front
+    maybe_reorder_dispatch_q(std::move(msg));


This was a bug if we pushed two messages one after the other before the queue is saturated (because the order would be messed up). This is fixed now :)

kostasrim · 2023-09-07T17:33:22Z

src/facade/dragonfly_connection.h

@@ -80,8 +80,8 @@ class Connection : public util::Connection {
  struct MonitorMessage : public std::string {};

  struct AclUpdateMessage {
-    std::string_view username;
-    uint64_t categories{0};
+    std::vector<std::string> username;


This was also a bug, std::string_view could have been deleted if the queue item was not consumed and the string_view associated with the name got deallocated (e.g, user deletion)

I remember I asked in the previous PR 😆

Yes and I replied wrongly because I did not think about this case. It's fine, we caught it and I now know better.

P.s. you don't forget do you ? 👀

dranikpg

Just a few general comments 🧚🏻‍♂️

dranikpg · 2023-09-07T17:38:59Z

src/facade/dragonfly_connection.cc

+  auto maybe_reorder_dispatch_q = [this](MessageHandle msg) {
+    std::vector<MessageHandle> updates;
+    while (std::holds_alternative<AclUpdateMessage>(dispatch_q_.front().handle)) {
+      updates.push_back(std::move(dispatch_q_.front()));
+      dispatch_q_.pop_front();
+    }
+    updates.push_back(std::move(msg));
+    for (auto elem = updates.rbegin(); elem != updates.rend(); ++elem) {
+      dispatch_q_.push_front(std::move(*elem));
+    }
+  };


you can insert into a deque with an iterator 🤓

dranikpg · 2023-09-07T17:39:28Z

src/facade/dragonfly_connection.h

@@ -80,8 +80,8 @@ class Connection : public util::Connection {
  struct MonitorMessage : public std::string {};

  struct AclUpdateMessage {
-    std::string_view username;
-    uint64_t categories{0};
+    std::vector<std::string> username;


I remember I asked in the previous PR 😆

dranikpg · 2023-09-07T17:42:18Z

src/server/acl/acl_family.cc

+  auto acl_file = absl::GetFlag(FLAGS_aclfile);
+  if (!acl_file.empty()) {
+    Load({}, nullptr);
+  }


If you don't want to add wrapper functions for this only case you can pass a capturing reply builder into ti

But I suggest not to and instead make an error-reply returning internal function 🙂

I just overloaded Load :)

dranikpg · 2023-09-07T17:44:01Z

tests/dragonfly/acl_family_test.py

+@pytest.mark.asyncio
+async def test_bad_acl_file(df_local_factory):


you can use @dfly_args to pass an acl path

and lets create a data folder inside the tests folder for your test acl configs

dranikpg · 2023-09-08T08:40:19Z

src/facade/dragonfly_connection.cc

+    for (; it < dispatch_q_.end(); ++it) {
+      if (!std::holds_alternative<AclUpdateMessage>(it->handle)) {
+        break;
+      }
+    }


while (it < end && holds<Acl>(it)>) ++it would be simpler 🙂

dranikpg · 2023-09-08T08:40:50Z

src/facade/dragonfly_connection.h

@@ -80,8 +80,8 @@ class Connection : public util::Connection {
  struct MonitorMessage : public std::string {};

  struct AclUpdateMessage {
-    std::string_view username;
-    uint64_t categories{0};
+    std::vector<std::string> username;


dranikpg · 2023-09-08T08:43:17Z

tests/dragonfly/acl_family_test.py

+        "aclfile": os.environ.get(
+            "DRAGONFLY_PATH", os.path.join(SCRIPTS_DIR, "../../build-dbg/ok.acl")
+        ),


I meant keeping it inside the pytests folder... It looks kinda fragile. What if its not run from build-dbg? What if I want to edit it? Then I have to search it. And once I edit it the next re-build will overwrite it. So I need to update it in the src/folder... and re-build to apply 🤯

Btw ok.acl is empty 😄

Btw ok.acl is empty

yes it's meant to be like that.

It looks kinda fragile. What if its not run from build-dbg?

It is fragile but we use the exact same thing to run the tests so I guess it's fine ? (See df_factory fixture)

Any other recommendation ?

Also you can set the env variable DRAGONFLY_PATH which overwrites this, so it's fine. We do the exact same thing in the CI as well:

34 export DRAGONFLY_PATH="${GITHUB_WORKSPACE}/${{inputs.build-folder-name}}/${{inputs.dfly-executable}}"

dranikpg · 2023-09-08T08:47:56Z

src/server/acl/acl_family.cc

+void AclFamily::StreamUpdatesToAllProactorConnections(const std::vector<std::string>& user,
+                                                      const std::vector<uint32_t>& update_cat) {


This is called for load and acl changes that occur rarely right? If it ever becomes a bottleneck you can pass the message data inside a shared_ptr so you don't have to copy the vectors every time

and single changes are now propagated with a vector...

Single changes are indeed a vector with a single element, this is insignificant....

This is called for load and acl changes that occur rarely right? If it ever becomes a bottleneck you can pass the message data inside a shared_ptr so you don't have to copy the vectors every time

Yes but I don't think it will so I opted in for the simplest change!

dranikpg · 2023-09-08T08:49:22Z

src/server/acl/acl_family.cc

+void AclFamily::Load() {
+  auto acl_file = absl::GetFlag(FLAGS_aclfile);
+  LoadToRegistryFromFile(acl_file, true);
+}


So what if it fails? The instance will wrongfully start without acl

Nope, it's fine, this is what Redis is doing. Otherwise we have to crash, I don't want to crash DF for that.

Moreover, you can set the ACL file dynamically, via CONFIG SET

We issue a warning and that's enough information :)

dranikpg · 2023-09-08T08:54:11Z

src/server/acl/acl_family.cc

+template <typename T>
+std::variant<User::UpdateRequest, ErrorReply> ParseAclSetUser(T args, bool hashed = false) {


Small nit: Maybe use absl::Span<T>

Just a random thought, using the parser would solve the problem because it could hide the mutability (if an argslice constructor is added)

I will refactor it at some point, I am expecting to add some changes here so I don't want to refactor right now

feat(AclFamily): load/store aclfile

b48878f

kostasrim requested review from dranikpg and adiholden September 7, 2023 17:31

kostasrim commented Sep 7, 2023

View reviewed changes

dranikpg reviewed Sep 7, 2023

View reviewed changes

kostasrim self-assigned this Sep 8, 2023

kostasrim added 2 commits September 8, 2023 11:13

chore: address gh comments, polish and fix pytests

da1db9d

Merge branch 'main' into acl_part_9_load_store

a7f0b56

kostasrim requested a review from dranikpg September 8, 2023 08:13

dranikpg approved these changes Sep 8, 2023

View reviewed changes

kostasrim merged commit 48488c5 into main Sep 8, 2023
7 checks passed

kostasrim deleted the acl_part_9_load_store branch September 8, 2023 11:20

kostasrim mentioned this pull request Sep 12, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AclFamily): load/store aclfile #1820

feat(AclFamily): load/store aclfile #1820

kostasrim commented Sep 7, 2023 •

edited

kostasrim Sep 7, 2023

kostasrim Sep 7, 2023

dranikpg Sep 7, 2023

kostasrim Sep 8, 2023

dranikpg Sep 8, 2023

dranikpg left a comment

dranikpg Sep 7, 2023

dranikpg Sep 7, 2023

dranikpg Sep 7, 2023

dranikpg Sep 7, 2023

kostasrim Sep 8, 2023

dranikpg Sep 7, 2023

dranikpg Sep 7, 2023

dranikpg Sep 8, 2023

dranikpg Sep 8, 2023

dranikpg Sep 8, 2023 •

edited

dranikpg Sep 8, 2023

kostasrim Sep 8, 2023

kostasrim Sep 8, 2023

kostasrim Sep 8, 2023

dranikpg Sep 8, 2023

dranikpg Sep 8, 2023

kostasrim Sep 8, 2023

dranikpg Sep 8, 2023

kostasrim Sep 8, 2023

kostasrim Sep 8, 2023

dranikpg Sep 8, 2023

kostasrim Sep 8, 2023

		@pytest.mark.asyncio
		async def test_bad_acl_file(df_local_factory):

		void AclFamily::StreamUpdatesToAllProactorConnections(const std::vector<std::string>& user,
		const std::vector<uint32_t>& update_cat) {

		template <typename T>
		std::variant<User::UpdateRequest, ErrorReply> ParseAclSetUser(T args, bool hashed = false) {

feat(AclFamily): load/store aclfile #1820

feat(AclFamily): load/store aclfile #1820

Conversation

kostasrim commented Sep 7, 2023 • edited

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

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 Sep 8, 2023 • edited

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

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 7, 2023 •

edited

dranikpg Sep 8, 2023 •

edited