feat(AclFamily): add acl log #1865

kostasrim · 2023-09-15T16:03:04Z

add acl log command
add very basic testing because of broken redis pyclient

Resolves #1808

kostasrim · 2023-09-15T16:15:27Z

src/facade/dragonfly_connection.cc

+  auto end = res.find("irq");
+  res.erase(start, end - start);
+
+  // The following are dummy fields and users should not rely on those unless


Ok, apparently I am running an "old" version of the client (mine is 4.5).

I think this is fixed for 5.0. We will need to update the version for pytests + test on 5.0. Will ping about this on Monday.

kostasrim · 2023-09-15T16:21:12Z

I had to slightly diverge from the original implementation https://redis.io/commands/acl-log/

In particular:

acllog-size is multiplied by the number of proactors. I do not want to introduce a single global acl-log and for that, if start up df with acllog-size of 36 and 4 proactors, that means that we log a total of 36 * 4 elements.
I omitted some of the fields. In particular count, because each time we would need to search the log entry within the log. This can be done but because it's on the hot path I do not want to introduce that. There is literally no point in doing that. Maybe this can be done when we gather the acl logs during an ACL LOG but I didn't bother.
There is no "timestamp-last-updated" since our implementation works only as an append only log (because of (2))
The fields top-level and entry-id are omitted. The former because we don't yet support acl for modules and the later because entry-id doesn't really make sense since we hold logs per proactor and the entry is relative. (Yes we could do something clever based on timestamp but it does not worth the effort for now)

kostasrim · 2023-09-15T16:22:12Z

@dranikpg I need to fix the tests, it's a small but + investigate for the pyclient version 5. Do not review :)

royjacobson · 2023-09-18T05:57:34Z

src/server/acl/acl_family.cc

+  }
+
+  std::vector<AclLog::LogType> logs(pool_->size());
+  pool_->AwaitFiberOnAll(


Maybe nicer to pass max_output into GetLog() so you don't have to copy redundant data in the first place.

royjacobson · 2023-09-18T06:06:20Z

src/server/acl/acl_family.cc

+      (*cntx)->StartArray(12);
+      (*cntx)->SendSimpleString("reason");
+      using Reason = AclLog::Reason;
+      std::string reason = entry.reason == Reason::COMMAND ? "COMMAND" : "AUTH";


Suggested change

std::string reason = entry.reason == Reason::COMMAND ? "COMMAND" : "AUTH";

std::string_view reason = entry.reason == Reason::COMMAND ? "COMMAND" : "AUTH";

I will make the change but this doesn't offer anything. SSO will kick in anyway for the string...

royjacobson · 2023-09-18T06:07:11Z

src/server/acl/acl_family.cc

+  }
+
+  (*cntx)->StartArray(total_entries);
+  for (auto& log : logs) {


We should merge + sort by timestamp otherwise the first shard can shadow everything from the other shards.

royjacobson · 2023-09-18T06:09:39Z

src/server/acl/acl_log.h

+
+ private:
+  LogType log_;
+  const size_t total_entries_allowed_;


We should make this configurable in runtime as well

Yep, just like acllog. This will be on a different PR

dranikpg

I'll drop my comments as well if Roy did 😆 (I've seen you asked not to review)

dranikpg · 2023-09-15T17:05:59Z

tests/dragonfly/acl_family_test.py

+        await async_client.execute_command("AUTH elon wrong")
+
+    res = await async_client.execute_command("ACL LOG")
+    assert 1 == len(res)


I'd test at least once the array has meaningful contents 🙂

dranikpg · 2023-09-15T17:15:52Z

src/server/acl/acl_family.h

@@ -23,7 +23,7 @@ namespace acl {

 class AclFamily final {
 public:
-  explicit AclFamily(UserRegistry* registry);
+  explicit AclFamily(UserRegistry* registry, util::ProactorPool* pool);


global shard_set->pool() is our default proactor pool

which is the same:

shard_set = new EngineShardSet(pp);

and above that:

629 Service::Service(ProactorPool* pp) 630 : pp_(*pp), 631 acl_family_(&user_registry_, pp), 632 server_family_(this), 633 cluster_family_(&server_family_) {

so shard_set->pool() returns pp 😛

Yes, so I meant that you don't need to pass it around (almost none of our code does) 😝

dranikpg · 2023-09-15T17:16:27Z

src/server/server_state.h

  acl::UserRegistry* user_registry;

+  acl::AclLog log;


I'd prefix acl stuff with acl_

dranikpg · 2023-09-15T17:17:46Z

src/server/acl/acl_log.cc

+  using clock = std::chrono::system_clock;
+  LogEntry entry = {std::move(username), std::move(client_info), std::move(object), reason,
+                    clock::now()};
+  log_.push_front(std::move(entry));


emplace_back 👀

it's sorted by time, that's why I am pushing at the front... Then we can perform an m-way merge of the sorted logs to finally print the output

Ah, I missed the "front". And you use a deque, right. We had a ring buffer in helio but it doesn't matter in this context

src/facade/dragonfly_connection.cc

dranikpg · 2023-09-15T17:20:37Z

src/facade/dragonfly_connection.cc

+  auto start = res.find("tid");
+  auto end = res.find("irq");
+  res.erase(start, end - start);


This looks fragile 🙂

dranikpg · 2023-09-15T17:24:21Z

src/facade/dragonfly_connection.cc

+  // we decide to implement them.
+  // This is only done because the redis pyclient parser for the field "client-info"
+  // for the command ACL LOG
+  // is completely *BROKEN* and it literally hardcodes the expected values.
+  // What is more preposterous is that that it actually doesn't conform to the
+  // actual expected values, since it's missing half of them. That is, even for
+  // redis-server, issuing an ACL LOG command via redis-cli and the pyclient
+  // will return different results! An example of ACL LOG ran with redis-cli is:
+  //
+  // "client-info"
+  // 14) "id=3 addr=127.0.0.1:57275 laddr=127.0.0.1:6379 fd=8 name= age=16
+  //      idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 qbuf=48 qbuf-free=16842
+  //      argv-mem=25 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0
+  //      tot-mem=18737 events=r cmd=auth user=default redir=-1 resp=2"
+  //
+  // Meanwhile the parser in the pyclient:
+  //


You forgot to add what the pyclient returns. It might very well be that it expects hardcoded values, but as long as it worked for hundreds of thousands of people I assume its not seriously broken. I'm also not sure we need this long comment 😆, you can just add a comment above strappends below that this if for compatibility with redis-py 4.X

It was completely broken. It basically filtered most of the output fields.

worked for hundreds of thousands of people I assume its not seriously broken

It worked so well that they refactored all of its code on the next version 😛

you can just add a comment above strappends below that this if for compatibility with redis-py 4.X

Yes I want to first check if all works with 5.0. But it was a rollercoaster, because my tests would fail because the pyclient could not parse the correct by the protocol output

dranikpg · 2023-09-15T17:29:53Z

src/server/acl/acl_log.cc

+ABSL_FLAG(size_t, acllog_max_len, 32,
+          "Specify the number of log entries. Logs are kept locally for each proactor "
+          "and therefore the total number of entries are acllog_max_len * proactors");


This might be confusing to people who don't know what a "proactor" is, lets call it thread. Also I assume that those events (unsuccessful login & unauthorized execution) are not that common to not use a centralized counter for this

dranikpg · 2023-09-18T16:36:06Z

src/server/acl/acl_family.h

@@ -23,7 +23,7 @@ namespace acl {

 class AclFamily final {
 public:
-  explicit AclFamily(UserRegistry* registry);
+  explicit AclFamily(UserRegistry* registry, util::ProactorPool* pool);


Yes, so I meant that you don't need to pass it around (almost none of our code does) 😝

kostasrim · 2023-09-18T16:56:29Z

Yes, so I meant that you don't need to pass it around (almost none of our code does)

Ohhh, I will fix this on the next PR <3

feat(AclFamily): add acl log

437bba9

kostasrim requested a review from dranikpg September 15, 2023 16:03

kostasrim commented Sep 15, 2023

View reviewed changes

royjacobson reviewed Sep 18, 2023

View reviewed changes

dranikpg reviewed Sep 18, 2023

View reviewed changes

kostasrim added 2 commits September 18, 2023 19:31

chore: address gh comments

1974b47

Merge branch 'main' into acl_part_11_acl_log

b2e2df3

kostasrim requested review from dranikpg and royjacobson September 18, 2023 16:33

dranikpg approved these changes Sep 18, 2023

View reviewed changes

kostasrim merged commit 8907619 into main Sep 18, 2023
14 checks passed

kostasrim deleted the acl_part_11_acl_log branch September 18, 2023 17:10

dranikpg pushed a commit that referenced this pull request Sep 19, 2023

feat(AclFamily): add acl log (#1865)

eb458f1

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): add acl log #1865

feat(AclFamily): add acl log #1865

kostasrim commented Sep 15, 2023

kostasrim Sep 15, 2023

kostasrim commented Sep 15, 2023

kostasrim commented Sep 15, 2023

royjacobson Sep 18, 2023

royjacobson Sep 18, 2023

kostasrim Sep 18, 2023 •

edited

royjacobson Sep 18, 2023

royjacobson Sep 18, 2023

kostasrim Sep 18, 2023

royjacobson Sep 18, 2023

kostasrim Sep 18, 2023

dranikpg left a comment •

edited

dranikpg Sep 15, 2023

dranikpg Sep 15, 2023

kostasrim Sep 18, 2023 •

edited

dranikpg Sep 18, 2023

dranikpg Sep 15, 2023

dranikpg Sep 15, 2023

kostasrim Sep 18, 2023 •

edited

dranikpg Sep 18, 2023

dranikpg Sep 15, 2023

dranikpg Sep 15, 2023

kostasrim Sep 18, 2023 •

edited

dranikpg Sep 15, 2023

dranikpg Sep 18, 2023

kostasrim commented Sep 18, 2023

	std::string reason = entry.reason == Reason::COMMAND ? "COMMAND" : "AUTH";
	std::string_view reason = entry.reason == Reason::COMMAND ? "COMMAND" : "AUTH";

feat(AclFamily): add acl log #1865

feat(AclFamily): add acl log #1865

Conversation

kostasrim commented Sep 15, 2023

Choose a reason for hiding this comment

kostasrim commented Sep 15, 2023

kostasrim commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 18, 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

dranikpg left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 18, 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

kostasrim Sep 18, 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

kostasrim Sep 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Sep 18, 2023

kostasrim Sep 18, 2023 •

edited

dranikpg left a comment •

edited

kostasrim Sep 18, 2023 •

edited

kostasrim Sep 18, 2023 •

edited

kostasrim Sep 18, 2023 •

edited