Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11734 )
Change subject: IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/11734/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11734/4//COMMIT_MSG@10 PS4, Line 10: original input role names > I saw CatalogObjectCache is where users/roles are stored from Authorization That's correct. The CatalogObjectCache stores can store they key in a case-sensitive way or case-insensitive way depending on the caseInsensitiveKeys_ flag. See: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java#L74. However, we store the TPrincipal name (https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/Principal.java#L47) in a case-preserving way. I also checked tested the code against the commit prior to the introduction of Principal and it looks like that's been the behavior for quite some time. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java File fe/src/main/java/org/apache/impala/util/SentryProxy.java: http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@172 PS4, Line 172: String > so this is lowercase or can it have casing that does not match the name tha This is lower case as of now. However, it's probably a good idea to not assume Sentry will always send this in a lower case. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@185 PS4, Line 185: existingRole > ... and this contains a name that is not lower-cased? Role.getName() will return the role name in a case-preserving way based on the original input role name. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@186 PS4, Line 186: sentryRole.getRoleName() > afaict, this will get lowercased when performing the lookup. That's correct if we're talking about getting the role name form the catalog. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@198 PS4, Line 198: getRoleName > ... and this is lowercased? Yes, this is lower case as of now. Since both listAllRoles and listAllRolesPriivleges come from Sentry, I would expect they will return the role names in a case-consistent manner. http://gerrit.cloudera.org:8080/#/c/11734/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@249 PS4, Line 249: sentryPrincipalName This is pretty much the problem. http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@386 PS4, Line 386: grp.getgrnam( > what's this? It's to get a group name. http://gerrit.cloudera.org:8080/#/c/11734/4/tests/authorization/test_grant_revoke.py@388 PS4, Line 388: "invalidate metadata" > I think its good to have this test, but is there a more direct way to test The issue is with role names having different cases and not with the privilege names. I can't think of an easy way of testing it without doing E2E test. -- To view, visit http://gerrit.cloudera.org:8080/11734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Gerrit-Change-Number: 11734 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 15:20:23 +0000 Gerrit-HasComments: Yes
