[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 Apr 2020 21:26:22 + Gerrit-HasComments: No
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 Apr 2020 17:34:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 Apr 2020 17:11:53 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 02 Apr 2020 09:23:56 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:09:11 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:08:33 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:31:18 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 21:13:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 Apr 2020 09:25:28 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:55:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 22:21:09 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:34:44 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:35:49 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 31 Mar 2020 19:00:55 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 30 Mar 2020 23:52:00 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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, _));
> 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
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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, _));
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 easier IMO. Mind writing this as (port_ != 0)?
[kudu-CR] KUDU-3081 Add Kerberos support to MiniRanger
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anonymous Coward (314) Gerrit-Reviewer: Grant Henke
