> On Oct. 21, 2015, 12:48 a.m., Sravya Tirukkovalur wrote: > > Thanks for the update Colin! Will it be possible for you to add description > > on how you are solving the problem. It would be easy to follow along and > > also would help review the approach before delving into the code details. > > Colin Ma wrote: > hi, Sravya, the following is the current idea for the cached privilege: > 1. CachedPrivilegeManagement is responsible for getPrivileges, and it > likes an agent for providerBackend. CachedPrivilegeManagement will be > initialized in HiveAuthzBinding according to the configuration file. > 2. All cached privileges are stored as HashMap in > CachedPrivilegeManagement. The key for this map is username, and the value is > wrapped in the CachedPrivilege including all privileges for this username and > the timestamp when refresh the privilege. > 3. When do the authorization, SimpleDBPolicyEngine.getPrivileges will be > called, and CachedPrivilegeManagement.getPrivileges will be called. If > CachedPrivilegeManagement is initialized and cached privilege enable in > configuraion, CachedPrivilegeManagement.getPrivileges will get the privileges > from cache first, if not exist or expire, get the privileges by backend and > refresh the cache. The expired time can be configured and default is 5 mins. > > The core part for this problem is: HiveAuthzBinding, CachedPrivilege, > CachedPrivilegeManagement, SimpleDBPolicyEngine, TestCachedPrivilege. The > other code is just for the interface change. > The disadvantage for this solution is if admin change the privilege for > user, this will not be effect immediately. There will be delay according to > the expire time setting in configuration file. I think this is accepted for > the authorization management. > > Feel free for any comments, thanks for your review.
Hi Colin, I think the main problem here is predicate push down. We should focus on what is the best way to be able to send the tables/ db names in the RPC when doing auth check instead of doing auth checks for each table/dbname. Also, we already have a cache provider, if we decide to improve performance of hive binding by moving to a cache provider, we should move everything to using a cache provider and also have a way to refresh this cache. Moving just one path to cache provider does not seem like a right approach as it can lead to behavior which is hard to reason. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28800/#review103333 ----------------------------------------------------------- On Oct. 16, 2015, 11:18 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28800/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2015, 11:18 a.m.) > > > Review request for sentry and Lenni Kuff. > > > Repository: sentry > > > Description > ------- > > Currently, when get the metadata from hive, eg, "show tables", "show > databases". Sentry will filter the result and output the authorized entities. > There will be many RPC calls when filtering the result. The related code is > in HiveAuthzBinding, for example, in filterShowTables: > > ...... > for (String tableName : queryResult) { > ...... > hiveAuthzBinding.authorize(operation, tableMetaDataPrivilege, subject, > inputHierarchy, > outputHierarchy, providedPrivileges); > ...... > } > ...... > > hiveAuthzBinding.authorize will get the privileges from sentry service, if > there are many tables in the hive, the filtering process will spend much > time. Considering sentry also need to filter the column, HiveAuthzBinding > should be improved to reduce the number of rpc calls when doing the filter. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 3071475 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java > 38a5b65 > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/CachedPrivilege.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/CachedPrivilegeManagement.java > PRE-CREATION > > sentry-policy/sentry-policy-db/src/main/java/org/apache/sentry/policy/db/SimpleDBPolicyEngine.java > a03794e > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/AbstractTestSimplePolicyEngine.java > d1151e3 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestPolicyParsingNegative.java > 5f7c671 > > sentry-policy/sentry-policy-db/src/test/java/org/apache/sentry/policy/db/TestSimpleDBPolicyEngineDFS.java > f8c36e2 > > sentry-policy/sentry-policy-indexer/src/main/java/org/apache/sentry/policy/indexer/SimpleIndexerPolicyEngine.java > 2f4bc1d > > sentry-policy/sentry-policy-indexer/src/test/java/org/apache/sentry/policy/indexer/AbstractTestIndexerPolicyEngine.java > d7d1ae2 > > sentry-policy/sentry-policy-indexer/src/test/java/org/apache/sentry/policy/indexer/TestIndexerPolicyNegative.java > 0706560 > > sentry-policy/sentry-policy-search/src/main/java/org/apache/sentry/policy/search/SimpleSearchPolicyEngine.java > bc518fb > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/AbstractTestSearchPolicyEngine.java > d1c415b > > sentry-policy/sentry-policy-search/src/test/java/org/apache/sentry/policy/search/TestSearchPolicyNegative.java > 2abe8f2 > > sentry-policy/sentry-policy-sqoop/src/main/java/org/apache/sentry/policy/sqoop/SimpleSqoopPolicyEngine.java > e8615a0 > > sentry-policy/sentry-policy-sqoop/src/test/java/org/apache/sentry/policy/sqoop/AbstractTestSqoopPolicyEngine.java > 1389fca > > sentry-policy/sentry-policy-sqoop/src/test/java/org/apache/sentry/policy/sqoop/TestSqoopPolicyNegative.java > 406e53f > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 06573b7 > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java > f57198a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > bc35742 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestCachedPrivilege.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > cc5daef > > Diff: https://reviews.apache.org/r/28800/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
