adutra commented on code in PR #12197:
URL: https://github.com/apache/iceberg/pull/12197#discussion_r1978944549
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -138,148 +133,42 @@ boolean keepTokenRefreshed() {
OAuth2Properties.TOKEN_REFRESH_ENABLED_DEFAULT);
}
- @VisibleForTesting
- ScheduledExecutorService tokenRefreshExecutor() {
- if (!keepTokenRefreshed()) {
- return null;
- }
-
- if (null == tokenRefreshExecutor) {
- synchronized (S3V4RestSignerClient.class) {
- if (null == tokenRefreshExecutor) {
- tokenRefreshExecutor =
ThreadPools.newScheduledPool("s3-signer-token-refresh", 1);
- }
- }
- }
-
- return tokenRefreshExecutor;
- }
-
- private Cache<String, AuthSession> authSessionCache() {
- if (null == authSessionCache) {
- synchronized (S3V4RestSignerClient.class) {
- if (null == authSessionCache) {
- long expirationIntervalMs =
- PropertyUtil.propertyAsLong(
- properties(),
- CatalogProperties.AUTH_SESSION_TIMEOUT_MS,
- CatalogProperties.AUTH_SESSION_TIMEOUT_MS_DEFAULT);
-
- authSessionCache =
- Caffeine.newBuilder()
- .expireAfterAccess(Duration.ofMillis(expirationIntervalMs))
- .removalListener(
- (RemovalListener<String, AuthSession>)
- (id, auth, cause) -> {
- if (null != auth) {
- LOG.trace("Stopping refresh for AuthSession");
- auth.stopRefreshing();
- }
- })
- .build();
- }
- }
- }
-
- return authSessionCache;
- }
-
private RESTClient httpClient() {
if (null == httpClient) {
synchronized (S3V4RestSignerClient.class) {
if (null == httpClient) {
- httpClient =
+ authManager = AuthManagers.loadAuthManager("s3-signer",
properties());
+ HTTPClient client =
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
.build();
+ 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);
+ } else if (credentialProvided()) {
+ properties.put(OAuth2Properties.CREDENTIAL, credential());
+ }
+ authSession = authManager.catalogSession(client,
properties.buildKeepingLast());
Review Comment:
Hmm I just spotted a subtle difference between `main` and this branch: on
`main`, the `credential` property is always included in the auth session, even
if there is also a `token` property:
https://github.com/apache/iceberg/blob/be9808fb3d75b1697fc4ed45c02602de9cfb6371/aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java#L223
In this branch, the `credential` property is only included if there is no
`token`:
https://github.com/apache/iceberg/blob/75ff5d72664c0a1f358ebe77a35e3f91322cc220/aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java#L156
This may trigger a different behavior in `OAuth2Manager`, since the manager
always tries to fetch a token if `credential` is present:
https://github.com/apache/iceberg/blob/be9808fb3d75b1697fc4ed45c02602de9cfb6371/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java#L113
I will update this branch to reflect what's in `main`.
Let me know if that solves your issue 🤞
--
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]
