Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

2019-01-29 Thread Na Li via Review Board 

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

(Updated Jan. 30, 2019, 5:02 a.m.)


Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/8/

Changes: https://reviews.apache.org/r/69702/diff/7-8/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

2019-01-29 Thread Na Li via Review Board 


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
> > Lines 113 (patched)
> > 
> >
> > Don't you think, 
> > senry.metastore.read.authorization.enabled is better?

AGREE.


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 158 (original), 163 (patched)
> > 
> >
> > Why is change needed?

user name should be case sensitive


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Lines 212 (patched)
> > 
> >
> > How about authorizeDatabaseRead?

the event type is read_database. Using authorizeReadDatabase seems clearer for 
me


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Lines 217 (patched)
> > 
> >
> > How about authorizeTableRead?

the event type is read_table, that is why authorizeReadTable is used


- Na


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


On Jan. 29, 2019, 4:15 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 29, 2019, 4:15 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2483
> https://issues.apache.org/jira/browse/sentry-2483
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add READ_DATABASE and READ_TABLE events support to provide read authorization 
> to HMS.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  9efd612 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
>  f14cbb6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/7/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

2019-01-29 Thread Na Li via Review Board 


> On Jan. 30, 2019, 12:35 a.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
> > Line 158 (original), 162 (patched)
> > 
> >
> > Why is thie removed?

User name should be case sensitive. See details in 
https://issues.apache.org/jira/browse/SENTRY-2432


- Na


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


On Jan. 29, 2019, 4:15 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 29, 2019, 4:15 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2483
> https://issues.apache.org/jira/browse/sentry-2483
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add READ_DATABASE and READ_TABLE events support to provide read authorization 
> to HMS.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  9efd612 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
>  f14cbb6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/7/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

2019-01-29 Thread kalyan kumar kalvagadda via Review Board 

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


Fix it, then Ship it!




Please consider the cosmetic comments added. You can address them if you agree 
and commit the change.


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Line 158 (original), 162 (patched)


Why is thie removed?



sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
Lines 113 (patched)


Don't you think, 
senry.metastore.read.authorization.enabled is better?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 142 (patched)


variable name, readAuthorizationEnabled would be clear,



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Line 158 (original), 163 (patched)


Why is change needed?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 212 (patched)


How about authorizeDatabaseRead?



sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
Lines 217 (patched)


How about authorizeTableRead?


- kalyan kumar kalvagadda


On Jan. 29, 2019, 4:15 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69702/
> ---
> 
> (Updated Jan. 29, 2019, 4:15 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2483
> https://issues.apache.org/jira/browse/sentry-2483
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add READ_DATABASE and READ_TABLE events support to provide read authorization 
> to HMS.
> 
> This is based on changed made by Sergio at 
> https://reviews.apache.org/r/69620/, and add code to fix unstable e2e tests
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
>  9efd612 
>   
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
>  328d2b5 
>   sentry-tests/sentry-tests-hive/pom.xml 74777bb 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
>  8bf486e 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
>  7d41348 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
>  f14cbb6 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
>  3c28fd0 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
>  f8f304f 
> 
> 
> Diff: https://reviews.apache.org/r/69702/diff/7/
> 
> 
> Testing
> ---
> 
> add new e2e tests for READ_DATABASE and READ_TABLE at HMS
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

2019-01-29 Thread Arjun Mishra via Review Board 

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

(Updated Jan. 29, 2019, 9:09 p.m.)


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


Repository: sentry


Description
---

When sentry is fetching snapshot from HMS, it should log periodically on where 
it stands in the snapshot process. This will help person debugging it and help 
him understand the progress.

 

This is important as this process could take magnitude of minutes when the HMS 
data is huge.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 2d21411e2 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 534bb51c1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 4ff3dc91a 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4baeb6725 


Diff: https://reviews.apache.org/r/69076/diff/5/

Changes: https://reviews.apache.org/r/69076/diff/4-5/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 69832: SENTRY-2488: Add privilege cache to sentry hive bindings in DefaultAccessValidator

2019-01-29 Thread Arjun Mishra via Review Board 

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

(Updated Jan. 29, 2019, 6:47 p.m.)


Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, and Na Li.


Changes
---

Fixed a bug where we try to clear an Immutable Set.


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


Repository: sentry


Description
---

We are not consistent with behavior in SentryHiveMetaStoreHook (not used 
anymore) which would cache privileges when authorizing show databases or show 
tables command. This needs to be added back


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 
  
sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimplePrivilegeCache.java
 0ad4616c0 


Diff: https://reviews.apache.org/r/69832/diff/3/

Changes: https://reviews.apache.org/r/69832/diff/2-3/


Testing
---


Thanks,

Arjun Mishra

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

2019-01-29 Thread Na Li via Review Board 

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

(Updated Jan. 29, 2019, 4:15 p.m.)


Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/7/

Changes: https://reviews.apache.org/r/69702/diff/6-7/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

2019-01-29 Thread Arjun Mishra via Review Board 

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

(Updated Jan. 29, 2019, 3:43 p.m.)


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


Repository: sentry


Description
---

When sentry is fetching snapshot from HMS, it should log periodically on where 
it stands in the snapshot process. This will help person debugging it and help 
him understand the progress.

 

This is important as this process could take magnitude of minutes when the HMS 
data is huge.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java
 2d21411e2 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 534bb51c1 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
 4ff3dc91a 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
 4baeb6725 


Diff: https://reviews.apache.org/r/69076/diff/4/

Changes: https://reviews.apache.org/r/69076/diff/3-4/


Testing
---


Thanks,

Arjun Mishra