You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by bhavik patel <bh...@gmail.com> on 2017/05/10 13:34:23 UTC
Review Request 59143: RANGER-1571 : Code Improvement To Follow Best
Practices
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/
-----------------------------------------------------------
Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
Bugs: RANGER-1571
https://issues.apache.org/jira/browse/RANGER-1571
Repository: ranger
Description
-------
Code Improvement To Follow Best Practices.
Diffs
-----
agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
Diff: https://reviews.apache.org/r/59143/diff/1/
Testing
-------
1. Tested Ranger Admin & Ranger KMS rest calls.
2. Verified Test-Connection for plugins on kerberized environment.
Thanks,
bhavik patel
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by bhavik patel <bh...@gmail.com>.
> On May 10, 2017, 1:53 p.m., Colm O hEigeartaigh wrote:
> > Please make sure there are spaces before open curly brackets, e.g. "if (aPassword.contains(",")){" and "}else{".
> >
> > The default values are defined both in "security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java" and "agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java". Why not just have them in agents-common and refer to them in the admin module?
I have updated the patch as per your comments.
- bhavik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174471
-----------------------------------------------------------
On May 11, 2017, 10:25 a.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 10:25 a.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/3/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174471
-----------------------------------------------------------
Please make sure there are spaces before open curly brackets, e.g. "if (aPassword.contains(",")){" and "}else{".
The default values are defined both in "security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java" and "agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java". Why not just have them in agents-common and refer to them in the admin module?
- Colm O hEigeartaigh
On May 10, 2017, 1:44 p.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 1:44 p.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/2/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by bhavik patel <bh...@gmail.com>.
> On May 10, 2017, 8:55 p.m., Sailaja Polavarapu wrote:
> > Couple of feedback items:
> > 1. I agree with Colm that it is better to have default values in one place (PasswordUtils.java)
> > 2. I noticed that the new method "setPropertiesvalues(String aPassword) in PasswordUtils.java contains very similar logic that is repeated in ServiceDBStore.java and RangerServiceService.java. Can we merge this common logic to one place?
> > 3. One quick question - In RangerServiceService.java - getConfigsWithDecryptedPassword(), I noticed that ServiceDBStore.CRYPT_ALGO is being updated every time. Didn't completely understand the logic on why we need to do this? Can you please provide some details.
> > 4. Are you planning to provide a different patch for ranger-0.7 branch? The current patch fails to apply on ranger-0.7 branch.
> > 5. And in case of upgrade, since these new properties are not going to be there, I am guessing the default values will be used.
1. Patch is upadted.
2. No, In PasswordUtil class all the methods are static and we are using static variable; if multiple plugins are installed using different algorithm than that will create problem because it will use previous values.
3. In RangerServiceService.java, whenver we are encrypting or decrypting password will require that specific string format;
so whenever we are getting value for password will get encrypted string that will be use to decrypt password directly but whenever we want to encrypt that decrypted string will reuired that padded string.
4. Yes, will provide different patch for ranger-0.7 branch.
5. In case of upgrade will use default logic for existing repos.
- bhavik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174551
-----------------------------------------------------------
On May 11, 2017, 10:25 a.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 10:25 a.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/3/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by Sailaja Polavarapu <sp...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174551
-----------------------------------------------------------
Couple of feedback items:
1. I agree with Colm that it is better to have default values in one place (PasswordUtils.java)
2. I noticed that the new method "setPropertiesvalues(String aPassword) in PasswordUtils.java contains very similar logic that is repeated in ServiceDBStore.java and RangerServiceService.java. Can we merge this common logic to one place?
3. One quick question - In RangerServiceService.java - getConfigsWithDecryptedPassword(), I noticed that ServiceDBStore.CRYPT_ALGO is being updated every time. Didn't completely understand the logic on why we need to do this? Can you please provide some details.
4. Are you planning to provide a different patch for ranger-0.7 branch? The current patch fails to apply on ranger-0.7 branch.
5. And in case of upgrade, since these new properties are not going to be there, I am guessing the default values will be used.
- Sailaja Polavarapu
On May 10, 2017, 1:57 p.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 1:57 p.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/2/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by Sailaja Polavarapu <sp...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174729
-----------------------------------------------------------
Ship it!
Thanks for the clarification and update Bhavik. Changes look fine to me.
- Sailaja Polavarapu
On May 11, 2017, 10:25 a.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 10:25 a.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/3/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by Velmurugan Periasamy <vp...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/#review174757
-----------------------------------------------------------
Ship it!
Ship It!
- Velmurugan Periasamy
On May 11, 2017, 10:25 a.m., bhavik patel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59143/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 10:25 a.m.)
>
>
> Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Bugs: RANGER-1571
> https://issues.apache.org/jira/browse/RANGER-1571
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Code Improvement To Follow Best Practices.
>
>
> Diffs
> -----
>
> agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
> agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
> security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
> security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
> security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
>
>
> Diff: https://reviews.apache.org/r/59143/diff/3/
>
>
> Testing
> -------
>
> 1. Tested Ranger Admin & Ranger KMS rest calls.
> 2. Verified Test-Connection for plugins on kerberized environment.
>
>
> Thanks,
>
> bhavik patel
>
>
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by bhavik patel <bh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/
-----------------------------------------------------------
(Updated May 11, 2017, 10:25 a.m.)
Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
Bugs: RANGER-1571
https://issues.apache.org/jira/browse/RANGER-1571
Repository: ranger
Description
-------
Code Improvement To Follow Best Practices.
Diffs (updated)
-----
agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
Diff: https://reviews.apache.org/r/59143/diff/3/
Changes: https://reviews.apache.org/r/59143/diff/2-3/
Testing
-------
1. Tested Ranger Admin & Ranger KMS rest calls.
2. Verified Test-Connection for plugins on kerberized environment.
Thanks,
bhavik patel
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by bhavik patel <bh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/
-----------------------------------------------------------
(Updated May 10, 2017, 1:57 p.m.)
Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
Bugs: RANGER-1571
https://issues.apache.org/jira/browse/RANGER-1571
Repository: ranger
Description
-------
Code Improvement To Follow Best Practices.
Diffs
-----
agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
Diff: https://reviews.apache.org/r/59143/diff/2/
Testing
-------
1. Tested Ranger Admin & Ranger KMS rest calls.
2. Verified Test-Connection for plugins on kerberized environment.
Thanks,
bhavik patel
Re: Review Request 59143: RANGER-1571 : Code Improvement To Follow
Best Practices
Posted by bhavik patel <bh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59143/
-----------------------------------------------------------
(Updated May 10, 2017, 1:44 p.m.)
Review request for ranger, Ankita Sinha, Gautam Borad, Abhay Kulkarni, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, Ramesh Mani, Selvamohan Neethiraj, and Velmurugan Periasamy.
Bugs: RANGER-1571
https://issues.apache.org/jira/browse/RANGER-1571
Repository: ranger
Description
-------
Code Improvement To Follow Best Practices.
Diffs (updated)
-----
agents-common/src/main/java/org/apache/ranger/plugin/client/HadoopConfigHolder.java 29983da
agents-common/src/main/java/org/apache/ranger/plugin/util/PasswordUtils.java f32355a
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java 33e0e85
security-admin/src/main/java/org/apache/ranger/service/RangerServiceService.java f194b18
security-admin/src/main/resources/conf.dist/ranger-admin-default-site.xml 0feecfe
Diff: https://reviews.apache.org/r/59143/diff/2/
Changes: https://reviews.apache.org/r/59143/diff/1-2/
Testing
-------
1. Tested Ranger Admin & Ranger KMS rest calls.
2. Verified Test-Connection for plugins on kerberized environment.
Thanks,
bhavik patel