You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sergio Pena via Review Board <no...@reviews.apache.org> on 2018/06/19 22:38:43 UTC
Re: Review Request 67649: SENTRY-2274: Grant and revoke owner
privileges based on HMS updates(server-side)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67649/#review205033
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1386 (patched)
<https://reviews.apache.org/r/67649/#comment287913>
This line has an unecessary extra ()
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1402-1415 (patched)
<https://reviews.apache.org/r/67649/#comment287912>
We need to catch the same exceptions that sentry_sync_notifications() throws.
Btw, if we catch a sync exception, then are we going to ignore the ownership grant?
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 1453 (patched)
<https://reviews.apache.org/r/67649/#comment287914>
'hive' user should not be hard-coded. Find a way to obtain the user that the Sentry server is using instead.
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
Lines 70 (patched)
<https://reviews.apache.org/r/67649/#comment287911>
Is this a compatible change? I see that implementations of this interface can be configured in the sentry-site.xml. Should we keep the method compatibility?
Same question for the next method.
- Sergio Pena
On June 19, 2018, 3:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67649/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:50 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2241
> https://issues.apache.org/jira/browse/SENTRY-2241
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases. Based on these notifications owner privileges are granted/revoked.
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67649/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67649: SENTRY-2274: Grant and revoke owner
privileges based on HMS updates(server-side)
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 19, 2018, 10:38 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1453 (patched)
> > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1453>
> >
> > 'hive' user should not be hard-coded. Find a way to obtain the user that the Sentry server is using instead.
>
> kalyan kumar kalvagadda wrote:
> Sentry is a service so there will neither be a user OR group for sentry.
>
> Actually there is no validation needed for the grantor here as permission is granted by the service but not by the user it self.
>
>
> The option i picked is hard coding the user to hive service user. You are right there is no guarentee that Hive is going to be the service user for hive service. Here is what i think the options are
> 1. Ignore checking the permissions of the grantor for owner privilege.
> 2. User sentry service principle as grantor and add code to authorize sentry service principle to grant privileges.
> 3. Other option is to add new API that does not consider granter and use it only for owner privileges.
>
> I prefer option-3 as there is no real validation that we need to make as the the API is invoked by sentry service.
picked option-3
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67649/#review205033
-----------------------------------------------------------
On June 21, 2018, 5:42 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67649/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 5:42 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2241
> https://issues.apache.org/jira/browse/SENTRY-2241
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases. Based on these notifications owner privileges are granted/revoked.
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67649/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 67649: SENTRY-2274: Grant and revoke owner
privileges based on HMS updates(server-side)
Posted by kalyan kumar kalvagadda via Review Board <no...@reviews.apache.org>.
> On June 19, 2018, 10:38 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1386 (patched)
> > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1386>
> >
> > This line has an unecessary extra ()
will fix
> On June 19, 2018, 10:38 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1402-1415 (patched)
> > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1402>
> >
> > We need to catch the same exceptions that sentry_sync_notifications() throws.
> >
> > Btw, if we catch a sync exception, then are we going to ignore the ownership grant?
will address it
> On June 19, 2018, 10:38 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
> > Lines 1453 (patched)
> > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1453>
> >
> > 'hive' user should not be hard-coded. Find a way to obtain the user that the Sentry server is using instead.
Sentry is a service so there will neither be a user OR group for sentry.
Actually there is no validation needed for the grantor here as permission is granted by the service but not by the user it self.
The option i picked is hard coding the user to hive service user. You are right there is no guarentee that Hive is going to be the service user for hive service. Here is what i think the options are
1. Ignore checking the permissions of the grantor for owner privilege.
2. User sentry service principle as grantor and add code to authorize sentry service principle to grant privileges.
3. Other option is to add new API that does not consider granter and use it only for owner privileges.
I prefer option-3 as there is no real validation that we need to make as the the API is invoked by sentry service.
> On June 19, 2018, 10:38 p.m., Sergio Pena wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/67649/diff/3/?file=2042472#file2042472line72>
> >
> > Is this a compatible change? I see that implementations of this interface can be configured in the sentry-site.xml. Should we keep the method compatibility?
> >
> > Same question for the next method.
Yes, you are right. implementation of this interface can be provided as a configuration.
It is not implemented by sentry component only. If this interface is implemented by other components then it would not be a compatible change.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67649/#review205033
-----------------------------------------------------------
On June 19, 2018, 3:50 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67649/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:50 p.m.)
>
>
> Review request for sentry, Na Li and Sergio Pena.
>
>
> Bugs: SENTRY-2241
> https://issues.apache.org/jira/browse/SENTRY-2241
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post listener in HMS. This listener should be extended to get the owner information of tables and databases. Based on these notifications owner privileges are granted/revoked.
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java PRE-CREATION
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b5e01e4c21473523b494cc624318b73ec5732408
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java 5424bff67bc31d3f4ed2300531706e062f82b9ae
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java 7f97ff7f054f797757aac94e243acdc5ee1127a2
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java d8c82970b56b1599a07f0e26edab8ed3d59b9948
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f0e373a9aa5342d1b507e8b192cfdbc444242227
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java 6bfe872320d255acdb10f69e09c19623c04d8951
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java c056446262ddcf61db307eafbb785eac42973c80
>
>
> Diff: https://reviews.apache.org/r/67649/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>