Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Access delegation #1033

Merged
merged 19 commits into from
Nov 6, 2024
+35 −1
Merged

Access delegation #1033

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b929c2e
s3_signer_endpoint
Aug 9, 2024
e1d9f64
prune any trailing whitespaces
guitcastro Aug 9, 2024
f3cfd37
fallback to default value instead of "endpoint" property
guitcastro Aug 9, 2024
989bd14
fix test_s3v4_rest_signer_endpoint
Aug 9, 2024
2045f83
Fix missing backtick
guitcastro Aug 9, 2024
cf54856
create access_delegation property
Aug 9, 2024
a197d28
rename S3_SIGNER_ENDPOINT_DEFAULT_VALUE to S3_SIGNER_ENDPOINT_DEFAULT
Aug 9, 2024
a425b69
fix s3.signer.endpoint docs
Aug 9, 2024
10cc361
fk typo in signer
Aug 9, 2024
07325e5
fix fmt
Aug 12, 2024
f4fc2ec
merge with s3_signer_endpoint
Aug 12, 2024
35c7cd3
Merge remote-tracking branch 'origin' into access_delegation
Aug 12, 2024
da3490b
rename ACCESS_DELEGATION_DEFAULT_VALUE to ACCESS_DELEGATION_DEFAULT
Aug 12, 2024
238df70
rename access_delegation to access-delegation
Aug 16, 2024
cf0dc4a
Merge remote-tracking branch 'origin' into access_delegation
Aug 16, 2024
ae4a677
fix grammar
guitcastro Aug 20, 2024
da322e6
Merge branch 'main' into access_delegation
guitcastro Sep 25, 2024
0c4feec
Suggestions for #1033
edgarrmondragon Oct 5, 2024
c1cc3c0
Merge pull request #1 from edgarrmondragon/1033-suggestions
guitcastro Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions mkdocs/docs/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,19 +198,20 @@ catalog:

<!-- markdown-link-check-disable -->

| Key | Example | Description |
| ------------------- | -------------------------------- | -------------------------------------------------------------------------------------------------- |
| uri | https://rest-catalog/ws | URI identifying the REST Server |
| ugi | t-1234:secret | Hadoop UGI for Hive client. |
| credential | t-1234:secret | Credential to use for OAuth2 credential flow when initializing the catalog |
| token | FEW23.DFSDF.FSDF | Bearer token value to use for `Authorization` header |
| scope | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog) |
| resource | rest_catalog.iceberg.com | URI for the target resource or service |
| audience | rest_catalog | Logical name of target resource or service |
| rest.sigv4-enabled | true | Sign requests to the REST Server using AWS SigV4 protocol |
| rest.signing-region | us-east-1 | The region to use when SigV4 signing a request |
| rest.signing-name | execute-api | The service signing name to use when SigV4 signing a request |
| oauth2-server-uri | https://auth-service/cc | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |
| Key | Example | Description |
| ------------------- | -------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| uri | https://rest-catalog/ws | URI identifying the REST Server |
| ugi | t-1234:secret | Hadoop UGI for Hive client. |
| credential | t-1234:secret | Credential to use for OAuth2 credential flow when initializing the catalog |
| token | FEW23.DFSDF.FSDF | Bearer token value to use for `Authorization` header |
| scope | openid offline corpds:ds:profile | Desired scope of the requested security token (default : catalog) |
| resource | rest_catalog.iceberg.com | URI for the target resource or service |
| audience | rest_catalog | Logical name of target resource or service |
| access_delegation | remote-signing | A comma-separated list of access mechanisms to signal the server that the client supports delegated access. It will to be sended in `X-Iceberg-Access-Delegation` header. (default: vended-credentials) |
guitcastro marked this conversation as resolved.
Show resolved Hide resolved
| rest.sigv4-enabled | true | Sign requests to the REST Server using AWS SigV4 protocol |
| rest.signing-region | us-east-1 | The region to use when SigV4 signing a request |
| rest.signing-name | execute-api | The service signing name to use when SigV4 signing a request |
| oauth2-server-uri | https://auth-service/cc | Authentication URL to use for client credentials authentication (default: uri + 'v1/oauth/tokens') |

<!-- markdown-link-check-enable-->

Expand Down
4 changes: 3 additions & 1 deletion pyiceberg/catalog/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class Endpoints:
rename_table: str = "tables/rename"


ACCESS_DELEGATION = "access_delegation"
guitcastro marked this conversation as resolved.
Show resolved Hide resolved
ACCESS_DELEGATION_DEFAULT = "vended-credentials"
AUTHORIZATION_HEADER = "Authorization"
BEARER_PREFIX = "Bearer"
CATALOG_SCOPE = "catalog"
Expand Down Expand Up @@ -532,7 +534,7 @@ def _config_headers(self, session: Session) -> None:
session.headers["Content-type"] = "application/json"
session.headers["X-Client-Version"] = ICEBERG_REST_SPEC_VERSION
session.headers["User-Agent"] = f"PyIceberg/{__version__}"
session.headers["X-Iceberg-Access-Delegation"] = "vended-credentials"
session.headers["X-Iceberg-Access-Delegation"] = self.properties.get(ACCESS_DELEGATION, ACCESS_DELEGATION_DEFAULT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we already have a way of setting custom headers in _extract_headers_from_properties. If I understand it correctly, if we set a property like: header.X-Iceberg-Access-Delegation = remote-signing then this should set the header "X-Iceberg-Access-Delegation" as remote-signing.

I think we could achieve this by setting the default header values, and then setting the property based values after the default values are set:

        session.headers["Content-type"] = "application/json"
        session.headers["X-Client-Version"] = ICEBERG_REST_SPEC_VERSION
        session.headers["User-Agent"] = f"PyIceberg/{__version__}"
        session.headers["X-Iceberg-Access-Delegation"] = "vended-credentials"
        header_properties = self._extract_headers_from_properties()
        session.headers.update(header_properties)

What do you think of this approach over introducing a different property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO having an explicit and documented property is better than setting custom header. But I can change if you think it's better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sungwy I tried your code, but some tests failed with:

    assert (
            catalog._session.headers.get("Content-type") == "application/json"
    ), "Expected 'Content-Type' default header not to be overwritten"

I guess the idea is not allow default header override. Thus I guess having a dedicate property still the best option. What are you thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @guitcastro - I appreciate you giving it a go. That error message is interesting because its specific to the Content-Type property, Did you intend to overwrite the Content-Type as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guitcastro I was able to make @sungwy's suggestion work in guitcastro#1, without breaking existing tests.


def _extract_headers_from_properties(self) -> Dict[str, str]:
return {key[len(HEADER_PREFIX) :]: value for key, value in self.properties.items() if key.startswith(HEADER_PREFIX)}
Expand Down
25 changes: 25 additions & 0 deletions tests/catalog/test_rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"X-Client-Version": "0.14.1",
"User-Agent": f"PyIceberg/{pyiceberg.__version__}",
"Authorization": f"Bearer {TEST_TOKEN}",
"X-Iceberg-Access-Delegation": "vended-credentials",
}
OAUTH_TEST_HEADERS = {
"Content-type": "application/x-www-form-urlencoded",
Expand Down Expand Up @@ -659,6 +660,30 @@ def test_load_table_200(rest_mock: Mocker, example_table_metadata_with_snapshot_
assert actual == expected


def test_load_table_honor_access_delegation(
rest_mock: Mocker, example_table_metadata_with_snapshot_v1_rest_json: Dict[str, Any]
) -> None:
test_headers_with_remote_signing = {**TEST_HEADERS, "X-Iceberg-Access-Delegation": "remote-signing"}
rest_mock.get(
f"{TEST_URI}v1/namespaces/fokko/tables/table",
json=example_table_metadata_with_snapshot_v1_rest_json,
status_code=200,
request_headers=test_headers_with_remote_signing,
)
catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN, access_delegation="remote-signing")
actual = catalog.load_table(("fokko", "table"))
expected = Table(
identifier=("fokko", "table"),
metadata_location=example_table_metadata_with_snapshot_v1_rest_json["metadata-location"],
metadata=TableMetadataV1(**example_table_metadata_with_snapshot_v1_rest_json["metadata"]),
io=load_file_io(),
catalog=catalog,
)
# First compare the dicts
assert actual.metadata.model_dump() == expected.metadata.model_dump()
assert actual == expected


def test_load_table_from_self_identifier_200(
rest_mock: Mocker, example_table_metadata_with_snapshot_v1_rest_json: Dict[str, Any]
) -> None:
Expand Down