You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/03/30 19:38:44 UTC
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Hello Andrew Wong, Anonymous Coward (314), Adar Dembo, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15601
to review the following change.
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements, some of which may not
actually needed to make it work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to
find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs and also to use a unique loopback
address for Ranger and Postgres. Unfortunately Ranger doesn't seem to
support setting the bind address, and it always binds to all interfaces,
we still need to use randomized ports as well.
This patch also changes the default "java" binary in Ranger client to be
the on in $JAVA_HOME/bin instead of the one in $PATH. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
During debugging the Ranger subprocess crashed which brought down the
master too in debug mode (DCHECK) which this patch also fixes.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 338 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/1
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 1:
(21 comments)
the rebase on master somehow introduced some flakiness even which I tried to work around by lowering the policy refresh period and sleeping after adding the policy, but it still didn't seem to solve the issue.
Interestingly enough, all tests passed before the rebase (http://dist-test.cloudera.org/job?job_id=abukor.1585593480.128299), I'll investigate this tomorrow.
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@13
PS1, Line 13: As adding Kerberos support proved to be trickier than expected this
: patch contains several other improvements, some of which may not
: actually needed to make it work.
> It'd be great if you could separate out the unnecessary ones out from this
Actually the only thing that may not be needed is the unique loopback trick (and I'm not 100% sure it's not needed), as the java path is needed for dist-test and the DCHECK is not part of this patch after the rebase as you pointed out. Do you think it's still worth separating them?
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@21
PS1, Line 21: credentails
> Nice.
Done
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@33
PS1, Line 33:
: During debugging the Ranger subprocess crashed which brought down the
: master too in debug mode (DCHECK) which this patch also fixes.
> This was merged in another patch, it seems.
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@391
PS1, Line 391: opts.extra_master_flags.emplace_back("--trusted_user_acl=impala");
: opts.extra_master_flags.emplace_back("--user_acl=test-user,impala");
> nit: should these use kImpalaUser and kTestUser respectively?
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@402
PS1, Line 402:
: ASSERT_OK(cluster_->kdc()->Kinit(kTestUser));
: ASSERT_OK(cluster_->CreateClient(nullptr, &client_));
> Not used?
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@410
PS1, Line 410: const char* const SentryITestBase::kAdminGroup = "admin";
: const char* const SentryITestBase::kAdminUser = "test-admin";
: const char* const SentryITestBase::kUserGroup = "user";
: const char* const SentryITestBase::kTestUser = "test-user";
: const char* const SentryITestBase::kImpalaUser = "impala";
: const char* const SentryITestBase::kDevRole = "developer";
: const char* const SentryITestBase::kAdminRole = "ad";
: const char* const SentryITestBase::kDatabaseName = "db";
: const char* const SentryITestBase::kTableName = "table";
: const char* const SentryITestBase::kSecondTable = "second_table";
> Why not just stick these in an anonymous namespace, rather than being stati
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@473
PS1, Line 473: TEST_F(MasterRangerTest, TestIntegration) {
> Would be good to also kinit as kTestUser and ensure that we get a rejection
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@484
PS1, Line 484: k
> nit: remove space
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@342
PS1, Line 342: set_secure
> Should this also be kerberos specific? E.g. we still haven't enabled TLS su
I was thinking we could use this with SSL as well eventually. Do you think it would be better to use separate methods?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@345
PS1, Line 345: rangeradmin
> nit: Are this and 'rangerlookup' hardcoded into Ranger? Maybe add a comment
They're not hardcoded, they're set in one of the config files, I just didn't think it's worth making these configurable. Should I add a comment about it or make it configurable?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@346
PS1, Line 346: ktpath
> nit: it feels kind of weird to be re-using a moved variable. Maybe just dec
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@346
PS1, Line 346: spn
> nit: just inline the substitute? Same below.
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/postgres/mini_postgres.cc@73
PS1, Line 73: !port_
> nit: I generally prefer only using this style of condition checking for boo
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h@94
PS1, Line 94: void set_admin_ktpath(std::string admin_ktpath) {
> The 'move' proliferation is getting a bit cumbersome IMO. I don't think we'
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h@106
PS1, Line 106: void set_host(std::string host) {
: host_ = host;
> nit: could you add a comment indicating why we'll need this? Same in MiniPo
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@98
PS1, Line 98: port_
> nit: same here
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@194
PS1, Line 194: FQDN
> nit: no longer FQDN
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@210
PS1, Line 210: if (!krb5_config.empty()) {
> nit: When might this be empty? Maybe add a comment. Or should this be a DCH
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@248
PS1, Line 248: EasyJson configs = service.Set("configs", EasyJson::kObject);
: configs.Set("policy.download.auth.users", "kudu");
: configs.Set("tag.download.auth.users", "kudu");
> nit: could you explain these so your typical Kudu developer can understand
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc@50
PS1, Line 50: $JAVA_HOME/java
> This should be $JAVA_HOME/bin/java, right?
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc@179
PS1, Line 179:
: ret
> nit: remove the extra line
Done
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 30 Mar 2020 22:42:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 14:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@375
PS3, Line 375: void SetUp() override {
> I think using NullGroupsMapping is good for now, but eventually we would wa
Hm we would either need MiniLdap for that or manipulate real groups/users on the host. I think there's also a test class where we can set up group mappings manually, but then we would need to expose methods to manipulate that in the Ranger subprocess. Anyway, we don't need to decide that now.
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/mini-cluster/external_mini_cluster.cc.orig
File src/kudu/mini-cluster/external_mini_cluster.cc.orig:
PS13:
> Accidentally added?
Done
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/postgres/mini_postgres.cc.orig
File src/kudu/postgres/mini_postgres.cc.orig:
PS13:
> Same here?
Done
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/ranger/mini_ranger.cc.orig
File src/kudu/ranger/mini_ranger.cc.orig:
PS13:
> Same here?
Done
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/ranger/mini_ranger.h.orig
File src/kudu/ranger/mini_ranger.h.orig:
PS13:
> Also this one?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 17:34:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#12).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
A src/kudu/mini-cluster/external_mini_cluster.cc.orig
M src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.cc.orig
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
A src/kudu/ranger/mini_ranger.cc.orig
M src/kudu/ranger/mini_ranger.h
A src/kudu/ranger/mini_ranger.h.orig
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
16 files changed, 2,643 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/12
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#3).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements, some of which may not
actually needed to make it work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs and also to use a unique loopback
address for Ranger and Postgres. Unfortunately Ranger doesn't seem to
support setting the bind address, and it always binds to all interfaces,
we still need to use randomized ports as well.
This patch also changes the default "java" binary in Ranger client to be
the on in $JAVA_HOME/bin instead of the one in $PATH. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 433 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/3
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@196
PS10, Line 196: ret.emplace_back(Substitute("-Djava.security.krb5.conf=$0", krb5_config));
> Nit: given we don't have any getters for this, maybe just call GetKrb5Confi
+1
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:09:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 10: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger_configs.h
File src/kudu/ranger/mini_ranger_configs.h:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger_configs.h@84
PS10, Line 84: // $4: admin keytab
Missing 5
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@196
PS10, Line 196: ret.emplace_back(Substitute("-Djava.security.krb5.conf=$0", krb5_config));
Nit: given we don't have any getters for this, maybe just call GetKrb5Config directly? Then we don't need the member or the argument here
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/security/init.cc
File src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/security/init.cc@513
PS10, Line 513: config_file = "/etc/krb5.conf";
Nit: can just return directly
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:31:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 11:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger.h@177
PS10, Line 177: // Determines how frequently clients fet
> nit: add a comment to explain what is policy_poll_interval_ms for? And why
Done
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger_configs.h
File src/kudu/ranger/mini_ranger_configs.h:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger_configs.h@84
PS10, Line 84: // $4: admin keytab
> Missing 5
Done
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@49
PS10, Line 49: (
> Can you add an example such as 'e.g. java'?
Done
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@51
PS10, Line 51: d. If
> Can you comment on if $JAVA_HOME is not found?
Done
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@196
PS10, Line 196:
> +1
Done
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/security/init.cc
File src/kudu/security/init.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/security/init.cc@513
PS10, Line 513: return "/etc/krb5.conf";
> Nit: can just return directly
Done
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 09:23:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#6).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 414 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/6
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#4).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 415 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/4
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#2).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements, some of which may not
actually needed to make it work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs and also to use a unique loopback
address for Ranger and Postgres. Unfortunately Ranger doesn't seem to
support setting the bind address, and it always binds to all interfaces,
we still need to use randomized ports as well.
This patch also changes the default "java" binary in Ranger client to be
the on in $JAVA_HOME/bin instead of the one in $PATH. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 433 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/2
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@13
PS1, Line 13: As adding Kerberos support proved to be trickier than expected this
: patch contains several other improvements needed for this to work.
:
> It'd then be great if we could figure out if we actually need the unique lo
Done
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@375
PS3, Line 375: void SetUp() override {
> It looks like the test is failing because it can't determine the user group
I believe that's a red herring, the same WARN appeared when the test passed as well. Also I don't think it's possible to do the same as with Ranger, but I can swtich to NullGroupsMapping which will silence the warning and each user appear to belong to 0 groups with no failures in name resolution.
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@437
PS3, Line 437: TEST_F(MasterRangerTest, TestCreateTableAuthorized) {
> Given the eventual goal is to parameterize the entirety of this test, we sh
hm I'm not sure it's worth rewriting the tests in ranger_client-test. As you said this file will be rewritten, so these tests might actually go away, just wanted to add some quick tests to verify the integration works properly.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@342
PS1, Line 342:
> If we ever expect to have tests that will use Kerberos and not SSL (or vice
Done
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@345
PS1, Line 345: _->CreateSe
> I'm fine with this with just a comment. Just want to make sure future reade
Done
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h@66
PS3, Line 66: CHECK_NE(0, port_);
> nit: const ref here too?
Done
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@55
PS3, Line 55: }
> In a separate patch, could we maybe loop 'psql' in MiniPostgres::Start() un
Ack
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@251
PS3, Line 251:
> nit: "download the list"
Done
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h
File src/kudu/ranger/mini_ranger_configs.h:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@378
PS3, Line 378: const char* kCoreSiteTemplate = R"(
> Unused?
Done
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@427
PS3, Line 427: ang
> How about making this configurable?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 19:00:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#9).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
15 files changed, 445 insertions(+), 145 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/9
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#10).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/master/ranger_authz_provider.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
15 files changed, 441 insertions(+), 145 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/10
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 21:26:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 10: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@375
PS3, Line 375: void SetUp() override {
> I believe that's a red herring, the same WARN appeared when the test passed
I think using NullGroupsMapping is good for now, but eventually we would want to test policy granting associate with groups.
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/mini_ranger.h@177
PS10, Line 177: uint32_t policy_poll_interval_ms_ = 200;
nit: add a comment to explain what is policy_poll_interval_ms for? And why default to 200.
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@49
PS10, Line 49: ,
Can you add an example such as 'e.g. java'?
http://gerrit.cloudera.org:8080/#/c/15601/10/src/kudu/ranger/ranger_client.cc@51
PS10, Line 51: d.");
Can you comment on if $JAVA_HOME is not found?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:08:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG");
> +1 to what Andrew said, and yeah, I do feel strongly about this (provided i
This is the one that's set by MiniKdc if it's started from EMC.
https://github.com/apache/kudu/blob/737fbdd3f0e751241a2128634b41488d7cb6c790/src/kudu/security/test/mini_kdc.cc#L86
https://github.com/apache/kudu/blob/737fbdd3f0e751241a2128634b41488d7cb6c790/src/kudu/security/test/mini_kdc.cc#L356-L358
MiniRanger and Ranger plugin actually didn't work with Kerberos enabled and java.security.krb5.conf set to the value of KRB5_CONFIG in EMC which is how I realized I need to set it.
I posted my concerns about the non-env var approach on the other thread, please let me know what you think.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG");
> The problem is that it's much tougher to track and understand the behavior
My issue with this approach is that KRB5_CONFIG is actually respected in Kudu right now (just tested it on a lab cluster to make sure), so this would technically be a breaking change. If someone has set KRB5_CONFIG to something other than /etc/krb5.conf and it will suddenly stop working after they enable Ranger. I would say it would be even harder to understand why Kudu works, but Ranger integration doesn't. How about we default to KRB5_CONFIG and document it and also add a flag that can override it and the description also mentions it defaults to $KRB5_CONFIG?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 09:25:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG");
> in this case this is not as important as in RangerClient, but I still think
Isn't it important for the KRB5_CONFIG used by MiniRanger to be the one from MiniKdc? Likewise, for the RangerClient, isn't it important for our tests that we use the environment variables produced by MiniKdc?
http://gerrit.cloudera.org:8080/#/c/15601/6/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/6/src/kudu/ranger/mini_ranger.cc@107
PS6, Line 107: "127.0.0.1"
nit: no longer needs to be configurable? Same below?
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG");
> KRB5_CONFIG is a standard env var used to set the location of krb5.conf if
Alternatively, we can expose a flag that, if set, will be used (e.g. in an EMC), and if not, will evaluate to getenv(KRB5_CONFIG).
http://gerrit.cloudera.org:8080/#/c/15601/6/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/6/src/kudu/ranger/ranger_client.cc@222
PS6, Line 222: java_path()
nit: make a local var and call once?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:21:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: args.emplace_back(Substitute("-Djava.security.krb5.conf=$0", krb5_config_));
> This is the one that's set by MiniKdc if it's started from EMC.
as agreed offline, changed it to use a setter here instead
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195:
> My issue with this approach is that KRB5_CONFIG is actually respected in Ku
As agreed offline, moved the getenv to security/init.cc
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:13:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc@647
PS4, Line 647: opts.extra_flags.emplace_back("--unlock_experimental_flags");
This is already done in each Kudu process (see ExternalDaemon::StartProcess).
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc@135
PS4, Line 135: config.append(Substitute("\nlisten_addresses = '127.0.0.1'\nport = $0\n", port_));
Why was this change needed? Isn't 127.0.0.1 the default listen address? Would be nice to doc with a comment.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG");
Again, this is an opaque way to pass Kerberos configuration information between cluster daemons. I'd prefer if you did what we did for Sentry/HMS: an EnableKerberos method that takes all the (unmarshalled) information it needs as separate arguments. Then, ExternalMiniCluster can call that method directly and feed in information from the MiniKdc's environment variables.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h@117
PS4, Line 117: static std::string java_path();
Nit: static (and then non-static) private member functions should be above the data members.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG");
If this is the Kerberization switch, would rather it be an explicit setter called on RangerClient, because otherwise it's tough to trace the relationship between whomever consumes KRB5_CONFIG (RangerClient) and whomever sets it (the EMC, presumably).
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@198
PS4, Line 198: LOG(INFO) << krb5_config;
This was for debugging I presume, remove now?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:35:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@13
PS1, Line 13: As adding Kerberos support proved to be trickier than expected this
: patch contains several other improvements, some of which may not
: actually needed to make it work.
> Actually the only thing that may not be needed is the unique loopback trick
It'd then be great if we could figure out if we actually need the unique loopback trick, and include it (or not) as needed :)
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@375
PS3, Line 375: void SetUp() override {
It looks like the test is failing because it can't determine the user group mapping for our test user. Do we need some CreateRoleAndAddToGroups equivalent for Ranger?
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/integration-tests/master_sentry-itest.cc@437
PS3, Line 437: TEST_F(MasterRangerTest, TestCreateTableAuthorized) {
Given the eventual goal is to parameterize the entirety of this test, we should consider replacing the tests in ranger_client-test with MiniRanger tests, and leaving this file untouched for now, rather than adding more bloat here.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@342
PS1, Line 342: set_secure
> I was thinking we could use this with SSL as well eventually. Do you think
If we ever expect to have tests that will use Kerberos and not SSL (or vice versa), we should make them separate. At the very least, add a comment in the declaration of set_secure() that it only enables Kerberos, and leave a TODO to add TLS support.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@345
PS1, Line 345: _->CreateSe
> They're not hardcoded, they're set in one of the config files, I just didn'
I'm fine with this with just a comment. Just want to make sure future readers of this code can understand where these seemingly magic strings become less magical.
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/postgres/mini_postgres.h@66
PS3, Line 66: void set_host(std::string host) {
nit: const ref here too?
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@55
PS3, Line 55: SleepFor(MonoDelta::FromMilliseconds(500));
In a separate patch, could we maybe loop 'psql' in MiniPostgres::Start() until we succeed?
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger.cc@251
PS3, Line 251: download list
nit: "download the list"
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h
File src/kudu/ranger/mini_ranger_configs.h:
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@378
PS3, Line 378: // $1: authz enabled (true or false)
Unused?
http://gerrit.cloudera.org:8080/#/c/15601/3/src/kudu/ranger/mini_ranger_configs.h@427
PS3, Line 427: 200
How about making this configurable?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 30 Mar 2020 23:52:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#11).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
12 files changed, 429 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/11
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed Anonymous Coward (314) from this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Removed reviewer null.
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/mini-cluster/external_mini_cluster.cc@647
PS4, Line 647: }
> This is already done in each Kudu process (see ExternalDaemon::StartProcess
Done
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/postgres/mini_postgres.cc@135
PS4, Line 135: config.append(Substitute("\nport = $0\n", port_));
> Why was this change needed? Isn't 127.0.0.1 the default listen address? Wou
Done
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG");
> Again, this is an opaque way to pass Kerberos configuration information bet
in this case this is not as important as in RangerClient, but I still think it makes sense to do it this way for the sake of consistency. I can change it if you feel strongly about it though.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h
File src/kudu/ranger/ranger_client.h:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.h@117
PS4, Line 117:
> Nit: static (and then non-static) private member functions should be above
Done
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG");
> If this is the Kerberization switch, would rather it be an explicit setter
KRB5_CONFIG is a standard env var used to set the location of krb5.conf if it's not the default location. Unfortunately Java doesn't respect this, and as this can also be problematic in a real environment, not only in MiniCluster, I thought it would be best to set it based on this env var instead of manually configuring it. This way the master and the subprocess are guaranteed to use the same krb5.conf (there still can be some discrepancies in behavior though, as Java doesn't support every option, but there's nothing we can do about that).
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@198
PS4, Line 198: }
> This was for debugging I presume, remove now?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:34:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 1:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@13
PS1, Line 13: As adding Kerberos support proved to be trickier than expected this
: patch contains several other improvements, some of which may not
: actually needed to make it work.
It'd be great if you could separate out the unnecessary ones out from this patch. That way they're not blocking and aren't blocked by the necessary bits.
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@21
PS1, Line 21: credentails
Nice.
Also FWIW if quoting logs or URLs, IMO it's fine to go over the column line length since it improves readability.
http://gerrit.cloudera.org:8080/#/c/15601/1//COMMIT_MSG@33
PS1, Line 33:
: During debugging the Ranger subprocess crashed which brought down the
: master too in debug mode (DCHECK) which this patch also fixes.
This was merged in another patch, it seems.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@391
PS1, Line 391: opts.extra_master_flags.emplace_back("--trusted_user_acl=impala");
: opts.extra_master_flags.emplace_back("--user_acl=test-user,impala");
nit: should these use kImpalaUser and kTestUser respectively?
Also, neither of these seem to be used in this test; could you remove them?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@402
PS1, Line 402:
: ASSERT_OK(cluster_->kdc()->Kinit(kTestUser));
: ASSERT_OK(cluster_->CreateClient(nullptr, &client_));
Not used?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@410
PS1, Line 410: const char* const SentryITestBase::kAdminGroup = "admin";
: const char* const SentryITestBase::kAdminUser = "test-admin";
: const char* const SentryITestBase::kUserGroup = "user";
: const char* const SentryITestBase::kTestUser = "test-user";
: const char* const SentryITestBase::kImpalaUser = "impala";
: const char* const SentryITestBase::kDevRole = "developer";
: const char* const SentryITestBase::kAdminRole = "ad";
: const char* const SentryITestBase::kDatabaseName = "db";
: const char* const SentryITestBase::kTableName = "table";
: const char* const SentryITestBase::kSecondTable = "second_table";
Why not just stick these in an anonymous namespace, rather than being static to the class? Then we can reuse them without this extra noise.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@473
PS1, Line 473: TEST_F(MasterRangerTest, TestIntegration) {
Would be good to also kinit as kTestUser and ensure that we get a rejection.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/integration-tests/master_sentry-itest.cc@484
PS1, Line 484: k
nit: remove space
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@342
PS1, Line 342: set_secure
Should this also be kerberos specific? E.g. we still haven't enabled TLS support, right?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@345
PS1, Line 345: rangeradmin
nit: Are this and 'rangerlookup' hardcoded into Ranger? Maybe add a comment indicating that if so.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@346
PS1, Line 346: spn
nit: just inline the substitute? Same below.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/mini-cluster/external_mini_cluster.cc@346
PS1, Line 346: ktpath
nit: it feels kind of weird to be re-using a moved variable. Maybe just declare different ktpaths for each of these? That also gives us the opportunity to give each ktpath variable an appropriate name.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/postgres/mini_postgres.cc@73
PS1, Line 73: !port_
nit: I generally prefer only using this style of condition checking for bools, pointers, and optionals. Conforming to that makes things like reasoning about boost::optional<int> easier IMO. Mind writing this as (port_ != 0)?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h@94
PS1, Line 94: void set_admin_ktpath(std::string admin_ktpath) {
The 'move' proliferation is getting a bit cumbersome IMO. I don't think we've got hard and fast rules, but for simple setters like this, unless the idea of ownership is important (e.g. often in constructors, where the inputs are 1:1 with the class itself) or this is performance sensitive, we may want to consider making these all pass by const ref. https://abseil.io/tips/117 has some notes on this too.
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.h@106
PS1, Line 106: void set_host(std::string host) {
: host_ = host;
nit: could you add a comment indicating why we'll need this? Same in MiniPostgres
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@98
PS1, Line 98: port_
nit: same here
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@194
PS1, Line 194: FQDN
nit: no longer FQDN
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@210
PS1, Line 210: if (!krb5_config.empty()) {
nit: When might this be empty? Maybe add a comment. Or should this be a DCHECK or similar?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/mini_ranger.cc@248
PS1, Line 248: EasyJson configs = service.Set("configs", EasyJson::kObject);
: configs.Set("policy.download.auth.users", "kudu");
: configs.Set("tag.download.auth.users", "kudu");
nit: could you explain these so your typical Kudu developer can understand why these are important?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc@50
PS1, Line 50: $JAVA_HOME/java
This should be $JAVA_HOME/bin/java, right?
http://gerrit.cloudera.org:8080/#/c/15601/1/src/kudu/ranger/ranger_client.cc@179
PS1, Line 179:
: ret
nit: remove the extra line
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 30 Mar 2020 21:43:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 13:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/mini-cluster/external_mini_cluster.cc.orig
File src/kudu/mini-cluster/external_mini_cluster.cc.orig:
PS13:
Accidentally added?
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/postgres/mini_postgres.cc.orig
File src/kudu/postgres/mini_postgres.cc.orig:
PS13:
Same here?
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/ranger/mini_ranger.cc.orig
File src/kudu/ranger/mini_ranger.cc.orig:
PS13:
Same here?
http://gerrit.cloudera.org:8080/#/c/15601/13/src/kudu/ranger/mini_ranger.h.orig
File src/kudu/ranger/mini_ranger.h.orig:
PS13:
Also this one?
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 17:11:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#14).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
12 files changed, 429 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/14
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
Reviewed-on: http://gerrit.cloudera.org:8080/15601
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/master/ranger_authz_provider.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
M src/kudu/security/init.cc
M src/kudu/security/init.h
12 files changed, 429 insertions(+), 139 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Grant Henke, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15601
to look at the new patch set (#5).
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
KUDU-3081 Add Kerberos support to MiniRanger
As integration tests using Sentry authz provider all depend on a
Kerberized mini cluster we also need to add Kerberos support to
MiniRanger to be able to parameterize the integration tests.
As adding Kerberos support proved to be trickier than expected this
patch contains several other improvements needed for this to work.
After configuring Kerberos for both MiniRanger and the Ranger subprocess
correctly I still ran into an error:
GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos credentails)
This turned out to be due to a mismatch in forward and reverse lookup of
MiniRanger's hostname. To fix this I switched to using IP addresses in
the configuration instead of FQDNs.
This patch also changes the default "java" binary in Ranger client to be
the one in $JAVA_HOME/bin instead of the one in $PATH as dist-test has
mismatching versions and it wouldn't start otherwise. To use the one in
$PATH a user can still simply set it to "java", or provide a full path.
Change-Id: I32118780ad912791fe5e371004345428b6459549
---
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/mini_ranger_configs.h
M src/kudu/ranger/ranger_client.cc
M src/kudu/ranger/ranger_client.h
9 files changed, 416 insertions(+), 137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/15601/5
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15601 )
Change subject: KUDU-3081 Add Kerberos support to MiniRanger
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/mini_ranger.cc@206
PS4, Line 206: string krb5_config = getenv("KRB5_CONFIG");
> Isn't it important for the KRB5_CONFIG used by MiniRanger to be the one fr
+1 to what Andrew said, and yeah, I do feel strongly about this (provided it's not an impossible task). I already find it difficult to understand how the various Kerberos-related environment variables affect the Kudu C++ security code; I'd rather we avoid adding more complexity there.
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc
File src/kudu/ranger/ranger_client.cc:
http://gerrit.cloudera.org:8080/#/c/15601/4/src/kudu/ranger/ranger_client.cc@195
PS4, Line 195: string krb5_config = getenv("KRB5_CONFIG");
> Alternatively, we can expose a flag that, if set, will be used (e.g. in an
The problem is that it's much tougher to track and understand the behavior of an env var "switch" vs. an explicit one.
Can we model this as we did kudu::thrift::ClientOptions::enable_kerberos?
1. The "library" code (RangerClient, in this case) provides an explicit setter or options struct to enable Kerberos. This setter/option must include all the information needed (i.e. if it needs a path to a krb5.conf file, then it expects a string containing that path).
2. The EMC code uses this setter/options struct accordingly using information from the MiniKdc.
3. Production code sets the setter/options using the value of a gflag. Or, in the case of path to krb5.conf, maybe even hardcodes it to /etc/krb5.conf, as I believe that's what the C++ security code always uses.
--
To view, visit http://gerrit.cloudera.org:8080/15601
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32118780ad912791fe5e371004345428b6459549
Gerrit-Change-Number: 15601
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:55:44 +0000
Gerrit-HasComments: Yes