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