Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[kalyan kumar kalvagadda via Review Board]

> On Sept. 21, 2018, 5:18 p.m., kalyan kumar kalvagadda wrote:
> > Sergio,
> > 
> > Sentry server is policy store and does not need to understand the 
> > action.Let's keep it that way. Logic of validiting the actions is present 
> > in the sentry bindings. I think this logic should go there instead of 
> > SentryPolicyStoreProcessor.
> 
> Sergio Pena wrote:
> I agree, but then we'll need to have the configuration in all the 
> bindings. Hive, Impala (and next SparkSQL) will need this configuration 
> otherwise they will be able to grant those privileges that we're trying to 
> avoid. If Impala does not configure those privileges correctly ,then Hive 
> will use them.

I understand that the approach I suggested would need configuration for both 
Hive and Impala. That is fine.

All these compute engines have are same. Each have them support different 
commands. Not all operations that Impala supports are supported by Hive and 
Impala would like to allow set of operatins which hive may not even support. It 
could be true other way around. I really feel each of them should have seperate 
configuration and should be validated at the client side. Sentry server is just 
a policy store and it may not be the right place to enforce this configuration.


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208868
---


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208868
---



Sergio,

Sentry server is policy store and does not need to understand the action.Let's 
keep it that way. Logic of validiting the actions is present in the sentry 
bindings. I think this logic should go there instead of 
SentryPolicyStoreProcessor.

- kalyan kumar kalvagadda


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68787: SENTRY-2406: Make sure inputHierarchy and outputHierarchy have unique values

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68787/
---

(Updated Sept. 21, 2018, 5:05 p.m.)


Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.


Changes
---

Implementing a set of list to remove duplicates


Bugs: SENTRY-2406
https://issues.apache.org/jira/browse/SENTRY-2406


Repository: sentry


Description
---

When building input or output hierarchy list, we iterate over all ReadEntity 
inputs received from Hive. The inputs particularly have accessed columns that 
can repeat for other ReadEntity objects. This happens definitively when a table 
has partitions. We should in general protect Sentry from not having to 
authorize over a list of DBModelAuthorizable objects when it has already been 
done.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBinding.java
 6a1556fa7 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 f0764767f 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 da1956b8e 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBinding.java
 5beda9f80 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 0909656be 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
 dd6936c19 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestHiveAuthzBindings.java
 3bbf6fb19 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/AccessURI.java
 dd8104d72 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Column.java
 305fd1f5a 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Database.java
 0d94805c7 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Server.java
 33e735ef2 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/Table.java
 b161d0291 
  
sentry-core/sentry-core-model-db/src/main/java/org/apache/sentry/core/model/db/View.java
 f6d84997e 


Diff: https://reviews.apache.org/r/68787/diff/2/

Changes: https://reviews.apache.org/r/68787/diff/1-2/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/
---

(Updated Sept. 21, 2018, 2:24 p.m.)


Review request for sentry.


Bugs: sentry-2413
https://issues.apache.org/jira/browse/sentry-2413


Repository: sentry


Description
---

Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
comma-separated list of privileges that can be granted by Sentry DB clients. 
If the value is empty, then any privilege can be granted as it works normally 
(this is the default behavior).


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 adc1947a1a1dd72ef9e6dd743e166979759709b2 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 8cdbde4f9e81b278c8737ea031820e20e0cf5704 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46f7c4335db18113574170f761da9a4ca 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 c23b3850f097b563912c06b29ffd26ea0709b9fd 


Diff: https://reviews.apache.org/r/68788/diff/2/

Changes: https://reviews.apache.org/r/68788/diff/1-2/


Testing
---

Unit tests added.


Thanks,

Sergio Pena

Re: Review Request 68787: SENTRY-2406: Make sure inputHierarchy and outputHierarchy have unique values

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68787/#review208857
---



Arjun,

Can you use an example to explain the optimization this chnage is going to make?

- kalyan kumar kalvagadda


On Sept. 20, 2018, 7:32 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68787/
> ---
> 
> (Updated Sept. 20, 2018, 7:32 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2406
> https://issues.apache.org/jira/browse/SENTRY-2406
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When building input or output hierarchy list, we iterate over all ReadEntity 
> inputs received from Hive. The inputs particularly have accessed columns that 
> can repeat for other ReadEntity objects. This happens definitively when a 
> table has partitions. We should in general protect Sentry from not having to 
> authorize over a list of DBModelAuthorizable objects when it has already been 
> done.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
>  da1956b8e 
> 
> 
> Diff: https://reviews.apache.org/r/68787/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 68727: SENTRY-2306: Reduce the HMS snapshot size by ignoring partitions at default locations

[kalyan kumar kalvagadda via Review Board]

> On Sept. 20, 2018, 3 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 4911 (patched)
> > 
> >
> > This method is already public, so you don't need the @VisibleForTesting 
> > annotation.

You are right. i made some refactoring beore publishing the final patch. I over 
looked it.


> On Sept. 20, 2018, 3 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
> > Lines 804-806 (patched)
> > 
> >
> > Would be a good idea to specify which tables contain HMS Metadata as 
> > this interface is shared by other implementations and would need help to 
> > understand what to delete.

Will update the comment


> On Sept. 20, 2018, 3 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 549-555 (patched)
> > 
> >
> > You could save time and code by checking if the partition location is 
> > part of the table location inside the PartitionTask.doTask() method 
> > instead. 
> > 
> > There is one part of the code there:
> > 
> > String partPath = pathFromURI(part.getSd().getLocation());
> > If (partPath != null) {
> >   partitionNames.add(partPath.intern());
> > }
> > 
> > If as part of the if() condition you also call 
> > !isPartitionLocatedWithinTableLocation(), then that should make the rest of 
> > the code work without too many changes.

Agree, This approach makes the change simpler. Let me look closer and make 
changes accordingly.


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68727/#review208797
---


On Sept. 17, 2018, 3:07 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68727/
> ---
> 
> (Updated Sept. 17, 2018, 3:07 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, and 
> Sergio Pena.
> 
> 
> Bugs: SENTRY-2306
> https://issues.apache.org/jira/browse/SENTRY-2306
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Ignore the partitions entries which are located in default location as they 
> share the same permissions as the table. This will reduce the snapshot size 
> decreasing the time to persist the snapshot and also time to send the full 
> update to Namenode. This is important as the partition information has lion’s 
> share in a HMS Full snapshot.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  1eda41b2b6bd940a404cc1ba09a861fe783ead04 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  0b4f4aa24bd3002c50bf4d80a6fa361f66052973 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  3e27d1bbee9bea9a60e7f7e012a367957610826c 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java
>  b3a4934dfc523d14eec216df00a6b7597c66c166 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/service/thrift/TestFullUpdateInitializer.java
>  589acbed12855ff09309a04c9214f8daf87ea1de 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java
>  574bc4b0432824abc81fe3c0dd30d1fe01f012e5 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentryService.java
>  PRE-CREATION 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2de41b6700a18a358f8d5e442104dd0585 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/SentrySrv.java
>  0e82b86a727f578013a63c005aa05279790344f1 
> 
> 
> Diff: https://reviews.apache.org/r/68727/diff/1/
> 
> 
> Testing
> ---
> 
> Made sure all the existing tests passed and also added unit and e2e tests to 
> verify the new behavior.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>