Re: Review Request 69076: SENTRY-2301: Log where sentry stands in the snapshot fetching process, periodically

[Arjun Mishra via Review Board]

> On Jan. 24, 2019, 11:52 p.m., kalyan kumar kalvagadda wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
> > Lines 107 (patched)
> > 
> >
> > Can you make configurarble? It might be helpfull.
> >  What do you say?

Sure


- Arjun


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


On Jan. 24, 2019, 6:24 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69076/
> ---
> 
> (Updated Jan. 24, 2019, 6:24 p.m.)
> 
> 
> Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and 
> Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> When sentry is fetching snapshot from HMS, it should log periodically on 
> where it stands in the snapshot process. This will help person debugging it 
> and help him understand the progress.
> 
>  
> 
> This is important as this process could take magnitude of minutes when the 
> HMS data is huge.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java
>  534bb51c1 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java
>  4ff3dc91a 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java
>  4baeb6725 
> 
> 
> Diff: https://reviews.apache.org/r/69076/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 10:14 p.m.)


Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.


Bugs: sentry-2483
https://issues.apache.org/jira/browse/sentry-2483


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/AbstractMetastoreTestWithStaticConfiguration.java
 f14cbb6 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/6/

Changes: https://reviews.apache.org/r/69702/diff/5-6/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69702: SENTRY-2483: Implement HMS PreReadEvent support in MetastoreAuthzBinding

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 9:03 p.m.)


Review request for sentry, Arjun Mishra and kalyan kumar kalvagadda.


Bugs: sentry-2483
https://issues.apache.org/jira/browse/sentry-2483


Repository: sentry


Description
---

Add READ_DATABASE and READ_TABLE events support to provide read authorization 
to HMS.

This is based on changed made by Sergio at https://reviews.apache.org/r/69620/, 
and add code to fix unstable e2e tests


Diffs (updated)
-

  
sentry-binding/sentry-binding-hive-conf/src/main/java/org/apache/sentry/binding/hive/conf/HiveAuthzConf.java
 9efd612 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java
 328d2b5 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  sentry-tests/sentry-tests-hive/pom.xml 74777bb 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationTogglingConf.java
 e504a8a 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java
 8bf486e 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java
 7d41348 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestAuthorizingObjectStore.java
 3c28fd0 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreEndToEnd.java
 f8f304f 


Diff: https://reviews.apache.org/r/69702/diff/5/

Changes: https://reviews.apache.org/r/69702/diff/4-5/


Testing
---

add new e2e tests for READ_DATABASE and READ_TABLE at HMS


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On Jan. 28, 2019, 6:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 28, 2019, 6:55 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[kalyan kumar kalvagadda via Review Board]

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



Looks good. Wiating for the tests to pass to give a +2.

- kalyan kumar kalvagadda


On Jan. 28, 2019, 6:55 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 28, 2019, 6:55 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/2/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[kalyan kumar kalvagadda via Review Board]

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


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 6:55 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


Bugs: sentry-2486
https://issues.apache.org/jira/browse/sentry-2486


Repository: sentry


Description
---

In insecure mode, the current login user name is passed from Sentry to HMS 
server when sentry HMSFollower gets full snapshot from HMS. 

The user name should be "sentry" instead of current login user.

This issue should not happen in production because secure mode is always used. 
Insecure mode is only used in test.


Diffs (updated)
-

  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
 31e58fd 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
 0d62941 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
 47f7466 


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

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


Testing
---

Tested manually and verified the user name now is "sentry" when sentry 
HMSFollower gets notifications from HMS server


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.
> 
> Na Li wrote:
> HiveSimpleConnectionFactory is used by HMSFollower to get notifications 
> from HMS server. It is not used for any other purposes in Sentry.
> 
> If we following your suggestion, the user will be the login user, it 
> could be "root" for one run, and "jenkins" for another run. How to make sure 
> fetching notification from sentry works in your suggested approach?
> 
> That is why I have this solution here. Make sure the user is "sentry" in 
> insecured mode, and add "sentry" as services in HMS server.
> 
> kalyan kumar kalvagadda wrote:
> Lina, Idea is to use the UserGroupInformation.getCurrentUser(). Please 
> look at the patch i sugessted. All you have to do is perform doAs() while 
> starting the service. I have sent you details offline.
> 
> What you are suggesting will effect the users who are using sentry in non 
> secure mode. Approach that i'm usggesting will address the issues with the 
> tests and not change the behavior.
> 
> Na Li wrote:
> Kalyan, your suggestion is the current code behavior without my code 
> change.
> 
> 1) Do you agree that when sentry HMS follower gets notification, the user 
> name should be "sentry" instead of your name, or my name?
> 2) If you agree above, then your suggestion of using 
> "UserGroupInformation.getCurrentUser()" won't work because it returns current 
> login name, which is your name when you run the test, and my name if I run 
> the test, or Jenkins name name if it runs on build machine. 
> 3) When we have read authorization, HMS needs to check if the user has 
> read access to the metadata or if user is service users. 
> 3.1) If your approach is used, how do we write a test for read 
> authorization? We don't know what user name to configure as service user, or 
> give read access.
> 3.2) If my approach is used, we can add "sentry" as service user in test 
> to pass read authorization, and sentry can get notifications

Thanks! I have updated according to your suggestion: change caller of the 
HiveSimpleConnectionFactory


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> 

Review Request 69850: SENTRY-2490: When building a full perm update for each object we only build 1 privilege per role

[Arjun Mishra via Review Board]

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

Review request for sentry and kalyan kumar kalvagadda.


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


Repository: sentry


Description
---

When building a perm full update we only include one privilege a role has on an 
object as opposed to the entire privilege set


Diffs
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryValidator.java
 9de47b338 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 63d8d1c09 


Diff: https://reviews.apache.org/r/69850/diff/1/


Testing
---

mvn -f sentry-service/sentry-service-server/pom.xml test -Dtest=TestSentryStore


Thanks,

Arjun Mishra

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

> On Jan. 25, 2019, 9:01 p.m., kalyan kumar kalvagadda wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
> > Lines 150 (patched)
> > 
> >
> > You are adding 250 msec delay. Is it good enough?
> > 
> > There are few tests that have sentryServers.size()> 1,  so we should be 
> > good even with higher delay.
> > 
> > 
> > Based on your tests if you think that this delay is good enough, i'm 
> > fine.
> 
> Na Li wrote:
> Higher delay is not necessarily good to avoid deadlock. The table 
> creation happens only when HMS followers gets notification and create table, 
> which happens after all sentry services started. So starting services with 
> delay itself does not fix the issue. What really fixes the issue is the space 
> out when HMS follower threads run. Those threads run periodically. The 
> biggest space between two threads is half of the cycle. The biggest space for 
> N threads is cycle/N.
> 
> I can make the code change to handle thread number > 2.

I have changed the code, so the first instance of sentry server does not wait 
(this avoid unnecessary wait in test). Only the following instances will wait 
for certain time to avoid creating table at the same time.


- Na


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


On Jan. 28, 2019, 4:19 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69840/
> ---
> 
> (Updated Jan. 28, 2019, 4:19 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2491
> https://issues.apache.org/jira/browse/sentry-2491
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In sentry unit tests, we don't create schema before running a test. Instead, 
> we use dataNucleus to create sentry tables when they are accessed. This 
> creates potential deadlock when running test for Sentry HA setup.
> 
> The solution is to let the instances of sentry service start with delay. 
> Specifically,
> let HMS follower threads separate as far as possible, i.e., half of the 
> interval.
> 
> This deadlock only exists in unit tests, and does not exist in production 
> because schema is created before starting Sentry services. Therefore, there 
> is no table creation after service starts.
> 
> 
> Diffs
> -
> 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
>  9fa42f2 
> 
> 
> Diff: https://reviews.apache.org/r/69840/diff/3/
> 
> 
> Testing
> ---
> 
> such deadlock does not happen with this fix.
> Other unit tests passed
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69840: SENTRY-2491: Sentry High availability unit tests run into deadlock sometimes

[Na Li via Review Board]

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

(Updated Jan. 28, 2019, 4:19 p.m.)


Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
kalvagadda.


Bugs: sentry-2491
https://issues.apache.org/jira/browse/sentry-2491


Repository: sentry


Description
---

In sentry unit tests, we don't create schema before running a test. Instead, we 
use dataNucleus to create sentry tables when they are accessed. This creates 
potential deadlock when running test for Sentry HA setup.

The solution is to let the instances of sentry service start with delay. 
Specifically,
let HMS follower threads separate as far as possible, i.e., half of the 
interval.

This deadlock only exists in unit tests, and does not exist in production 
because schema is created before starting Sentry services. Therefore, there is 
no table creation after service starts.


Diffs (updated)
-

  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java
 9fa42f2 


Diff: https://reviews.apache.org/r/69840/diff/3/

Changes: https://reviews.apache.org/r/69840/diff/2-3/


Testing
---

such deadlock does not happen with this fix.
Other unit tests passed


Thanks,

Na Li

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.
> 
> Na Li wrote:
> HiveSimpleConnectionFactory is used by HMSFollower to get notifications 
> from HMS server. It is not used for any other purposes in Sentry.
> 
> If we following your suggestion, the user will be the login user, it 
> could be "root" for one run, and "jenkins" for another run. How to make sure 
> fetching notification from sentry works in your suggested approach?
> 
> That is why I have this solution here. Make sure the user is "sentry" in 
> insecured mode, and add "sentry" as services in HMS server.
> 
> kalyan kumar kalvagadda wrote:
> Lina, Idea is to use the UserGroupInformation.getCurrentUser(). Please 
> look at the patch i sugessted. All you have to do is perform doAs() while 
> starting the service. I have sent you details offline.
> 
> What you are suggesting will effect the users who are using sentry in non 
> secure mode. Approach that i'm usggesting will address the issues with the 
> tests and not change the behavior.

Kalyan, your suggestion is the current code behavior without my code change.

1) Do you agree that when sentry HMS follower gets notification, the user name 
should be "sentry" instead of your name, or my name?
2) If you agree above, then your suggestion of using 
"UserGroupInformation.getCurrentUser()" won't work because it returns current 
login name, which is your name when you run the test, and my name if I run the 
test, or Jenkins name name if it runs on build machine. 
3) When we have read authorization, HMS needs to check if the user has read 
access to the metadata or if user is service users. 
3.1) If your approach is used, how do we write a test for read authorization? 
We don't know what user name to configure as service user, or give read access.
3.2) If my approach is used, we can add "sentry" as service user in test to 
pass read authorization, and sentry can get notifications


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[kalyan kumar kalvagadda via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.
> 
> Na Li wrote:
> HiveSimpleConnectionFactory is used by HMSFollower to get notifications 
> from HMS server. It is not used for any other purposes in Sentry.
> 
> If we following your suggestion, the user will be the login user, it 
> could be "root" for one run, and "jenkins" for another run. How to make sure 
> fetching notification from sentry works in your suggested approach?
> 
> That is why I have this solution here. Make sure the user is "sentry" in 
> insecured mode, and add "sentry" as services in HMS server.

Lina, Idea is to use the UserGroupInformation.getCurrentUser(). Please look at 
the patch i sugessted. All you have to do is perform doAs() while starting the 
service. I have sent you details offline.

What you are suggesting will effect the users who are using sentry in non 
secure mode. Approach that i'm usggesting will address the issues with the 
tests and not change the behavior.


- kalyan kumar


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 69841: SENTRY-2486: Wrong user name when sentry HMSFollower gets full snapshot from HMS at insecure mode

[Na Li via Review Board]

> On Jan. 28, 2019, 1:55 a.m., kalyan kumar kalvagadda wrote:
> > Idea here is to make sure that hive client knows the details of the user 
> > who is sending the request. In this specific case, hive should know the 
> > details of the user who running sentry service. Using 
> > sentry.service.server.principal and entry.service.realm doesn’t seem 
> > correct.
> > 
> > 
> > I have a thought.
> > ```
> > public HMSClient connect() throws IOException, InterruptedException, 
> > MetaException 
> > {?
> >   UserGroupInformation clientUGI = null;
> >   if (insecure) {?   
> >   clientUGI = UserGroupInformation.getCurrentUser();?
> >   } else {?  
> >   clientUGI = 
> > UserGroupInformation.getUGIFromSubject(kerberosContext.getSubject());?
> >   }?  
> >   return new HMSClient(clientUGI.doAs(new 
> > PrivilegedExceptionAction()
> >   {?  
> >  @Override?  
> >  public HiveMetaStoreClient run() throws MetaException {? 
> >return new HiveMetaStoreClient(hiveConf);?   
> >}? 
> >   }));
> > }
> > 
> > ```
> > All you have additionally do is change the tests to run sentry server as 
> > user “sentry”. 
> > 
> > Here is the sample code. I have tested it locally.

HiveSimpleConnectionFactory is used by HMSFollower to get notifications from 
HMS server. It is not used for any other purposes in Sentry.

If we following your suggestion, the user will be the login user, it could be 
"root" for one run, and "jenkins" for another run. How to make sure fetching 
notification from sentry works in your suggested approach?

That is why I have this solution here. Make sure the user is "sentry" in 
insecured mode, and add "sentry" as services in HMS server.


- Na


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


On Jan. 25, 2019, 9:07 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69841/
> ---
> 
> (Updated Jan. 25, 2019, 9:07 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Haley Reeve, and kalyan kumar 
> kalvagadda.
> 
> 
> Bugs: sentry-2486
> https://issues.apache.org/jira/browse/sentry-2486
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> In insecure mode, the current login user name is passed from Sentry to HMS 
> server when sentry HMSFollower gets full snapshot from HMS. 
> 
> The user name should be "sentry" instead of current login user.
> 
> This issue should not happen in production because secure mode is always 
> used. Insecure mode is only used in test.
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveSimpleConnectionFactory.java
>  31e58fd 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollower.java
>  0d62941 
>   
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java
>  47f7466 
> 
> 
> Diff: https://reviews.apache.org/r/69841/diff/1/
> 
> 
> Testing
> ---
> 
> Tested manually and verified the user name now is "sentry" when sentry 
> HMSFollower gets notifications from HMS server
> 
> 
> Thanks,
> 
> Na Li
> 
>