You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/01/17 16:43:54 UTC
[PR] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
adutra opened a new pull request, #11992:
URL: https://github.com/apache/iceberg/pull/11992
4th PR for the Auth Manager API. Previous ones:
* #11844
* #11809
* #11769
This PR introduces the required changes to `RESTClient` and `HTTPClient`. It also introduces a `BaseHTTPClient` abstract class to facilitate the creation and execution of `HTTPRequest`s in a consistent way.
The biggest change in actually in `TestRESTCatalog`, because almost every `Mockito.verify()` call needs to be adapted.
The AuthManager API is still "unplugged" at this point.
\cc @nastra @danielcweeks
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926123223
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -214,92 +225,63 @@ private static void throwFailure(
throw new RESTException("Unhandled error: %s", errorResponse);
}
- private URI buildUri(String path, Map<String, String> params) {
- // if full path is provided, use the input path as path
- if (path.startsWith("/")) {
- throw new RESTException(
- "Received a malformed path for a REST request: %s. Paths should not start with /", path);
- }
- String fullPath =
- (path.startsWith("https://") || path.startsWith("http://"))
- ? path
- : String.format("%s/%s", uri, path);
- try {
- URIBuilder builder = new URIBuilder(fullPath);
- if (params != null) {
- params.forEach(builder::addParameter);
- }
- return builder.build();
- } catch (URISyntaxException e) {
- throw new RESTException(
- "Failed to create request URI from base %s, params %s", fullPath, params);
- }
- }
-
- /**
- * Method to execute an HTTP request and process the corresponding response.
- *
- * @param method - HTTP method, such as GET, POST, HEAD, etc.
- * @param queryParams - A map of query parameters
- * @param path - URI to send the request to
- * @param requestBody - Content to place in the request body
- * @param responseType - Class of the Response type. Needs to have serializer registered with
- * ObjectMapper
- * @param errorHandler - Error handler delegated for HTTP responses which handles server error
- * responses
- * @param <T> - Class type of the response for deserialization. Must be registered with the
- * ObjectMapper.
- * @return The response entity, parsed and converted to its type T
- */
- private <T> T execute(
- Method method,
+ @Override
+ protected HTTPRequest buildRequest(
+ HTTPMethod method,
String path,
Map<String, String> queryParams,
- Object requestBody,
- Class<T> responseType,
Map<String, String> headers,
- Consumer<ErrorResponse> errorHandler) {
- return execute(
- method, path, queryParams, requestBody, responseType, headers, errorHandler, h -> {});
+ Object body) {
+
+ ImmutableHTTPRequest.Builder builder =
+ ImmutableHTTPRequest.builder()
+ .baseUri(baseUri)
+ .mapper(mapper)
+ .method(method)
+ .path(path)
Review Comment:
Never mind, it looks like that's being handled by value.check
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#issuecomment-2634301895
I see @nastra approved. I did a new pass and it looks good to me. @danielcweeks wdyt ?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1945236123
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -75,10 +76,13 @@ private RESTClient httpClient() {
if (null == client) {
synchronized (this) {
if (null == client) {
+ DefaultAuthSession authSession =
+ DefaultAuthSession.of(
+ HTTPHeaders.of(OAuth2Util.authHeaders(properties.get(OAuth2Properties.TOKEN))));
Review Comment:
Yes. I don't mind looking into it once we have the whole API merged 👍
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #11992:
URL: https://github.com/apache/iceberg/pull/11992
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922258470
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1324,89 +1222,67 @@ public void testTableAuth(
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/config", catalogHeaders),
eq(ConfigResponse.class),
- eq(catalogHeaders),
+ any(),
any());
// session client credentials flow
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- any(),
+ reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
eq(OAuthTokenResponse.class),
- eq(catalogHeaders),
+ any(),
any());
// create table request
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq("v1/namespaces/ns/tables"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.POST, "v1/namespaces/ns/tables", expectedContextHeaders),
eq(LoadTableResponse.class),
- eq(expectedContextHeaders),
+ any(),
any());
// if the table returned a bearer token or a credential, there will be no token request
if (!tableConfig.containsKey("token") && !tableConfig.containsKey("credential")) {
// token exchange to get a table token
Mockito.verify(adapter, times(1))
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- any(),
+ reqMatcher(HTTPMethod.POST, oauth2ServerUri, expectedContextHeaders),
eq(OAuthTokenResponse.class),
- eq(expectedContextHeaders),
+ any(),
any());
}
if (expectedContextHeaders.equals(expectedTableHeaders)) {
// load table from catalog + refresh loaded table
Mockito.verify(adapter, times(2))
.execute(
- eq(HTTPMethod.GET),
- eq("v1/namespaces/ns/tables/table"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/namespaces/ns/tables/table", expectedTableHeaders),
eq(LoadTableResponse.class),
- eq(expectedTableHeaders),
+ any(),
any());
} else {
// load table from catalog
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/namespaces/ns/tables/table"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/namespaces/ns/tables/table", expectedContextHeaders),
eq(LoadTableResponse.class),
- eq(expectedContextHeaders),
+ any(),
any());
// refresh loaded table
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/namespaces/ns/tables/table"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/namespaces/ns/tables/table", expectedTableHeaders),
eq(LoadTableResponse.class),
- eq(expectedTableHeaders),
+ any(),
any());
}
}
@ParameterizedTest
@ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
public void testCatalogTokenRefresh(String oauth2ServerUri) {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
same as above
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942704258
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,6 +192,7 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)
Review Comment:
I will however move this call to the `authSession()` method and add a comment to explain why an `EMPTY` session is needed in this specific case.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932459105
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -84,6 +85,7 @@ private RESTClient httpClient() {
private LoadCredentialsResponse fetchCredentials() {
return httpClient()
+ .withAuthSession(AuthSession.EMPTY)
Review Comment:
I just took a quick look because I'm also working on the HttpClient code and was curious about this PR.
This might have been commented, but wouldn't it be possible to pass the AuthSession to the HttpClient during creation, maybe through a Builder? Then we could avoid adding these 'withAuthSession' calls when we use the httpClient.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#issuecomment-2635399088
I feel like there may have been a small understanding with how we handle the initial authSession. We don't want to default it to empty otherwise if someone adds a request like `client.get(...)` it will proceed without an auth session as opposed to getting an error because they didn't use the proper `client.withAuthSession(session).get(...)`.
It's better to fail early here so that we know the issue was a lack of auth info as part of the request because we'll only get a 401/403 back without knowing that we just failed to apply auth.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926105323
##########
core/src/main/java/org/apache/iceberg/rest/BaseHTTPClient.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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.iceberg.rest;
+
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.auth.AuthSession;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A base class for {@link RESTClient} implementations.
+ *
+ * <p>All methods in {@link RESTClient} are implemented in the same way: first, an {@link
+ * HTTPRequest} is {@linkplain #buildRequest(HTTPMethod, String, Map, Map, Object) built from the
+ * method arguments}, then {@linkplain #execute(HTTPRequest, Class, Consumer, Consumer) executed}.
+ *
+ * <p>This allows subclasses to provide a consistent way to execute all requests, regardless of the
+ * method or arguments.
+ */
+public abstract class BaseHTTPClient implements RESTClient {
+
+ @Override
+ public abstract RESTClient withAuthSession(AuthSession session);
+
+ @Override
+ public void head(
+ String path, Supplier<Map<String, String>> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers.get(), null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers, null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public <T extends RESTResponse> T delete(
Review Comment:
Why are we implementing all of the interface methods if there are defaults that redirect to another implementation?. It seems like there may be a case or two where the defaults in the interface need to be updated, but a lot of these implementations don't seem to be necessary. (Maybe I'm missing something?)
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926121826
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -214,92 +225,63 @@ private static void throwFailure(
throw new RESTException("Unhandled error: %s", errorResponse);
}
- private URI buildUri(String path, Map<String, String> params) {
- // if full path is provided, use the input path as path
- if (path.startsWith("/")) {
- throw new RESTException(
- "Received a malformed path for a REST request: %s. Paths should not start with /", path);
- }
- String fullPath =
- (path.startsWith("https://") || path.startsWith("http://"))
- ? path
- : String.format("%s/%s", uri, path);
- try {
- URIBuilder builder = new URIBuilder(fullPath);
- if (params != null) {
- params.forEach(builder::addParameter);
- }
- return builder.build();
- } catch (URISyntaxException e) {
- throw new RESTException(
- "Failed to create request URI from base %s, params %s", fullPath, params);
- }
- }
-
- /**
- * Method to execute an HTTP request and process the corresponding response.
- *
- * @param method - HTTP method, such as GET, POST, HEAD, etc.
- * @param queryParams - A map of query parameters
- * @param path - URI to send the request to
- * @param requestBody - Content to place in the request body
- * @param responseType - Class of the Response type. Needs to have serializer registered with
- * ObjectMapper
- * @param errorHandler - Error handler delegated for HTTP responses which handles server error
- * responses
- * @param <T> - Class type of the response for deserialization. Must be registered with the
- * ObjectMapper.
- * @return The response entity, parsed and converted to its type T
- */
- private <T> T execute(
- Method method,
+ @Override
+ protected HTTPRequest buildRequest(
+ HTTPMethod method,
String path,
Map<String, String> queryParams,
- Object requestBody,
- Class<T> responseType,
Map<String, String> headers,
- Consumer<ErrorResponse> errorHandler) {
- return execute(
- method, path, queryParams, requestBody, responseType, headers, errorHandler, h -> {});
+ Object body) {
+
+ ImmutableHTTPRequest.Builder builder =
+ ImmutableHTTPRequest.builder()
+ .baseUri(baseUri)
+ .mapper(mapper)
+ .method(method)
+ .path(path)
Review Comment:
It appears that we may have dropped some of the path validation (e.g. asserting that paths don't start with a slash)
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1930359628
##########
core/src/main/java/org/apache/iceberg/rest/BaseHTTPClient.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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.iceberg.rest;
+
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.auth.AuthSession;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A base class for {@link RESTClient} implementations.
+ *
+ * <p>All methods in {@link RESTClient} are implemented in the same way: first, an {@link
+ * HTTPRequest} is {@linkplain #buildRequest(HTTPMethod, String, Map, Map, Object) built from the
+ * method arguments}, then {@linkplain #execute(HTTPRequest, Class, Consumer, Consumer) executed}.
+ *
+ * <p>This allows subclasses to provide a consistent way to execute all requests, regardless of the
+ * method or arguments.
+ */
+public abstract class BaseHTTPClient implements RESTClient {
+
+ @Override
+ public abstract RESTClient withAuthSession(AuthSession session);
+
+ @Override
+ public void head(
+ String path, Supplier<Map<String, String>> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers.get(), null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers, null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public <T extends RESTResponse> T delete(
Review Comment:
I believe what @danielcweeks means here is that it should be enough to override the non-default methods to achieve the same result. All the other methods will delegate to these non-default methods. The below diff should be enough in this case:
```
@Override
public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers, null);
execute(request, null, errorHandler, h -> {});
}
@Override
public <T extends RESTResponse> T delete(
String path,
Class<T> responseType,
Map<String, String> headers,
Consumer<ErrorResponse> errorHandler) {
HTTPRequest request = buildRequest(HTTPMethod.DELETE, path, null, headers, null);
return execute(request, responseType, errorHandler, h -> {});
}
@Override
public <T extends RESTResponse> T get(
String path,
Map<String, String> queryParams,
Class<T> responseType,
Map<String, String> headers,
Consumer<ErrorResponse> errorHandler) {
HTTPRequest request = buildRequest(HTTPMethod.GET, path, queryParams, headers, null);
return execute(request, responseType, errorHandler, h -> {});
}
@Override
public <T extends RESTResponse> T post(
String path,
RESTRequest body,
Class<T> responseType,
Map<String, String> headers,
Consumer<ErrorResponse> errorHandler) {
HTTPRequest request = buildRequest(HTTPMethod.POST, path, null, headers, body);
return execute(request, responseType, errorHandler, h -> {});
}
@Override
public <T extends RESTResponse> T postForm(
String path,
Map<String, String> formData,
Class<T> responseType,
Map<String, String> headers,
Consumer<ErrorResponse> errorHandler) {
HTTPRequest request = buildRequest(HTTPMethod.POST, path, null, headers, formData);
return execute(request, responseType, errorHandler, h -> {});
}
```
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942060653
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,6 +192,7 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)
Review Comment:
Why are we setting this to EMPTY? In order to sign we need an auth session. Wouldn't this result in no headers/sigs being produced if a call was made without explicitly setting the auth session? We want enforce that an auth session is used, but this results in falling back to nothing.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1930471448
##########
core/src/main/java/org/apache/iceberg/rest/BaseHTTPClient.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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.iceberg.rest;
+
+import java.util.Map;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.apache.iceberg.rest.HTTPRequest.HTTPMethod;
+import org.apache.iceberg.rest.auth.AuthSession;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+
+/**
+ * A base class for {@link RESTClient} implementations.
+ *
+ * <p>All methods in {@link RESTClient} are implemented in the same way: first, an {@link
+ * HTTPRequest} is {@linkplain #buildRequest(HTTPMethod, String, Map, Map, Object) built from the
+ * method arguments}, then {@linkplain #execute(HTTPRequest, Class, Consumer, Consumer) executed}.
+ *
+ * <p>This allows subclasses to provide a consistent way to execute all requests, regardless of the
+ * method or arguments.
+ */
+public abstract class BaseHTTPClient implements RESTClient {
+
+ @Override
+ public abstract RESTClient withAuthSession(AuthSession session);
+
+ @Override
+ public void head(
+ String path, Supplier<Map<String, String>> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers.get(), null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
+ HTTPRequest request = buildRequest(HTTPMethod.HEAD, path, null, headers, null);
+ execute(request, null, errorHandler, h -> {});
+ }
+
+ @Override
+ public <T extends RESTResponse> T delete(
Review Comment:
Okay I did as suggested, but there are 2 other methods that need to be overridden because the default methods throw `UnsupportedOperationException`.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1930517106
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -88,33 +84,30 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting static final String REST_SOCKET_TIMEOUT_MS = "rest.client.socket-timeout-ms";
- private final String uri;
+ private final URI baseUri;
private final CloseableHttpClient httpClient;
+ private final Map<String, String> baseHeaders;
private final ObjectMapper mapper;
+ private final AuthSession authSession;
private HTTPClient(
- String uri,
+ URI baseUri,
HttpHost proxy,
CredentialsProvider proxyCredsProvider,
Map<String, String> baseHeaders,
ObjectMapper objectMapper,
HttpRequestInterceptor requestInterceptor,
Map<String, String> properties,
HttpClientConnectionManager connectionManager) {
- this.uri = uri;
+ this.baseUri = baseUri;
+ this.baseHeaders = baseHeaders;
this.mapper = objectMapper;
+ this.authSession = AuthSession.EMPTY;
Review Comment:
I think that makes sense to enforce/validate that a proper auth session is being used
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942701609
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,6 +192,7 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)
Review Comment:
I agree with your suggestions for other components, but here I disagree: the HTTP client _must_ be configured with an empty auth session because it will be used for token refreshes. If it contains, say, token `t1`, and token `t1` gets refreshed and exchanged for `t2`, there would be a clash between the token from the HTTP client (`t1`) and the one from the refreshed auth session (`t2`).
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942704258
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,6 +192,7 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)
Review Comment:
I will however move this call to the `authSession()` and add a comment to explain why an `EMPTY` session is needed in this specific case.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932536821
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -84,6 +85,7 @@ private RESTClient httpClient() {
private LoadCredentialsResponse fetchCredentials() {
return httpClient()
+ .withAuthSession(AuthSession.EMPTY)
Review Comment:
Different sessions are used depending on the context, so it can't be restricted to a single one.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932146669
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -206,21 +206,24 @@ private AuthSession authSession() {
return authSessionCache()
.get(
token,
- id ->
- AuthSession.fromAccessToken(
- httpClient(),
- tokenRefreshExecutor(),
- token,
- expiresAtMillis(properties()),
- new AuthSession(
- ImmutableMap.of(),
- AuthConfig.builder()
- .token(token)
- .credential(credential())
- .scope(SCOPE)
- .oauth2ServerUri(oauth2ServerUri())
- .optionalOAuthParams(optionalOAuthParams())
- .build())));
+ id -> {
+ RESTClient client =
+ httpClient().withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
Review Comment:
Indeed. The child client is lightweight though, but it's probably better to move this call to `httpClient()`.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1931252722
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -88,33 +84,30 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting static final String REST_SOCKET_TIMEOUT_MS = "rest.client.socket-timeout-ms";
- private final String uri;
+ private final URI baseUri;
private final CloseableHttpClient httpClient;
+ private final Map<String, String> baseHeaders;
private final ObjectMapper mapper;
+ private final AuthSession authSession;
private HTTPClient(
- String uri,
+ URI baseUri,
HttpHost proxy,
CredentialsProvider proxyCredsProvider,
Map<String, String> baseHeaders,
ObjectMapper objectMapper,
HttpRequestInterceptor requestInterceptor,
Map<String, String> properties,
HttpClientConnectionManager connectionManager) {
- this.uri = uri;
+ this.baseUri = baseUri;
+ this.baseHeaders = baseHeaders;
this.mapper = objectMapper;
+ this.authSession = AuthSession.EMPTY;
Review Comment:
I understand that may seem like the case here, but I think we're just saying that you create it initially as set to `null`, but validate when `execute` is called that it is not null. You can still create the HTTPClient, but you can't make a request unless there is an active auth session. This just prevents unintentional use of an "empty" auth session (it needs to be intentional).
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1933705991
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,7 +192,8 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
- .build();
+ .build()
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
Review Comment:
Builder method added.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926099736
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -214,92 +225,63 @@ private static void throwFailure(
throw new RESTException("Unhandled error: %s", errorResponse);
}
- private URI buildUri(String path, Map<String, String> params) {
- // if full path is provided, use the input path as path
- if (path.startsWith("/")) {
- throw new RESTException(
- "Received a malformed path for a REST request: %s. Paths should not start with /", path);
- }
- String fullPath =
- (path.startsWith("https://") || path.startsWith("http://"))
- ? path
- : String.format("%s/%s", uri, path);
- try {
- URIBuilder builder = new URIBuilder(fullPath);
- if (params != null) {
- params.forEach(builder::addParameter);
- }
- return builder.build();
- } catch (URISyntaxException e) {
- throw new RESTException(
- "Failed to create request URI from base %s, params %s", fullPath, params);
- }
- }
-
- /**
- * Method to execute an HTTP request and process the corresponding response.
- *
- * @param method - HTTP method, such as GET, POST, HEAD, etc.
- * @param queryParams - A map of query parameters
- * @param path - URI to send the request to
- * @param requestBody - Content to place in the request body
- * @param responseType - Class of the Response type. Needs to have serializer registered with
- * ObjectMapper
- * @param errorHandler - Error handler delegated for HTTP responses which handles server error
- * responses
- * @param <T> - Class type of the response for deserialization. Must be registered with the
- * ObjectMapper.
- * @return The response entity, parsed and converted to its type T
- */
- private <T> T execute(
- Method method,
+ @Override
+ protected HTTPRequest buildRequest(
+ HTTPMethod method,
String path,
Map<String, String> queryParams,
- Object requestBody,
- Class<T> responseType,
Map<String, String> headers,
- Consumer<ErrorResponse> errorHandler) {
- return execute(
- method, path, queryParams, requestBody, responseType, headers, errorHandler, h -> {});
+ Object body) {
+
+ ImmutableHTTPRequest.Builder builder =
+ ImmutableHTTPRequest.builder()
+ .baseUri(baseUri)
+ .mapper(mapper)
+ .method(method)
+ .path(path)
+ .body(body)
+ .queryParameters(queryParams == null ? Map.of() : queryParams);
+
+ Map<String, String> allHeaders = Maps.newLinkedHashMap();
+ if (headers != null) {
+ allHeaders.putAll(headers);
+ }
+
+ allHeaders.putIfAbsent(HttpHeaders.ACCEPT, ContentType.APPLICATION_JSON.getMimeType());
+
+ // Many systems require that content type is set regardless and will fail,
+ // even on an empty bodied request.
+ // Encode maps as form data (application/x-www-form-urlencoded),
+ // and other requests are assumed to contain JSON bodies (application/json).
+ ContentType mimeType =
+ body instanceof Map
+ ? ContentType.APPLICATION_FORM_URLENCODED
+ : ContentType.APPLICATION_JSON;
+ allHeaders.putIfAbsent(HttpHeaders.CONTENT_TYPE, mimeType.getMimeType());
+
+ // Apply base headers now to mimic the behavior of
+ // org.apache.hc.client5.http.protocol.RequestDefaultHeaders
+ // We want these headers applied *before* the AuthSession authenticates the request.
+ if (baseHeaders != null) {
Review Comment:
Doesn't this mean that `baseHeaders` will override the provided specific headers, the mime type headers and accept headers? It feels like this should go first.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1930539690
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -88,33 +84,30 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting static final String REST_SOCKET_TIMEOUT_MS = "rest.client.socket-timeout-ms";
- private final String uri;
+ private final URI baseUri;
private final CloseableHttpClient httpClient;
+ private final Map<String, String> baseHeaders;
private final ObjectMapper mapper;
+ private final AuthSession authSession;
private HTTPClient(
- String uri,
+ URI baseUri,
HttpHost proxy,
CredentialsProvider proxyCredsProvider,
Map<String, String> baseHeaders,
ObjectMapper objectMapper,
HttpRequestInterceptor requestInterceptor,
Map<String, String> properties,
HttpClientConnectionManager connectionManager) {
- this.uri = uri;
+ this.baseUri = baseUri;
+ this.baseHeaders = baseHeaders;
this.mapper = objectMapper;
+ this.authSession = AuthSession.EMPTY;
Review Comment:
We are going to run into a chicken and egg problem: to create an auth session I need an HTTP client; but now, to create an HTTP client, I would need an auth session.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1931940876
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -88,33 +84,30 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting static final String REST_SOCKET_TIMEOUT_MS = "rest.client.socket-timeout-ms";
- private final String uri;
+ private final URI baseUri;
private final CloseableHttpClient httpClient;
+ private final Map<String, String> baseHeaders;
private final ObjectMapper mapper;
+ private final AuthSession authSession;
private HTTPClient(
- String uri,
+ URI baseUri,
HttpHost proxy,
CredentialsProvider proxyCredsProvider,
Map<String, String> baseHeaders,
ObjectMapper objectMapper,
HttpRequestInterceptor requestInterceptor,
Map<String, String> properties,
HttpClientConnectionManager connectionManager) {
- this.uri = uri;
+ this.baseUri = baseUri;
+ this.baseHeaders = baseHeaders;
this.mapper = objectMapper;
+ this.authSession = AuthSession.EMPTY;
Review Comment:
Ah, now I get it. Let me try your suggestion.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932371958
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -338,6 +339,7 @@ public SdkHttpFullRequest sign(
Consumer<Map<String, String>> responseHeadersConsumer = responseHeaders::putAll;
S3SignResponse s3SignResponse =
httpClient()
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)
Review Comment:
I believe this change can be reverted now that the session is set in `httpClient()`
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922263736
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1767,29 +1635,23 @@ public void testCatalogValidBearerTokenIsNotRefreshed() {
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/config", catalogHeaders),
eq(ConfigResponse.class),
- eq(catalogHeaders),
+ any(),
any());
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.HEAD),
- eq("v1/namespaces/ns/tables/table"),
- any(),
+ reqMatcher(
+ HTTPMethod.HEAD, "v1/namespaces/ns/tables/table", OAuth2Util.authHeaders(token)),
any(),
any(),
- eq(OAuth2Util.authHeaders(token)),
any());
}
@ParameterizedTest
@ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
public void testCatalogTokenRefreshFailsAndUsesCredentialForRefresh(String oauth2ServerUri) {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
here too
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922262816
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1653,7 +1517,6 @@ public void testCatalogExpiredBearerTokenIsRefreshedWithCredential(String oauth2
// expires at epoch second = 1
String token =
"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.gQADTbdEv-rpDWKSkGLbmafyB5UUjTdm9B_1izpuZ6E";
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
same here
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#issuecomment-2624547764
@danielcweeks @nastra is there anything else I need to change?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1933758553
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -214,92 +227,64 @@ private static void throwFailure(
throw new RESTException("Unhandled error: %s", errorResponse);
}
- private URI buildUri(String path, Map<String, String> params) {
- // if full path is provided, use the input path as path
- if (path.startsWith("/")) {
- throw new RESTException(
- "Received a malformed path for a REST request: %s. Paths should not start with /", path);
- }
- String fullPath =
- (path.startsWith("https://") || path.startsWith("http://"))
- ? path
- : String.format("%s/%s", uri, path);
- try {
- URIBuilder builder = new URIBuilder(fullPath);
- if (params != null) {
- params.forEach(builder::addParameter);
- }
- return builder.build();
- } catch (URISyntaxException e) {
- throw new RESTException(
- "Failed to create request URI from base %s, params %s", fullPath, params);
- }
- }
-
- /**
- * Method to execute an HTTP request and process the corresponding response.
- *
- * @param method - HTTP method, such as GET, POST, HEAD, etc.
- * @param queryParams - A map of query parameters
- * @param path - URI to send the request to
- * @param requestBody - Content to place in the request body
- * @param responseType - Class of the Response type. Needs to have serializer registered with
- * ObjectMapper
- * @param errorHandler - Error handler delegated for HTTP responses which handles server error
- * responses
- * @param <T> - Class type of the response for deserialization. Must be registered with the
- * ObjectMapper.
- * @return The response entity, parsed and converted to its type T
- */
- private <T> T execute(
- Method method,
+ @Override
+ protected HTTPRequest buildRequest(
+ HTTPMethod method,
String path,
Map<String, String> queryParams,
- Object requestBody,
- Class<T> responseType,
Map<String, String> headers,
- Consumer<ErrorResponse> errorHandler) {
- return execute(
- method, path, queryParams, requestBody, responseType, headers, errorHandler, h -> {});
+ Object body) {
+
+ ImmutableHTTPRequest.Builder builder =
+ ImmutableHTTPRequest.builder()
+ .baseUri(baseUri)
+ .mapper(mapper)
+ .method(method)
+ .path(path)
+ .body(body)
+ .queryParameters(queryParams == null ? Map.of() : queryParams);
+
+ Map<String, String> allHeaders = Maps.newLinkedHashMap();
+ if (headers != null) {
+ allHeaders.putAll(headers);
+ }
+
+ allHeaders.putIfAbsent(HttpHeaders.ACCEPT, ContentType.APPLICATION_JSON.getMimeType());
+
+ // Many systems require that content type is set regardless and will fail,
+ // even on an empty bodied request.
+ // Encode maps as form data (application/x-www-form-urlencoded),
+ // and other requests are assumed to contain JSON bodies (application/json).
+ ContentType mimeType =
+ body instanceof Map
+ ? ContentType.APPLICATION_FORM_URLENCODED
+ : ContentType.APPLICATION_JSON;
+ allHeaders.putIfAbsent(HttpHeaders.CONTENT_TYPE, mimeType.getMimeType());
+
+ // Apply base headers now to mimic the behavior of
+ // org.apache.hc.client5.http.protocol.RequestDefaultHeaders
+ // We want these headers applied *before* the AuthSession authenticates the request.
+ if (baseHeaders != null) {
+ baseHeaders.forEach(allHeaders::putIfAbsent);
+ }
+
+ Preconditions.checkState(authSession != null, "no AuthSession available");
Review Comment:
```suggestion
Preconditions.checkState(authSession != null, "Invalid auth session: 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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942062420
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -74,7 +75,11 @@ private RESTClient httpClient() {
if (null == client) {
synchronized (this) {
if (null == client) {
- client = HTTPClient.builder(properties).uri(properties.get(URI)).build();
+ client =
+ HTTPClient.builder(properties)
+ .uri(properties.get(URI))
+ .withAuthSession(AuthSession.EMPTY)
Review Comment:
Same here? The default should not be EMPTY, that just cases it to fall through.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1945150977
##########
aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java:
##########
@@ -75,10 +76,13 @@ private RESTClient httpClient() {
if (null == client) {
synchronized (this) {
if (null == client) {
+ DefaultAuthSession authSession =
+ DefaultAuthSession.of(
+ HTTPHeaders.of(OAuth2Util.authHeaders(properties.get(OAuth2Properties.TOKEN))));
Review Comment:
@adutra It's beyond the scope of this PR since this is an existing problem, but somehow we need the AuthManager/AuthSession to be able to refresh OAuth tokens here as well. The token here will expire at which point the credential provider will not be able to get new vended credentials.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922256104
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -337,27 +337,20 @@ public void testCatalogBasicBearerToken() {
// the bearer token should be used for all interactions
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/config", catalogHeaders),
eq(ConfigResponse.class),
- eq(catalogHeaders),
+ any(),
any());
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.HEAD),
- eq("v1/namespaces/ns/tables/table"),
+ reqMatcher(HTTPMethod.HEAD, "v1/namespaces/ns/tables/table", catalogHeaders),
any(),
any(),
- any(),
- eq(catalogHeaders),
any());
}
@Test
public void testCatalogCredentialNoOauth2ServerUri() {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
nit: we might want to consider keeping this so that it's clearer that empty headers are expected
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922265637
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -2122,23 +1948,18 @@ public void testCatalogTokenRefreshDisabledWithCredential(String oauth2ServerUri
"scope",
"catalog");
Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
+ .postForm(
eq(oauth2ServerUri),
- any(),
- Mockito.argThat(fetchTokenFromCredential::equals),
+ argThat(fetchTokenFromCredential::equals),
eq(OAuthTokenResponse.class),
- eq(ImmutableMap.of()),
+ anyMap(),
Review Comment:
should this rather verify that this should be an empty map instead of any map?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1926113976
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -88,33 +84,30 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting static final String REST_SOCKET_TIMEOUT_MS = "rest.client.socket-timeout-ms";
- private final String uri;
+ private final URI baseUri;
private final CloseableHttpClient httpClient;
+ private final Map<String, String> baseHeaders;
private final ObjectMapper mapper;
+ private final AuthSession authSession;
private HTTPClient(
- String uri,
+ URI baseUri,
HttpHost proxy,
CredentialsProvider proxyCredsProvider,
Map<String, String> baseHeaders,
ObjectMapper objectMapper,
HttpRequestInterceptor requestInterceptor,
Map<String, String> properties,
HttpClientConnectionManager connectionManager) {
- this.uri = uri;
+ this.baseUri = baseUri;
+ this.baseHeaders = baseHeaders;
this.mapper = objectMapper;
+ this.authSession = AuthSession.EMPTY;
Review Comment:
I think defaulting to empty opens up a risk that requests will be made without an explicit session. This somewhat hides the case where we missed a call that should have auth. I would think we would leave this a null and require that a session be used (e.g. validate the auth was set in the `execute` call). Thoughts @nastra?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1942068160
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -242,7 +242,10 @@ public void initialize(String name, Map<String, String> unresolved) {
}
String oauth2ServerUri =
props.getOrDefault(OAuth2Properties.OAUTH2_SERVER_URI, ResourcePaths.tokens());
- try (RESTClient initClient = clientBuilder.apply(props)) {
+ try (RESTClient initClient =
+ clientBuilder
+ .apply(props)
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY)) {
Review Comment:
Same here, we shouldn't default to EMPTY
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -283,7 +286,10 @@ public void initialize(String name, Map<String, String> unresolved) {
mergedProps,
OAuth2Properties.TOKEN_REFRESH_ENABLED,
OAuth2Properties.TOKEN_REFRESH_ENABLED_DEFAULT);
- this.client = clientBuilder.apply(mergedProps);
+ this.client =
+ clientBuilder
+ .apply(mergedProps)
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
Review Comment:
Here as well.
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922256880
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -489,39 +463,29 @@ public void testCatalogBearerTokenWithClientCredential(String oauth2ServerUri) {
// use the bearer token for config
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/config", catalogHeaders),
eq(ConfigResponse.class),
- eq(catalogHeaders),
+ any(),
any());
// use the bearer token to fetch the context token
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- any(),
+ reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
eq(OAuthTokenResponse.class),
- eq(catalogHeaders),
+ any(),
any());
// use the context token for table existence check
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.HEAD),
- eq("v1/namespaces/ns/tables/table"),
+ reqMatcher(HTTPMethod.HEAD, "v1/namespaces/ns/tables/table", contextHeaders),
any(),
any(),
- any(),
- eq(contextHeaders),
any());
}
@ParameterizedTest
@ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
same as above
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922256503
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -373,39 +366,29 @@ public void testCatalogCredentialNoOauth2ServerUri() {
// no token or credential for catalog token exchange
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.POST, "v1/oauth/tokens", Map.of()),
eq(OAuthTokenResponse.class),
- eq(emptyHeaders),
+ any(),
any());
// no token or credential for config
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
+ reqMatcher(HTTPMethod.GET, "v1/config", catalogHeaders),
eq(ConfigResponse.class),
- eq(catalogHeaders),
+ any(),
any());
// use the catalog token for all interactions
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.HEAD),
- eq("v1/namespaces/ns/tables/table"),
+ reqMatcher(HTTPMethod.HEAD, "v1/namespaces/ns/tables/table", catalogHeaders),
any(),
any(),
- any(),
- eq(catalogHeaders),
any());
}
@ParameterizedTest
@ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
public void testCatalogCredential(String oauth2ServerUri) {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
same as above
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922260527
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1508,20 +1373,21 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) {
"Bearer token-exchange-token:sub=client-credentials-token:sub=catalog");
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- Mockito.argThat(secondRefreshRequest::equals),
+ reqMatcher(
+ HTTPMethod.POST,
+ oauth2ServerUri,
+ secondRefreshHeaders,
+ Map.of(),
+ secondRefreshRequest),
eq(OAuthTokenResponse.class),
- eq(secondRefreshHeaders),
+ any(),
any());
});
}
@ParameterizedTest
@ValueSource(strings = {"v1/oauth/tokens", "https://auth-server.com/token"})
public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) {
- Map<String, String> emptyHeaders = ImmutableMap.of();
Review Comment:
same as above
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1922259637
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1486,12 +1349,14 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) {
"scope", "catalog");
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- Mockito.argThat(firstRefreshRequest::equals),
+ reqMatcher(
+ HTTPMethod.POST,
+ oauth2ServerUri,
+ catalogHeaders,
+ Map.of(),
Review Comment:
shouldn't this be the `catalogHeaders` as before?
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1486,12 +1349,14 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) {
"scope", "catalog");
Mockito.verify(adapter)
.execute(
- eq(HTTPMethod.POST),
- eq(oauth2ServerUri),
- any(),
- Mockito.argThat(firstRefreshRequest::equals),
+ reqMatcher(
+ HTTPMethod.POST,
+ oauth2ServerUri,
+ catalogHeaders,
+ Map.of(),
Review Comment:
shouldn't this be the `catalogHeaders` as before?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932128629
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -206,21 +206,24 @@ private AuthSession authSession() {
return authSessionCache()
.get(
token,
- id ->
- AuthSession.fromAccessToken(
- httpClient(),
- tokenRefreshExecutor(),
- token,
- expiresAtMillis(properties()),
- new AuthSession(
- ImmutableMap.of(),
- AuthConfig.builder()
- .token(token)
- .credential(credential())
- .scope(SCOPE)
- .oauth2ServerUri(oauth2ServerUri())
- .optionalOAuthParams(optionalOAuthParams())
- .build())));
+ id -> {
+ RESTClient client =
+ httpClient().withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
Review Comment:
wouldn't this create a new http client every time this is called?
--
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] Auth Manager API part 4: RESTClient, HTTPClient [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11992:
URL: https://github.com/apache/iceberg/pull/11992#discussion_r1932371163
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -192,7 +192,8 @@ private RESTClient httpClient() {
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
- .build();
+ .build()
+ .withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
Review Comment:
maybe worth having a `withAuthSession` on the builder class itself(similar to `withObjectMapper`)
--
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