Skip to content

Conversation

@geruh
Copy link
Contributor

@geruh geruh commented Jun 9, 2025

Which issue does this PR close?

related to: #6213

Rationale for this change

This PR extends the functionality introduced in #6215 by exposing presign_xxx_options API methods, allowing users to pass detailed options when generating presigned URLs across all services supporting presign.

What changes are included in this PR?

Users can now specify detailed options when presigning:

let signed_req = op.presign_read_options(
    "file.txt",
    Duration::from_secs(3600),
    ReadOptions {
        override_content_type: Some("application/json".to_string()),
        ..Default::default()
    }
).await?;

During testing, I was adding some tests to ensure that options were being set properly across the services for presigning, and I noticed some of the options aren't being set.

service Issues Found
S3 Required write options were not being passed to put operations in the backend.
Azure Options are not passed in. Although, adding them has limited support. Azure requires headers to be embedded during SAS token generation, but reqsign also doesn't support custom headers during SAS generation. This prevents implementing options like cache-control, content-disposition, and conditional headers without upstream reqsign library changes. Also, OpenDAL doesn't leverage reqsign's token generation and requires it to be manually passed in or the capability is disabled. hence, why testing doesn't pick this up.
B2 Options are not passed to presign operations and some headers are hard coded.
COS Limited write support, but GET/STAT overrides are not handled.
OBS Limited write support, but GET/STAT overrides are not handled.
OSS Supports write and some GET/STAT operations, but some fields are hard coded.

Today, the tests with limited support will fail, and it might be better to just omit these tests, keep the same behavior as today, and address each service in a separate PR.

Are there any user-facing changes?

No extending the functionality to be able to pass in options to all presigning operations

Manual testing

S3 passes & Azurite some passes

@geruh geruh requested a review from Xuanwo as a code owner June 9, 2025 07:57
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels Jun 9, 2025
@Xuanwo
Copy link
Member

Xuanwo commented Jun 9, 2025

Thank you @geruh, that's really great work! Would you like to start an issue to track them?

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jun 9, 2025
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you, really love this change!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 10, 2025
@Xuanwo Xuanwo merged commit 03d6c77 into apache:main Jun 10, 2025
308 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants