You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao <ha...@cloudera.com> on 2015/11/04 04:51:29 UTC

Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

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

Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Repository: sentry


Description
-------

Paths that are sentry managed should not succesfully chmod/chown/removeACL.
We should update setGroup/setUser/setPermission and removeAclFeature.

Old behavior:
chmod/chown
    if not under prefix + unmanaged: writes to hdfs.
    if managed: writes to hdfs.
Removing acls:
    if not under prefix + unmanaged: removes from hdfs.
    if managed: removes from hdfs.

New behavior:
If not under prefix + unmanaged: writes to/removes from hdfs.
If managed : no op.


Diffs
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

> On Nov. 5, 2015, 11:45 p.m., Sravya Tirukkovalur wrote:
> >
> 
> Hao Hao wrote:
>     In getUser / getGroup and some otehr APIs we are doing the same checking, if the condition is not good, then we should change all of them at the same time to be consistent?
> 
> Sravya Tirukkovalur wrote:
>     There is a difference between the two
>     
>     if (!authzInfo.isManaged(pathElements)
>                 || !authzInfo.doesBelongToAuthzObject(pathElements)) //Either not in prefix or not a hive object
>     
>     
>     
>     if (!authzInfo.isManaged(pathElements)) { //If not in prefix
>           group = getDefaultProviderGroup(node, snapshotId);
>         } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { //If in prefix and not a hive object
>           group = getDefaultProviderGroup(node, snapshotId);
>         }
> 
> Hao Hao wrote:
>     If a path is associated with hive object but not in prefix, the following statement will be evaluated to be true and writes to hdfs, which is what we desire, right? No op is only for paths inside the prefix + hive object. 
>     
>     if (!authzInfo.isManaged(pathElements)
>                 || !authzInfo.doesBelongToAuthzObject(pathElements))
> 
> Ryan Pridgeon wrote:
>     Going to have to side with Hao on this one. Hive objects which reside outside of a prefix should be managed by the DefaultAuthorizationProvider and not Sentry's implementation. Technically Hive objects which reside outside of a prefix should not be found in UpdateableAuthzPath anyway. The initial check just acts a short-circuit for the latter evaluation. That being said an or evaluation serves the same purpose. It also improve readability.

My bad. I overlooked the not part in the first case. I feel silly :-P


- Sravya


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


On Nov. 6, 2015, 1:29 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 1:29 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.

> On Nov. 5, 2015, 11:45 p.m., Sravya Tirukkovalur wrote:
> >
> 
> Hao Hao wrote:
>     In getUser / getGroup and some otehr APIs we are doing the same checking, if the condition is not good, then we should change all of them at the same time to be consistent?
> 
> Sravya Tirukkovalur wrote:
>     There is a difference between the two
>     
>     if (!authzInfo.isManaged(pathElements)
>                 || !authzInfo.doesBelongToAuthzObject(pathElements)) //Either not in prefix or not a hive object
>     
>     
>     
>     if (!authzInfo.isManaged(pathElements)) { //If not in prefix
>           group = getDefaultProviderGroup(node, snapshotId);
>         } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { //If in prefix and not a hive object
>           group = getDefaultProviderGroup(node, snapshotId);
>         }

If a path is associated with hive object but not in prefix, the following statement will be evaluated to be true and writes to hdfs, which is what we desire, right? No op is only for paths inside the prefix + hive object. 

if (!authzInfo.isManaged(pathElements)
            || !authzInfo.doesBelongToAuthzObject(pathElements))


- Hao


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


On Nov. 5, 2015, 5:38 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 5:38 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanag: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix: writes to/removes from hdfs.
> If else: no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.

> On Nov. 5, 2015, 11:45 p.m., Sravya Tirukkovalur wrote:
> >

In getUser / getGroup and some otehr APIs we are doing the same checking, if the condition is not good, then we should change all of them at the same time to be consistent?


- Hao


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


On Nov. 5, 2015, 5:38 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 5:38 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanag: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix: writes to/removes from hdfs.
> If else: no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.

> On Nov. 5, 2015, 11:45 p.m., Sravya Tirukkovalur wrote:
> >
> 
> Hao Hao wrote:
>     In getUser / getGroup and some otehr APIs we are doing the same checking, if the condition is not good, then we should change all of them at the same time to be consistent?

There is a difference between the two

if (!authzInfo.isManaged(pathElements)
            || !authzInfo.doesBelongToAuthzObject(pathElements)) //Either not in prefix or not a hive object


if (!authzInfo.isManaged(pathElements)) { //If not in prefix
      group = getDefaultProviderGroup(node, snapshotId);
    } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { //If in prefix and not a hive object
      group = getDefaultProviderGroup(node, snapshotId);
    }


- Sravya


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


On Nov. 5, 2015, 5:38 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 5:38 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanag: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix: writes to/removes from hdfs.
> If else: no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Ryan Pridgeon <rn...@cloudera.com>.

> On Nov. 5, 2015, 11:45 p.m., Sravya Tirukkovalur wrote:
> >
> 
> Hao Hao wrote:
>     In getUser / getGroup and some otehr APIs we are doing the same checking, if the condition is not good, then we should change all of them at the same time to be consistent?
> 
> Sravya Tirukkovalur wrote:
>     There is a difference between the two
>     
>     if (!authzInfo.isManaged(pathElements)
>                 || !authzInfo.doesBelongToAuthzObject(pathElements)) //Either not in prefix or not a hive object
>     
>     
>     
>     if (!authzInfo.isManaged(pathElements)) { //If not in prefix
>           group = getDefaultProviderGroup(node, snapshotId);
>         } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { //If in prefix and not a hive object
>           group = getDefaultProviderGroup(node, snapshotId);
>         }
> 
> Hao Hao wrote:
>     If a path is associated with hive object but not in prefix, the following statement will be evaluated to be true and writes to hdfs, which is what we desire, right? No op is only for paths inside the prefix + hive object. 
>     
>     if (!authzInfo.isManaged(pathElements)
>                 || !authzInfo.doesBelongToAuthzObject(pathElements))

Going to have to side with Hao on this one. Hive objects which reside outside of a prefix should be managed by the DefaultAuthorizationProvider and not Sentry's implementation. Technically Hive objects which reside outside of a prefix should not be found in UpdateableAuthzPath anyway. The initial check just acts a short-circuit for the latter evaluation. That being said an or evaluation serves the same purpose. It also improve readability.


- Ryan


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


On Nov. 6, 2015, 1:29 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 1:29 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/#review105329
-----------------------------------------------------------



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 202)
<https://reviews.apache.org/r/39928/#comment163887>

    Also, I just realized this would mean, if a path is associated with hive object but not in prefix: This condition would still pass. Which is not good, sentry hdfs sync should only affect paths inside prefix.


- Sravya Tirukkovalur


On Nov. 5, 2015, 5:38 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 5:38 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanag: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix: writes to/removes from hdfs.
> If else: no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/#review105524
-----------------------------------------------------------

Ship it!


Ship It!

- Sravya Tirukkovalur


On Nov. 6, 2015, 10:49 p.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 10:49 p.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> SENTRY-994: Changes the use cases.
> 
> Change-Id: I55a7a6c89c3ca42f28f85daa855caa9da5a86916
> 
> SENTRY-944: Removed the exception in the test cases.
> 
> Change-Id: I5c33d5be357b4141f4e38f64f91b3f54058f5789
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 10:49 p.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Changes
-------

Removed the exceptions in the test cases since it is no longer be thrown.


Repository: sentry


Description (updated)
-------

Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b

SENTRY-994: Changed the Logger level and added more test case for removeFeature.

Change-Id: I851344e088155e28c8978203759fe56884b29e41

SENTRY-994: Changes the use cases.

Change-Id: I55a7a6c89c3ca42f28f85daa855caa9da5a86916

SENTRY-944: Removed the exception in the test cases.

Change-Id: I5c33d5be357b4141f4e38f64f91b3f54058f5789


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 10:43 p.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Changes
-------

Reorg the test cased and added the test cases for paths under prefix and not hive objects.


Repository: sentry


Description (updated)
-------

Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b

SENTRY-994: Changed the Logger level and added more test case for removeFeature.

Change-Id: I851344e088155e28c8978203759fe56884b29e41

SENTRY-994: Changes the use cases.

Change-Id: I55a7a6c89c3ca42f28f85daa855caa9da5a86916


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.

> On Nov. 6, 2015, 5:55 p.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java, line 202
> > <https://reviews.apache.org/r/39928/diff/2/?file=1117508#file1117508line202>
> >
> >     Should this be /user/authz/xxx (in prefix but not a hive object)? Looks like you already tested for /user/authz/obj (in prefix and hive object)?

Yeah, I am testing inside prefix and is a hive object. Will change the comment correspondingly.


- Hao


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


On Nov. 6, 2015, 1:29 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 1:29 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/#review105476
-----------------------------------------------------------

Ship it!



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 419)
<https://reviews.apache.org/r/39928/#comment164096>

    Nit: Edit the comment: for paths in prefix?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java (line 202)
<https://reviews.apache.org/r/39928/#comment164098>

    Should this be /user/authz/xxx (in prefix but not a hive object)? Looks like you already tested for /user/authz/obj (in prefix and hive object)?


- Sravya Tirukkovalur


On Nov. 6, 2015, 1:29 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 1:29 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 1:29 a.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
-------

Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b

SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.

Change-Id: I851344e088155e28c8978203759fe56884b29e41


Diffs
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 1:28 a.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Changes
-------

Changed the Logger level and added more test case for removeFeature based on the code review


Repository: sentry


Description (updated)
-------

Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b

SENTRY-994: Changed the Logger level and added more test case for removeFeature.

Change-Id: I851344e088155e28c8978203759fe56884b29e41


Diffs (updated)
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 1:18 a.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
-------

Paths that are sentry managed should not succesfully chmod/chown/removeACL.
We should update setGroup/setUser/setPermission and removeAclFeature.

Old behavior:
chmod/chown
    if under prefix + hive object: writes to hdfs.
    if unmanaged: writes to hdfs.
Removing acls:
    if under prefix + hive object: removes from hdfs.
    if unmanaged: removes from hdfs.

New behavior for chmod/chown:
    If under prefix + hive object: no op.
    If unmanaged: writes to hdfs.
    
New behavior for removing acls:
    If under prefix: removes from hdfs.
    If else: no op.


Diffs
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 6, 2015, 1:08 a.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
-------

Paths that are sentry managed should not succesfully chmod/chown/removeACL.
We should update setGroup/setUser/setPermission and removeAclFeature.

Old behavior:
chmod/chown
    if not under prefix + unmanaged: writes to hdfs.
    if managed: writes to hdfs.
Removing acls:
    if not under prefix + unmanaged: removes from hdfs.
    if managed: removes from hdfs.

New behavior:
If not under prefix: writes to/removes from hdfs.
If else: no op.


Diffs
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/
-----------------------------------------------------------

(Updated Nov. 5, 2015, 5:38 a.m.)


Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.


Repository: sentry


Description (updated)
-------

Paths that are sentry managed should not succesfully chmod/chown/removeACL.
We should update setGroup/setUser/setPermission and removeAclFeature.

Old behavior:
chmod/chown
    if not under prefix + unmanaged: writes to hdfs.
    if managed: writes to hdfs.
Removing acls:
    if not under prefix + unmanag: removes from hdfs.
    if managed: removes from hdfs.

New behavior:
If not under prefix: writes to/removes from hdfs.
If else: no op.


Diffs
-----

  sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
  sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 

Diff: https://reviews.apache.org/r/39928/diff/


Testing
-------

Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.


Thanks,

Hao Hao


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Anne Yu <an...@cloudera.com>.

> On Nov. 4, 2015, 6:27 p.m., Anne Yu wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java, line 202
> > <https://reviews.apache.org/r/39928/diff/1/?file=1115325#file1115325line202>
> >
> >     One question, will this also take care of hive.warehouse.dir? Details can be found from CDH-33778. See if we can also fix this one with the same cr. Is the configured hive.warehouse.dir also a managed object or not?

The tracking no should be ENTRY-951: move hive warehouse dir to /hive, the dir doesn't have hive:hive as owner.


- Anne


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


On Nov. 5, 2015, 5:38 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 5:38 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanag: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix: writes to/removes from hdfs.
> If else: no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Anne Yu <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/#review105112
-----------------------------------------------------------



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 202)
<https://reviews.apache.org/r/39928/#comment163518>

    One question, will this also take care of hive.warehouse.dir? Details can be found from CDH-33778. See if we can also fix this one with the same cr. Is the configured hive.warehouse.dir also a managed object or not?


- Anne Yu


On Nov. 4, 2015, 3:51 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2015, 3:51 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanaged: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix + unmanaged: writes to/removes from hdfs.
> If managed : no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.

> On Nov. 5, 2015, 12:48 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java, line 415
> > <https://reviews.apache.org/r/39928/diff/1/?file=1115325#file1115325line415>
> >
> >     I believe that you are not checking the authzInfo.doesBelongToAuthzObject(pathElements) to keep it symmetric with addAclFeature. If that is the case, can you update the jira/RB description and file a follow on jira which corrects this behavior for both add/removeAclFeature?

Yeah, it is to ensure the behaivors are matching for add/removeAclFeature. Will change the jira and file a new one.


- Hao


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


On Nov. 4, 2015, 3:51 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2015, 3:51 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanaged: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix + unmanaged: writes to/removes from hdfs.
> If managed : no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Hao Hao <ha...@cloudera.com>.

> On Nov. 5, 2015, 12:48 a.m., Sravya Tirukkovalur wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java, line 415
> > <https://reviews.apache.org/r/39928/diff/1/?file=1115325#file1115325line415>
> >
> >     I believe that you are not checking the authzInfo.doesBelongToAuthzObject(pathElements) to keep it symmetric with addAclFeature. If that is the case, can you update the jira/RB description and file a follow on jira which corrects this behavior for both add/removeAclFeature?
> 
> Hao Hao wrote:
>     Yeah, it is to ensure the behaivors are matching for add/removeAclFeature. Will change the jira and file a new one.

Added the Jira-954 for fixing add/removeAclFeature.


- Hao


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


On Nov. 6, 2015, 1:29 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 1:29 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Change-Id: I7c65bf182c44075f41de16943c5b7eb66e3dec0b
> 
> SENTRY-994: Changed the Logger level and added more test case for removeACLFeature.
> 
> Change-Id: I851344e088155e28c8978203759fe56884b29e41
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>


Re: Review Request 39928: SENTRY-944: Setting HDFS rules on Sentry managed hdfs paths should not affect original hdfs rules.

Posted by Sravya Tirukkovalur <sr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39928/#review105189
-----------------------------------------------------------


Thanks for your patch Hao! Mostly looks good to me. Left some minor comments.


sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 207)
<https://reviews.apache.org/r/39928/#comment163609>

    Can we actually log an error? If log level is anything more than debug, it silently fails and can be confusing for users.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java (line 415)
<https://reviews.apache.org/r/39928/#comment163611>

    I believe that you are not checking the authzInfo.doesBelongToAuthzObject(pathElements) to keep it symmetric with addAclFeature. If that is the case, can you update the jira/RB description and file a follow on jira which corrects this behavior for both add/removeAclFeature?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java (line 198)
<https://reviews.apache.org/r/39928/#comment163612>

    You may want to assert that the acl has been actually added and removed?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java (line 205)
<https://reviews.apache.org/r/39928/#comment163613>

    Also add a acl add and remove test for pathInside?


- Sravya Tirukkovalur


On Nov. 4, 2015, 3:51 a.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39928/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2015, 3:51 a.m.)
> 
> 
> Review request for sentry, Anne Yu, Lenni Kuff, and Sravya Tirukkovalur.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Paths that are sentry managed should not succesfully chmod/chown/removeACL.
> We should update setGroup/setUser/setPermission and removeAclFeature.
> 
> Old behavior:
> chmod/chown
>     if not under prefix + unmanaged: writes to hdfs.
>     if managed: writes to hdfs.
> Removing acls:
>     if not under prefix + unmanaged: removes from hdfs.
>     if managed: removes from hdfs.
> 
> New behavior:
> If not under prefix + unmanaged: writes to/removes from hdfs.
> If managed : no op.
> 
> 
> Diffs
> -----
> 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java 419ab68e0d03f995c55d229b762453468de47571 
>   sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java fd5146f079d93687738a522f42beaa59031a4f82 
> 
> Diff: https://reviews.apache.org/r/39928/diff/
> 
> 
> Testing
> -------
> 
> Added several new unit tests for setPermission/setUser/setGroup/removeAcl cases validation.
> 
> 
> Thanks,
> 
> Hao Hao
> 
>