-
Notifications
You must be signed in to change notification settings - Fork 392
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
Changes from 12 commits
691f1c1
ae3d7ae
f1abac8
ea216e6
f5ce9b7
24e1ea7
beb35c3
23bd8e2
2d144dd
f89eab2
f643de6
06780e2
a6134b9
cf6b4bf
43e379b
224737b
e9348dc
7b5ea94
03d4bc8
50c7132
e4b3183
78ed449
1b999c5
69b2aab
2cd9518
e86fecb
af859af
734c3b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
/* | ||
* Copyright 2024 Datastrato Pvt Ltd. | ||
* This software is licensed under the Apache License version 2. | ||
*/ | ||
package com.datastrato.gravitino.authorization; | ||
|
||
import com.datastrato.gravitino.Configs; | ||
import com.datastrato.gravitino.Entity; | ||
import com.datastrato.gravitino.EntityAlreadyExistsException; | ||
import com.datastrato.gravitino.EntityStore; | ||
import com.datastrato.gravitino.GravitinoEnv; | ||
import com.datastrato.gravitino.NameIdentifier; | ||
import com.datastrato.gravitino.Namespace; | ||
import com.datastrato.gravitino.exceptions.UserAlreadyExistsException; | ||
import com.datastrato.gravitino.meta.AuditInfo; | ||
import com.datastrato.gravitino.meta.BaseMetalake; | ||
import com.datastrato.gravitino.meta.CatalogEntity; | ||
import com.datastrato.gravitino.meta.SchemaEntity; | ||
import com.datastrato.gravitino.meta.UserEntity; | ||
import com.datastrato.gravitino.storage.IdGenerator; | ||
import com.datastrato.gravitino.utils.PrincipalUtils; | ||
import com.google.common.collect.Lists; | ||
import java.io.IOException; | ||
import java.time.Instant; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
/** | ||
* There are two kinds of admin roles in the system: service admin and metalake admin. The service | ||
* admin is configured instead of managing by APIs. It is responsible for creating metalake admin. | ||
* If Gravitino enables authorization, service admin is required. Metalake admin can create a | ||
* metalake or drops its metalake. The metalake admin will be responsible for managing the access | ||
* control. | ||
*/ | ||
public class AdminManager { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(AdminManager.class); | ||
|
||
private final EntityStore store; | ||
private final IdGenerator idGenerator; | ||
|
||
public AdminManager(EntityStore store, IdGenerator idGenerator) { | ||
this.store = store; | ||
this.idGenerator = idGenerator; | ||
} | ||
|
||
/** | ||
* Adds a new metalake admin. | ||
* | ||
* @param user The name of the User. | ||
* @return The added User instance. | ||
* @throws UserAlreadyExistsException If a User with the same identifier already exists. | ||
* @throws RuntimeException If adding the User encounters storage issues. | ||
*/ | ||
public User addMetalakeAdmin(String user) { | ||
|
||
UserEntity userEntity = | ||
UserEntity.builder() | ||
.withId(idGenerator.nextId()) | ||
.withName(user) | ||
.withNamespace( | ||
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 commentThe 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 commentThe 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. |
||
.withRoles(Lists.newArrayList()) | ||
.withAuditInfo( | ||
AuditInfo.builder() | ||
.withCreator(PrincipalUtils.getCurrentPrincipal().getName()) | ||
.withCreateTime(Instant.now()) | ||
.build()) | ||
.build(); | ||
try { | ||
store.put(userEntity, false /* overwritten */); | ||
return userEntity; | ||
} catch (EntityAlreadyExistsException e) { | ||
LOG.warn("User {} in the metalake admin already exists", user, e); | ||
throw new UserAlreadyExistsException("User %s in the metalake admin already exists", user); | ||
} catch (IOException ioe) { | ||
LOG.error("Adding user {} failed to the metalake admin due to storage issues", user, ioe); | ||
throw new RuntimeException(ioe); | ||
} | ||
} | ||
|
||
/** | ||
* Removes a metalake admin. | ||
* | ||
* @param user The name of the User. | ||
* @return `true` if the User was successfully removed, `false` otherwise. | ||
* @throws RuntimeException If removing the User encounters storage issues. | ||
*/ | ||
public boolean removeMetalakeAdmin(String user) { | ||
try { | ||
return store.delete(ofMetalakeAdmin(user), Entity.EntityType.USER); | ||
} catch (IOException ioe) { | ||
LOG.error( | ||
"Removing user {} from the metalake admin {} failed due to storage issues", user, ioe); | ||
throw new RuntimeException(ioe); | ||
} | ||
} | ||
|
||
/** | ||
* Judges whether the user is the service admin. | ||
* | ||
* @param user the name of the user | ||
* @return true, if the user is service admin, otherwise false. | ||
*/ | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
|
||
/** | ||
* Judges whether the user is the metalake admin. | ||
* | ||
* @param user the name of the user | ||
* @return true, if the user is metalake admin, otherwise false. | ||
*/ | ||
public boolean isMetalakeAdmin(String user) { | ||
try { | ||
return store.exists(ofMetalakeAdmin(user), Entity.EntityType.USER); | ||
} catch (IOException ioe) { | ||
LOG.error( | ||
"Fail to check whether {} is the metalake admin {} due to storage issues", user, ioe); | ||
throw new RuntimeException(ioe); | ||
} | ||
} | ||
|
||
private NameIdentifier ofMetalakeAdmin(String user) { | ||
return NameIdentifier.of( | ||
BaseMetalake.SYSTEM_METALAKE_RESERVED_NAME, | ||
CatalogEntity.AUTHORIZATION_CATALOG_NAME, | ||
SchemaEntity.ADMIN_SCHEMA_NAME, | ||
user); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
public static final Field ID = | ||
Field.required("id", Long.class, "The metalake's unique identifier"); | ||
public static final Field NAME = Field.required("name", String.class, "The metalake's name"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. We put special schema names into the SchemaEntity. |
||
public static final String GROUP_SCHEMA_NAME = "group"; | ||
public static final String ADMIN_SCHEMA_NAME = "admin"; | ||
|
||
private Long id; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
*/ | ||
package com.datastrato.gravitino.server; | ||
|
||
import com.datastrato.gravitino.Configs; | ||
import com.datastrato.gravitino.GravitinoEnv; | ||
import com.datastrato.gravitino.authorization.AccessControlManager; | ||
import com.datastrato.gravitino.catalog.CatalogManager; | ||
|
@@ -58,6 +59,11 @@ 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
|
||
JettyServerConfig jettyServerConfig = | ||
JettyServerConfig.fromConfig(serverConfig, WEBSERVER_CONF_PREFIX); | ||
server.initialize(jettyServerConfig, SERVER_NAME, true /* shouldEnableUI */); | ||
|
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.