You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "munendrasn (via GitHub)" <gi...@apache.org> on 2025/01/16 06:28:06 UTC
[PR] Core,REST: extend httpClient builder to support tls factory [iceberg]
munendrasn opened a new pull request, #11979:
URL: https://github.com/apache/iceberg/pull/11979
Adds support to specify custom TlsSocketStrategy for HttpClient. This is useful in case of establishing the mTLS connection
Possible alternative to https://github.com/apache/iceberg/pull/11548
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2978535724
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11979: Core,REST: extend httpClient builder to support tls factory
URL: https://github.com/apache/iceberg/pull/11979
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1957818213
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -374,13 +375,17 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
- static HttpClientConnectionManager configureConnectionManager(Map<String, String> properties) {
+ static HttpClientConnectionManager configureConnectionManager(
+ Map<String, String> properties, TlsSocketStrategy tlsSocketStrategy) {
PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
PoolingHttpClientConnectionManagerBuilder.create();
ConnectionConfig connectionConfig = configureConnectionConfig(properties);
if (connectionConfig != null) {
connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
}
+ if (tlsSocketStrategy != null) {
+ connectionManagerBuilder.setTlsSocketStrategy(tlsSocketStrategy);
Review Comment:
typically stuff like this is all configured via the `properties` map. Otherwise there isn't a way to configure this e.g. when using the REST catalog with Spark
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "mrcnc (via GitHub)" <gi...@apache.org>.
mrcnc commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2860319177
> This is useful in case of establishing the mTLS connection
Is this not already possible to configure via system properties (e.g. `javax.net.ssl.keyStore*`)? I'm wondering if there are use cases where this approach isn't sufficient.
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1971573647
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -480,6 +486,11 @@ public Builder withObjectMapper(ObjectMapper objectMapper) {
return this;
}
+ public Builder withTlsSocketStrategy(TlsSocketStrategy socketStrategy) {
+ this.tlsSocketStrategy = socketStrategy;
Review Comment:
I have included simple tests, please 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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1973048206
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -480,6 +486,13 @@ public Builder withObjectMapper(ObjectMapper objectMapper) {
return this;
}
+ public Builder withTlsSocketStrategy(TlsSocketStrategy socketStrategy) {
+ Preconditions.checkNotNull(
+ socketStrategy, "Invalid tls socket strategy for http client proxy: null");
Review Comment:
As `tlsSocketStrategy` can be null when not explicitly set, validation can't be added to `build`. Hence, ensuring that if socketStrategy is explicitly set, it should be non-null
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2954354849
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2661159077
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1973048206
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -480,6 +486,13 @@ public Builder withObjectMapper(ObjectMapper objectMapper) {
return this;
}
+ public Builder withTlsSocketStrategy(TlsSocketStrategy socketStrategy) {
+ Preconditions.checkNotNull(
+ socketStrategy, "Invalid tls socket strategy for http client proxy: null");
Review Comment:
As `tlsSocketStrategy` can be null when not explicitly set, validation can't be added to `build`. Hence, ensuring that if socketStrategy is explicitly set, it should be non-null
This similar to handling other configuration on the 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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2852817058
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2866670146
One use case where this can useful, being able to connect to multiple external RESTCatalog from single process.
We fetch the latest metadata pointer lazily from these catalogs. Some of these catalogs require mTLS connection, while others doesn't.
In such cases, using java system properties to specify the keystore/truststore is not an option - we have faced errors when system properties and every Catalog instance tries to create mTLS connection.
This particular change would help in those cases to create Catalog instance for each such sources with different config
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1971872975
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -480,6 +486,13 @@ public Builder withObjectMapper(ObjectMapper objectMapper) {
return this;
}
+ public Builder withTlsSocketStrategy(TlsSocketStrategy socketStrategy) {
+ Preconditions.checkNotNull(
+ socketStrategy, "Invalid tls socket strategy for http client proxy: null");
Review Comment:
that sort of validation is typically better handled inside `build()`(same as all other validations)
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2780850140
I welcome the addition of TLS factory support. It could be beneficial also when contacting an IDP over TLS.
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1958273011
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -374,13 +375,17 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
- static HttpClientConnectionManager configureConnectionManager(Map<String, String> properties) {
+ static HttpClientConnectionManager configureConnectionManager(
+ Map<String, String> properties, TlsSocketStrategy tlsSocketStrategy) {
PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
PoolingHttpClientConnectionManagerBuilder.create();
ConnectionConfig connectionConfig = configureConnectionConfig(properties);
if (connectionConfig != null) {
connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
}
+ if (tlsSocketStrategy != null) {
+ connectionManagerBuilder.setTlsSocketStrategy(tlsSocketStrategy);
Review Comment:
@nastra
The idea was similar to how [httpProxy](https://github.com/apache/iceberg/blob/f9b9621e77ef44d4e5fd72bf6014e3037514ba98/core/src/main/java/org/apache/iceberg/rest/HTTPClient.java#L455-L461) is supported. HttpProxy can be configured via system properties (note: it is not configurable via properties map) or via explicitly specifying host and port where applicable (might not be possible in case of spark). Same in case of `ProxyCredentialProvider` too
Similarly, here, I'm adding an additional way to configure tlsSocketStrategy where more granular control is required like custom keystore to construct sslContext and so on.
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1958432024
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -480,6 +486,11 @@ public Builder withObjectMapper(ObjectMapper objectMapper) {
return this;
}
+ public Builder withTlsSocketStrategy(TlsSocketStrategy socketStrategy) {
+ this.tlsSocketStrategy = socketStrategy;
Review Comment:
we should make sure in the build method that this isn't null. Also we should have some tests when this is null or configured to a real value
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2663464772
@danielcweeks @amogh-jahagirdar can you guys review this as well please?
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2662294516
@nastra could you please review this?
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#discussion_r1958434510
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -374,13 +375,17 @@ static HttpRequestInterceptor loadInterceptorDynamically(
return instance;
}
- static HttpClientConnectionManager configureConnectionManager(Map<String, String> properties) {
+ static HttpClientConnectionManager configureConnectionManager(
+ Map<String, String> properties, TlsSocketStrategy tlsSocketStrategy) {
PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder =
PoolingHttpClientConnectionManagerBuilder.create();
ConnectionConfig connectionConfig = configureConnectionConfig(properties);
if (connectionConfig != null) {
connectionManagerBuilder.setDefaultConnectionConfig(connectionConfig);
}
+ if (tlsSocketStrategy != null) {
+ connectionManagerBuilder.setTlsSocketStrategy(tlsSocketStrategy);
Review Comment:
ok this is probably fine to just be configured through the builder. I'm ok with this change but let's see what other reviewers think of this
--
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2764304534
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] Core,REST: extend httpClient builder to support tls factory [iceberg]
Posted by "munendrasn (via GitHub)" <gi...@apache.org>.
munendrasn commented on PR #11979:
URL: https://github.com/apache/iceberg/pull/11979#issuecomment-2765267128
@nastra @danielcweeks @amogh-jahagirdar
is community still interested in this change to be merged to upstream?
--
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