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 <[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 21:43:23 +0000 Gerrit-HasComments: Yes
