Re: Review Request 72513: Add support for Presto 333

2020-05-25 Thread Pradeep Agrawal 

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



Since Patch is already committed in apache ranger master branch therefore, it 
will be helpful(for tracking) if you can please change the RR status to 
"submitted".

- Pradeep Agrawal


On May 17, 2020, 7:38 a.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 17, 2020, 7:38 a.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
>   
> security-admin/src/main/java/org/apache/ranger/patch/PatchForPrestoToSupportPresto333_J10037.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>

Re: Review Request 72513: Add support for Presto 333

2020-05-17 Thread Madhan Neethiraj 

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


Ship it!




Ship It!

- Madhan Neethiraj


On May 17, 2020, 7:38 a.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 17, 2020, 7:38 a.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
>   
> security-admin/src/main/java/org/apache/ranger/patch/PatchForPrestoToSupportPresto333_J10037.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>

Re: Review Request 72513: Add support for Presto 333

2020-05-17 Thread Bolke de Bruin 


> On May 15, 2020, 2:43 p.m., Madhan Neethiraj wrote:
> > agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json
> > Lines 164 (patched)
> > 
> >
> > Changes in service-def (like addition of resources/access-types, 
> > changes in access-types/data-mask/row-filter) will not be applied to 
> > existing service-def in Ranger. Updating existing service-def would require 
> > a Java patch. Please file a JIRA to track Java patch to handle this.

RANGER-2795 is tracking this


> On May 15, 2020, 2:43 p.m., Madhan Neethiraj wrote:
> > agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json
> > Lines 266 (patched)
> > 
> >
> > I suggest to not change itemId of existing entries; and assign 
> > itemId=13 for the new entry 'execute'.

Sure. Is itemid used anywhere?


> On May 15, 2020, 2:43 p.m., Madhan Neethiraj wrote:
> > plugin-presto/src/test/resources/presto-policies.json
> > Line 1013 (original), 1187 (patched)
> > 
> >
> > Row-filter expressions are likely to refer to columns in the table for 
> > which the filter is applied. When wildcard is allowed in row-filter 
> > policies, it might be challenging to make sure that the row-filter 
> > expression is valid for all the tables covered by the wildcard. When the 
> > row-filter is invalid for a table, the query will fail. Hence wildCard was 
> > disabled for row-filters. Please review to make sure that this is clearly 
> > understood.

I understand your point, however, this is already in the master, you are 
commenting on an update to the test policies which are a reflection of what is 
currently in master.


- Bolke


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


On May 17, 2020, 7:38 a.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 17, 2020, 7:38 a.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
>   
> security-admin/src/main/java/org/apache/ranger/patch/PatchForPrestoToSupportPresto333_J10037.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>

Re: Review Request 72513: Add support for Presto 333

2020-05-17 Thread Bolke de Bruin 

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

(Updated May 17, 2020, 7:38 a.m.)


Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, and 
Ramesh Mani.


Changes
---

- Added upgrade patch
- Addressed comments


Bugs: https://jira.apache.org/jira/browse/RANGER-2826

https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826


Repository: ranger


Description
---

Presto 332/333 are backwards incompatible.


Diffs (updated)
-

  agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
4d5b79582 
  
plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
 d4521a392 
  
plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
 c00d51986 
  plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
  pom.xml ebce7c9f0 
  
ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
 bfb3a5961 
  
security-admin/src/main/java/org/apache/ranger/patch/PatchForPrestoToSupportPresto333_J10037.java
 PRE-CREATION 


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

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


Testing
---

Unit tests updated. Production.


Thanks,

Bolke de Bruin

Re: Review Request 72513: Add support for Presto 333

2020-05-15 Thread Madhan Neethiraj 

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




agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json
Lines 164 (patched)


Changes in service-def (like addition of resources/access-types, changes in 
access-types/data-mask/row-filter) will not be applied to existing service-def 
in Ranger. Updating existing service-def would require a Java patch. Please 
file a JIRA to track Java patch to handle this.



agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json
Lines 266 (patched)


I suggest to not change itemId of existing entries; and assign itemId=13 
for the new entry 'execute'.



plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
Lines 328 (patched)


Consider removing "==> " from this debug log, as prefix is used at 
function-entry; and this log is not about function-entry.

Please review and update other such log messages intrdocued in this patch.



plugin-presto/src/test/resources/presto-policies.json
Line 1013 (original), 1187 (patched)


Row-filter expressions are likely to refer to columns in the table for 
which the filter is applied. When wildcard is allowed in row-filter policies, 
it might be challenging to make sure that the row-filter expression is valid 
for all the tables covered by the wildcard. When the row-filter is invalid for 
a table, the query will fail. Hence wildCard was disabled for row-filters. 
Please review to make sure that this is clearly understood.


- Madhan Neethiraj


On May 14, 2020, 8:25 p.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 14, 2020, 8:25 p.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>

Re: Review Request 72513: Add support for Presto 333

2020-05-14 Thread Bolke de Bruin 


> On May 15, 2020, 3:57 a.m., Pradeep Agrawal wrote:
> > Presto plugin was added in ranger-2.0. Will these changes be available for 
> > users who upgrade from ranger-2.0 ? What will be the behaviour with this 
> > change in ranger upgrade case.

the plugin itself is backwards compatible with older Prestos.

note I recently committed an uodate to this plugin "after ranger 2.0" tbis is 
merely a follow up.


- Bolke


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


On May 14, 2020, 8:25 p.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 14, 2020, 8:25 p.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>

Re: Review Request 72513: Add support for Presto 333

2020-05-14 Thread Pradeep Agrawal 

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



Presto plugin was added in ranger-2.0. Will these changes be available for 
users who upgrade from ranger-2.0 ? What will be the behaviour with this change 
in ranger upgrade case.

- Pradeep Agrawal


On May 14, 2020, 8:25 p.m., Bolke de Bruin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72513/
> ---
> 
> (Updated May 14, 2020, 8:25 p.m.)
> 
> 
> Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, 
> and Ramesh Mani.
> 
> 
> Bugs: https://jira.apache.org/jira/browse/RANGER-2826
> 
> https://issues.apache.org/jira/browse/https://jira.apache.org/jira/browse/RANGER-2826
> 
> 
> Repository: ranger
> 
> 
> Description
> ---
> 
> Presto 332/333 are backwards incompatible.
> 
> 
> Diffs
> -
> 
>   agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json 
> 4d5b79582 
>   
> plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  d4521a392 
>   
> plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java
>  c00d51986 
>   plugin-presto/src/test/resources/presto-policies.json 28eabf2d6 
>   pom.xml ebce7c9f0 
>   
> ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java
>  bfb3a5961 
> 
> 
> Diff: https://reviews.apache.org/r/72513/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests updated. Production.
> 
> 
> Thanks,
> 
> Bolke de Bruin
> 
>