Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5582: Store sentry privileges in lower case
......................................................................
Patch Set 2:
(1 comment)
I think we need to figure out a way to test this. A test could be added to
test_grant_revoke.py which includes a delay. This might be a good time to
expose a way to configure the Sentry polling period, see SentryProxy.java
// Sentry Service is enabled.
// TODO: Make this configurable
policyReader_.scheduleAtFixedRate(new PolicyReader(), 0, 60,
TimeUnit.SECONDS);
http://gerrit.cloudera.org:8080/#/c/7332/2/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java:
PS2, Line 132: if (dbName_ != null)
privilege.setDb_name(dbName_.toLowerCase());
: if (tableName_ != null)
privilege.setTable_name(tableName_.getTbl().toLowerCase());
: if (uri_ != null) privilege.setUri(uri_.toString());
: if (columnName != null)
privilege.setColumn_name(columnName.toLowerCase());
This seems like it could be a brittle fix, because it looks like the relevant
check for removal upon the Sentry policy update thread is checking the
"privilege name" (SentryProxy.java:148 - see below). So if we ever change what
gets stored in the "privilege name" (e.g. adding a field) or create TPrivileges
in some other way, then we can have this issue again. I think it probably makes
sense to change RolePrivilege.buildRolePrivilegeName() to lower case the
returned privilege name.
// Assume all privileges should be removed. Privileges that still
exist are
// deleted from this set and we are left with the set of privileges
that need
// to be removed.
Set<String> privilegesToRemove = role.getPrivilegeNames();
// Check all the privileges that are part of this role.
for (TSentryPrivilege sentryPriv:
sentryPolicyService_.listRolePrivileges(processUser_,
role.getName())) {
TPrivilege thriftPriv =
SentryPolicyService.sentryPrivilegeToTPrivilege(sentryPriv);
thriftPriv.setRole_id(role.getId());
privilegesToRemove.remove(thriftPriv.getPrivilege_name().toLowerCase());
--
To view, visit http://gerrit.cloudera.org:8080/7332
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ide3dfa601fcf77f5acc6adce9bea443aea600901
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <[email protected]>
Gerrit-Reviewer: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: anujphadke <[email protected]>
Gerrit-HasComments: Yes
