Re: Review Request 68831: SENTRY-2417: fix LocalGroupMappingService INI format class docs

2018-09-24 Thread Alexander Kolbasov 

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


Ship it!




Ship It!

- Alexander Kolbasov


On Sept. 24, 2018, 11:45 p.m., Dan Burkert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68831/
> ---
> 
> (Updated Sept. 24, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.
> 
> 
> Bugs: SENTRY-2417
> https://issues.apache.org/jira/browse/SENTRY-2417
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> fix LocalGroupMappingService INI format class docs
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java
>  4430ce73b 
> 
> 
> Diff: https://reviews.apache.org/r/68831/diff/1/
> 
> 
> Testing
> ---
> 
> none - this is a doc only patch.
> 
> 
> Thanks,
> 
> Dan Burkert
> 
>

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Hao Hao via Review Board 


> On Sept. 24, 2018, 11:23 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 915 (patched)
> > 
> >
> > you have got groups in line 908 if the requestor is the same as 
> > principlaName. It should improve performance to reuse it under this 
> > situation

I don't think the requestor is the same as principal? Here it is getting the 
principal's group.


> On Sept. 24, 2018, 11:23 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
> > Line 227 (original), 229 (patched)
> > 
> >
> > This is semi-public API. It is safer to add function than change the 
> > returned type of existing function. Inside implementation, you can let one 
> > function call another one to re-use code.

I was hesitate to add another API to complicate the code base. Also I do not 
see any other consumer of this API other than SentryPolicyStoreProcessor so I 
made change on top of the existing one. But if you prefer to add a new one to 
avoid unknown dependency, I will update this.


- Hao


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


On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 24, 2018, 9:31 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> 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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/2/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68831: SENTRY-2417: fix LocalGroupMappingService INI format class docs

2018-09-24 Thread Dan Burkert 

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

(Updated Sept. 24, 2018, 11:45 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, and Na Li.


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


Repository: sentry


Description
---

fix LocalGroupMappingService INI format class docs


Diffs
-

  
sentry-provider/sentry-provider-file/src/main/java/org/apache/sentry/provider/file/LocalGroupMappingService.java
 4430ce73b 


Diff: https://reviews.apache.org/r/68831/diff/1/


Testing
---

none - this is a doc only patch.


Thanks,

Dan Burkert

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Na Li via Review Board 

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




sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
Lines 66 (patched)


Can you change the function from "list_sentry_privileges_for_user" to 
"list_sentry_privileges_by_user_and_itsgroups" to make it clear that it 
includes privileges directly assigned to this user and privileges through its 
groups?

You don't have to use "list_sentry_privileges_by_user_and_itsgroups" 
exactly, but use some name similar to it.



sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Lines 416 (patched)


"and the group" to "and the groups" since a user can belong to multiple 
groups.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
Lines 113 (patched)


change the function name from "list-privileges-for-user" to the one you 
will use in sentry_policy_service.thrift



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 57 (patched)


It is better to add the new function in SentryStoreInterface instead of 
SentryStore as this class deals with the interface, not its implementation



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 915 (patched)


you have got groups in line 908 if the requestor is the same as 
principlaName. It should improve performance to reuse it under this situation



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
Line 227 (original), 229 (patched)


This is semi-public API. It is safer to add function than change the 
returned type of existing function. Inside implementation, you can let one 
function call another one to re-use code.



sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
Lines 26 (patched)


If you only add new function in SentryStoreInterface, you can make it part 
of the TestSentryStore if those testing cases don't exsit.


- Na Li


On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 24, 2018, 9:31 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> 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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>

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

2018-09-24 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated Sept. 24, 2018, 10:28 p.m.)


Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, and Sergio 
Pena.


Changes
---

Addressed reivew comments.


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 (updated)
-

  
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/2/

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


Testing
---

Made sure all the existing tests passed and also added unit and e2e tests to 
verify the new behavior.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Hao Hao via Review Board 


> On Sept. 24, 2018, 3:35 p.m., Sergio Pena wrote:
> > Btw, are you going to have another JIRA for the client to call this new API?
> 
> Hao Hao wrote:
> Sure, I can create a jira for adding the client side logical.

SENTRY-2418 is filed.


- Hao


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


On Sept. 24, 2018, 9:31 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 24, 2018, 9:31 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> 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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/2/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Hao Hao via Review Board 

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

(Updated Sept. 24, 2018, 9:31 p.m.)


Review request for sentry, Na Li and Sergio Pena.


Repository: sentry


Description
---

This commit adds a new thrift API list_sentry_privileges_for_user to List 
sentry privileges granted to the given user and the group the user associated 
with, filterted based on authorization hierarchy if present.
Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.


Diffs (updated)
-

  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
 0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 2e79e5646ae9102d8c0c28da4260a539254fcd15 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
 236a07bdf5191cdc0f167f20a406b721b3dc506d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46f7c4335db18113574170f761da9a4ca 
  
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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 


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

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


Testing
---

Unit test.


Thanks,

Hao Hao

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Hao Hao via Review Board 


> On Sept. 24, 2018, 3:35 p.m., Sergio Pena wrote:
> > Btw, are you going to have another JIRA for the client to call this new API?

Sure, I can create a jira for adding the client side logical.


- Hao


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


On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 23, 2018, 8:52 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> 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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/1/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Re: Review Request 68822: SENTRY-2371 Add a new thrift API for getting all privileges a user has

2018-09-24 Thread Sergio Pena via Review Board 

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



Btw, are you going to have another JIRA for the client to call this new API?

- Sergio Pena


On Sept. 23, 2018, 8:52 p.m., Hao Hao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68822/
> ---
> 
> (Updated Sept. 23, 2018, 8:52 p.m.)
> 
> 
> Review request for sentry, Na Li and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> This commit adds a new thrift API list_sentry_privileges_for_user to List 
> sentry privileges granted to the given user and the group the user associated 
> with, filterted based on authorization hierarchy if present.
> Under the hood, this API is using sentryStore.listSentryPrivilegesForProvider.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/SentryPolicyService.java
>  0cbd8ab0a624d4c09aead4097f72762e12d1d21b 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>  2e79e5646ae9102d8c0c28da4260a539254fcd15 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  236a07bdf5191cdc0f167f20a406b721b3dc506d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> 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/test/java/org/apache/sentry/api/service/thrift/TestTSentryPrivilegeToAuthorizable.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  d8c0ab4fa82ba09c60bc995eb4f53a78a7fae346 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java
>  25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 
> 
> 
> Diff: https://reviews.apache.org/r/68822/diff/1/
> 
> 
> Testing
> ---
> 
> Unit test.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

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

2018-09-24 Thread Sergio Pena 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.
> 
> kalyan kumar kalvagadda wrote:
> 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.

The configuration accepts any list of privileges. The idea behind this config 
is that admins can prevent that Sentry clients grant DB policies that are not 
authorized. Doing it on the clients will not enforce this as any user with the 
access to the Sentry API will be able to grant them. This config must be in the 
server for those reasons. It is up to the Admin to decide which privileges will 
be permitted.


- Sergio


---
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
> 
>