impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5582: Store sentry privileges in lower case
Date Wed, 12 Jul 2017 20:32:36 GMT
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 <aphadke@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message