Review Request 69517: SENTRY-2475: Add the list of authorizables to the export file

2018-12-06 Thread kalyan kumar kalvagadda via Review Board 

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

Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

With the new changes proposed, sentry should be able to accept a list of 
authorizables for which permissions should be exported. In this process, sentry 
should update the the list authorizables to the export file along with the 
permissions/role/groups etc.

This information can be used by sentry to cleanup the existing permissions 
before starting importing the data.


Diffs
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
 216d8612bee688057aa834b1a96045bb7df6b9c4 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864cfbdf18057d87a65a04af8991292aadccf 


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


Testing
---

Updated tests to verify this new functionality added


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

2018-12-06 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated Dec. 7, 2018, 12:05 a.m.)


Review request for sentry and Sergio Pena.


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


Repository: sentry


Description
---

New sentry API should be implemented to fetch the privileges granted to 
authorizables and it's children. authorizables include database, tables, 
columns and URI's.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
 e48eea377b842475f72b6fab4567a82c8fd93098 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
 ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 


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

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


Testing
---

Added new unit tests to test the API added.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

2018-12-06 Thread kalyan kumar kalvagadda via Review Board 


> On Dec. 6, 2018, 5:15 p.m., Sergio Pena wrote:
> > I'd like to see the part of the SentryPolicyStoreProcessor with this patch 
> > to give me a bette idea of how both classes will work together. I saw that 
> > the previous export method requires role -> privileges mapping, but this 
> > returns only the list of privileges. I don't know what's the idea, but if 
> > you write the code for SentryPolicyStoreProcessor too would be helpful.

Here is the basic idea. This is the suedo code to understand how the new api is 
used.

  public Map> 
getRoleNameTPrivilegesMap(List authorizables) throws 
Exception {
return tm.executeTransaction(
pm -> {
  List mSentryPrivileges = 
**getPrivilegesForAuthorizables(pm, authorizables);**
  return getRolePrivilegesMap(mSentryPrivileges);
}); 
  }


- kalyan kumar


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


On Nov. 15, 2018, 9:06 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Nov. 15, 2018, 9:06 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377b842475f72b6fab4567a82c8fd93098 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/1/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69484: SENTRY-2460: Export sentry permission information to HDFS location

2018-12-06 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated Dec. 6, 2018, 9:19 p.m.)


Review request for sentry and Sergio Pena.


Changes
---

Addressed review comments.


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


Repository: sentry


Description
---

Sentry should be able to export permission information to a HDFS location.

This can be done using hadoop-common library.


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
 b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
  
sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryPolicyFileFormatter.java
 4f465b3671c1fdd6de7cd0773a29108af40311c8 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java
 5f1e3e916d71c51e19d3d37ba788f902ed6b7e27 
  
sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
 7baeee17339d29576b841480e600a3cfaf14adf3 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 092060c450c6a906850630cb10454737157af5fe 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java
 df9299c5df26c891ceebbc59b79dd0f7cd3ceeb4 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImportExport.java
 b048989b42bde0318c6d0fa0e9353a2a59954407 


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

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


Testing
---

Added new tests to verify the new behavior added.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

2018-12-06 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated Dec. 6, 2018, 8:23 p.m.)


Review request for sentry and Sergio Pena.


Changes
---

Addressed reivew comments.


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


Repository: sentry


Description
---

TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
used to send export and import requests to sentry server should be changed to 
be able to send a list authorizables for which permission information has to be 
exported/imported.

These requests should accommodate source and target cluster information as well.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
 d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
  
sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
 6eb85217b9229438b9adbd1546a408dc507f1645 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
 13e57e04dd779825e2986b1e527f4e556271a162 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
 21f3bdf5e304fe2f49684c999d0aa0e0362320de 
  
sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
 cea868f0524c25bf63cfb7c532829354a280df28 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 83393a98df5e10b324f74dc12f10c20bc5f02651 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
 68d864cfbdf18057d87a65a04af8991292aadccf 
  
sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 3364648dc88acbb8de1cc925fe8c512f34a4b064 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 2dc0975f482f760c18c438c1418630a7ef6a4cbb 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 b9e3bf2921a0696da639e1b1bee5d83cf2b9cee0 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryServiceImportExport.java
 cf1fdab382034c8950da2613ef9b0cf1af912e33 


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

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


Testing
---

Made sure that existiung tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 69353: SENTRY-2454: Add new sentrys tore api to gather the privileges for a list of authorizables.

2018-12-06 Thread Sergio Pena via Review Board 

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



I'd like to see the part of the SentryPolicyStoreProcessor with this patch to 
give me a bette idea of how both classes will work together. I saw that the 
previous export method requires role -> privileges mapping, but this returns 
only the list of privileges. I don't know what's the idea, but if you write the 
code for SentryPolicyStoreProcessor too would be helpful.

- Sergio Pena


On Nov. 15, 2018, 9:06 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69353/
> ---
> 
> (Updated Nov. 15, 2018, 9:06 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2454
> https://issues.apache.org/jira/browse/SENTRY-2454
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> New sentry API should be implemented to fetch the privileges granted to 
> authorizables and it's children. authorizables include database, tables, 
> columns and URI's.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e2d6c85ac6b285e286c12f7eec669b841cf76e9d 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java
>  e48eea377b842475f72b6fab4567a82c8fd93098 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  ca8c41610d3dcd12b267579c3d3cbe5b7ba7b208 
> 
> 
> Diff: https://reviews.apache.org/r/69353/diff/1/
> 
> 
> Testing
> ---
> 
> Added new unit tests to test the API added.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 69352: SENTRY-2452: Change the thrift interface to send the list of authorizable to sentry server

2018-12-06 Thread Sergio Pena via Review Board 

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




sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 107-108 (patched)


What if objectTrimmed is empty? Should we add a check and continue with the 
loop if so? This could happen if the 'objects' object is db=db1,



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 108 (patched)


Shouldn't we support the server as well? Sentry has support for it, so 
perhaps we should do the same thing for the export commands?



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Line 106 (original), 110 (patched)


This may throw an exception if the value is empty. The message would be 
only 'Value cannot be empty'. Should we catch that and throw a better message 
specified which key does not have a value?

An empty key may throw it too, but if you add the empty check after you 
trim the object and continue the loop, then keys won't never be empty at this 
point.



sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
Lines 120-123 (patched)


is the expectation of the method to just log errors instead of throwing an 
exception? I feel we should make the contract of this method to throw an 
exception if at least one of the KeyValue are invalid.

That helps you do better testing here, and avoid an admin had to go to the 
logs to find why a certain value was not exported.



sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
Line 298 (original), 298 (patched)


Are you making this incompatible with Sentry 2.1?



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


I don't see a usage of these two versions yet. Should we remove them and 
discuss what is the real intention?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Line 1333 (original), 1332 (patched)


request.getAuthorizables() may return a null value as this is optional in 
thrift.


- Sergio Pena


On Dec. 5, 2018, 11:23 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69352/
> ---
> 
> (Updated Dec. 5, 2018, 11:23 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2452
> https://issues.apache.org/jira/browse/SENTRY-2452
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> TSentryExportMappingDataRequest and TSentryImportMappingDataRequest which are 
> used to send export and import requests to sentry server should be changed to 
> be able to send a list authorizables for which permission information has to 
> be exported/imported.
> 
> These requests should accommodate source and target cluster information as 
> well.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-hdfs/sentry-hdfs-common/src/gen/thrift/gen-javabean/org/apache/sentry/hdfs/service/thrift/TPrivilegePrincipalType.java
>  6eb85217b9229438b9adbd1546a408dc507f1645 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryExportMappingDataRequest.java
>  13e57e04dd779825e2986b1e527f4e556271a162 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryImportMappingDataRequest.java
>  21f3bdf5e304fe2f49684c999d0aa0e0362320de 
>   
> sentry-service/sentry-service-api/src/gen/thrift/gen-javabean/org/apache/sentry/api/service/thrift/TSentryPrivilegesResponse.java
>  cea868f0524c25bf63cfb7c532829354a280df28 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  83393a98df5e10b324f74dc12f10c20bc5f02651 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyServiceClientDefaultImpl.java
>  68d864cfbdf18057d87a65a04af8991292aadccf 
>   
> sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
>

Re: Review Request 69484: SENTRY-2460: Export sentry permission information to HDFS location

2018-12-06 Thread Sergio Pena via Review Board 

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




sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Line 62 (original), 66 (patched)


I think this line goes in the resourcePath line, right?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Lines 94-96 (patched)


Does it accept relative paths (especially in the local filesystem)?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Line 101 (original), 113 (patched)


I know this line was already here, but don't you think that printing the 
contents of the file in the log is unsecure and for big contents the log will 
be too verbose?

Just after the line is logged, the same contents is written to the file, so 
there is duplicated values in two files. Should we remove the contents from be 
sent to the logs?



sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
Lines 115-118 (patched)


Is the fileSystem closed if an exception is thrown on the br.write() and 
br.close()? Should these line be called inside a try-with-resources block?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 72 (patched)


Is this assert needed? I think the createTempDir() throws an exception if 
the directory is not created, so this is not necessary.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 74 (patched)


Same question, is this assert needed? Don't you just need to call mkddirs() 
only?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 77 (patched)


Why do we need 2 nodes for testing an export operation? MiniDFS clusters 
add time on running tests, the less we use them or less resources we ask, the 
test will be faster.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 87 (patched)


When is baseDir a null in this environment? the setupClass should not 
create a null baseDir, shouldn't it?



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 87-92 (patched)


When is baseDir a null in this environment? the setupClass should not 
create a null baseDir, shouldn't it?

Same question for dfsCluster.



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Line 174 (original), 224 (patched)


Change the test case name to testImportExportFromLocalFile() so we 
understand the difference between this and testImportExportFromHDFS()



sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestSentryIniPolicyFileFormatter.java
Lines 272 (patched)


Isn't it better to run resourcePath.toUri().toString() once and keep the 
value in a string before starting the tests? It is more readable + the string 
is not being generated every time it is called.


- Sergio Pena


On Dec. 5, 2018, 10:18 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69484/
> ---
> 
> (Updated Dec. 5, 2018, 10:18 p.m.)
> 
> 
> Review request for sentry and Sergio Pena.
> 
> 
> Bugs: SENTRY-2460
> https://issues.apache.org/jira/browse/SENTRY-2460
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry should be able to export permission information to a HDFS location.
> 
> This can be done using hadoop-common library.
> 
> 
> Diffs
> -
> 
>   
> sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/SentryIniPolicyFileFormatter.java
>  b8ec8a1abadd3b7f99160893c5a4adb2608ea927 
>   
>