feat: implement vended credential refresh - #3751
Conversation
350dc82 to
010d6bc
Compare
|
@kevinjqliu I opened this PR as the previous one was closed for being stale. Here I've hopefully addressed your comments!
|
|
+1 (non-binding). GCS support would be pretty to add to this. |
| catalog=self, | ||
| ) | ||
|
|
||
| def _attach_credentials_provider( |
There was a problem hiding this comment.
Can we rename this to _attach_credentials_provider_to_io?
| longest-prefix matching against the freshly vended credentials. | ||
| """ | ||
| if storage_credentials and property_as_bool(self.properties, REFRESH_CREDENTIALS_ENABLED, False): | ||
| io.set_credentials_provider( |
There was a problem hiding this comment.
This breaks pickling support on FileIO. We can't pickle lambdas.
We have some tests for pickling FileIO, but none of them break because this is behind REFRESH_CREDENTIALS_ENABLE
|
|
||
| def _can_refresh(self, location: str) -> bool: | ||
| scheme = urlparse(location).scheme | ||
| refresh_by_scheme = self._needs_refresh_by_scheme.get(scheme) |
There was a problem hiding this comment.
If the TTL on a S3 token is < 300s (the default value), we'll attempt to refresh on every call. That's a lot of possibly unnecessary refreshes.
It seems like we should track when the last refresh was and use that information to determine when we should next refresh.
Java does something similar in scheduleCredentialRefresh
| The refresh callback returns the full LoadCredentialsResponse so the provider can re-run | ||
| longest-prefix matching against the freshly vended credentials. | ||
| """ | ||
| if storage_credentials and property_as_bool(self.properties, REFRESH_CREDENTIALS_ENABLED, False): |
There was a problem hiding this comment.
We should check that the server has access to the LoadCredentials endpoint. Otherwise, it'll probably throw an error.
(If you're setting this property, your catalog probably has the proper endpoints, but we should verify anyways)
3 participants
Third time's a charm (prev PR closed due to staleness) :) Closes #3506
Rationale for this change
Implement automatic vended credential refresh in PyIceberg.
Are these changes tested?
Yes,
Are there any user-facing changes?
Yes, users will have the option to set the refresh credentials configurations in the catalog