-
Notifications
You must be signed in to change notification settings - Fork 382
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
[#2759] feat(server,core): Add service admin and metalake admin #2758
Conversation
@@ -27,6 +27,9 @@ public class SchemaEntity implements Entity, Auditable, HasIdentifier { | |||
Field.optional("comment", String.class, "The comment or description of the schema"); | |||
public static final Field PROPERTIES = | |||
Field.optional("properties", Map.class, "The properties of the schema"); | |||
public static final String USER_SCHEMA_NAME = "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.
We put special schema names into the SchemaEntity.
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
@Path("/admins") |
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 prefer using admins
instead of metalakeadmins
. It's more clear.
Namespace.of( | ||
BaseMetalake.SYSTEM_METALAKE_RESERVED_NAME, | ||
CatalogEntity.AUTHORIZATION_CATALOG_NAME, | ||
SchemaEntity.ADMIN_SCHEMA_NAME)) |
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.
metalake admin is still a user with some specific permissions, shall we separate them or unify them with normal users but have special permissions?
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.
Metalake admin can't bind to any metalake. It's hard to unify them.
.doc("The admin of Gravitino service") | ||
.version(ConfigConstants.VERSION_0_5_0) | ||
.stringConf() | ||
.create(); |
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 was thinking that we can support more than one service admin using comma separated configuration.
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.
Done.
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.
Don't forget to add this to the doc.
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 have created an issue #2818 to track this. Because I need to split the security document into three parts. I don't add the document in the pull request.
*/ | ||
public boolean isServiceAdmin(String user) { | ||
String admin = GravitinoEnv.getInstance().config().get(Configs.SERVICE_ADMIN); | ||
return admin.equals(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.
We can get the service admin at initialization, no need to get them each time from configration.
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.
Done.
@@ -24,6 +24,8 @@ | |||
@ToString | |||
public class BaseMetalake implements Metalake, Entity, Auditable, HasIdentifier { | |||
|
|||
public static final String SYSTEM_METALAKE_RESERVED_NAME = "system"; |
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'd better have a place to manage all the reserved names, not split them all around.
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.
Done.
@@ -58,6 +59,11 @@ public GravitinoServer(ServerConfig config) { | |||
public void initialize() { | |||
gravitinoEnv.initialize(serverConfig); | |||
|
|||
boolean enableAuthorization = serverConfig.get(Configs.ENABLE_AUTHORIZATION); | |||
if (enableAuthorization && serverConfig.get(Configs.SERVICE_ADMIN) == null) { | |||
throw new IllegalArgumentException("The service admin can't be null"); |
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.
You should add more comments to tell user what's going on 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.
Done.
TreeLockUtils.doWithTreeLock( | ||
ident, | ||
LockType.WRITE, | ||
() -> accessControlManager.addMetalakeAdmin(request.getName())))))); |
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 think it is heavy and not necessary to add a tree lock for access control manager related operations, since we now don't have two step operations, also we can manage the locks in access control manage code, no need to use this global tree 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.
Done. I use own lock.
public AdminManager(EntityStore store, IdGenerator idGenerator, Config config) { | ||
this.store = store; | ||
this.idGenerator = idGenerator; | ||
this.admins = config.get(Configs.SERVICE_ADMINS); |
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 change to serviceAdmin
to distinguish with the metalakeAdmin.
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.
OK.
public static final String USER_SCHEMA_NAME = "user"; | ||
public static final String GROUP_SCHEMA_NAME = "group"; | ||
public static final String ADMIN_SCHEMA_NAME = "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.
I don't think we have to create a new file for this and put it in utils
, we can put all of them in Entity
, WDYT?
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.
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.
This is not fixed, right?
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.
This is fixed. I have moved it to core/src/main/java/com/datastrato/gravitino/meta/EntitySpecificConstants.java
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 can put all of them in Entity
I suggest we move all of them to Entity
class.
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 think you still don't address this issue like what I mentioned above, right?
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.
Sorry to miss this comment. I will correct it.
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.
Done.
@jerryshao All comments are addressed. |
@@ -58,6 +59,13 @@ public GravitinoServer(ServerConfig config) { | |||
public void initialize() { | |||
gravitinoEnv.initialize(serverConfig); | |||
|
|||
boolean enableAuthorization = serverConfig.get(Configs.ENABLE_AUTHORIZATION); |
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 found that this configuration is only used here for checking the validity of service admin configuration.
I think this configuration should control the overall mechanism of AccessControlManager. If this configuration is set to false, I think we don't have to enable access control related mechanism, as well as the related rest endpoints. So please rethink how to leverage this configuration, not just doing a check 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.
Done.
boolean enableAuthorization = serverConfig.get(Configs.ENABLE_AUTHORIZATION); | ||
if (enableAuthorization) { | ||
List<String> accessControlPaths = | ||
Lists.newArrayList("/api/users", "/api/groups", "/api/admins"); |
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.
The path is not correct, it should api/metalakes/{metalake}/users
, right?
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.
Yes, I will correct the paths and test it.
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.
Done.
Gently ping @jerryshao |
resp.sendError( | ||
SC_METHOD_NOT_ALLOWED, | ||
String.format( | ||
"You should set '%s' to true in the server side", |
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.
Make this error message more meaningful to let users to know what's going 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.
Done.
Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse.getCode()); | ||
Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse.getType()); | ||
} | ||
} |
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.
Can you please add tests about authorization set to false.
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.
Done.
@jerryshao All comments are addressed. |
…apache#2758) ### What changes were proposed in this pull request? Role in the Gravitino has 3 kinds: service admins, metalake admins, normal roles. Service admins are responsible for creating metalake admins. They can't be changed by API. It is a config option. Metalake admin is used for creating metalake. It can't be bind to any metalake. So it's hard to unify it with normal users. This pull request add the management of service admins and metalake admins. ### Why are the changes needed? Fix: apache#2759 ### Does this PR introduce _any_ user-facing change? Yes, I will add openapi and document. ### How was this patch tested? New ut. --------- Co-authored-by: Heng Qin <qqtt@123.com>
What changes were proposed in this pull request?
Role in the Gravitino has 3 kinds: service admins, metalake admins, normal roles.
Service admins are responsible for creating metalake admins. They can't be changed by API. It is a config option.
Metalake admin is used for creating metalake. It can't be bind to any metalake. So it's hard to unify it with normal users.
This pull request add the management of service admins and metalake admins.
Why are the changes needed?
Fix: #2759
Does this PR introduce any user-facing change?
Yes, I will add openapi and document.
How was this patch tested?
New ut.