Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24383 )
Change subject: KUDU-3781: add TLS transport support for HMS client ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_catalog.cc@76 PS3, Line 76: DEFINE_string(hive_metastore_tls_trusted_ca_cert_file, "", > We already have a -trusted_certificate_file flag. Wouldn't it make more sen >From what I can see, --trusted_certificate_file is currently used for >verifying authenticity of TLS certificates securing HTTP/HTTPS connections. >It's applicable for requests performed by internal curl client to HTTP-based >APIs (e.g., KMS, JWKS-related APIs). But it's not for everything: for KRPC >we use a separate --rpc_ca_certificate_file flag. Since HMS-Kudu >communication is not HTTP-based (it's Thrift, actually), I added this separate >flag to mirror --rpc_ca_certificate_file. Having an option to use separate controls makes more sense IMO. The rationale is simple: if the CA certs are the same, it's easy to point different flags to the same file. But if there is a need to refer to the system-wide CA trust bundle for HTTPS connections, but use custom CA certificate for HMS Thrift connections, then it would be impossible to make it work using a single flag. However, I agree that for some use-cases it might be convenient to source the CA certificates from --trusted_certificate_file for the HMS client as well. To provide both options, I added a new --hive_metastore_tls_use_https_trusted_ca_cert flag; it's set to 'true' by default. http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_client-test.cc@229 PS3, Line 229: ASSERT_STR_CONTAINS(CreateTable(client.get(), database_name, table_name, "", cluster_id).ToString(), > nit: long line Done http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_client-test.cc@699 PS3, Line 699: MiniHms hms; : hms.EnableTls(true); : : // Set the `KUDU_HMS_SYNC_ENABLED` environment variable in the : // HMS environment to manually enable HMS synchronization checks. : // This means we don't need to stand up a Kudu Cluster for this test. : hms.AddEnvVar("KUDU_HMS_SYNC_ENABLED", "1"); : ASSERT_OK(hms.Start()); > nit: Can the repetition in these four lines across the tests be avoided wit Done http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/hms/hms_client.cc@201 PS3, Line 201: //DCHECK_NE(nullptr, (*client)->get()); > nit: should this be removed? Done http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/24383/3/src/kudu/integration-tests/master_hms-itest.cc@104 PS3, Line 104: emf.emplace_back("--hive_metastore_tls_enabled"); > what happens if TLS is enabled on Kudu side, but not on HMS side? Please see HmsClientTest.TlsEnabledOnlyOnClient for the expected behavior: https://gerrit.cloudera.org/#/c/24383/3/src/kudu/hms/hms_client-test.cc@615 -- To view, visit http://gerrit.cloudera.org:8080/24383 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9658323ed9aebdfdf8e6847c9c3129f5c8517c65 Gerrit-Change-Number: 24383 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[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: Wed, 03 Jun 2026 06:53:37 +0000 Gerrit-HasComments: Yes
