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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 30 Mar 2020 22:42:14 +0000 Gerrit-HasComments: Yes
