feat(catalog,io): refresh vended credentials#795
feat(catalog,io): refresh vended credentials#795rockwotj wants to merge 3 commits intoapache:mainfrom
Conversation
| keyS3TokenExpiresAtMs = "s3.session-token-expires-at-ms" | ||
| keyAdlsSasExpiresAtMs = "adls.sas-token-expires-at-ms" | ||
| keyGcsOAuthExpiresAt = "gcs.oauth2.token-expires-at" | ||
| keyExpirationTime = "expiration-time" | ||
|
|
||
| defaultVendedCredentialsTTL = 60 * time.Minute |
There was a problem hiding this comment.
This matches snowflake behavior here: https://docs.snowflake.com/en/user-guide/tables-iceberg-configure-catalog-integration-vended-credentials
zeroshade
changed the title
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
It looks like there's a storage-credential value in the LoadTable response that we should be checking. According to the docs we should be checking that first before we load credentials from the config. So it looks like there's two changes that are needed:
- The change i suggest below using the
..../credentialsendpoint rather than refreshing the entire table - The table load should check for the
storage-credentialsfield in the response and then we should attach the list of credentials to the table IO. When creating a new IO instance we should first check the storage-credentials that were given, and then fallback to looking at the config properties if we don't find credentials in there.
rockwotj
force-pushed
the
vendor-creds-refresh
branch
from
e43594d to
12e0c3d
Compare
rockwotj
force-pushed
the
vendor-creds-refresh
branch
from
12e0c3d to
803a82e
Compare
rockwotj
force-pushed
the
vendor-creds-refresh
branch
2 times, most recently
from
f342e7f to
d3795dc
Compare
It uses mainline of iceberg-go: apache/iceberg-go@6842055 With the following PRs applied on top: - apache/iceberg-go#795 - apache/iceberg-go#803
|
BTW I ran a test for a couple hours in aws with vended creds in polaris and it seemed to work. |
catalog/rest/rest.go
Outdated
|
|
||
| func (r *Catalog) tableFromResponse(ctx context.Context, identifier []string, metadata table.Metadata, loc string, config iceberg.Properties) (*table.Table, error) { | ||
| func (r *Catalog) tableFromResponse(_ context.Context, identifier []string, metadata table.Metadata, loc string, config iceberg.Properties) (*table.Table, error) { | ||
| refresher := &vendedCredentialRefresher{ |
There was a problem hiding this comment.
maybe we need to only do this if credentials are vended, otherwise the refresher could spam a bunch of extra API calls retrying to refresh credentials
There was a problem hiding this comment.
would it spam refreshes if there's no expiration provided for the creds?
There was a problem hiding this comment.
would it spam refreshes if there's no expiration provided for the creds?
No it defaults to 60 minute refresh, it's just if the catalog doesn't implement this endpoint and returns a 404 or something, then it could get spammed
There was a problem hiding this comment.
I'm kicking off a couple hour test with polaris on aws to ensure the credential fetching works, but now we only do credential fetching if storage credentials field is populated from the catalog.
There was a problem hiding this comment.
yeah my soak test passed with this updated PR
Currently credential lifetimes are tied to the table instead, only load table refreshes the credentials. This commit caches and refreshes the credentials based on the response included in the table load response and refreshes it dynamically.
Skip setting up the credential refresher when the catalog does not vend storage credentials. Without this, the refresher would spam the /credentials endpoint with API calls that return errors for catalogs that do not support vended credentials.
rockwotj
force-pushed
the
vendor-creds-refresh
branch
from
d3795dc to
92b6250
Compare
2 participants
Currently credential lifetimes are tied to the table instead, only load
table refreshes the credentials. This commit caches and refreshes the
credentials based on the response included in the table load response
and refreshes it dynamically.
This does still have the effect that holding on to a table IO for a long time does not refresh the table, but this makes steps in that direction.
Fixes: #792