-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21995/#review44372
-----------------------------------------------------------
Thanks for the update Arun! I just realized that the way you are building the
authorizable object for RPC needs some work(details in the comment). Otherwise
it looks good.
As I thought a little more about it, I feel we still have some places(some
unrelated to this patch as such) where we can clean up/improve, but lets do
them as multiple follow on jiras.
1. Getting rid of maintaining active role set struct and
list_sentry_privileges_for_provider in thrift
I think we should handle active roles on hive side outside of sentry service,
as we do not really store these mappings in the db. And does not make sense to
store these in db as these are per session variables. If we do this, we can
clean up the thrift interface a bit and just have:
TListSentryPrivilegesResponse
list_sentry_privileges(1:TListSentryPrivilegesRequest request)
struct TListSentryPrivilegesRequest {
1: required i32 protocol_version = sentry_common_service.TSENTRY_SERVICE_V1,
2: required string requestorUserName, # user on whose behalf the request is
issued
3: required set<string> roleNames # get privileges assigned for this role
4: optional TSentryAuthorizable authorizableHierarchy
}
And we will do the set intersection of rolesforGroup and active roles in the
hive binding.
2. Accept list of authorizables: Set<TSentryAuthorizable> authorizables
Hive commands can involve multiple authorizable objects, for example for joins,
we need permissions on two tables. Today we will have to do multiple RPCs.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
<https://reviews.apache.org/r/21995/#comment78729>
wonder why this was final in the first place? And how did anything work
with it :-)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/21995/#comment78744>
We will have to do one RPC for each authorizable. Or change the RPC to
accept list of authorizables. Imagine we have a join of two tables, in which
case our authorizables {(db1,tb1), (db2,tb2), in which case current logic
fails. Also would be good to add some end to end tests, we can do that as a
follow if you prefer after SENTRY-153 is committed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
<https://reviews.apache.org/r/21995/#comment78745>
Same as above
- Sravya Tirukkovalur
On May 30, 2014, 1:27 a.m., Arun Suresh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21995/
> -----------------------------------------------------------
>
> (Updated May 30, 2014, 1:27 a.m.)
>
>
> Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya
> Tirukkovalur.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Patch for SENTRY-157(https://issues.apache.org/jira/browse/SENTRY-157)
>
> To support push-down of Authorizable Hierarchy
>
>
> Diffs
> -----
>
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java
> f1e6247
>
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java
> c378a38
>
> sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java
> a95ef7b
>
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java
> 4625d6f
>
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java
> e88ae32
>
> sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java
> 08f84a3
>
> sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java
> 8adcb6f
>
> sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java
> 1b0aba6
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java
> a175245
>
> sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
> e1e7f4a
>
> sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java
> ece740b
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java
> 54c1d6d
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java
> 952ee78
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> 7e2323c
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java
> 2aac409
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java
> f92c78a
>
> sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift
> b4281c7
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> 67b05e6
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java
> 56dcaf9
>
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/SimpleFileProviderBackend.java
> 6e8f02f
>
> Diff: https://reviews.apache.org/r/21995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Arun Suresh
>
>
