-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
user management in Meta Server #257
Conversation
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of it.
As for User design. I think we should have some concepts firstly.
- Role: it includes admin and normal types
- Entity: it includes Space, Schema
- Privilege: it includes Create/Drop/Alter/Read/Write/Grant
Each user has one role. Admin could grant any Privilege to one user with some entity.
By default, we have Privilege Table list below:
\ | User | Space | Schema |
---|---|---|---|
Admin | cdarwg | cdarw | cdarw |
Normal | - | rw | r |
wdyt ?
@@ -181,6 +184,48 @@ struct GetPartsAllocResp { | |||
3: map<common.PartitionID, list<common.HostAddr>>(cpp.template = "std::unordered_map") parts, | |||
} | |||
|
|||
// user management | |||
struct CreateUserReq { | |||
1: common.GraphSpaceID space_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need space_id here when create user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is that every schema has its own users. Each user has one role. There are only three types of roles:
admin: this is a super role, anything can be done. includes Space, Schema
user: a producer role, can only access(read and write) the data what he belongs to.
guest: is a read-only role for a given Graph Space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boshengchen Please refer the issue #190. A user does not have to be associated with any space, although in most cases, a user has a default space associated. A user could be associated with multiple spaces, but could only have one default space. A user could have different roles in different spaces
@@ -25,6 +25,8 @@ using EdgeType = int32_t; | |||
using EdgeRanking = int64_t; | |||
using EdgeVersion = int64_t; | |||
|
|||
using RoleType = int8_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use one enum to represent RoleType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second @dangleptr
If you want to use 8-bit enum, you can do
enum class RoleType : public int8_t {
...
};
auto key = MetaUtils::userKey(1, "nebula"); | ||
std::string val; | ||
auto ret = kv.get()->get(0, 0, std::move(key), &val); | ||
ASSERT_TRUE(ret == kvstore::ResultCode::SUCCEEDED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSERT_EQ
would be better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch.
// user management | ||
struct CreateUserReq { | ||
1: common.GraphSpaceID space_id, | ||
2: bool missing_ok, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when user existed and missing_ok
is set, the result will be SUCC ? It's make a confuse. What's your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing_ok should be turn on when "IF NOT EXISTS" or " IF EXISTS" is set. the result will be SUCCEEDED whether the user exists or not.
for example:
- CREATE USER [IF NOT EXISTS] username [WITH] [PASSWORD 'password'] [ROLE role_option]
- DROP USER [ IF EXISTS ] username
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have above explanation as a comment
|
||
struct DropUserReq { | ||
1: common.GraphSpaceID space_id, | ||
2: bool missing_ok, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -181,6 +184,48 @@ struct GetPartsAllocResp { | |||
3: map<common.PartitionID, list<common.HostAddr>>(cpp.template = "std::unordered_map") parts, | |||
} | |||
|
|||
// user management | |||
struct CreateUserReq { | |||
1: common.GraphSpaceID space_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateUser
should not indicate space, it's be related with Grunt
struct UserItem { | ||
1: string user_name, | ||
2: string user_pwd, | ||
3: RoleType role, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RoleType
is undefined ?enum is a good choice
{ | ||
ASSERT_EQ(TestUtils::createUser(kv.get(), 1, true, "nebula", "nebula", RoleTypeE::T_ADMIN), | ||
cpp2::ErrorCode::SUCCEEDED); | ||
ASSERT_EQ(TestUtils::createUser(kv.get(), 1, true, "nebula", "nebula", RoleTypeE::T_ADMIN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated with previous code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test purpose : if user exist and missing_ok is turn on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to help other people understand the logic
cpp2::ErrorCode::SUCCEEDED); | ||
} | ||
{ | ||
cpp2::DropUserReq req(FRAGILE, 1, true, "nebula"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateUser, DropUser and another command could join into one
} | ||
|
||
struct ListUsersReq { | ||
1: common.GraphSpaceID space_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
} | ||
|
||
struct AlterUserReq { | ||
1: common.GraphSpaceID space_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to separate the "Change Password" operation from AlterUserReq. "Change password" usually requires the old password
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
return; | ||
} | ||
userVal = MetaUtils::userVal(req.get_role(), req.get_password()); | ||
resp_.set_code(cpp2::ErrorCode::SUCCEEDED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set code here ?
Great work! Thanks for taking care of this Regarding the meaning of different roles, please refer issue #190 |
T_GUEST = 0x03, | ||
}; | ||
|
||
#define LOCK_CHECK(lock) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the first glance on this macro name, I didn't expect it would acquire the lock.
}; | ||
|
||
#define LOCK_CHECK(lock) \ | ||
auto& lock##Lock = LockUtils::lock##Lock(); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent these code properly.
} | ||
|
||
void AlterUserProcessor::process(const cpp2::AlterUserReq& req) { | ||
LOCK_CHECK(user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the first glance, I thought the userLock
had not been released when this function finishes.
I sugguest to rename doPut
to doPutLocked
, and add another interface doPut
if needed who acquires the lock and invoke doPutLocked
. Besides, these interfaces should be commented on the locks' usage.
@dangleptr What do you think?
Jenkins go |
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking care of this.
I think my biggest concern is the PR makes the user as an entity in a given space (the key/value is designed base on that assumption), but that's not the case (Please refer the issue #190). I think we need to redesign the way we store the user information
@@ -25,6 +25,8 @@ using EdgeType = int32_t; | |||
using EdgeRanking = int64_t; | |||
using EdgeVersion = int64_t; | |||
|
|||
using RoleType = int8_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second @dangleptr
If you want to use 8-bit enum, you can do
enum class RoleType : public int8_t {
...
};
@@ -21,6 +21,8 @@ typedef i64 (cpp.type = "nebula::VertexID") VertexID | |||
typedef i32 (cpp.type = "nebula::IPv4") IPv4 | |||
typedef i32 (cpp.type = "nebula::Port") Port | |||
|
|||
typedef byte (cpp.type = "nebula::RoleType") RoleType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like 👍
@@ -73,5 +75,11 @@ struct HostAddr { | |||
2: Port port, | |||
} | |||
|
|||
struct UserItem { | |||
1: string user_name, | |||
2: string user_pwd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at the following code, I understand the password here is encoded. I would highly recommend to add a comment here
In addition, I would suggest to rename it to something like "encoded_pwd". Simple and clear
@@ -24,6 +24,9 @@ enum ErrorCode { | |||
E_NO_HOSTS = -21, | |||
E_SPACE_EXISTED = -22, | |||
E_NOT_FOUND = -23, | |||
E_TABLE_LOCKED = -24, | |||
E_USER_EXISTED = -25, | |||
E_ROLE_ERROR = -26, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If here you meant the user does not have the proper role, I would suggest to name it E_INPROPER_ROLE
@@ -181,6 +184,48 @@ struct GetPartsAllocResp { | |||
3: map<common.PartitionID, list<common.HostAddr>>(cpp.template = "std::unordered_map") parts, | |||
} | |||
|
|||
// user management | |||
struct CreateUserReq { | |||
1: common.GraphSpaceID space_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boshengchen Please refer the issue #190. A user does not have to be associated with any space, although in most cases, a user has a default space associated. A user could be associated with multiple spaces, but could only have one default space. A user could have different roles in different spaces
1: ErrorCode code, | ||
3: list<common.UserItem> users; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to have one more request for assigning users their default spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your idea @sherman-the-tank . I was updated #190 , Let's discuss this via #190.
@@ -9,6 +9,7 @@ | |||
#include "meta/processors/ListHostsProcessor.h" | |||
#include "meta/processors/ListSpacesProcessor.h" | |||
#include "meta/processors/GetPartsAllocProcessor.h" | |||
#include <meta/processors/AuthenticationProcessor.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use <> for our own header files
namespace meta { | ||
|
||
void CreateUserProcessor::process(const cpp2::CreateUserReq& req) { | ||
if (spaceExist(req.get_space_id()) == Status::SpaceNotFound()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we don't need lock here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a read lock in spaceExist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the lock from spaceExist to the top of the method seems more reasonable, WDYT?
{ | ||
ASSERT_EQ(TestUtils::createUser(kv.get(), 1, true, "nebula", "nebula", RoleTypeE::T_ADMIN), | ||
cpp2::ErrorCode::SUCCEEDED); | ||
ASSERT_EQ(TestUtils::createUser(kv.get(), 1, true, "nebula", "nebula", RoleTypeE::T_ADMIN), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment to help other people understand the logic
@@ -26,6 +26,23 @@ enum IDType { | |||
EDGE, | |||
}; | |||
|
|||
class LockUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea here to have a lock factory class. 👍 I prefer this way rather than simply defining a lock in the base class.
As a general rule, I think any base class should not make its lock visible to the inherited classes, especially these inherited classes could be implemented by many other people
This storage structure is inappropriate,I‘ve redesigned it as below, Also comment it in #190 :
|
} | ||
|
||
void DropUserProcessor::process(const cpp2::DropUserReq& req) { | ||
LOCK_CHECK(user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, bool try_lock_shared_for(t)
try to get reader permission on the lock. BUT, here should acquire an exclusive
lock instead of shared
. That is, SharedMutex::WriteHolder
should be used.
@dutor What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good finding 👍
I usually recommend to avoid dealing with locks in macro, but it hides (at least makes it not very clear) the detail and is error prone
Opened a new PR #400 instead of this one. Close this one. |
related issue #180.