Repository navigation
REST: Allow disabling remote signing with s3.remote-signing-enabled - #4134
Ankitraj-builds wants to merge 2 commits into
Conversation
| metadata_location = "s3://warehouse/database/table/metadata/00001.metadata.json" | ||
| rest_mock.get( | ||
| f"{TEST_URI}v1/namespaces/fokko/tables/table", | ||
| json={ | ||
| "metadata-location": metadata_location, | ||
| "metadata": example_table_metadata_with_snapshot_v1, | ||
| "config": {"s3.signer": "S3V4RestSigner", "s3.signer.uri": "http://signer"}, | ||
| }, | ||
| status_code=200, | ||
| request_headers=TEST_HEADERS, | ||
| ) | ||
| catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN, **{"s3.signer": ""}) | ||
| table = catalog.load_table(("fokko", "table")) |
There was a problem hiding this comment.
This seems akward to me. The catalog should pass valid information. @c-thiel what is Lakekeeper doing here?
| table_properties: Properties = EMPTY_DICT, | ||
| ) -> FileIO: | ||
| merged_properties = {**table_properties, **self.properties, **properties} | ||
| # An explicitly empty s3.signer in the catalog config opts out of remote signing, |
There was a problem hiding this comment.
In this situation, a null s3.signer is handled differently than one with the empty string?
I'm a little worried about users setting a property to "" thinking that's null...and it turns out it isn't.
There was a problem hiding this comment.
Fair point. Would an explicit boolean like s3.remote-signing-enabled=false be better? I can switch to that.
There was a problem hiding this comment.
That would be way better in my opinion.
There was a problem hiding this comment.
@rambleraptor Switched to s3.remote-signing-enabled in the latest commit. Also added it to the REST catalog docs.
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
I've got a quick question.
Ankitraj-builds
changed the title
Closes #3888
Rationale for this change
REST catalogs like Lakekeeper return remote signing settings (
s3.signer,s3.signer.uri) in the table response config. Since #3956 the FileIO property order is table metadata < catalog config < server config < vended credentials, so the server's signer always wins. A client that already has its own S3 credentials cannot turn remote signing off, and every S3 request pays an extra round trip to the signer.This adds a catalog property,
s3.remote-signing-enabled. When the catalog config sets it tofalse,RestCatalog._load_file_io()removess3.signerfrom the merged FileIO properties, so remote signing is off even if the server config sets a signer. When it is not set ortrue, nothing changes. This coversTable,StagedTableand scan planning FileIOs.Based on review feedback, this replaces the first version, which used an empty
s3.signeras the opt-out.Are these changes tested?
Yes, in
tests/catalog/test_rest.py:test_load_table_remote_signing_disabled_in_catalog_config: the server returnss3.signer, the catalog setss3.remote-signing-enabled=false, and the table FileIO has nos3.signer. This test fails onmain.test_load_table_server_signer_kept_unless_disabled: with the property not set, or set totrue, the server signer is kept.All 177 tests in
tests/catalog/test_rest.pypass locally, and lint passes.Are there any user-facing changes?
Yes. New REST catalog property
s3.remote-signing-enabled(defaulttrue), documented inconfiguration.md. Nothing changes for users who do not set it.