feat:pika support acl #2013

lqxhub · 2023-09-22T13:10:02Z

related issue #2190

这个是 pika 支持 Redis acl的功能, 目前 redis 的 acl相关测试已经跑通

把上次的review发现的问题已经修复

在这个pr中, 把 hello 命令的问题一起解决了 #2190

目前根据Redis的分类标准，对命令做了简单分类，但是有一个问题。Redis中 , config get 和 config set 属于两个命令，这个两个命令可以属于不同的分类(category)，但是在pika中, 这两个命令属于同一个命令，然后根据不同的参数做了区分，执行不同的逻辑，所以在pika中只能属于同一个分类. 类似的还有 ACL 相关的命令等等有子命令的命令。

如果对这个部分有建议和不用的看法还请不吝赐教

有关 pika ACL 相关的讨论在这个issue下 #1284

对以前的改动:

配置文件中去掉了原来的 userpass,只保留requirepass配置, 和redis保持相同
读取配置文件加了 GetConfStrMulti 函数,实现多行读取 https://github.com/OpenAtomFoundation/pika/pull/2013/files#diff-02ec4106d4dadab327423f6791330502cb6365957d105a74f43018266c7f43baR234
修复pstd::StringTrim 函数错误,并加单测 https://github.com/OpenAtomFoundation/pika/pull/2013/files#diff-7880ecffec99b660051b9ca83b4fb3f624a29f2ff91465e447abb58f846d21e2R719
对以前所有命令的 flag 做了补充,加入了ACL需要的参数
对以前的 CmdFlags 做了修改 https://github.com/OpenAtomFoundation/pika/pull/2013/files#diff-96b2e654cfee5e668db837f1c01c4cbaea767b949413087a1093e0327f8f4438R243
对 CmdRes 做了修改 https://github.com/OpenAtomFoundation/pika/pull/2013/files#diff-96b2e654cfee5e668db837f1c01c4cbaea767b949413087a1093e0327f8f4438L274

ACL的主要实现逻辑在 acl.h 和 acl.cc文件中, acl命令处理在 pika_acl.h 和 pika_acl.cc中

AlexStocks · 2023-09-23T03:26:05Z

这个实现作为 3.5.3。先把 ci 失败处理了。

lqxhub · 2023-09-23T04:04:53Z

这个实现作为 3.5.3。先把 ci 失败处理了。

好的在看失败原因了

# Conflicts: # src/pika_server.cc

# Conflicts: # include/pika_server.h # src/pika_server.cc

dingxiaoshuai123 · 2023-12-18T08:11:00Z

src/acl.cc

+      }
+      delpass = op.data() + 1;
+    }
+    //    passwords_.erase(delpass);


这个注释不用保留了吧，已经调用RemovePassword了

可以去掉

dingxiaoshuai123 · 2023-12-18T08:32:16Z

src/acl.cc

+  } else if (!strcasecmp(op.data(), "clearselectors")) {
+    selectors_.clear();
+    return pstd::Status::OK();
+  } else if (!strcasecmp(op.data(), "reset")) {


这里面是否需要调用 SetUser(clearselectors)? 我看redis文档中，reset需要清空selector

效果都是一样的吧, 这里的selectors_.clear() 把 selectors_ 这个链表清空了

在 131行, 调用了status = SetUser("-@all"); 把 selector 设置为 -@all 了,所以不用 clean, redis也是这样处理的

dingxiaoshuai123 · 2023-12-18T09:11:43Z

include/acl.h

+
+  std::set<std::string> passwords_;  // passwords for this user
+
+  std::list<std::shared_ptr<AclSelector>> selectors_; /* A set of selectors this user validates commands


如果只需要单向遍历的话，是否使用std::forward_list比list有更好的空间利用率？

一开始没考虑那么多直接用了 list, 后来想换成 std::forward_list 的时候发现了两个问题

std::forward_list 不能 push_back, 这个问题不大

std::forward_list 不能 for (:) 遍历, 改动的地方挺多的

考虑到这两个差别不大就一直没改, 如果性能性能空间差距较大的话可以改了

行，这个数据量不大的情况下就不改了吧

AlexStocks · 2023-12-18T10:24:29Z

@lqxhub pls merge latest codes

dingxiaoshuai123 · 2023-12-18T10:29:25Z

src/acl.cc

+  AclDeniedCmd res = AclDeniedCmd::OK;
+  for (const auto& selector : selectors_) {
+    res = selector->CheckCanExecCmd(cmd, subCmdIndex, keys, errKey);
+    if (res == AclDeniedCmd::OK) {


这里的逻辑是不是写反了？顺序检查所有的Selector，如果符合一个Selector，直接就返回OK了，后面的Selector都没有执行检查。

这里的逻辑是不是写反了？顺序检查所有的Selector，如果符合一个Selector，直接就返回OK了，后面的Selector都没有执行检查。

这个逻辑应该就是这样的, 多个 selector中,有一个符合就可以通过

奥，好的

lqxhub · 2023-12-18T10:51:10Z

@lqxhub pls merge latest codes

已经merge了, 因为测试不通过我还没有提交, 等我修复缓存的那个pr合了再提吧

src/acl.cc

dingxiaoshuai123 · 2023-12-19T09:04:27Z

conf/pika.conf

+# resetchannels: revokes access to all Pub/Sub channels
+#
+# acl-pubsub-default defaults to 'resetchannels' permission.
+# acl-pubsub-default : resetchannels


这个配置怎么注释了？

这段配置全用默认就行,所以就没开

pika的配置文件风格是什么?

src/pika_dispatch_thread.cc

dingxiaoshuai123 · 2023-12-19T12:14:18Z

src/pika_acl.cc

+    return;
+  }
+
+  PikaCmdArgsType args;


下面是redis

dingxiaoshuai123 · 2023-12-20T11:06:05Z

在添加channel的规则的时候，会先检查key pattern，如果已经存在，那就不会将channel的规则添加到channels中，所以导致channel命令没有权限。上图为pika，下图为redis

# Conflicts: # include/pika_admin.h # include/pika_bit.h # include/pika_command.h # include/pika_geo.h # include/pika_hash.h # include/pika_kv.h # include/pika_list.h # include/pika_pubsub.h # include/pika_set.h # include/pika_transaction.h # include/pika_zset.h # src/pika_command.cc

lqxhub · 2023-12-30T14:48:59Z

在添加channel的规则的时候，会先检查key pattern，如果已经存在，那就不会将channel的规则添加到channels中，所以导致channel命令没有权限。上图为pika，下图为redis

done

# Conflicts: # include/pika_conf.h

AlexStocks · 2024-01-07T07:20:37Z

include/pika_conf.h

-    std::shared_lock l(rwlock_);
-    return user_blacklist_;
-  }
+  //  std::string userpass() {


怎么还有这种注释代码？如果不需要，可以删除

AlexStocks · 2024-01-07T07:20:47Z

include/pika_conf.h

-    }
-  }
-  void SetSlotMigrate(const std::string &value) {
+  //  void SetUserPass(const std::string& value) {


AlexStocks · 2024-01-07T07:29:39Z

src/pika_slot_command.cc

-        return nullptr;
-      }
-    }
+//    std::string userpass = g_pika_conf->userpass();


这些代码留着是为了什么？

这些都是在改原来代码的时候,先注释原来的,然后再改的, 后面就一直没有删, 等会我统一删了

AlexStocks · 2024-01-07T07:32:33Z

tests/unit/acl.tcl

+    }
+}
+
+# test on local machine is passed


这个测试注释掉，是为何？

这个是因为这个测试很慢, 有时候重启pika会有问题, 我本地测试没问题后就去掉了

copy from redis acl --------- Co-authored-by: Chengyu Liu <[email protected]>

lqxhub force-pushed the acl branch from d7f9fc1 to 1dba127 Compare September 22, 2023 13:20

lqxhub force-pushed the acl branch from 0c8f5fd to e4836fc Compare October 15, 2023 10:27

lqxhub force-pushed the acl branch from 2c2ed36 to 9977fe2 Compare November 8, 2023 14:32

lqxhub marked this pull request as ready for review November 9, 2023 01:33

github-actions bot added the Invalid PR Title label Nov 24, 2023

lqxhub changed the title ~~pika support acl~~ feat:pika support acl Nov 24, 2023

github-actions bot removed the Invalid PR Title label Nov 24, 2023

lqxhub linked an issue Nov 24, 2023 that may be closed by this pull request

Pika 支持 ACL 多租户功能 #1284

Closed

AlexStocks requested a review from dingxiaoshuai123 November 24, 2023 12:03

lqxhub added 19 commits November 24, 2023 21:25

resolve file conflict

adfcba3

add acl sub command

66f29fd

acl load rule, a part set user

2e88a06

acl load done

0585996

done acl rule

9ab0f4c

All but authentication is done

1104bb1

a part of acl check

bffaf08

fix acl permission

f4e4ce3

rebase onto unstable, format code

0136f6b

Unified cmd flag

0619a15

add acl genpass

136dacb

tidy cmd flags and aclCategory

c72193b

fix build fail on macos

ae4bad3

fix build fail

47bdae6

fix auth fail

3e21f83

fix write categories command execute fail

0b1adcc

Fix according to test script

5bbe144

Fixed multi-thread locking errors and modified E2E tests

e77af36

add acl log

9f1a3a7

lqxhub added 4 commits December 7, 2023 23:34

Modify data length

846f656

Merge remote-tracking branch 'upstream/unstable' into acl

50067ff

# Conflicts: # src/pika_server.cc

fix cmd flag error

16aa1f9

Merge remote-tracking branch 'upstream/unstable' into acl

efe080a

# Conflicts: # include/pika_server.h # src/pika_server.cc

dingxiaoshuai123 reviewed Dec 18, 2023

View reviewed changes

dingxiaoshuai123 reviewed Dec 19, 2023

View reviewed changes

src/acl.cc Show resolved Hide resolved

dingxiaoshuai123 reviewed Dec 19, 2023

View reviewed changes

src/pika_dispatch_thread.cc Show resolved Hide resolved

dingxiaoshuai123 reviewed Dec 19, 2023

View reviewed changes

src/pika_acl.cc

return;

}

PikaCmdArgsType args;

Copy link

Collaborator

dingxiaoshuai123 Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

下面是redis

lqxhub added 2 commits December 30, 2023 16:53

merge upstream and fix review error

2949c2e

Merge remote-tracking branch 'upstream/unstable' into acl

cb7bec3

# Conflicts: # include/pika_conf.h

AlexStocks reviewed Jan 7, 2024

View reviewed changes

lqxhub and others added 2 commits January 7, 2024 16:37

Delete useless code

93a6f05

Merge branch 'unstable' into acl

b255e1f

chengyu-l approved these changes Jan 10, 2024

View reviewed changes

chengyu-l merged commit b7f6c58 into OpenAtomFoundation:unstable Jan 10, 2024
10 of 13 checks passed

KKorpse pushed a commit to KKorpse/pika that referenced this pull request Jan 11, 2024

feat:pika support acl (OpenAtomFoundation#2013)

9c7e27c

copy from redis acl --------- Co-authored-by: Chengyu Liu <[email protected]>

bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024

feat:pika support acl (OpenAtomFoundation#2013)

8d67486

copy from redis acl --------- Co-authored-by: Chengyu Liu <[email protected]>

cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024

feat:pika support acl (OpenAtomFoundation#2013)

93598a3

copy from redis acl --------- Co-authored-by: Chengyu Liu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:pika support acl #2013

feat:pika support acl #2013

lqxhub commented Sep 22, 2023 •

edited

Loading

AlexStocks commented Sep 23, 2023

lqxhub commented Sep 23, 2023

dingxiaoshuai123 Dec 18, 2023

lqxhub Dec 18, 2023

dingxiaoshuai123 Dec 18, 2023 •

edited

Loading

lqxhub Dec 18, 2023

lqxhub Dec 30, 2023

dingxiaoshuai123 Dec 18, 2023

lqxhub Dec 18, 2023

dingxiaoshuai123 Dec 18, 2023

AlexStocks commented Dec 18, 2023

dingxiaoshuai123 Dec 18, 2023

lqxhub Dec 18, 2023

dingxiaoshuai123 Dec 18, 2023

lqxhub commented Dec 18, 2023

dingxiaoshuai123 Dec 19, 2023

lqxhub Dec 19, 2023

dingxiaoshuai123 Dec 19, 2023

dingxiaoshuai123 commented Dec 20, 2023 •

edited

Loading

lqxhub commented Dec 30, 2023

AlexStocks Jan 7, 2024

lqxhub Jan 7, 2024

AlexStocks Jan 7, 2024

AlexStocks Jan 7, 2024

lqxhub Jan 7, 2024

AlexStocks Jan 7, 2024

lqxhub Jan 7, 2024


		std::set<std::string> passwords_; // passwords for this user

		std::list<std::shared_ptr<AclSelector>> selectors_; /* A set of selectors this user validates commands

feat:pika support acl #2013

feat:pika support acl #2013

Conversation

lqxhub commented Sep 22, 2023 • edited Loading

AlexStocks commented Sep 23, 2023

lqxhub commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiaoshuai123 Dec 18, 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

AlexStocks commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqxhub commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxiaoshuai123 commented Dec 20, 2023 • edited Loading

lqxhub commented Dec 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

lqxhub commented Sep 22, 2023 •

edited

Loading

dingxiaoshuai123 Dec 18, 2023 •

edited

Loading

dingxiaoshuai123 commented Dec 20, 2023 •

edited

Loading