You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "lunar-shadow (via GitHub)" <gi...@apache.org> on 2026/03/04 08:35:38 UTC
[PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
lunar-shadow opened a new pull request, #15506:
URL: https://github.com/apache/iceberg/pull/15506
### Summary
Add `http-client.proxy-use-system-property-values` and `http-client.proxy-use-environment-variable-values` properties to control whether AWS SDK reads proxy settings from Java system properties and environment variables.
Both default to `true` (AWS SDK default). Applied to both `ApacheHttpClient` and `UrlConnectionHttpClient`.
### Motivation
In environments where proxy environment variables (`HTTP_PROXY`, `HTTPS_PROXY`) are set globally but should not apply to AWS/S3 traffic, there is currently no way to disable them without unsetting the variables system-wide.
### Test plan
- Added unit tests in `TestHttpClientConfigurations` covering both HTTP client types with `true` and `false` values for each property
- Default behavior (not set) covered by existing tests
- Documentation updated in `docs/docs/aws.md`
Closes #15504
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#issuecomment-4233102775
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#issuecomment-4290002034
Thanks @lunar-shadow for the PR. Thanks @anuragmantri for the review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #15506:
URL: https://github.com/apache/iceberg/pull/15506
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#discussion_r3103717836
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -137,4 +137,140 @@ public void testApacheDefaultConfigurations() {
Mockito.verify(spyApacheHttpClientBuilder, Mockito.never())
.proxyConfiguration(Mockito.any(ProxyConfiguration.class));
}
+
+ @Test
+ public void testApacheProxySystemPropertyValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "false");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxyEnvironmentVariableValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "false");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxySystemPropertyValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "false");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxyEnvironmentVariableValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "false");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxySystemPropertyValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "true");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxyEnvironmentVariableValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "true");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxySystemPropertyValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "true");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxyEnvironmentVariableValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "true");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
Review Comment:
Minor: we may collapse all tests to two. Here are reasons: We don't verify the different behaviors when it is true or false anyways. And it may not worth to do so given that there is no getters for useSystemPropertyValues or useEnvironmentVariableValues.
```
@ParameterizedTest
@ValueSource(strings = {
HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES,
HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES
})
public void testApacheProxyFlagTriggersProxyConfig(String propertyKey) {
Map<String, String> properties = Maps.newHashMap();
properties.put(propertyKey, "false");
ApacheHttpClient.Builder spy = Mockito.spy(ApacheHttpClient.builder());
ApacheHttpClientConfigurations.create(properties)
.configureApacheHttpClientBuilder(spy);
Mockito.verify(spy).proxyConfiguration(Mockito.any(ProxyConfiguration.class));
}
@ParameterizedTest
@ValueSource(strings = {
HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES,
HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES
})
public void testUrlConnectionProxyFlagTriggersProxyConfig(String propertyKey) {
Map<String, String> properties = Maps.newHashMap();
properties.put(propertyKey, "false");
UrlConnectionHttpClient.Builder spy = Mockito.spy(UrlConnectionHttpClient.builder());
UrlConnectionHttpClientConfigurations.create(properties)
.configureUrlConnectionHttpClientBuilder(spy);
Mockito.verify(spy).proxyConfiguration(
Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "lunar-shadow (via GitHub)" <gi...@apache.org>.
lunar-shadow commented on code in PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#discussion_r3107340646
##########
aws/src/test/java/org/apache/iceberg/aws/TestHttpClientConfigurations.java:
##########
@@ -137,4 +137,140 @@ public void testApacheDefaultConfigurations() {
Mockito.verify(spyApacheHttpClientBuilder, Mockito.never())
.proxyConfiguration(Mockito.any(ProxyConfiguration.class));
}
+
+ @Test
+ public void testApacheProxySystemPropertyValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "false");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxyEnvironmentVariableValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "false");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxySystemPropertyValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "false");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxyEnvironmentVariableValues() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "false");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxySystemPropertyValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "true");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testApacheProxyEnvironmentVariableValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "true");
+ ApacheHttpClientConfigurations apacheHttpClientConfigurations =
+ ApacheHttpClientConfigurations.create(properties);
+ ApacheHttpClient.Builder apacheHttpClientBuilder = ApacheHttpClient.builder();
+ ApacheHttpClient.Builder spyApacheHttpClientBuilder = Mockito.spy(apacheHttpClientBuilder);
+
+ apacheHttpClientConfigurations.configureApacheHttpClientBuilder(spyApacheHttpClientBuilder);
+
+ Mockito.verify(spyApacheHttpClientBuilder)
+ .proxyConfiguration(Mockito.any(ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxySystemPropertyValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_SYSTEM_PROPERTY_VALUES, "true");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
+
+ @Test
+ public void testUrlConnectionProxyEnvironmentVariableValuesExplicitTrue() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(HttpClientProperties.PROXY_USE_ENVIRONMENT_VARIABLE_VALUES, "true");
+ UrlConnectionHttpClientConfigurations urlConnectionHttpClientConfigurations =
+ UrlConnectionHttpClientConfigurations.create(properties);
+ UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder =
+ UrlConnectionHttpClient.builder();
+ UrlConnectionHttpClient.Builder spyUrlConnectionHttpClientBuilder =
+ Mockito.spy(urlConnectionHttpClientBuilder);
+
+ urlConnectionHttpClientConfigurations.configureUrlConnectionHttpClientBuilder(
+ spyUrlConnectionHttpClientBuilder);
+
+ Mockito.verify(spyUrlConnectionHttpClientBuilder)
+ .proxyConfiguration(
+ Mockito.any(software.amazon.awssdk.http.urlconnection.ProxyConfiguration.class));
+ }
Review Comment:
Thanks @flyrain, updated per your suggestion. I collapsed the proxy-flag tests into two parameterized tests: `testApacheProxyFlagTriggersProxyConfig` and `testUrlConnectionProxyFlagTriggersProxyConfig`, covering:
- `PROXY_USE_SYSTEM_PROPERTY_VALUES`
- `PROXY_USE_ENVIRONMENT_VARIABLE_VALUES`
Both tests verify that setting either flag triggers proxy configuration on the corresponding HTTP client builder.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#discussion_r3076117355
##########
aws/src/main/java/org/apache/iceberg/aws/UrlConnectionHttpClientConfigurations.java:
##########
@@ -69,9 +77,26 @@ void configureUrlConnectionHttpClientBuilder(
urlConnectionHttpClientBuilder.socketTimeout(
Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
}
- if (proxyEndpoint != null) {
- urlConnectionHttpClientBuilder.proxyConfiguration(
- ProxyConfiguration.builder().endpoint(URI.create(proxyEndpoint)).build());
+ configureProxy(urlConnectionHttpClientBuilder);
+ }
+
+ private void configureProxy(UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder) {
+ if (proxyEndpoint != null
+ || proxyUseSystemPropertyValues != null
+ || proxyUseEnvironmentVariableValues != null) {
+ ProxyConfiguration.Builder proxyBuilder = ProxyConfiguration.builder();
+
+ if (proxyEndpoint != null) {
+ proxyBuilder.endpoint(URI.create(proxyEndpoint));
+ }
+ if (proxyUseSystemPropertyValues != null) {
+ proxyBuilder.useSystemPropertyValues(proxyUseSystemPropertyValues);
+ }
+ if (proxyUseEnvironmentVariableValues != null) {
+ proxyBuilder.useEnvironmentVariablesValues(proxyUseEnvironmentVariableValues);
Review Comment:
Just a nit. I see that `UrlConnectionHttpClient.Builder` has `proxyBuilder.useEnvironmentVariablesValues()` with plural for variables. To be consistent, can we use the same for the local variable?
`proxyUseEnvironmentVariableValues` -> `proxyUseEnvironmentVariablesValues`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Add proxy system property and environment variable configuration for HTTP clients [iceberg]
Posted by "lunar-shadow (via GitHub)" <gi...@apache.org>.
lunar-shadow commented on code in PR #15506:
URL: https://github.com/apache/iceberg/pull/15506#discussion_r3078106618
##########
aws/src/main/java/org/apache/iceberg/aws/UrlConnectionHttpClientConfigurations.java:
##########
@@ -69,9 +77,26 @@ void configureUrlConnectionHttpClientBuilder(
urlConnectionHttpClientBuilder.socketTimeout(
Duration.ofMillis(httpClientUrlConnectionSocketTimeoutMs));
}
- if (proxyEndpoint != null) {
- urlConnectionHttpClientBuilder.proxyConfiguration(
- ProxyConfiguration.builder().endpoint(URI.create(proxyEndpoint)).build());
+ configureProxy(urlConnectionHttpClientBuilder);
+ }
+
+ private void configureProxy(UrlConnectionHttpClient.Builder urlConnectionHttpClientBuilder) {
+ if (proxyEndpoint != null
+ || proxyUseSystemPropertyValues != null
+ || proxyUseEnvironmentVariableValues != null) {
+ ProxyConfiguration.Builder proxyBuilder = ProxyConfiguration.builder();
+
+ if (proxyEndpoint != null) {
+ proxyBuilder.endpoint(URI.create(proxyEndpoint));
+ }
+ if (proxyUseSystemPropertyValues != null) {
+ proxyBuilder.useSystemPropertyValues(proxyUseSystemPropertyValues);
+ }
+ if (proxyUseEnvironmentVariableValues != null) {
+ proxyBuilder.useEnvironmentVariablesValues(proxyUseEnvironmentVariableValues);
Review Comment:
Thanks for the review @anuragmantri. I already looked into this and it turns out the AWS SDK itself is inconsistent between its HTTP clients:
- URL Connection client uses plural [useEnvironmentVariablesValues() ](https://github.com/aws/aws-sdk-java-v2/blob/d4ceee0fa568bb0567862bc2ff926127a52164f7/http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/ProxyConfiguration.java#L254)
- Apache client uses singular[ useEnvironmentVariableValues() ](https://github.com/aws/aws-sdk-java-v2/blob/d4ceee0fa568bb0567862bc2ff926127a52164f7/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ProxyConfiguration.java#L301)
Since we use the same property for both clients, I went with singular to stay consistent with our own property constant (PROXY_USE_ENVIRONMENT_VARIABLE_VALUES) and config key (http-client.proxy-use-environment-variable-values).
Going plural would match the URL Connection side but mismatch the Apache side and our own naming.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org