nevzheng commented on code in PR #10975:
URL: https://github.com/apache/gravitino/pull/10975#discussion_r3608693079
##########
server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java:
##########
@@ -95,4 +95,40 @@ public void testStopWithNullServer() {
public void testStartWithoutInitialise() throws InterruptedException {
assertThrows(RuntimeException.class, () -> jettyServer.start());
}
+
+ @Test
+ public void testInitializeHttpsWithClientAuthRequiresTrustStore() throws
IOException {
Review Comment:
**[Blocking] The PR does not currently satisfy the functional end-to-end
requirement of #9836.**
The added tests only verify that client and server objects can be
constructed. They do not establish a TLS connection, perform a handshake,
present a client certificate, or make an actual request.
Please add functional end-to-end coverage that proves the feature works,
including both success and failure cases. At minimum:
- A server requiring client authentication accepts a request from a client
configured with a trusted certificate and private key.
- The same server rejects a client that does not provide a certificate.
- The server rejects a client whose certificate is not trusted.
- The client rejects an untrusted server certificate.
- Existing behavior still works when no custom TLS configuration is supplied
and client authentication is not required.
The negative client-certificate tests should still trust the server
certificate so that they fail specifically because client authentication failed.
This coverage is required to demonstrate that the PR actually implements the
requested mTLS behavior, rather than only adding configuration plumbing.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoAdminClient.java:
##########
@@ -65,7 +66,28 @@ private GravitinoAdminClient(
boolean checkVersion,
Map<String, String> headers,
Map<String, String> properties) {
- super(uri, authDataProvider, checkVersion, headers, properties);
+ this(uri, authDataProvider, checkVersion, headers, properties, null);
+ }
+
+ /**
+ * Constructs a new GravitinoClient with the given URI, authenticator and
AuthDataProvider.
+ *
+ * @param uri The base URI for the Gravitino API.
+ * @param authDataProvider The provider of the data which is used for
authentication.
+ * @param checkVersion Whether to check the version of the Gravitino server.
Gravitino does not
+ * support the case that the client-side version is higher than the
server-side version.
+ * @param headers The base header for Gravitino API.
+ * @param properties A map of properties (key-value pairs) used to configure
the Gravitino client.
+ * @param tlsConfigurer The TLS configurer for the Gravitino client.
+ */
+ private GravitinoAdminClient(
Review Comment:
**[Non-blocking design] Inject HTTP behavior instead of extending the
Gravitino client hierarchy**
My main concern is that this change extends `GravitinoAdminClient`,
`GravitinoClient`, and `GravitinoClientBase` with a transport-specific concern.
We are not adding new Gravitino client behavior—we are only changing how the
underlying HTTP connection is constructed.
I suggest keeping `TLSConfigurer` entirely within `HTTPClient` and
introducing a shared client-construction path that can configure the HTTP
builder. For example:
```java
GravitinoClientFactory factory =
GravitinoClientFactory.builder(uri)
.configureHttp(http -> http.withTlsConfigurer(tlsConfigurer))
.build();
GravitinoClient client = factory.createClient(metalakeName);
AdminClient admin = factory.createAdminClient();
```
The implementation could use a non-null, no-op HTTP configuration hook:
```java
private Consumer<HTTPClient.Builder> httpConfigurer = ignored -> {};
public Builder configureHttp(Consumer<HTTPClient.Builder> configurer) {
this.httpConfigurer = Objects.requireNonNull(configurer);
return this;
}
private RESTClient createRestClient() {
HTTPClient.Builder http =
HTTPClient.builder(properties)
.uri(uri)
.withAuthDataProvider(authDataProvider)
.withHeaders(headers);
httpConfigurer.accept(http);
return http.build();
}
```
This gives us one HTTP extension point without adding `TLSConfigurer`
fields, constructor parameters, or duplicated builder methods to every
Gravitino client type. It also avoids nullable TLS state: the default behavior
is simply a no-op HTTP configuration.
As a follow-up, URI, authentication, headers, and other HTTP settings could
move into this same transport-construction path, and the concrete Gravitino
clients could accept only the resulting `RESTClient`.
I am okay accepting the current structure in this PR, but I recommend at
least removing the unused protected `tlsConfigurer` field now. The functional
focus of this change should remain configuring `HTTPClient` with the
`SSLContext` required for client-certificate authentication.
--
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]
