Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-17 Thread Sergio Pena via Review Board 

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


Ship it!




Ship It!

- Sergio Pena


On May 16, 2018, 6:41 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 16, 2018, 6:41 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/3/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-16 Thread Na Li via Review Board 

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


Ship it!




Ship It!

- Na Li


On May 16, 2018, 6:41 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 16, 2018, 6:41 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/3/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-16 Thread kalyan kumar kalvagadda via Review Board 


> On May 16, 2018, 3:52 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 278 (patched)
> > 
> >
> > Is it abnormal for this to happen? If so, we need to log at warning 
> > level. If it is normal, log at debug level
> 
> kalyan kumar kalvagadda wrote:
> It is abnormal to happen. it is just a safeguard. we need to think very 
> carefully  before adding a log here as this code gets hit every time there is 
> autirozation request from HDFS.

removed that code.


- kalyan kumar


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


On May 15, 2018, 11:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 15, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-16 Thread Sergio Pena via Review Board 

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




sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 99-101 (original), 99-101 (patched)


This exception is not neeed anymore because we are preventing to happen 
from the asGroup() and asUser() methods. So the exception will never happen.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 103 (patched)


Isn't HdfsAclEntity.asUser() clear enough to let the developers that you're 
creating a user entity? I'm just trying to keep a cleaner api for us, but it is 
not necessary to change it.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 275-279 (patched)


will aclEntry be null? The static methods will never return null so we 
should be safe here.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 288 (patched)


Can we have a comment here why FsAction.NONE is none for users and no for 
groups?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 290 (patched)


aclEntry will never be null now that we use the static constructor, so we 
should be safe here.


- Sergio Pena


On May 15, 2018, 11:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 15, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-16 Thread kalyan kumar kalvagadda via Review Board 


> On May 16, 2018, 3:52 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 245 (patched)
> > 
> >
> > should you check the type if user here? Just to guard against future 
> > change that the AclEntryType is not group nor user.

will add check.


> On May 16, 2018, 3:52 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 278 (patched)
> > 
> >
> > Is it abnormal for this to happen? If so, we need to log at warning 
> > level. If it is normal, log at debug level

It is abnormal to happen. it is just a safeguard. we need to think very 
carefully  before adding a log here as this code gets hit every time there is 
autirozation request from HDFS.


> On May 16, 2018, 3:52 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 288 (patched)
> > 
> >
> > why should this be FsAction.NONE instead of "fsAction = 
> > perms.get(aclEntry) first. When it is null, then assign it to be  
> > FsAction.NONE"?

"perms" map is constructed every time when there is authorization check done 
from HDFS. 
I see that your comment is based on what we have for ROLE. While getting perms 
for roles, code looks for permissions for all the groups that this is granted 
so we have logic "fsAction = perms.get(aclEntry)" there.
when it come to users, entry in perms is not expected that is why there is no 
need to have similar logic for users.


- kalyan kumar


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


On May 15, 2018, 11:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 15, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-16 Thread Na Li via Review Board 

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




sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 245 (patched)


should you check the type if user here? Just to guard against future change 
that the AclEntryType is not group nor user.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 278 (patched)


Is it abnormal for this to happen? If so, we need to log at warning level. 
If it is normal, log at debug level



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 288 (patched)


why should this be FsAction.NONE instead of "fsAction = perms.get(aclEntry) 
first. When it is null, then assign it to be  FsAction.NONE"?


- Na Li


On May 15, 2018, 11:45 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 15, 2018, 11:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/2/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-15 Thread kalyan kumar kalvagadda via Review Board 

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

(Updated May 15, 2018, 11:45 p.m.)


Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.


Changes
---

addressed review comments from sergio.


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


Repository: sentry


Description
---

SentryAuthorizationProvider should now additionally provide the ACL entries 
with the permissions that users have along with the permissions for the groups.

With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have role 
to permission mapping. it will also have user to privilege mapping information.

SentryAuthorizationProvider should be using the new information added in 
PrivilegeInfo to add ACL for users.


Diffs (updated)
-

  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
 a88d8e25ad745effe354aa6267252998b189a252 
  
sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
 dbce40538cf4721b601cf63b1da713f3d57fc981 


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

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


Testing
---

Added new unit tests and also made sure that all the existing tests pass.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 67131: SENTRY-2174: Sentry authorization provider should now generate ACL for users

2018-05-15 Thread Sergio Pena via Review Board 

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




sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 92-93 (patched)


should these be private final?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 95-102 (patched)


I usually try to avoid throwing exceptions in the constructor because makes 
things easier to read and code.

So I was wondering if we could make this constructor private and use a 
static method to initialize the arguments intead, like:

public static HdfsAclEntity asGroup(String groupname) {
  return new HdfsAclEntity(AclEntryType.GROUP, groupname);
}

public static HdfsAclEntity asUser(String username) {
  return new HdfsAclEntity(AclEntryType.USER, username);
}

As you see, it above methods will avoid throwing exceptions. 
Do you think we should do the same here?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 141-143 (original), 214-216 (patched)


Should we have an if() that prevents constructing permissions if a 
TPrivilegeEntityType.GROUP is sent (which is not supported)?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 143-144 (original), 216-217 (patched)


Why are you constructing permissions twice?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 157-158 (original), 236-242 (patched)


Doesn't this if() be on the getPerms() function like it was on the 
getGrouopPerms?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Line 176 (original), 268-273 (patched)


we could use HdfsAclEntity.asGroup(group) here to avoid catching exceptions 
that should not happen.



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Line 181 (original), 283-286 (patched)


we could use HdfsAclEntity.asUser(user) here to avoid catching exceptions 
that should not happen.


- Sergio Pena


On May 15, 2018, 4:03 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67131/
> ---
> 
> (Updated May 15, 2018, 4:03 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2174
> https://issues.apache.org/jira/browse/SENTRY-2174
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> SentryAuthorizationProvider should now additionally provide the ACL entries 
> with the permissions that users have along with the permissions for the 
> groups.
> 
> With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have 
> role to permission mapping. it will also have user to privilege mapping 
> information.
> 
> SentryAuthorizationProvider should be using the new information added in 
> PrivilegeInfo to add ACL for users.
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
>  a88d8e25ad745effe354aa6267252998b189a252 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java
>  dbce40538cf4721b601cf63b1da713f3d57fc981 
> 
> 
> Diff: https://reviews.apache.org/r/67131/diff/1/
> 
> 
> Testing
> ---
> 
> Added new unit tests and also made sure that all the existing tests pass.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>