amogh-jahagirdar commented on code in PR #10052:
URL: https://github.com/apache/iceberg/pull/10052#discussion_r1550644916
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -121,6 +128,92 @@ public void testHeadFailure() throws
JsonProcessingException {
testHttpMethodOnFailure(HttpMethod.HEAD);
}
+ /** Tests that requests go via the proxy server in case the client is set up
with one */
+ @Test
+ public void testHttpClientProxyServerInteraction() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ try (ClientAndServer proxyServer = startClientAndServer(proxyPort);
+ RESTClient clientWithProxy =
+ HTTPClient.builder(ImmutableMap.of())
+ .uri(URI)
+ .withProxy(proxyHostName, proxyPort)
+ .build()) {
+ // Set up the servers to match against a provided request
+ String path = "v1/config";
+
+ HttpRequest mockRequest =
+ request("/" +
path).withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+
+ HttpResponse mockResponse = response().withStatusCode(200);
+
+ mockServer.when(mockRequest).respond(mockResponse);
+ proxyServer.when(mockRequest).respond(mockResponse);
+
+ restClient.head(path, ImmutableMap.of(), (onError) -> {});
+ mockServer.verify(mockRequest, VerificationTimes.exactly(1));
+ proxyServer.verify(mockRequest, VerificationTimes.never());
+
+ // Validate that the proxy server is hit only if the client is set up
with one
+ clientWithProxy.head(path, ImmutableMap.of(), (onError) -> {});
+ proxyServer.verify(mockRequest, VerificationTimes.exactly(1));
+ }
+ }
+
+ /** Negative test for basic username password authentication on the proxy
server */
+ @Test
+ public void testHttpClientProxyAuthenticationFailure() throws IOException {
Review Comment:
same as above, I don't think a test comment is required here, the name does
a good job capturing what the test is validating
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -121,6 +128,92 @@ public void testHeadFailure() throws
JsonProcessingException {
testHttpMethodOnFailure(HttpMethod.HEAD);
}
+ /** Tests that requests go via the proxy server in case the client is set up
with one */
+ @Test
+ public void testHttpClientProxyServerInteraction() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ try (ClientAndServer proxyServer = startClientAndServer(proxyPort);
+ RESTClient clientWithProxy =
+ HTTPClient.builder(ImmutableMap.of())
+ .uri(URI)
+ .withProxy(proxyHostName, proxyPort)
+ .build()) {
+ // Set up the servers to match against a provided request
+ String path = "v1/config";
+
+ HttpRequest mockRequest =
+ request("/" +
path).withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+
+ HttpResponse mockResponse = response().withStatusCode(200);
+
+ mockServer.when(mockRequest).respond(mockResponse);
+ proxyServer.when(mockRequest).respond(mockResponse);
+
+ restClient.head(path, ImmutableMap.of(), (onError) -> {});
+ mockServer.verify(mockRequest, VerificationTimes.exactly(1));
+ proxyServer.verify(mockRequest, VerificationTimes.never());
+
+ // Validate that the proxy server is hit only if the client is set up
with one
+ clientWithProxy.head(path, ImmutableMap.of(), (onError) -> {});
+ proxyServer.verify(mockRequest, VerificationTimes.exactly(1));
+ }
+ }
+
+ /** Negative test for basic username password authentication on the proxy
server */
+ @Test
+ public void testHttpClientProxyAuthenticationFailure() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ String authorizedUsername = "test-username";
+ String authorizedPassword = "test-password";
+ String invalidPassword = "invalid-password";
+
+ // Let's build a basic credentials provider with invalid password
+ HttpHost proxy = new HttpHost(proxyHostName, proxyPort);
+ BasicCredentialsProvider credentialsProvider = new
BasicCredentialsProvider();
+ credentialsProvider.setCredentials(
+ new AuthScope(proxy),
+ new UsernamePasswordCredentials(authorizedUsername,
invalidPassword.toCharArray()));
+
+ try (ClientAndServer proxyServer =
+ startClientAndServer(
+ new Configuration()
+ .proxyAuthenticationUsername(authorizedUsername)
+ .proxyAuthenticationPassword(authorizedPassword),
+ proxyPort);
+ // Inject the client with invalid credentials provider
+ RESTClient clientWithProxy =
+ HTTPClient.builder(ImmutableMap.of())
+ .uri(URI)
+ .withProxy(proxyHostName, proxyPort)
+ .withProxyCredentialsProvider(credentialsProvider)
+ .build()) {
+
+ ErrorHandler onError =
+ new ErrorHandler() {
+ @Override
+ public ErrorResponse parseResponse(int code, String responseBody) {
+ return null;
+ }
+
+ @Override
+ public void accept(ErrorResponse errorResponse) {
+ throw new RuntimeException(errorResponse.message() + " - " +
errorResponse.code());
+ }
+ };
+
+ int expectedErrorCode = HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED;
+ String expectedErrorMsg = "Proxy Authentication Required";
+
+ // Validate that we hit a 407 Proxy Authentication Required exception
Review Comment:
same as above, I don;t think this inline comment is needed since it's
apparent.
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -121,6 +128,92 @@ public void testHeadFailure() throws
JsonProcessingException {
testHttpMethodOnFailure(HttpMethod.HEAD);
}
+ /** Tests that requests go via the proxy server in case the client is set up
with one */
Review Comment:
Nit: While I personally like the idea of commenting tests typically in the
project we don't add them and try to just write test names which are self
evident in what they are testing. I think in this case just naming the test
`testHttpClientProxyServer` is sufficient (no additional context needed I think
for the happy case)
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -121,6 +128,92 @@ public void testHeadFailure() throws
JsonProcessingException {
testHttpMethodOnFailure(HttpMethod.HEAD);
}
+ /** Tests that requests go via the proxy server in case the client is set up
with one */
+ @Test
+ public void testHttpClientProxyServerInteraction() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ try (ClientAndServer proxyServer = startClientAndServer(proxyPort);
+ RESTClient clientWithProxy =
+ HTTPClient.builder(ImmutableMap.of())
+ .uri(URI)
+ .withProxy(proxyHostName, proxyPort)
+ .build()) {
+ // Set up the servers to match against a provided request
+ String path = "v1/config";
+
+ HttpRequest mockRequest =
+ request("/" +
path).withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+
+ HttpResponse mockResponse = response().withStatusCode(200);
+
+ mockServer.when(mockRequest).respond(mockResponse);
+ proxyServer.when(mockRequest).respond(mockResponse);
+
+ restClient.head(path, ImmutableMap.of(), (onError) -> {});
+ mockServer.verify(mockRequest, VerificationTimes.exactly(1));
+ proxyServer.verify(mockRequest, VerificationTimes.never());
Review Comment:
I see the intent but I don't think we need this validation since other tests
will validate this. The reason is, I think this verification becomes the
equivalent of "restClient should be hitting mockServer directly, and there
shouldn't be some other server in between" and the other tests will validate
that just by leveraging the response that mockServer returns.
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -468,6 +483,19 @@ public Builder uri(String path) {
return this;
}
+ public Builder withProxy(String hostname, int port) {
+ Preconditions.checkNotNull(hostname, "Invalid hostname for http client
proxy: null");
+ this.proxy = new HttpHost(hostname, port);
+ return this;
+ }
+
+ public Builder withProxyCredentialsProvider(CredentialsProvider
credentialsProvider) {
+ Preconditions.checkNotNull(
+ credentialsProvider, "Invalid credentials provider for http client
proxy: null");
+ this.credentialsProvider = credentialsProvider;
Review Comment:
I think `proxyCredentialsProvider` would be a better name. Also I think when
`build` is called there should be a validation that if
`proxyCredentialsProvider` is specified there is a proxy (it should not be the
case that someone somehow specifies a proxyCredentialsProvider without having a
proxy defined, if so build should fail).
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -121,6 +128,92 @@ public void testHeadFailure() throws
JsonProcessingException {
testHttpMethodOnFailure(HttpMethod.HEAD);
}
+ /** Tests that requests go via the proxy server in case the client is set up
with one */
+ @Test
+ public void testHttpClientProxyServerInteraction() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ try (ClientAndServer proxyServer = startClientAndServer(proxyPort);
+ RESTClient clientWithProxy =
+ HTTPClient.builder(ImmutableMap.of())
+ .uri(URI)
+ .withProxy(proxyHostName, proxyPort)
+ .build()) {
+ // Set up the servers to match against a provided request
+ String path = "v1/config";
+
+ HttpRequest mockRequest =
+ request("/" +
path).withMethod(HttpMethod.HEAD.name().toUpperCase(Locale.ROOT));
+
+ HttpResponse mockResponse = response().withStatusCode(200);
+
+ mockServer.when(mockRequest).respond(mockResponse);
+ proxyServer.when(mockRequest).respond(mockResponse);
+
+ restClient.head(path, ImmutableMap.of(), (onError) -> {});
+ mockServer.verify(mockRequest, VerificationTimes.exactly(1));
+ proxyServer.verify(mockRequest, VerificationTimes.never());
+
+ // Validate that the proxy server is hit only if the client is set up
with one
+ clientWithProxy.head(path, ImmutableMap.of(), (onError) -> {});
+ proxyServer.verify(mockRequest, VerificationTimes.exactly(1));
+ }
+ }
+
+ /** Negative test for basic username password authentication on the proxy
server */
+ @Test
+ public void testHttpClientProxyAuthenticationFailure() throws IOException {
+ int proxyPort = 1070;
+ String proxyHostName = "localhost";
+ String authorizedUsername = "test-username";
+ String authorizedPassword = "test-password";
+ String invalidPassword = "invalid-password";
+
+ // Let's build a basic credentials provider with invalid password
Review Comment:
Nit: Inline comment not required here since I think it's apparent. also in
comments typically we try and avoid first person/personal pronouns like "Let's"
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
