> On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java > > Lines 233-236 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191008#file2191008line233> > > > > Did you see any issue with out this change?
yes. without this change, if the same hook is called the second time to filter databases or tables, the privileges are not refectched, and may not be up-to-date. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java > > Line 46 (original) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191021#file2191021line46> > > > > I agree. this function was added by SENTRY-1291 https://reviews.apache.org/r/47872/diff/6#9. I want to keep this Interface the same as before SENTRY-1291 was committed, and all changes in SENTRY-1291 and SENTRY-2539 in another interface to maintain backward compatibility. In this way, if the changes in SENTRY-1291 and SENTRY-2539 cause problem, we can easily rollback to the behavior before SENTRY-1291 and SENTRY-2539. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java > > Lines 75-78 (original), 76-84 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191022#file2191022line76> > > > > Why should be do this up-casting? > > > > If the below API is not removed from Privilge cache we would need this > > special check. > > > > Set<String> listPrivileges(Set<String> groups, Set<String> users, > > ActiveRoleSet roleSet, > > Authorizable... authorizationhierarchy); > > > > > > Is there any reason to remove this API. This might even break > > Impala which implements this interface. "Is there any reason to remove this API. This might even break Impala which implements this interface." this function was added by SENTRY-1291 https://reviews.apache.org/r/47872/diff/6#9, and Impala rejected this change, and does NOT suppor this function. To avoid breaking Impala, we should not keep this function in Interface PrivilegeCache.java. I want to keep the Interface PrivilegeCache.java the same as before SENTRY-1291 was committed, and all changes in SENTRY-1291 and SENTRY-2539 in another interface to maintain backward compatibility. In this way, if the changes in SENTRY-1291 and SENTRY-2539 cause problem, we can easily rollback to the behavior before SENTRY-1291 and SENTRY-2539. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java > > Lines 170 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line170> > > > > Second argument here is basically is partIndex. > > You sending a -1 and using it by incrementing by one. > > Instead, I think you should just pass 0. If I pass 0 to the function getChildResourceValue(), the index will be 0 + 1 = 1, and the result will be wrong. To make it work, I implemented the function getTopLevelResourceValue() that is even simpler, and does not need caller to pass in any index. The equivalent index is 0, which is (- 1 + 1) = 0. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java > > Lines 174 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191025#file2191025line174> > > > > First argument here is basically is partIndex. > > You sending a -1 and using it by incrementing by one. > > Instead, I think you should just pass 0. > > > > > > +1 on vehang's comment. I have added new function getResourceValue() to make code more readable. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > > Lines 217-225 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191033#file2191033line217> > > > > Why are you converting the privilge object to string and performening > > the check? > > > > Something like > > > > private boolean hasOnlyServerPrivilege(Privilege privObj) { > > if(privObj.getParts().size() == 1 && > > privObj.getParts().get(0).getKey().equalsIgnoreCase("server")) { > > return privObj.getParts().get(0).getValue().endsWith("+"); > > } > > return false; > > } Thanks for the suggestion. I did that before adding function getParts() in Privilege. After adding this function, I don't need to convert the privilege to string and can use your suggestion. > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > > Lines 111-151 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191035#file2191035line111> > > > > Can you avoid code duplication? removed duplication > On Dec. 21, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > > Lines 137-150 (patched) > > <https://reviews.apache.org/r/71915/diff/9/?file=2191036#file2191036line137> > > > > This method is incomplete. If you intent that this API not be used > > adding a comment here may not help. > > > > @Override > > public ImmutableSet<Privilege> getPrivilegeObjects(Set<String> > > groups, Set<String> users, > > ActiveRoleSet roleSet, Authorizable... authorizableHierarchy) { > > throw Exception(); // Appropriate exception > > } > > > > This way if someone calls this method they would know that it is not > > supported. I have added comment in the interface ProviderBackend. Its caller is ResourceAuthorizationProvider, which does not know what cache to be used. Throwing exception for each authorization check is too expensive. ResourceAuthorizationProvider's behavior is to call getPrivilegeObjects(). If return set is empty, it will call getPrivileges() - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71915/#review219091 ----------------------------------------------------------- On Dec. 20, 2019, 9:27 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71915/ > ----------------------------------------------------------- > > (Updated Dec. 20, 2019, 9:27 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda and Vihang Karajgaonkar. > > > Bugs: sentry-2359 > https://issues.apache.org/jira/browse/sentry-2359 > > > Repository: sentry > > > Description > ------- > > Right now, the PolicyEngine Interface only returns the list of privileges in > the form of String. As a result, every authorization check has to convert the > privilege string to privilege object even though the cached privilege objects > are of the correct type already. > > We should add a new function that returns privilege object directly to avoid > the overhead of conversion. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java > 5c7f84f > > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java > 90fcfc3 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > de88705 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java > 2940a1e > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java > 8e09490 > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/privilege/hive/TestCommonPrivilegeForHive.java > 6a8b871 > > sentry-binding/sentry-binding-kafka/src/test/java/org/apache/sentry/privilege/kafka/TestKafkaWildcardPrivilege.java > 0a0e2f0 > > sentry-binding/sentry-binding-solr/src/test/java/org/apache/sentry/privilege/solr/TestCommonPrivilegeForSolr.java > 6782089 > > sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/privilege/sqoop/TestCommonPrivilegeForSqoop.java > 94e9919 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/KeyValue.java > b6a1faa > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/DBModelAuthorizables.java > 7bc94c9 > > sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/validator/AbstractDBPrivilegeValidator.java > fa28716 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/CommonPrivilege.java > 5b261e3 > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PolicyEngine.java > 504b5ea > > sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/Privilege.java > 6c2737a > > sentry-policy/sentry-policy-engine/src/main/java/org/apache/sentry/policy/engine/common/CommonPolicyEngine.java > a819bb0 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/FilteredPrivilegeCache.java > PRE-CREATION > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/PrivilegeCache.java > 4bb6d32 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java > ddb4ec5 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleFilteredPrivilegeCache.java > PRE-CREATION > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java > 5de3135 > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeCache.java > PRE-CREATION > > sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/TreePrivilegeNode.java > PRE-CREATION > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/PrivilegeCacheTestImpl.java > f2f735b > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimpleFilteredPrivilegeCache.java > PRE-CREATION > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestSimplePrivilegeCache.java > 891c1d9 > > sentry-provider/sentry-provider-cache/src/test/java/org/apache/sentry/provider/cache/TestTreePrivilegeCache.java > PRE-CREATION > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/CacheProvider.java > d50a0bc > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ProviderBackend.java > b244dba > > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java > 222b77a > > sentry-provider/sentry-provider-common/src/test/java/org/apache/sentry/provider/common/TestGetGroupMapping.java > ccc505f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SimpleDBProviderBackend.java > 277f6b3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > f8dc211 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestColumnEndToEnd.java > 3881692 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 4c09e68 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > cc0465a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java > cb201bb > > > Diff: https://reviews.apache.org/r/71915/diff/9/ > > > Testing > ------- > > Performance test in > > 1) > Privilege: db[1000], table[10] > Authorizable: db[12000], table[1] > > TreePrivilegeCache - total time on list string: 448 ms > (TestTreePrivilegeCache.testListPrivilegesPerf) > TreePrivilegeCache - total time on list obj: 365 ms > (TestTreePrivilegeCache.testListPrivilegeObjectsPerf) > > SimplePrivilegeCache - total time on list string: 717 ms > (TestSimplePrivilegeCache.testListPrivilegesPerf) > SimplePrivilegeCache - total time on list obj: 794 ms > (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf) > > 2) > Privilege: db[1000], table[10] > Authorizable: db[12000], table[10] > > TreePrivilegeCache - total time on list string: 1302 ms > (TestTreePrivilegeCache.testListPrivilegesPerf) > TreePrivilegeCache - total time on list obj: 604 ms > (TestTreePrivilegeCache.testListPrivilegeObjectsPerf) > > SimplePrivilegeCache - total time on list string: 4436 ms > (TestSimplePrivilegeCache.testListPrivilegesPerf) > SimplePrivilegeCache - total time on list obj: 3732 ms > (TestSimplePrivilegeCache.testListPrivilegeObjectsPerf) > > > Thanks, > > Na Li > >
