apache / arrow-rs

Official Rust implementation of Apache Arrow
https://arrow.apache.org/
Apache License 2.0
2.63k stars 803 forks source link

CredentialProvider.get_credential() should use &mut self. #6718

Closed rtyler closed 1 week ago

rtyler commented 1 week ago

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Most of the ObjectStore calls, at least in the AmazonS3 code, will invoke their credential provider on every method invocation. The current get_credential(&self) function signature3 works well when we assume low/zero-cost credential resolution.

In the case of assuming an IAM role, as an example, there is substantial cost insofar that an AWS STS API call must be made.

There is obvious benefits of re-resolving credentials on every call, especially when tokens expire. I don't believe the non-mutable CredentialProvider allows for easy caching of expensive credential resolution however.

Describe the solution you'd like

In my use-case I want to cache repeated invocations to the same CredentialProvider in the same process for the appropriate amount of time dictated by the credential issued. For example, when a role is assumed in AWS there will be temporary credentials vended which come with an expiration.

Every subsequent invocation of get_credential( should return cached/stored temporary credentials on the CredentialProvider implementation until those credentials expire, at which point they should be re-resolveed/assumed and an updated Credential should be cached/stored inside the CredentialProvider.

:thinking: I believe that get_credential(&mut self) on the trait would allow my implementations sufficient flexibility here.

If this is an okay thing to do, I can pop out a PR for it

Describe alternatives you've considered

Yucky statics external to the CredentialProvider in the process to act as a lookup table. :nauseated_face:

tustvold commented 1 week ago

We already do something similar to this in this crate, see TokenProvider.

Switching to &mut self would require the same change to the ObjectStore trait to match, which is definitely not possible. It also would not work with Arc.

Implementations should just use an interior mutability primitive, e.g. Mutex or RWLock, as appropriate