Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/24374 )
Change subject: KUDU-3781: Add TLS support to MiniHms ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/Untitled File src/kudu/hms/Untitled: PS2: > What is this? It looks like I messed up a git command :D http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.h@50 PS2, Line 50: EnableTls(b > Even if we don't have a use-case when we re-use the same MiniHms instance f Done http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@268 PS2, Line 268: > Why is this part necessary? Please add a comment. It's not, removed it http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@272 PS2, Line 272: if (env->FileExists(success_marker)) { : return Status::OK(); : } : : // Clean up leftovers from an earlier failed attempt before invoking keytool. : for (const auto& path : tls_artifacts) { : if (env->FileExists(path)) { : RETURN_NOT_OK(env->DeleteFile(path)); : } : } > Instead, shouldn't this be done on the an automatic/scope clean-up path if I added the scoped cleanup, I'm wondering if a crash could prevent that also from properly cleaning up, so I left in a "manual" deletion, before we run the keytool, but this might be overkill. What do you think? http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@284 PS2, Line 284: artifacts) { > Why to copy the args vector? Could we pass reference to 'args' instead? Done http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@286 PS2, Line 286: WARN_NOT_OK(env->DeleteFile(path), : Subs > nit: could shorten this to be Done http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@303 PS2, Line 303: -------------------------------- > Ah, interesting: the keytool on macOS worked without requiring password to Yes, HMS uses hive.metastore.keystore.password as the password passed into the Thrift SSL keystore setup. In Hive, HiveMetaStore reads SSL_KEYSTORE_PASSWORD and passes it to SecurityUtils.getServerSSLSocket()[1], which calls TSSLTransportParameters.setKeyStore(…)[2][3] with that single password. Thrift’s server SSL setup uses that password for loading/initializing the keystore/key material. Also -keypass is also passed with the same value as -storepass when generating the server keypair, and writes that same value into hive.metastore.keystore.password. If the key password didn’t match what HMS uses, MiniHms startup would fail while creating the SSL server socket. [1] https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java#L562?? [2] https://github.com/apache/hive/blob/master/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java#L334?? [3] https://github.com/apache/hive/blob/master/standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java#L323 http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@305 PS2, Line 305: private key and certifi > Whoops, it's my bad -- I provided non-consistent information offline for t Ack http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@380 PS2, Line 380: > nit: consider creating an empty file instead: it's a bit misleading to add Thanks for the suggestions! Done http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@511 PS2, Line 511: : static const string kTlsEnabledConfigTemplate = R"( : <property> : <name>hive.metastore.use.SSL</name> : <value>tr > I'm curious: is it any better than relying on default setting for the ive.m I'm just afraid of changing defaults, but we can go with omitting this part when TLS is disabled -- To view, visit http://gerrit.cloudera.org:8080/24374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fca647fd900fafc6d737c93a4af4565631839b7 Gerrit-Change-Number: 24374 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 29 May 2026 12:30:04 +0000 Gerrit-HasComments: Yes
