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/06/08 12:26:01 UTC
Review Request 67506: SENTRY-2260: Update HDFS ACL's based on owner
privileges.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67506/
-----------------------------------------------------------
Review request for sentry, Na Li and Sergio Pena.
Bugs: SENTRY-2260
https://issues.apache.org/jira/browse/SENTRY-2260
Repository: sentry
Description
-------
When owner privileges are implicitly granted/revoked, sentry ACL's in Namenode plug-in should be updated accordingly.
Diffs
-----
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 71ef5f9251f182825177c00a8456ca3166b2d095
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 6974d37aac9cdcbc21edfe66b1794dd1df6597fb
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 10d52b43729426c53c0168fc0b7d0cdf0e307b57
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java 60696cccc0d679dfa6b3e6c06cb5120942facc14
sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java d2d5391182d0da395562ad23f742d704d9a0ceb5
Diff: https://reviews.apache.org/r/67506/diff/1/
Testing
-------
Added new unit tests to verify the functionality.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 67506: SENTRY-2260: Update HDFS ACL's based on
owner privileges.
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/67506/#review204567
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On June 8, 2018, 12:26 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67506/
> -----------------------------------------------------------
>
> (Updated June 8, 2018, 12:26 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2260
> https://issues.apache.org/jira/browse/SENTRY-2260
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When owner privileges are implicitly granted/revoked, sentry ACL's in Namenode plug-in should be updated accordingly.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 71ef5f9251f182825177c00a8456ca3166b2d095
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 6974d37aac9cdcbc21edfe66b1794dd1df6597fb
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 10d52b43729426c53c0168fc0b7d0cdf0e307b57
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java 60696cccc0d679dfa6b3e6c06cb5120942facc14
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java d2d5391182d0da395562ad23f742d704d9a0ceb5
>
>
> Diff: https://reviews.apache.org/r/67506/diff/1/
>
>
> Testing
> -------
>
> Added new unit tests to verify the functionality.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67506: SENTRY-2260: Update HDFS ACL's based on
owner privileges.
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On June 11, 2018, 2:57 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
> > Lines 70-74 (patched)
> > <https://reviews.apache.org/r/67506/diff/1/?file=2036472#file2036472line70>
> >
> > I don't feel necessary to do this loop to translate OWNER privileges prior to sending them to HDFS. HDFS can know what to do with an OWNER privilege the same it does with the ALL privilege in the UpdateableAuthzPermissions used in the binding side (that's one single line of code).
> >
> > I thought about what you said what if in the future we add a configuration for what OWNER will mean. If we do that, which I don't think will happen because we're behaving like other DBs where OWNER = ALL, but if we do, then this change in HDFS will not be affected because but that time Sentry will not send the OWNER anymore, but the new privilege used in the configuration.
> >
> > I prefer to translate the OWNER privilege in the NN side instead of adding another loop that walks through all the privileges send to the NN. This is adding an extra overhead on the updates for the NN.
>
> kalyan kumar kalvagadda wrote:
> I had to add a loop just to be sure. Ideally there will be only one permission change in permsUpdate. It is be one entry either in add privilege/delete privileges. Except for rename, there will be one entry in each.
> There is no overhead because of this logic, when a snapshot is taken the translation in in-line with the snapshot creation. For delta updates, each entry when it is extracted from database and de-serialized, translation is done.
Since Name Node does not need owner privilege in any way, there is no benefit to expose it to sentry client at Name Node. We can just translate it at sentry server. The performance impact should be small.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67506/#review204543
-----------------------------------------------------------
On June 8, 2018, 12:26 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67506/
> -----------------------------------------------------------
>
> (Updated June 8, 2018, 12:26 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2260
> https://issues.apache.org/jira/browse/SENTRY-2260
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When owner privileges are implicitly granted/revoked, sentry ACL's in Namenode plug-in should be updated accordingly.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 71ef5f9251f182825177c00a8456ca3166b2d095
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 6974d37aac9cdcbc21edfe66b1794dd1df6597fb
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 10d52b43729426c53c0168fc0b7d0cdf0e307b57
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java 60696cccc0d679dfa6b3e6c06cb5120942facc14
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java d2d5391182d0da395562ad23f742d704d9a0ceb5
>
>
> Diff: https://reviews.apache.org/r/67506/diff/1/
>
>
> Testing
> -------
>
> Added new unit tests to verify the functionality.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67506: SENTRY-2260: Update HDFS ACL's based on
owner privileges.
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 11, 2018, 2:57 p.m., Sergio Pena wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
> > Lines 70-74 (patched)
> > <https://reviews.apache.org/r/67506/diff/1/?file=2036472#file2036472line70>
> >
> > I don't feel necessary to do this loop to translate OWNER privileges prior to sending them to HDFS. HDFS can know what to do with an OWNER privilege the same it does with the ALL privilege in the UpdateableAuthzPermissions used in the binding side (that's one single line of code).
> >
> > I thought about what you said what if in the future we add a configuration for what OWNER will mean. If we do that, which I don't think will happen because we're behaving like other DBs where OWNER = ALL, but if we do, then this change in HDFS will not be affected because but that time Sentry will not send the OWNER anymore, but the new privilege used in the configuration.
> >
> > I prefer to translate the OWNER privilege in the NN side instead of adding another loop that walks through all the privileges send to the NN. This is adding an extra overhead on the updates for the NN.
I had to add a loop just to be sure. Ideally there will be only one permission change in permsUpdate. It is be one entry either in add privilege/delete privileges. Except for rename, there will be one entry in each.
There is no overhead because of this logic, when a snapshot is taken the translation in in-line with the snapshot creation. For delta updates, each entry when it is extracted from database and de-serialized, translation is done.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67506/#review204543
-----------------------------------------------------------
On June 8, 2018, 12:26 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67506/
> -----------------------------------------------------------
>
> (Updated June 8, 2018, 12:26 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2260
> https://issues.apache.org/jira/browse/SENTRY-2260
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When owner privileges are implicitly granted/revoked, sentry ACL's in Namenode plug-in should be updated accordingly.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 71ef5f9251f182825177c00a8456ca3166b2d095
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 6974d37aac9cdcbc21edfe66b1794dd1df6597fb
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 10d52b43729426c53c0168fc0b7d0cdf0e307b57
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java 60696cccc0d679dfa6b3e6c06cb5120942facc14
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java d2d5391182d0da395562ad23f742d704d9a0ceb5
>
>
> Diff: https://reviews.apache.org/r/67506/diff/1/
>
>
> Testing
> -------
>
> Added new unit tests to verify the functionality.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67506: SENTRY-2260: Update HDFS ACL's based on
owner privileges.
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/67506/#review204543
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java
Lines 70-74 (patched)
<https://reviews.apache.org/r/67506/#comment287098>
I don't feel necessary to do this loop to translate OWNER privileges prior to sending them to HDFS. HDFS can know what to do with an OWNER privilege the same it does with the ALL privilege in the UpdateableAuthzPermissions used in the binding side (that's one single line of code).
I thought about what you said what if in the future we add a configuration for what OWNER will mean. If we do that, which I don't think will happen because we're behaving like other DBs where OWNER = ALL, but if we do, then this change in HDFS will not be affected because but that time Sentry will not send the OWNER anymore, but the new privilege used in the configuration.
I prefer to translate the OWNER privilege in the NN side instead of adding another loop that walks through all the privileges send to the NN. This is adding an extra overhead on the updates for the NN.
- Sergio Pena
On June 8, 2018, 12:26 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67506/
> -----------------------------------------------------------
>
> (Updated June 8, 2018, 12:26 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2260
> https://issues.apache.org/jira/browse/SENTRY-2260
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When owner privileges are implicitly granted/revoked, sentry ACL's in Namenode plug-in should be updated accordingly.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java 71ef5f9251f182825177c00a8456ca3166b2d095
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java 6974d37aac9cdcbc21edfe66b1794dd1df6597fb
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java 10d52b43729426c53c0168fc0b7d0cdf0e307b57
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java 60696cccc0d679dfa6b3e6c06cb5120942facc14
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java d2d5391182d0da395562ad23f742d704d9a0ceb5
>
>
> Diff: https://reviews.apache.org/r/67506/diff/1/
>
>
> Testing
> -------
>
> Added new unit tests to verify the functionality.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>