Auth Manager API part 4: RESTClient, HTTPClient #11992
Conversation
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
Outdated
Show resolved
Hide resolved
| this.mapper = objectMapper; | ||
| this.authSession = AuthSession.EMPTY; |
There was a problem hiding this 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?
There was a problem hiding this comment.
I think that makes sense to enforce/validate that a proper auth session is being used
There was a problem hiding this 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.
There was a problem hiding this 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).
There was a problem hiding this comment.
Ah, now I get it. Let me try your suggestion.
| .build()))); | ||
| id -> { | ||
| RESTClient client = | ||
| httpClient().withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY); |
There was a problem hiding this comment.
wouldn't this create a new http client every time this is called?
There was a problem hiding this comment.
Indeed. The child client is lightweight though, but it's probably better to move this call to httpClient().
| @@ -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); | |||
There was a problem hiding this comment.
maybe worth having a withAuthSession on the builder class itself(similar to withObjectMapper)
There was a problem hiding this comment.
Builder method added.
| @@ -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) | |||
There was a problem hiding this comment.
I believe this change can be reverted now that the session is set in httpClient()
| @@ -84,6 +85,7 @@ private RESTClient httpClient() { | |||
|
|
|||
| private LoadCredentialsResponse fetchCredentials() { | |||
| return httpClient() | |||
| .withAuthSession(AuthSession.EMPTY) | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
Different sessions are used depending on the context, so it can't be restricted to a single one.
| baseHeaders.forEach(allHeaders::putIfAbsent); | ||
| } | ||
|
|
||
| Preconditions.checkState(authSession != null, "no AuthSession available"); |
There was a problem hiding this comment.
| Preconditions.checkState(authSession != null, "no AuthSession available"); | |
| Preconditions.checkState(authSession != null, "Invalid auth session: null"); |
|
@danielcweeks @nastra is there anything else I need to change? |
4th PR for the Auth Manager API. Previous ones:
This PR introduces the required changes to
RESTClientandHTTPClient. It also introduces aBaseHTTPClientabstract class to facilitate the creation and execution ofHTTPRequests in a consistent way.The biggest change in actually in
TestRESTCatalog, because almost everyMockito.verify()call needs to be adapted.The AuthManager API is still "unplugged" at this point.
\cc @nastra @danielcweeks