Load table credentials from credentials endpoint#3500
Conversation
kevinjqliu
force-pushed
the
codex/load-credentials-table-integration
branch
15 times, most recently
from
eddce8f to
0f80215
Compare
| if Capability.V1_LOAD_CREDENTIALS not in self._supported_endpoints: | ||
| return False | ||
|
|
||
| access_delegation = cast(str, self._session.headers.get(ACCESS_DELEGATION_HEADER, "")) |
There was a problem hiding this comment.
nit: do we need to cast or does it break linter? locally it shows unnecessary cast; type is already 'str'
| # Load credentials only when vended-credentials is requested. | ||
| return any(delegation.strip().lower() == ACCESS_DELEGATION_DEFAULT for delegation in access_delegation.split(",")) | ||
|
|
||
| def _resolve_table_credentials_from_response_or_endpoint( |
There was a problem hiding this comment.
Compared this to Java RESTSessionCatalog and VendedCredentialsProvider. They don't call the /credentials api during loadTable and instead seems to rely on inline storage-credentials on load_table call, and uses the credential endpoint for refresh in FileIO.
We don't have that layer, today so this eager fallback makes sense to me. And just putting this here so it's clear.
| config=table_response.config, | ||
| ) | ||
|
|
||
| def _response_to_staged_table(self, identifier_tuple: tuple[str, ...], table_response: TableResponse) -> StagedTable: |
There was a problem hiding this comment.
Looks like we are still inlining here still and not using _resolve_table_credentials_from_response_or_endpoint, or are you handling that as a followup?
kevinjqliu
force-pushed
the
codex/load-credentials-table-integration
branch
from
0f80215 to
321413a
Compare
yea i realized that. I thought the fallback is cheeky, but it diverges from the java implementation. I dont want to create subtle behavioral difference. this also aligns with what @gabeiglio mentioned here: #3499 (comment) I'm going to close this PR in favor of #3507, which should properly use the |
2 participants
Summary
/credentialsduring REST table response handling when inline credentials are absent.loadCredentialsandvended-credentialsaccess delegation is requested.storage-credentialsprecedence over catalog config./credentialsrequest for remote signing and catalogs that do not advertise the endpoint.Stacked on #3499.
Testing
PYTHONPATH=. pytest tests/catalog/test_rest.py -k 'load_credentials or storage_credentials or load_table_honor_access_delegation or load_table_without_storage_credentials'ruff check pyiceberg/catalog/rest/__init__.py tests/catalog/test_rest.pyruff format --check pyiceberg/catalog/rest/__init__.py tests/catalog/test_rest.pygit commit