nevzheng commented on code in PR #10975:
URL: https://github.com/apache/gravitino/pull/10975#discussion_r3650636477
##########
clients/client-java/src/test/java/org/apache/gravitino/client/TestHTTPClient.java:
##########
@@ -356,6 +356,23 @@ public void testSocketTimeout() throws IOException {
}
@Test
+ public void testBuildWithoutTLSConfigurer() {
+ HTTPClient client =
HTTPClient.builder(ImmutableMap.of()).uri("http://localhost";).build();
+
+ Assertions.assertNotNull(client);
+ }
+
+ @Test
+ public void testBuildWithTLSConfigurer() {
Review Comment:
**blocking:** Please test the Gravitino client TLS path
The current tests exercise Jetty via `java.net.http.HttpClient` with a
hand-built `SSLContext`. That does not cover `HTTPClient.withTlsConfigurer` /
`configureHttp(...)`.
Please add CUJ-level tests on the Gravitino client path. Example shape:
```java
TLSConfigurer tlsConfigurer =
new TLSConfigurer() {
@Override
public SSLContext sslContext() {
// load client truststore (+ optional client keystore) from test
fixtures
return sslContext;
}
};
try (HTTPClient client =
HTTPClient.builder(ImmutableMap.of())
.uri("https://localhost:"; + port)
.withTlsConfigurer(tlsConfigurer)
.build()) {
// assert success or handshake failure per case below
}
```
Same idea via the public builder is also fine:
```java
GravitinoClient.builder("https://localhost:"; + port)
.configureHttp(http -> http.withTlsConfigurer(tlsConfigurer))
.build();
```
### Test matrix
| # | Server | Client truststore | Client keystore | Expected |
|---|---|---|---|---|
| 1 | HTTPS, client auth **required** | `test-client-truststore.p12` |
`test-trusted-client-keystore.p12` | **Success** |
| 2 | HTTPS, client auth **required** | `test-client-truststore.p12` |
_(none)_ | **Handshake failure** |
| 3 | HTTPS, client auth **required** | `test-client-truststore.p12` |
`test-untrusted-client-keystore.p12` | **Handshake failure** |
| 4 | HTTPS, client auth **off** | `test-client-truststore.p12` | _(none)_ |
**Success** |
| 5 | HTTPS, client auth **off** | `test-untrusted-server-truststore.p12` |
_(none)_ | **Handshake failure** |
##########
clients/client-java/build.gradle.kts:
##########
@@ -27,6 +27,7 @@ dependencies {
implementation(project(":common")) {
exclude(group = "org.apache.logging.log4j")
}
+ implementation(project(":catalogs:hive-metastore-common"))
Review Comment:
**issue (blocking):** Remove the unused Hive dependency from the Java client
This PR adds `implementation(project(":catalogs:hive-metastore-common"))` to
`clients/client-java/build.gradle.kts`, but nothing in the change uses that
module. Please remove it — it pulls catalog/Hive dependencies into the public
client artifact.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/TLSConfigurer.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.client;
+
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLContext;
+import org.apache.hc.client5.http.ssl.HttpsSupport;
+import org.apache.hc.core5.ssl.SSLContexts;
+
+/** Configures TLS settings for the HTTP client. */
+public interface TLSConfigurer {
Review Comment:
**praise:** Nice progress on the `configureHttp` / `TLSConfigurer` injection
and the PKCS12 + Jetty mTLS fixtures.
**suggestion (non-blocking):** Follow-up — better client UX for TLS setup
`TLSConfigurer` is a solid extension point, but callers still have to
assemble an `SSLContext` by hand. As a follow-up, please add a small helper so
the common CUJ is just paths + passwords, for example:
```java
TLSConfigurer tls =
TLSConfigurers.builder()
.trustStore(Path.of("/path/to/truststore.p12"), "changeit")
.keyStore(Path.of("/path/to/keystore.p12"), "changeit") // optional,
for mTLS
.storeType("PKCS12") // optional, default PKCS12
.build();
GravitinoClient.builder("https://gravitino.example.com";)
.configureHttp(http -> http.withTlsConfigurer(tls))
.build();
```
Ideally also support the same via env vars / client config, plus a short doc
section for:
1. HTTPS with custom truststore
2. mTLS with client keystore + truststore
If you prefer not to do that in this PR, please file a ticket describing
that follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
