singhpk234 commented on code in PR #17914:
URL: https://github.com/apache/iceberg/pull/17914#discussion_r4553812538
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -279,7 +281,7 @@ public SdkHttpFullRequest sign(
.build();
Key cacheKey = Key.from(remoteSigningRequest);
- SignedComponent cachedSignedComponent =
SIGNED_COMPONENT_CACHE.getIfPresent(cacheKey);
+ SignedComponent cachedSignedComponent =
signedComponentCache().getIfPresent(cacheKey);
Review Comment:
> Not per sign request, but per FileIO instance. And we could have hundreds
if not thousands of those in the same JVM.
Ah i see its lazy annotated, thats fair
> Other auth managers may include other types of credentials, and since
1.12, catalog servers can also send credential-like data in the
remote-signing-config
are we treating remote-signing-config a credential now ? my understanding is
its opaque, and server ideally should revalisate that these belong to calling
prinicipal / context before skipping the re-authZ ... what case you have in
mind @adutra or you are just being cautious and want isolation to response as
much as possible ?
+1 to the signing endpoint also part of the cache key ... i think we already
do the same in AuthManager the same
https://github.com/apache/iceberg/blob/acea99d28a4be93a8c0dfc7bd8ece4a237324243/core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java#L200
--
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]
