-
Notifications
You must be signed in to change notification settings - Fork 479
Core: Pass REST auth manager to S3 signer #2846
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
Changes from 3 commits
203bbb4
d7d57ea
a99dcad
72be520
570ba7c
92bbc7a
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 |
|---|---|---|
|
|
@@ -26,14 +26,7 @@ | |
| from tenacity import RetryCallState, retry, retry_if_exception_type, stop_after_attempt | ||
|
|
||
| from pyiceberg import __version__ | ||
| from pyiceberg.catalog import ( | ||
| BOTOCORE_SESSION, | ||
| TOKEN, | ||
| URI, | ||
| WAREHOUSE_LOCATION, | ||
| Catalog, | ||
| PropertiesUpdateSummary, | ||
| ) | ||
| from pyiceberg.catalog import AUTH_MANAGER, BOTOCORE_SESSION, TOKEN, URI, WAREHOUSE_LOCATION, Catalog, PropertiesUpdateSummary | ||
| from pyiceberg.catalog.rest.auth import AuthManager, AuthManagerAdapter, AuthManagerFactory, LegacyOAuth2AuthManager | ||
| from pyiceberg.catalog.rest.response import _handle_non_200_response | ||
| from pyiceberg.exceptions import ( | ||
|
|
@@ -49,7 +42,7 @@ | |
| TableAlreadyExistsError, | ||
| UnauthorizedError, | ||
| ) | ||
| from pyiceberg.io import AWS_ACCESS_KEY_ID, AWS_REGION, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN | ||
| from pyiceberg.io import AWS_ACCESS_KEY_ID, AWS_REGION, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN, FileIO, load_file_io | ||
| from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC, PartitionSpec, assign_fresh_partition_spec_ids | ||
| from pyiceberg.schema import Schema, assign_fresh_schema_ids | ||
| from pyiceberg.table import ( | ||
|
|
@@ -218,6 +211,7 @@ class ListViewsResponse(IcebergBaseModel): | |
| class RestCatalog(Catalog): | ||
| uri: str | ||
| _session: Session | ||
| _auth_manager: AuthManager | None | ||
|
|
||
| def __init__(self, name: str, **properties: str): | ||
| """Rest Catalog. | ||
|
|
@@ -229,6 +223,7 @@ def __init__(self, name: str, **properties: str): | |
| properties: Properties that are passed along to the configuration. | ||
| """ | ||
| super().__init__(name, **properties) | ||
| self._auth_manager: AuthManager | None = None | ||
| self.uri = properties[URI] | ||
| self._fetch_config() | ||
| self._session = self._create_session() | ||
|
|
@@ -263,16 +258,24 @@ def _create_session(self) -> Session: | |
| if auth_type != CUSTOM and auth_impl: | ||
| raise ValueError("auth.impl can only be specified when using custom auth.type") | ||
|
|
||
| session.auth = AuthManagerAdapter(AuthManagerFactory.create(auth_impl or auth_type, auth_type_config)) | ||
| self._auth_manager = AuthManagerFactory.create(auth_impl or auth_type, auth_type_config) | ||
| session.auth = AuthManagerAdapter(self._auth_manager) | ||
| else: | ||
| session.auth = AuthManagerAdapter(self._create_legacy_oauth2_auth_manager(session)) | ||
| self._auth_manager = self._create_legacy_oauth2_auth_manager(session) | ||
| session.auth = AuthManagerAdapter(self._auth_manager) | ||
|
Comment on lines
+382
to
+383
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. nit: take |
||
|
|
||
| # Configure SigV4 Request Signing | ||
| if property_as_bool(self.properties, SIGV4, False): | ||
| self._init_sigv4(session) | ||
|
|
||
| return session | ||
|
|
||
| def _load_file_io(self, properties: Properties = EMPTY_DICT, location: str | None = None) -> FileIO: | ||
| merged_properties = {**self.properties, **properties} | ||
| if self._auth_manager: | ||
| merged_properties[AUTH_MANAGER] = self._auth_manager | ||
| return load_file_io(merged_properties, location) | ||
|
|
||
|
010Soham marked this conversation as resolved.
|
||
| def is_rest_scan_planning_enabled(self) -> bool: | ||
| """Check if rest server-side scan planning is enabled. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from fsspec.implementations.local import LocalFileSystem | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from requests import HTTPError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pyiceberg.catalog import TOKEN, URI | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pyiceberg.catalog import AUTH_MANAGER, TOKEN, URI | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pyiceberg.exceptions import SignError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pyiceberg.io import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ADLS_ACCOUNT_HOST, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -121,9 +121,19 @@ def __call__(self, request: "AWSRequest", **_: Any) -> None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_url = self.properties.get(S3_SIGNER_URI, self.properties[URI]).rstrip("/") # type: ignore | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_endpoint = self.properties.get(S3_SIGNER_ENDPOINT, S3_SIGNER_ENDPOINT_DEFAULT) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_headers = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_headers: dict[str, str] = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auth_header: str | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if token := self.properties.get(TOKEN): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_headers = {"Authorization": f"Bearer {token}"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
010Soham marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auth_header = f"Bearer {token}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
125
to
+131
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. nit: maybe we can get rid of accessing
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 may be out of scope for this bug-fix PR, but it looks like we’re tightly coupling It might be worth revisiting this design in more detail in the future to ensure we don’t introduce fallback logic that’s driven by configuration properties rather than clearer separation of concerns
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. yea great point. i think it would be better to split out the "rest signer" from fileio. there's a good example already in the REST catalog, iceberg-python/pyiceberg/catalog/rest/__init__.py Lines 409 to 459 in a99dcad
it might also be easier to just pass in the request Session from the REST catalog to the Signer. So we dont need to recreate the auth header directly but again, we can refactor this after the bug fix :)
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. agreed - just leaving a comment so we don't forget 🙂
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. in case we forget, #2862
Contributor
Author
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. that sounds good, lets address the cleaner design in #2862 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif auth_manager := self.properties.get(AUTH_MANAGER): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| header = getattr(auth_manager, "auth_header", None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if callable(header): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auth_header = header() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
010Soham marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if auth_header: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_headers["Authorization"] = auth_header | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_headers.update(get_header_properties(self.properties)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signer_body = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.