Re: Review Request 63878: [SENTRY-1480] Solr/Sentry permissions migration tool

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Nov. 21, 2017, 7:08 p.m., Hrishikesh Gadre wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63878/
> ---
> 
> (Updated Nov. 21, 2017, 7:08 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Bugs: SENTRY-1480
> https://issues.apache.org/jira/browse/SENTRY-1480
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1475 upgraded the Sentry/SOLR plugin to use the latest version of SOLR 
> (7.1.0) which provides pluggable authorization framework. With this, the way 
> admin privileges are defined is changed. This patch provides a command-line 
> tool to migrate existing privileges to this new model. It supports both (a) 
> file based Sentry and (b) Sentry service configuration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
>  6b625ffb61159fa50a7d556762fd08f33e873c40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  ca2de7a165f0c8c37efe2b0c1ab3858addc15acf 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/StrictStringTokenizer.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/Version.java
>  PRE-CREATION 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeUtils.java
>  6628a2f3cbb338542a6f02a0387b6fd7a0e092dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolSolr.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestPermissionsMigrationToolSolr.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63878/diff/2/
> 
> 
> Testing
> ---
> 
> All unit tests are passing.
> Added a unit test to verify this migration tool.
> 
> 
> Thanks,
> 
> Hrishikesh Gadre
> 
>

Re: Review Request 63878: [SENTRY-1480] Solr/Sentry permissions migration tool

[kalyan kumar kalvagadda via Review Board]

> On Nov. 16, 2017, 10 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
> > Lines 236 (patched)
> > 
> >
> > If you implement Collection 
> > transformPrivileges(Collection privileges). You can 
> > avaoid over ahead of converting TSentryPrivilege to string and back to 
> > TSentryPrivilege.
> > 
> > Ideally you should handle exceptions and roll back to a state the 
> > database was before migration was done.
> 
> Hrishikesh Gadre wrote:
> Same as above.
> 
> Hrishikesh Gadre wrote:
> sorry missed the original question. The problem here is that file_based 
> sentry provides privilege as a String, while the Sentry service provides it 
> as a TSentryPrivilege object. In order to avoid code duplication (eg in 
> PermissionsMigrationToolSolr::transformPrivileges), I am using String version 
> of it. Even with your proposal, we would have to translate String privilege 
> to TSentryPrivilege in case of file based Sentry. Hence for simplicity, I 
> think we should be OK with this function signature.

Yes, you are right. Functionally they are duplicate but one would work in 
privileges in string format and the other one on privileges in TSentryPrivilege.
That is fine. we can live with that for now. I will open a jira for that 
optimization and target it for setry 2.1.1.


- kalyan kumar


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


On Nov. 21, 2017, 7:08 p.m., Hrishikesh Gadre wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63878/
> ---
> 
> (Updated Nov. 21, 2017, 7:08 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Bugs: SENTRY-1480
> https://issues.apache.org/jira/browse/SENTRY-1480
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1475 upgraded the Sentry/SOLR plugin to use the latest version of SOLR 
> (7.1.0) which provides pluggable authorization framework. With this, the way 
> admin privileges are defined is changed. This patch provides a command-line 
> tool to migrate existing privileges to this new model. It supports both (a) 
> file based Sentry and (b) Sentry service configuration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
>  6b625ffb61159fa50a7d556762fd08f33e873c40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  ca2de7a165f0c8c37efe2b0c1ab3858addc15acf 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/StrictStringTokenizer.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/Version.java
>  PRE-CREATION 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeUtils.java
>  6628a2f3cbb338542a6f02a0387b6fd7a0e092dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolSolr.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestPermissionsMigrationToolSolr.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63878/diff/2/
> 
> 
> Testing
> ---
> 
> All unit tests are passing.
> Added a unit test to verify this migration tool.
> 
> 
> Thanks,
> 
> Hrishikesh Gadre
> 
>

Re: Review Request 63878: [SENTRY-1480] Solr/Sentry permissions migration tool

[Hrishikesh Gadre via Review Board]

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

(Updated Nov. 21, 2017, 7:08 p.m.)


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


Changes
---

Addressed review comments.


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


Repository: sentry


Description
---

SENTRY-1475 upgraded the Sentry/SOLR plugin to use the latest version of SOLR 
(7.1.0) which provides pluggable authorization framework. With this, the way 
admin privileges are defined is changed. This patch provides a command-line 
tool to migrate existing privileges to this new model. It supports both (a) 
file based Sentry and (b) Sentry service configuration.


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
 6b625ffb61159fa50a7d556762fd08f33e873c40 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
 ca2de7a165f0c8c37efe2b0c1ab3858addc15acf 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/StrictStringTokenizer.java
 PRE-CREATION 
  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/Version.java
 PRE-CREATION 
  
sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeUtils.java
 6628a2f3cbb338542a6f02a0387b6fd7a0e092dc 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolSolr.java
 PRE-CREATION 
  
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestPermissionsMigrationToolSolr.java
 PRE-CREATION 


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

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


Testing
---

All unit tests are passing.
Added a unit test to verify this migration tool.


Thanks,

Hrishikesh Gadre

Re: Review Request 63878: [SENTRY-1480] Solr/Sentry permissions migration tool

[Hrishikesh Gadre via Review Board]

> On Nov. 16, 2017, 10 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
> > Lines 236 (patched)
> > 
> >
> > If you implement Collection 
> > transformPrivileges(Collection privileges). You can 
> > avaoid over ahead of converting TSentryPrivilege to string and back to 
> > TSentryPrivilege.
> > 
> > Ideally you should handle exceptions and roll back to a state the 
> > database was before migration was done.
> 
> Hrishikesh Gadre wrote:
> Same as above.

sorry missed the original question. The problem here is that file_based sentry 
provides privilege as a String, while the Sentry service provides it as a 
TSentryPrivilege object. In order to avoid code duplication (eg in 
PermissionsMigrationToolSolr::transformPrivileges), I am using String version 
of it. Even with your proposal, we would have to translate String privilege to 
TSentryPrivilege in case of file based Sentry. Hence for simplicity, I think we 
should be OK with this function signature.


- Hrishikesh


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


On Nov. 16, 2017, 2:31 p.m., Hrishikesh Gadre wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63878/
> ---
> 
> (Updated Nov. 16, 2017, 2:31 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Bugs: SENTRY-1480
> https://issues.apache.org/jira/browse/SENTRY-1480
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1475 upgraded the Sentry/SOLR plugin to use the latest version of SOLR 
> (7.1.0) which provides pluggable authorization framework. With this, the way 
> admin privileges are defined is changed. This patch provides a command-line 
> tool to migrate existing privileges to this new model. It supports both (a) 
> file based Sentry and (b) Sentry service configuration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
>  6b625ffb61159fa50a7d556762fd08f33e873c40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  ca2de7a165f0c8c37efe2b0c1ab3858addc15acf 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/StrictStringTokenizer.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/Version.java
>  PRE-CREATION 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeUtils.java
>  6628a2f3cbb338542a6f02a0387b6fd7a0e092dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolSolr.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestPermissionsMigrationToolSolr.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63878/diff/1/
> 
> 
> Testing
> ---
> 
> All unit tests are passing.
> Added a unit test to verify this migration tool.
> 
> 
> Thanks,
> 
> Hrishikesh Gadre
> 
>

Re: Review Request 63878: [SENTRY-1480] Solr/Sentry permissions migration tool

[Hrishikesh Gadre via Review Board]

> On Nov. 16, 2017, 10 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
> > Lines 218 (patched)
> > 
> >
> > grantPrivilege and revokePrivilege methods can throw exceptions. I=In 
> > the event one these API's throw an exception the dabtabase will be left in 
> > an inconsistent state and there is no way to reover.

The sentry client/server protocol does not support transactional (i.e. 
all_or_nothing) behavior. Given that privilege grant/revoke methods are 
idempotent, we just need to rerun the tool to fix any inconsistencies. I have 
added a --dry-run option as well, which will allow user to figure out the 
change in advance to mitigate the impact of failed migration.


> On Nov. 16, 2017, 10 p.m., kalyan kumar kalvagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
> > Lines 236 (patched)
> > 
> >
> > If you implement Collection 
> > transformPrivileges(Collection privileges). You can 
> > avaoid over ahead of converting TSentryPrivilege to string and back to 
> > TSentryPrivilege.
> > 
> > Ideally you should handle exceptions and roll back to a state the 
> > database was before migration was done.

Same as above.


- Hrishikesh


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


On Nov. 16, 2017, 2:31 p.m., Hrishikesh Gadre wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63878/
> ---
> 
> (Updated Nov. 16, 2017, 2:31 p.m.)
> 
> 
> Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.
> 
> 
> Bugs: SENTRY-1480
> https://issues.apache.org/jira/browse/SENTRY-1480
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SENTRY-1475 upgraded the Sentry/SOLR plugin to use the latest version of SOLR 
> (7.1.0) which provides pluggable authorization framework. With this, the way 
> admin privileges are defined is changed. This patch provides a command-line 
> tool to migrate existing privileges to this new model. It supports both (a) 
> file based Sentry and (b) Sentry service configuration.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFileConstants.java
>  6b625ffb61159fa50a7d556762fd08f33e873c40 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PolicyFiles.java
>  ca2de7a165f0c8c37efe2b0c1ab3858addc15acf 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/StrictStringTokenizer.java
>  PRE-CREATION 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/Version.java
>  PRE-CREATION 
>   
> sentry-policy/sentry-policy-common/src/main/java/org/apache/sentry/policy/common/PrivilegeUtils.java
>  6628a2f3cbb338542a6f02a0387b6fd7a0e092dc 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolCommon.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/PermissionsMigrationToolSolr.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestPermissionsMigrationToolSolr.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63878/diff/1/
> 
> 
> Testing
> ---
> 
> All unit tests are passing.
> Added a unit test to verify this migration tool.
> 
> 
> Thanks,
> 
> Hrishikesh Gadre
> 
>