-
Notifications
You must be signed in to change notification settings - Fork 556
feat: implement vended credential refresh #3751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,11 @@ | |
| from pyiceberg import __version__ | ||
| from pyiceberg.catalog import BOTOCORE_SESSION, TOKEN, URI, WAREHOUSE_LOCATION, Catalog, PropertiesUpdateSummary | ||
| from pyiceberg.catalog.rest.auth import AUTH_MANAGER, AuthManager, AuthManagerAdapter, AuthManagerFactory, LegacyOAuth2AuthManager | ||
| from pyiceberg.catalog.rest.credential_provider import ( | ||
| REFRESH_CREDENTIALS_ENABLED, | ||
| CredentialsProvider, | ||
| resolve_storage_credentials, | ||
| ) | ||
| from pyiceberg.catalog.rest.response import _handle_non_200_response | ||
| from pyiceberg.catalog.rest.scan_planning import ( | ||
| FetchScanTasksRequest, | ||
|
|
@@ -466,26 +471,6 @@ def _create_session(self) -> Session: | |
|
|
||
| return session | ||
|
|
||
| @staticmethod | ||
| def _resolve_storage_credentials(storage_credentials: list[StorageCredential], location: str | None) -> Properties: | ||
| """Resolve the best-matching storage credential by longest prefix match. | ||
|
|
||
| Mirrors the Java implementation in S3FileIO.clientForStoragePath() which iterates | ||
| over storage credential prefixes and selects the one with the longest match. | ||
|
|
||
| See: https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java | ||
| """ | ||
| if not storage_credentials or not location: | ||
| return {} | ||
|
|
||
| best_match: StorageCredential | None = None | ||
| for cred in storage_credentials: | ||
| if location.startswith(cred.prefix): | ||
| if best_match is None or len(cred.prefix) > len(best_match.prefix): | ||
| best_match = cred | ||
|
|
||
| return best_match.config if best_match else {} | ||
|
|
||
| def _load_file_io(self, properties: Properties = EMPTY_DICT, location: str | None = None) -> FileIO: | ||
| merged_properties = {**self.properties, **properties} | ||
| if self._auth_manager: | ||
|
|
@@ -827,37 +812,50 @@ def add_headers(self, request: PreparedRequest, **kwargs: Any) -> None: # pylin | |
|
|
||
| def _response_to_table(self, identifier_tuple: tuple[str, ...], table_response: TableResponse) -> Table: | ||
| # Per Iceberg spec: storage-credentials take precedence over config | ||
| credential_config = self._resolve_storage_credentials( | ||
| table_response.storage_credentials, table_response.metadata_location | ||
| credential_config = resolve_storage_credentials(table_response.storage_credentials, table_response.metadata_location) | ||
| io = self._load_file_io( | ||
| {**table_response.metadata.properties, **table_response.config, **credential_config}, | ||
| table_response.metadata_location, | ||
| ) | ||
| self._attach_credentials_provider(io, identifier_tuple, table_response.storage_credentials) | ||
| return Table( | ||
| identifier=identifier_tuple, | ||
| metadata_location=table_response.metadata_location, # type: ignore | ||
| metadata=table_response.metadata, | ||
| io=self._load_file_io( | ||
| {**table_response.metadata.properties, **table_response.config, **credential_config}, | ||
| table_response.metadata_location, | ||
| ), | ||
| io=io, | ||
| catalog=self, | ||
| config=table_response.config, | ||
| ) | ||
|
|
||
| def _response_to_staged_table(self, identifier_tuple: tuple[str, ...], table_response: TableResponse) -> StagedTable: | ||
| # Per Iceberg spec: storage-credentials take precedence over config | ||
| credential_config = self._resolve_storage_credentials( | ||
| table_response.storage_credentials, table_response.metadata_location | ||
| credential_config = resolve_storage_credentials(table_response.storage_credentials, table_response.metadata_location) | ||
| io = self._load_file_io( | ||
| {**table_response.metadata.properties, **table_response.config, **credential_config}, | ||
| table_response.metadata_location, | ||
| ) | ||
| self._attach_credentials_provider(io, identifier_tuple, table_response.storage_credentials) | ||
| return StagedTable( | ||
| identifier=identifier_tuple, | ||
| metadata_location=table_response.metadata_location, # type: ignore | ||
| metadata=table_response.metadata, | ||
| io=self._load_file_io( | ||
| {**table_response.metadata.properties, **table_response.config, **credential_config}, | ||
| table_response.metadata_location, | ||
| ), | ||
| io=io, | ||
| catalog=self, | ||
| ) | ||
|
|
||
| def _attach_credentials_provider( | ||
| self, io: FileIO, identifier: str | Identifier, storage_credentials: list[StorageCredential] | ||
| ) -> None: | ||
| """Attach a CredentialsProvider to io if credential refresh is enabled and credentials were vended. | ||
|
|
||
| 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): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| io.set_credentials_provider( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| CredentialsProvider(storage_credentials, refresh_fn=lambda: self._load_credentials(identifier)) | ||
| ) | ||
|
|
||
| def _response_to_view(self, identifier_tuple: tuple[str, ...], view_response: ViewResponse) -> View: | ||
| return View( | ||
| identifier=identifier_tuple, | ||
|
|
@@ -1124,7 +1122,7 @@ def load_credentials( | |
| ) -> Properties: | ||
| """Load vended storage credentials and return the best match for a location.""" | ||
| credentials_response = self._load_credentials(identifier) | ||
| return self._resolve_storage_credentials(credentials_response.storage_credentials, location) | ||
| return resolve_storage_credentials(credentials_response.storage_credentials, location) | ||
|
|
||
| @retry(**_RETRY_ARGS) | ||
| @override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| from __future__ import annotations | ||
|
|
||
| import threading | ||
| from collections.abc import Callable | ||
| from datetime import datetime | ||
| from typing import TYPE_CHECKING | ||
| from urllib.parse import urlparse | ||
|
|
||
| from pyiceberg.catalog.rest.scan_planning import StorageCredential | ||
| from pyiceberg.io import S3_SESSION_TOKEN_EXPIRES_AT_MS | ||
| from pyiceberg.typedef import Properties | ||
| from pyiceberg.utils.properties import get_first_property_value | ||
|
|
||
| if TYPE_CHECKING: | ||
| from pyiceberg.catalog.rest import LoadCredentialsResponse | ||
|
|
||
| REFRESH_CREDENTIALS_ENABLED = "client.refresh-credentials-enabled" | ||
|
|
||
|
|
||
| def is_s3_credential_expired(config: Properties, threshold_seconds: int = 300) -> bool: | ||
| """Return True if the S3 session token expires within threshold_seconds (5 mins).""" | ||
| if expiry := get_first_property_value(config, S3_SESSION_TOKEN_EXPIRES_AT_MS): | ||
| expires_at = datetime.fromtimestamp(int(expiry) / 1000) | ||
| seconds_remaining = (expires_at - datetime.now()).total_seconds() | ||
| return seconds_remaining < threshold_seconds | ||
| return False | ||
|
|
||
|
|
||
| # Per-scheme hooks for detecting whether a resolved credential needs to be refreshed. | ||
| # Other schemes (e.g. gs, abfss) can register here later. | ||
| NEEDS_REFRESH_BY_SCHEME: dict[str, Callable[[Properties], bool]] = { | ||
| "s3": is_s3_credential_expired, | ||
| "s3a": is_s3_credential_expired, | ||
| "s3n": is_s3_credential_expired, | ||
| } | ||
|
|
||
|
|
||
| def resolve_storage_credentials(storage_credentials: list[StorageCredential], location: str | None) -> Properties: | ||
| """Resolve the best-matching storage credential by longest prefix match. | ||
|
|
||
| Mirrors the Java implementation in S3FileIO.clientForStoragePath() which iterates | ||
| over storage credential prefixes and selects the one with the longest match. | ||
|
|
||
| See: https://github.com/apache/iceberg/blob/main/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java | ||
| """ | ||
| if not storage_credentials or not location: | ||
| return {} | ||
|
|
||
| best_match: StorageCredential | None = None | ||
| for cred in storage_credentials: | ||
| if location.startswith(cred.prefix): | ||
| if best_match is None or len(cred.prefix) > len(best_match.prefix): | ||
| best_match = cred | ||
|
|
||
| return best_match.config if best_match else {} | ||
|
|
||
|
|
||
| class CredentialsProvider: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this class ultimately needs to less S3-specific and delegate some functionality into scheme-specific handlers. I don't think that has to be part of this PR though, it could be part of a following that adds support for other schemes, such as GCS. |
||
| """Vended-credential refresh and location-based lookup for a REST catalog table.""" | ||
|
|
||
| _storage_credentials: list[StorageCredential] | ||
| _refresh_fn: Callable[[], LoadCredentialsResponse] | ||
| _needs_refresh_by_scheme: dict[str, Callable[[Properties], bool]] | ||
| _lock: threading.Lock | ||
|
|
||
| def __init__( | ||
| self, | ||
| storage_credentials: list[StorageCredential], | ||
| refresh_fn: Callable[[], LoadCredentialsResponse], | ||
| needs_refresh_by_scheme: dict[str, Callable[[Properties], bool]] | None = None, | ||
| ): | ||
| self._storage_credentials = storage_credentials | ||
| self._refresh_fn = refresh_fn | ||
| self._needs_refresh_by_scheme = ( | ||
| needs_refresh_by_scheme if needs_refresh_by_scheme is not None else NEEDS_REFRESH_BY_SCHEME | ||
| ) | ||
| self._lock = threading.Lock() | ||
|
|
||
| def _can_refresh(self, location: str) -> bool: | ||
| scheme = urlparse(location).scheme | ||
| refresh_by_scheme = self._needs_refresh_by_scheme.get(scheme) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| config = resolve_storage_credentials(self._storage_credentials, location) | ||
| return config != {} and refresh_by_scheme is not None and refresh_by_scheme(config) | ||
|
|
||
| def properties_for(self, location: str) -> Properties: | ||
| """Return the credential properties that apply to the given location, refreshing if needed.""" | ||
| config = resolve_storage_credentials(self._storage_credentials, location) | ||
|
|
||
| if self._can_refresh(location): | ||
| with self._lock: | ||
| if self._can_refresh(location): | ||
| response = self._refresh_fn() | ||
| self._storage_credentials = response.storage_credentials | ||
| config = resolve_storage_credentials(self._storage_credentials, location) | ||
|
|
||
| return config | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to _attach_credentials_provider_to_io?