You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org> on 2018/05/15 16:03:04 UTC
Review Request 67131: SENTRY-2174: Sentry authorization provider
should now generate ACL for users
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67131/
-----------------------------------------------------------
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
Re: Review Request 67131: SENTRY-2174: Sentry authorization provider
should now generate ACL for users
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/67131/#comment285223>
should these be private final?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
Lines 95-102 (patched)
<https://reviews.apache.org/r/67131/#comment285234>
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)
<https://reviews.apache.org/r/67131/#comment285236>
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)
<https://reviews.apache.org/r/67131/#comment285235>
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)
<https://reviews.apache.org/r/67131/#comment285238>
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)
<https://reviews.apache.org/r/67131/#comment285240>
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)
<https://reviews.apache.org/r/67131/#comment285241>
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
>
>
Re: Review Request 67131: SENTRY-2174: Sentry authorization provider
should now generate ACL for users
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Addressed reiview comments and fixed a bug with user permissions.
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/3/
Changes: https://reviews.apache.org/r/67131/diff/2-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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line291>
> >
> > why should this be FsAction.NONE instead of "fsAction = perms.get(aclEntry) first. When it is null, then assign it to be FsAction.NONE"?
>
> kalyan kumar kalvagadda wrote:
> "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.
Ignore my previous comment. there is a bug here. I will fix in the new patch.
- 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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line281>
> >
> > 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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line248>
> >
> > 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)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line281>
> >
> > 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)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line291>
> >
> > 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
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/67131/#comment285323>
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)
<https://reviews.apache.org/r/67131/#comment285324>
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)
<https://reviews.apache.org/r/67131/#comment285325>
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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On May 16, 2018, 4:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 99-101 (original), 99-101 (patched)
> > <https://reviews.apache.org/r/67131/diff/1-2/?file=2023041#file2023041line99>
> >
> > This exception is not neeed anymore because we are preventing to happen from the asGroup() and asUser() methods. So the exception will never happen.
HdfsAclEntity is inner class. Inspite making the constructor private, Methods in SentryPermissions can still instantiate HdfsAclEntity. That is why i did not remove the exception.
> On May 16, 2018, 4:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 103 (patched)
> > <https://reviews.apache.org/r/67131/diff/1-2/?file=2023041#file2023041line103>
> >
> > 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.
i feel "constructAclEntityForUser" is self explanatory.
> On May 16, 2018, 4:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 275-279 (patched)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line278>
> >
> > will aclEntry be null? The static methods will never return null so we should be safe here.
i left them as they are safety checks. I will remove it.
> On May 16, 2018, 4:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 288 (patched)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line291>
> >
> > Can we have a comment here why FsAction.NONE is none for users and no for groups?
there is a bug here. I will fix in the new patch.
> On May 16, 2018, 4:46 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java
> > Lines 290 (patched)
> > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line293>
> >
> > aclEntry will never be null now that we use the static constructor, so we should be safe here.
i left them as they are safety checks. I will remove it.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67131/#review203240
-----------------------------------------------------------
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
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/67131/#comment285332>
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)
<https://reviews.apache.org/r/67131/#comment285334>
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)
<https://reviews.apache.org/r/67131/#comment285335>
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)
<https://reviews.apache.org/r/67131/#comment285337>
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)
<https://reviews.apache.org/r/67131/#comment285336>
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
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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