[#2234] feat(core): Add the support of User entity #2481

qqqttt123 · 2024-03-08T11:24:42Z

What changes were proposed in this pull request?

Add the UserEntity. Gravitino doesn't manage users, just sets up the relationship between the metalake and the user. So we don't bring too many fields in the User entity. More user information should be managed by external user system.

Why are the changes needed?

Fix: #2234

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT.

jerryshao · 2024-03-08T11:31:56Z

core/src/main/java/com/datastrato/gravitino/meta/UserEntity.java

+  @Override
+  public Namespace namespace() {
+    return namespace;
+  }


Why do we need namespace for User?

User is under a metalake. So we should have a namespace.

it's weird to use namespace here, we don't have a namespace concept for "User".

You can use metalake directly, or remove this interface. Since I don't see the usage of namespace from the current code.

jerryshao · 2024-03-08T11:33:03Z

meta/src/main/proto/gravitino_meta.proto

+  uint64 id = 1;
+  string name = 2;
+  map<string, string> properties = 3;
+  AuditInfo audit_info = 4;


Do you need to add some more basic information for user?

I hesitate to add them. If some properties are very important, we can make them become fields later. At the beginning, we can put them into the properties.

qqqttt123 · 2024-03-14T11:38:30Z

core/src/main/java/com/datastrato/gravitino/meta/MetalakeUser.java

+
+/** A class representing a user metadata entity in Gravitino. */
+@ToString
+public class MetalakeUser implements User, Entity, Auditable, HasIdentifier {


Not to choose to name this CombinedUser Because the underlying system may have different users. We can't map them one to one.
Not to choose to name this BaseUser. Because BaseUser looks like an abstract class although we already have a class BaseMetalake.

I don't like this name, user is just a user, MetalakeUser will make me think that this user is only for metalake.

How about concreteUser, UserImpl and BasicUser?

How about ManagedUser?

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

core/src/main/java/com/datastrato/gravitino/tenant/AccessControlManager.java

core/src/main/java/com/datastrato/gravitino/tenant/SupportsUserManagement.java

api/src/main/java/com/datastrato/gravitino/exceptions/UserAlreadyExistsException.java

jerryshao · 2024-03-15T16:46:12Z

core/src/main/java/com/datastrato/gravitino/meta/MetalakeUser.java

+  public static final Field NAME =
+      Field.required("name", String.class, "The name of the user entity.");
+  public static final Field PROPERTIES =
+      Field.optional("properties", Map.class, "The properties of the user entity.");


So you define properties here, but don't have a related method in the User interface, how do users use this?

I have defined the method properties() in User.java #24L.

jerryshao · 2024-03-15T17:02:04Z

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

@@ -390,6 +407,7 @@ public boolean delete(NameIdentifier ident, EntityType entityType, boolean casca
                    .build());
          }

+          deleteUserEntitiesIfNecessary(ident, entityType);
          return transactionalKvBackend.delete(dataKey);
        });
  }


@yuqi1129 please help to check this part. I don't think the change is enough to support User.

@qqqttt123

If a catalog and a user share the same entity, Gravitino will receive an error, so we need to take actions to prevent this from happening.

You may also need to implement rational entity stores.

How to prevent it?

I prefer doing it in another pr.

How to prevent it?

We need to consider it any way, let me think about it also.

@qqqttt123
We'd better add some comments about it, If the name of a user and catalog is the same, they will share the same id in the id-name mapping, If we drop any one( the user or the catalog), the ID of it may also be removed( though we will not abandon the id of the name when dropping an entity), so I suggest we need to add some comment about it.

You can add the following comments here:

// As the name of users and catalogs shares the same ID if their names are the same, we must be cautious // that we should NOT drop the id when remove a user or catalog, for more please refer to `KvNameMappingService`

https://github.com/datastrato/gravitino/pull/2481/files#:~:text=USER%2C-,new%20String,-%5B%5D%20%7BUSER.

In my thought,

Metalake is a tenant concept us. We have users, groups and roles under the metalake. They will be isolated among metalakes.

What I want is that the semantic is correct to support different operations, no matter what underlying mapping you use.

Besides, I don't think the storage hierarchy should be directly mapping the semantic layer. Like user, it is a concept under the metalake. But we can store them in a system catalog/table, we don't have to store it directly under metalake.

Another potential issue is that renaming may not go as planned. For instance, the catalog 'A' and user A share the same id 1, when we rename catalog A to B, then the mapping then become B to 1 and user A can't be found.

This problem should also exist for other entities, it should be fixed. For example, renaming table from A to B, then the fileset A cannot be found either.

You should never assume that there's only one entity type under the specific namespace.

Do we need to wait for @yuqi1129 's fix or we can merge it beforehand?

yuqi1129 · 2024-03-18T06:38:30Z

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

@@ -390,6 +407,7 @@ public boolean delete(NameIdentifier ident, EntityType entityType, boolean casca
                    .build());
          }

+          deleteUserEntitiesIfNecessary(ident, entityType);
          return transactionalKvBackend.delete(dataKey);
        });
  }


@qqqttt123

If a catalog and a user share the same entity, Gravitino will receive an error, so we need to take actions to prevent this from happening.

You may also need to implement rational entity stores.

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

yuqi1129 · 2024-03-18T06:46:38Z

core/src/main/java/com/datastrato/gravitino/tenant/AccessControlManager.java

+import org.slf4j.LoggerFactory;
+
+/* AccessControlManager is used for manage users, roles, grant information, this class is
+ * an important class for tenant management. */


This appears to be not a standard Java document format.

qqqttt123 · 2024-03-20T07:23:08Z

From the Snowflake, the user model is as follows.

CREATE [ OR REPLACE ] USER [ IF NOT EXISTS ] <name>
  [ objectProperties ]
  [ objectParams ]
  [ sessionParams ]
  [ [ WITH ] TAG ( <tag_name> = '<tag_value>' [ , <tag_name> = '<tag_value>' , ... ] )

objectProperties ::=
  PASSWORD = '<string>'
  LOGIN_NAME = <string>
  DISPLAY_NAME = <string>
  FIRST_NAME = <string>
  MIDDLE_NAME = <string>
  LAST_NAME = <string>
  EMAIL = <string>
  MUST_CHANGE_PASSWORD = TRUE | FALSE
  DISABLED = TRUE | FALSE
  DAYS_TO_EXPIRY = <integer>
  MINS_TO_UNLOCK = <integer>
  DEFAULT_WAREHOUSE = <string>
  DEFAULT_NAMESPACE = <string>
  DEFAULT_ROLE = <string>
  DEFAULT_SECONDARY_ROLES = ( 'ALL' )
  MINS_TO_BYPASS_MFA = <integer>
  RSA_PUBLIC_KEY = <string>
  RSA_PUBLIC_KEY_2 = <string>
  COMMENT = '<string_literal>'


sessionParams ::=
  ABORT_DETACHED_QUERY = TRUE | FALSE
  AUTOCOMMIT = TRUE | FALSE
  BINARY_INPUT_FORMAT = <string>
  BINARY_OUTPUT_FORMAT = <string>
  DATE_INPUT_FORMAT = <string>
  DATE_OUTPUT_FORMAT = <string>
  ERROR_ON_NONDETERMINISTIC_MERGE = TRUE | FALSE
  ERROR_ON_NONDETERMINISTIC_UPDATE = TRUE | FALSE
  JSON_INDENT = <num>
  LOCK_TIMEOUT = <num>
  QUERY_TAG = <string>
  ROWS_PER_RESULTSET = <num>
  SIMULATED_DATA_SHARING_CONSUMER = <string>
  STATEMENT_TIMEOUT_IN_SECONDS = <num>
  STRICT_JSON_OUTPUT = TRUE | FALSE
  TIMESTAMP_DAY_IS_ALWAYS_24H = TRUE | FALSE
  TIMESTAMP_INPUT_FORMAT = <string>
  TIMESTAMP_LTZ_OUTPUT_FORMAT = <string>
  TIMESTAMP_NTZ_OUTPUT_FORMAT = <string>
  TIMESTAMP_OUTPUT_FORMAT = <string>
  TIMESTAMP_TYPE_MAPPING = <string>
  TIMESTAMP_TZ_OUTPUT_FORMAT = <string>
  TIMEZONE = <string>
  TIME_INPUT_FORMAT = <string>
  TIME_OUTPUT_FORMAT = <string>
  TRANSACTION_DEFAULT_ISOLATION_LEVEL = <string>
  TWO_DIGIT_CENTURY_START = <num>
  UNSUPPORTED_DDL_ACTION = <string>
  USE_CACHED_RESULT = TRUE | FALSE
  WEEK_OF_YEAR_POLICY = <num>
  WEEK_START = <num>

We could ignore the session properties. Gravitino doesn't have the concept of the session. For object projecties, some of them may be useful like

displayName
email
defaultRole (If a user have multiple roles which have the privillege to create item, the item use the default role to create item)
comment

qqqttt123 · 2024-03-20T07:33:16Z

From the Databricks, https://docs.databricks.com/api/workspace/users/create
You can see

{
  "schemas": [
    "urn:ietf:params:scim:schemas:core:2.0:User"
  ],
  "id": "string",
  "userName": "user@example.com",
  "emails": [
    {
      "$ref": "string",
      "value": "string",
      "display": "string",
      "primary": true,
      "type": "string"
    }
  ],
  "name": {
    "givenName": "string",
    "familyName": "string"
  },
  "displayName": "string",
  "groups": [
    {
      "$ref": "string",
      "value": "string",
      "display": "string",
      "primary": true,
      "type": "string"
    }
  ],
  "roles": [
    {
      "$ref": "string",
      "value": "string",
      "display": "string",
      "primary": true,
      "type": "string"
    }
  ],
  "entitlements": [
    {
      "$ref": "string",
      "value": "string",
      "display": "string",
      "primary": true,
      "type": "string"
    }
  ],
  "externalId": "string",
  "active": true
}

active seems an important property. Snowflake has similar property DISABLED.
Some of properties are like Snowflake. We can add these properties, too.
It is worth noticing about the property schema. It is related to scim. But I think this may be an advanced property. We can consider it later.

qqqttt123 · 2024-03-20T07:41:24Z

From the Ranger, https://ranger.apache.org/apidocs/resource_XUserREST.html#resource_XUserREST_createXUser_POST

{
  "name" : "...",
  "firstName" : "...",
  "lastName" : "...",
  "emailAddress" : "...",
  "password" : "...",
  "description" : "...",
  "credStoreId" : 12345,
  "groupIdList" : [ 12345, 12345 ],
  "myClassType" : 12345,
  "status" : 12345,
  "isVisible" : 12345,
  "userSource" : 12345,
  "userRoleList" : [ "...", "..." ],
  "groupNameList" : [ "...", "..." ],
  "otherAttributes" : "...",
  "syncSource" : "...",
  "id" : 12345,
  "createDate" : 12345,
  "updateDate" : 12345,
  "owner" : "...",
  "updatedBy" : "..."
}

Some properties are similar to the Snowflake. Description is similar to the comment.

qqqttt123 · 2024-03-20T07:47:07Z

From the surveys above, I think we can add the properties

firstName
lastName
displayName
emailAddress
comment
active
defaultRole
groups
roles

roles and groups may have some consistent issues when we drop a user, because we lack the transaction, but it should be acceptable.
For example,
we delete a user from a group first. Then we delete the user.
If service goes down after we delete a user from a group, the user won't be deleted, but the group has deleted the user.

qqqttt123 · 2024-03-26T03:44:12Z

CI isn't stable. It's not caused by this pull request.

jerryshao · 2024-03-26T17:07:02Z

api/src/main/java/com/datastrato/gravitino/exceptions/UserAlreadyExistsException.java

+import com.google.errorprone.annotations.FormatMethod;
+import com.google.errorprone.annotations.FormatString;
+
+/** An exception thrown when a resource already exists. */


/** An exception thrown when a user already exists. */

jerryshao · 2024-03-26T17:09:56Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

+   * @throws UserAlreadyExistsException If a User with the same identifier already exists.
+   * @throws RuntimeException If adding the User encounters storage issues.
+   */
+  public User addUser(String metalake, String name) throws UserAlreadyExistsException {


I think we can have a UserManager under this AccessControlManager, so that we don't have to put everything in the AccessControlManager.

jerryshao · 2024-03-26T17:21:45Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

+            .withNamespace(
+                Namespace.of(
+                    metalake,
+                    AuthorizationConstants.SYSTEM_CATALOG_RESERVED_NAME,
+                    AuthorizationConstants.USER_SCHEMA_NAME))


You'd better define this reserved Namespace in the entity/user entity, not here in auth constants.

Put SYSTEM_CATALOG_RESERVED_NAME into CatalogEntity.
Put USER_SCHEMA_NAME into UserEntity.

jerryshao · 2024-03-26T17:24:50Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

+    try {
+      return store.get(ofUser(metalake, user), Entity.EntityType.USER, UserEntity.class);
+    } catch (NoSuchEntityException e) {
+      LOG.warn("user {} does not exist in the metalake {}", user, metalake, e);


Capitalize the first letter.

jerryshao · 2024-03-26T17:25:52Z

core/src/main/java/com/datastrato/gravitino/catalog/CatalogManager.java

@@ -268,6 +269,10 @@ public Catalog createCatalog(
      Map<String, String> properties)
      throws NoSuchMetalakeException, CatalogAlreadyExistsException {

+    if (AuthorizationConstants.SYSTEM_CATALOG_RESERVED_NAME.equals(ident.name())) {


I think this should also added to the catalog's common rule? @mchades

jerryshao · 2024-03-26T17:27:39Z

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java

+            .startInclusive(true)
+            .end(Bytes.increment(Bytes.wrap(prefix)).get())
+            .build());
+  }


What is the purpose of this method, I cannot get it.

If we delete a metalake, we should remove the users under the metalake.

qqqttt123 · 2024-03-27T07:28:20Z

I resolved the conflict with the main branch.

jerryshao · 2024-03-27T16:58:18Z

core/src/main/java/com/datastrato/gravitino/authorization/UserManager.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** UserManager is used for manage users */


This is meaningless, please add more meaningful description.

jerryshao · 2024-03-27T16:58:35Z

core/src/main/java/com/datastrato/gravitino/authorization/UserManager.java

+  private final IdGenerator idGenerator;
+
+  /**
+   * Constructs a User instance.


What is "User instance"?

Done. Removed the comment.

jerryshao · 2024-03-27T17:05:19Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

+   *
+   * @param store The EntityStore to use for managing access control.
+   * @param idGenerator The IdGenerator to use for generating identifiers.
+   */


I think we can remove some of the javadoc here, it's not so useful.

done. Removed the comment.

qqqttt123 self-assigned this Mar 8, 2024

jerryshao reviewed Mar 8, 2024

View reviewed changes

qqqttt123 force-pushed the ISSUE-2234 branch from 003c9e3 to 360489b Compare March 14, 2024 11:13

qqqttt123 commented Mar 14, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/storage/kv/KvEntityStore.java Outdated Show resolved Hide resolved

qqqttt123 commented Mar 14, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/tenant/AccessControlManager.java Outdated Show resolved Hide resolved

qqqttt123 commented Mar 14, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/tenant/SupportsUserManagement.java Outdated Show resolved Hide resolved

qqqttt123 changed the title ~~[#2234] core: Add the UserEntity~~ [#2234] core: Add the UserEntity and UserOperation Mar 14, 2024

qqqttt123 changed the title ~~[#2234] core: Add the UserEntity and UserOperation~~ [#2234][#2238] core: Add the UserEntity and UserOperation Mar 14, 2024

qqqttt123 changed the title ~~[#2234][#2238] core: Add the UserEntity and UserOperation~~ [#2234][#2238] core: Add the support of User Mar 14, 2024

qqqttt123 closed this Mar 14, 2024

qqqttt123 reopened this Mar 14, 2024

qqqttt123 force-pushed the ISSUE-2234 branch from d4984a3 to 21ae721 Compare March 15, 2024 01:41

qqqttt123 changed the title ~~[#2234][#2238] core: Add the support of User~~ [#2234][#2238] feat(core): Add the support of User Mar 15, 2024

qqqttt123 force-pushed the ISSUE-2234 branch 2 times, most recently from 1e79fd7 to 2808f2d Compare March 15, 2024 03:39

qqqttt123 changed the title ~~[#2234][#2238] feat(core): Add the support of User~~ [#2234] feat(core): Add the support of User entity Mar 15, 2024

jerryshao requested review from FANNG1, yuqi1129 and mchades March 15, 2024 16:42

jerryshao reviewed Mar 15, 2024

View reviewed changes

api/src/main/java/com/datastrato/gravitino/exceptions/UserAlreadyExistsException.java Show resolved Hide resolved

jerryshao reviewed Mar 15, 2024

View reviewed changes

yuqi1129 reviewed Mar 18, 2024

View reviewed changes

apache deleted a comment from jerqi Mar 19, 2024

qqqttt123 closed this Mar 26, 2024

qqqttt123 reopened this Mar 26, 2024

jerryshao reviewed Mar 26, 2024

View reviewed changes

Heng Qin added 7 commits March 27, 2024 10:07

Address comments

55185db

fix style

b39bf36

remove

69e8284

Merge remote-tracking branch 'upstream/main' into ISSUE-2234

a597089

resolve conflict

5066ca0

fix

fd698dd

compile

f909921

Heng Qin added 7 commits March 27, 2024 16:21

Readd the file

20d958d

Remove groups

9ae0a71

Remove protobuf fields

39a0c58

rever unnecessary change

41765fd

fix ut

79b88c3

Add annotation

cda7b99

fix style

5e20699

jerryshao reviewed Mar 27, 2024

View reviewed changes

Heng Qin added 6 commits March 28, 2024 09:48

Address comments

1bf0a02

fix grammar error

ea26bbc

fix style

332a93d

fix

f8bfed4

fix

804a43a

Merge remote-tracking branch 'upstream/main' into ISSUE-2234

5388cb7

jerryshao approved these changes Mar 29, 2024

View reviewed changes

jerryshao merged commit 6641d70 into apache:main Mar 29, 2024
14 checks passed

[#2234] feat(core): Add the support of User entity #2481

[#2234] feat(core): Add the support of User entity #2481

Conversation

qqqttt123 commented Mar 8, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

qqqttt123 Mar 11, 2024 • 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

jerryshao Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 Mar 22, 2024 • 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

yuqi1129 Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Mar 20, 2024 • edited Loading

qqqttt123 commented Mar 20, 2024 • edited Loading

qqqttt123 commented Mar 20, 2024

qqqttt123 commented Mar 20, 2024 • edited Loading

qqqttt123 commented Mar 26, 2024

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

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

qqqttt123 commented Mar 27, 2024

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

qqqttt123 commented Mar 8, 2024 •

edited

Loading

qqqttt123 Mar 11, 2024 •

edited

Loading

jerryshao Mar 15, 2024 •

edited

Loading

yuqi1129 Mar 18, 2024 •

edited

Loading

yuqi1129 Mar 22, 2024 •

edited

Loading

yuqi1129 Mar 18, 2024 •

edited

Loading

qqqttt123 commented Mar 20, 2024 •

edited

Loading

qqqttt123 commented Mar 20, 2024 •

edited

Loading

qqqttt123 commented Mar 20, 2024 •

edited

Loading