danielcweeks commented on code in PR #12197:
URL: https://github.com/apache/iceberg/pull/12197#discussion_r1990185526
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -200,86 +151,40 @@ private RESTClient httpClient() {
return httpClient;
}
- private AuthSession authSession() {
- String token = token().get();
- if (null != token) {
- return authSessionCache()
- .get(
- token,
- id -> {
- // this client will be reused for token refreshes; it must
contain an empty auth
- // session in order to avoid interfering with refreshed tokens
- RESTClient refreshClient =
-
httpClient().withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
- return AuthSession.fromAccessToken(
- refreshClient,
- tokenRefreshExecutor(),
- token,
- expiresAtMillis(properties()),
- new AuthSession(
- ImmutableMap.of(),
- AuthConfig.builder()
- .token(token)
- .credential(credential())
- .scope(SCOPE)
- .oauth2ServerUri(oauth2ServerUri())
- .optionalOAuthParams(optionalOAuthParams())
- .build()));
- });
- }
-
- if (credentialProvided()) {
- return authSessionCache()
- .get(
- credential(),
- id -> {
- AuthSession session =
- new AuthSession(
- ImmutableMap.of(),
- AuthConfig.builder()
- .credential(credential())
- .scope(SCOPE)
- .oauth2ServerUri(oauth2ServerUri())
- .optionalOAuthParams(optionalOAuthParams())
- .build());
- long startTimeMillis = System.currentTimeMillis();
- // this client will be reused for token refreshes; it must
contain an empty auth
- // session in order to avoid interfering with refreshed tokens
- RESTClient refreshClient =
-
httpClient().withAuthSession(org.apache.iceberg.rest.auth.AuthSession.EMPTY);
- OAuthTokenResponse authResponse =
- OAuth2Util.fetchToken(
- refreshClient,
- session.headers(),
- credential(),
- SCOPE,
- oauth2ServerUri(),
- optionalOAuthParams());
- return AuthSession.fromTokenResponse(
- refreshClient, tokenRefreshExecutor(), authResponse,
startTimeMillis, session);
- });
+ @VisibleForTesting
+ AuthSession authSession() {
+ if (null == authSession) {
+ synchronized (S3V4RestSignerClient.class) {
+ if (null == authSession) {
+ authManager = AuthManagers.loadAuthManager("s3-signer",
properties());
+ ImmutableMap.Builder<String, String> properties =
+ ImmutableMap.<String, String>builder()
+ .putAll(properties())
+ .putAll(optionalOAuthParams())
+ .put(OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri())
+ .put(OAuth2Properties.TOKEN_REFRESH_ENABLED,
String.valueOf(keepTokenRefreshed()))
+ .put(OAuth2Properties.SCOPE, SCOPE);
+ String token = token().get();
+ if (null != token) {
+ properties.put(OAuth2Properties.TOKEN, token);
+ }
+
+ if (credentialProvided()) {
+ properties.put(OAuth2Properties.CREDENTIAL, credential());
+ }
+
+ authSession = authManager.catalogSession(httpClient(),
properties.buildKeepingLast());
Review Comment:
@adutra I still have an issue with using `catalogSession` here. This is
fundamentally a table session, but I realize it doesn't fit the API nicely
because the table session takes an id, properties and parent session, which we
don't really have access to in this context.
@nastra thoughts on 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
