Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15416 )
Change subject: [ranger] pass 'principal' and 'keytab' to the subprocess ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/15416/4/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15416/4/src/kudu/ranger/ranger_client.cc@130 PS4, Line 130: : static bool ValidateRangerJavaPath() { : if (!FLAGS_ranger_config_path.empty() && : !Env::Default()->FileExists(FLAGS_ranger_java_path)) { : LOG(ERROR) << Substitute("FLAGS_ranger_java_path has invalid java binary path: $0", : FLAGS_ranger_java_path); : return false; : } : return true; : } : GROUP_FLAG_VALIDATOR(ranger_java_path_flags, ValidateRangerJavaPath); : : static bool ValidateRangerJarPath() { : if (!FLAGS_ranger_config_path.empty() && : !Env::Default()->FileExists(FLAGS_ranger_jar_path)) { : LOG(ERROR) << Substitute("FLAGS_ranger_jar_path has invalid JAR file path for the " : "Ranger subprocess: $0", FLAGS_ranger_jar_path); : return false; : } : return true; : } : GROUP_FLAG_VALIDATOR(ranger_jar_path_flags, ValidateRangerJarPath); Not sure if we do this elsewhere, but would it make sense to do all this validation as a single validator predicated on FLAGS_ranger_config_path? Also should we ensure the config is a file too? http://gerrit.cloudera.org:8080/#/c/15416/4/src/kudu/ranger/ranger_client.cc@191 PS4, Line 191: Status BuildArgv(vector<string>* argv) { : DCHECK(argv); : // When Kerberos is enabled in Kudu, pass both Kudu principal and keytab file : // to the Ranger subprocess. : if (!FLAGS_keytab_file.empty()) { : string configured_principal; : RETURN_NOT_OK_PREPEND(security::GetConfiguredPrincipal(FLAGS_principal, &configured_principal), : Substitute("unable to get the configured principal from ($0) for " : "the Ranger subprocess", FLAGS_principal)); : *argv = std::move(vector<string>({ FLAGS_ranger_java_path, "-cp", GetJavaClasspath(), : kMainClass, "-i", configured_principal, : "-k", FLAGS_keytab_file })); : } else { : // Pass the required arguments to run the Ranger subprocess. : argv->emplace_back(FLAGS_ranger_java_path); : argv->emplace_back("-cp"); : argv->emplace_back(GetJavaClasspath()); : argv->emplace_back(kMainClass); : }; : return Status::OK(); nit: this would be cleaner as: vector<string> ret = { FLAGS_ranger_java_path, -cp, GetJavaClasspath(), kMainClass }; if (!FLAGS_keytab_file.empty()) { RETURN_NOT_OK(evaluate principal); ret.emplace_back(-i); ret.emplace_back(principal); ret.emplace_back(-k); ret.emplace_back(FLAGS_keytab_file); } *argv = std::move(ret); return Status::OK(); -- To view, visit http://gerrit.cloudera.org:8080/15416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie30b835b6d44ddb51d95c587f1329bfefebeb37c Gerrit-Change-Number: 15416 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 15 Mar 2020 01:50:44 +0000 Gerrit-HasComments: Yes
