Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11956 )
Change subject: [sentry] add mini Sentry to the external mini cluster ...................................................................... Patch Set 3: (20 comments) http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11956/2//COMMIT_MSG@13 PS2, Line 13: we can do nit: this reads a bit like this is what's implemented. Mind changing it to "one approach would be to:" http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.h@54 PS2, Line 54: Configures the mini HMS to enable the Sentry plugin. nit: doc what 'sentry_service_principal' is? What happens when it's boost::none? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@81 PS2, Line 81: "Enabling Sentry for HMS"; nit: maybe add the sentry address? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@164 PS2, Line 164: Status wait = WaitForTcpBind(hms_process_->pid(), &port_, none, nit: Can you add a comment about why `none` is appropriate here? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@318 PS2, Line 318: Configures the HMS to use the Sentry plugin for filtering : // read results. I'm not sure I understand this. What is a "read result" in this context? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@322 PS2, Line 322: Configures the HMS to use the Sentry event listener for : // authorization metadata operations. Just making sure I understand this, this is configured to ensure the HMS will consult Sentry for authorization metadata? What is considered an authorization metadata operation? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@328 PS2, Line 328: service. nit: missing space http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@327 PS2, Line 327: synchronizes the notification logs with the Sentry : // service. Just making sure I understand this, this is synchronizing authz metadata entries in the HMS with changes in Sentry? I'm unsure of where the notification logs come into play. http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/hms/mini_hms.cc@359 PS2, Line 359: HNS HMS http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@55 PS2, Line 55: const ExternalMiniCluster::BindMode bind_mode; Hrm, where is this used? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@86 PS2, Line 86: { ExternalMiniCluster::BindMode::LOOPBACK, true, }, : { ExternalMiniCluster::BindMode::LOOPBACK, false, }, Mind commenting what the behavior for this test is for mac? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@103 PS2, Line 103: Substitute("$0.$1", kDatabaseName, kTableName) nit: maybe pull this out into a kTableIdentifier? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/integration-tests/master_sentry-itest.cc@117 PS2, Line 117: if (params.enable_kerberos) { : // Create a principal 'kudu' and configure to use it. : ASSERT_OK(cluster_->kdc()->CreateUserPrincipal("kudu")); : ASSERT_OK(cluster_->kdc()->Kinit("kudu")); : ASSERT_OK(cluster_->kdc()->SetKrb5Environment()); : hms_client_opts.enable_kerberos = true; : hms_client_opts.service_principal = "hive"; : } : hms::HmsClient hms_client(cluster_->hms()->address(), hms_client_opts); : ASSERT_OK(hms_client.Start()); : : hive::Table hms_table; : ASSERT_OK(hms_client.GetTable(kDatabaseName, kTableName, &hms_table)); Are there authz metadata entries we should be checking here? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h@159 PS2, Line 159: // If true, set up a Sentry service as part of this ExternalMiniCluster. nit: Is there a default worth mentioning here? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.h@419 PS2, Line 419: // Index to identify the number of external servers being started. I'm a little confused about this variable. Usually an index is a pointer into some list, but this comment makes it out to be some kind of count? Mind clarifying? http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/2/src/kudu/mini-cluster/external_mini_cluster.cc@226 PS2, Line 226: // Start Sentry. : if (opts_.enable_sentry) { : string host = GetBindIpForExternalServer(external_server_index_++); : HostPort sentry_address(host, 0); : RETURN_NOT_OK_PREPEND(StartSentry(sentry_address), : "Failed to start the Sentry service"); : } : : // Start the HMS. : if (opts_.hms_mode == HmsMode::ENABLE_HIVE_METASTORE || : opts_.hms_mode == HmsMode::ENABLE_METASTORE_INTEGRATION) { : hms_.reset(new hms::MiniHms()); : hms_->SetDataRoot(opts_.cluster_root); : : if (opts_.enable_kerberos) { : string spn = Substitute("hive/$0", hms_->address().host()); : string ktpath; : RETURN_NOT_OK_PREPEND(kdc_->CreateServiceKeytab(spn, &ktpath), : "could not create keytab"); : hms_->EnableKerberos(kdc_->GetEnvVars()["KRB5_CONFIG"], spn, ktpath, : rpc::SaslProtection::kAuthentication); : } : : if (opts_.enable_sentry) { : string sentry_spn = Substitute("sentry/$0...@krbtest.com", sentry_->address().host()); : hms_->EnableSentry(sentry_->address(), sentry_spn); : } : : RETURN_NOT_OK_PREPEND(hms_->Start(), : "Failed to start the Hive Metastore"); : : // Restart Sentry with the HMS address. : if (opts_.enable_sentry) { : HostPort sentry_address = sentry_->address(); : RETURN_NOT_OK_PREPEND(StopSentry(), : "Failed to stop the Sentry service"); : RETURN_NOT_OK_PREPEND(StartSentry(sentry_address), : "Failed to start the Sentry service"); : } : } You kind of allude to what this is doing in the commit message, but I think it would be helpful with comments interspersed in this block describing the little dance we're doing here. http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/mini-cluster/mini_cluster.cc@52 PS3, Line 52: int idx; : switch (type) { : case MASTER: : idx = kServersMaxNum - index; : break; : case TSERVER: : idx = index + 1; : break; : case EXTERNAL_SERVER: : idx = kServersMaxNum / 3 + index; : break; Not really your fault, but could you add a comment here walking through what these are? http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc File src/kudu/util/test_util.cc: http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@a421 PS3, Line 421: : This structure of the pairs is lost in the new comment. Mind keeping it around? (particularly, it isn't clear anymore what f123 is, even if we are ignoring it) http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@402 PS3, Line 402: // The '-Ffn' flag gets lsof to output something like: : // p5801 : // f548 : // n127.0.0.1:43954->127.0.0.1:43617 : // f549 : // n*:8038 Hrm, I think I'm missing something. The command didn't change, right? Why did the output of lsof change then? http://gerrit.cloudera.org:8080/#/c/11956/3/src/kudu/util/test_util.cc@425 PS3, Line 425: Status s = Subprocess::Call(cmd, "", &lsof_out).AndThen([&] () { : StripTrailingNewline(&lsof_out); : vector<string> lines = strings::Split(lsof_out, "\n"); : for (int index = 2; index < lines.size(); index += 2) { : StringPiece cur_line(lines[index]); : if (HasPrefixString(cur_line.ToString(), addr_pattern) && : !cur_line.contains("->")) { : if (!safe_strto32(lines[index].substr(addr_pattern.size()), &p)) { : return Status::RuntimeError("unexpected lsof output", lsof_out); : } : : return Status::OK(); : } : } : : return Status::RuntimeError("unexpected lsof output", lsof_out); : }); nit: merge these `if`s into one conditional block? Also mind adding a comment explaining this? I think it'd be helpful to elaborate on the above example what the expected behavior is. It's probably also worth explaining the new behavior of this method? -- To view, visit http://gerrit.cloudera.org:8080/11956 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f02e6085bd239570d10ec629f48856d37ed6e59 Gerrit-Change-Number: 11956 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 20 Nov 2018 01:32:21 +0000 Gerrit-HasComments: Yes
