-
Notifications
You must be signed in to change notification settings - Fork 358
Update fsspec.py to respect s3.signer.uri property #741
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
Conversation
@amogh-jahagirdar @HonahX @Fokko any thoughts about this PR? Would this be OK to incorporate? |
@c-thiel Since it is in Java, I think it is fair to add it here. Two things:
|
cbad6a9
to
4b7df0d
Compare
@Fokko thanks a lot for your feedback - I added docs and the constant. I saw that tabular.io actually provides explicit S3 credentials (on top of remote signing), presumably via AWS STS, if "vended-credentials" are requested (https://github.com/apache/iceberg/blob/b3c25fb7608934d975a054b353823ca001ca3742/open-api/rest-catalog-open-api.yaml#L1495). This is something that can only ever work for AWS S3 and is noticeably slower than using remote signing. As remote signing works also with on-prem deployments, I really hope this is going to become the default for all clients and not vended-credentials. tabular does this only for pyiceberg. Spark requests remote-signing so there is no need to go the extra mile and generate S3 creds. Right now unfortunately in pyiceberg, "vended-credentials" is hardcoded iceberg-python/pyiceberg/catalog/rest.py Line 501 in 42afc43
, even though "remote-signing" is actually supported via fsspec. If the server decides to just ignore what the client requests and push remote signing anyway together with:
it works like a charm. |
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.
This looks good @c-thiel thanks for working on this 👍
@c-thiel The idea is to support both. The problem is that with Arrow it is hard to inject in the signing process, since most of the code is pushed down to the C level, and there are no hooks to use a custom signer. Support for remote signing is something that we definitely want to continue to support. |
Unfortunately, for nessie, when using |
Fixes #740