-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Enable uploads via pre-signed URLs #17349
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
crates/uv-publish/src/lib.rs
Outdated
| })?; | ||
|
|
||
| // Use a raw reqwest client for S3 (no auth middleware). | ||
| let s3_response = client |
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.
Since this is a streaming request, does it need to have an outer retry wrapper?
zanieb
left a comment
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 reasonable though @woodruffw or @konstin should be final reviewers.
33174d4 to
3adbebb
Compare
crates/uv/src/commands/publish.rs
Outdated
| &form_metadata, | ||
| &publish_url, | ||
| &upload_client, | ||
| &oidc_client, |
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.
nit: we should rename this client
crates/uv-publish/src/lib.rs
Outdated
| ) | ||
| })?; | ||
|
|
||
| let s3_response = s3_client |
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.
That's the client with the regular settings wrt to retries and timeouts, and with no auth middleware
crates/uv-publish/src/lib.rs
Outdated
| }; | ||
|
|
||
| debug!( | ||
| "Got pre-signed URL for upload: {}", |
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.
Is that URL confidential, should we redact it in logs?
crates/uv-publish/src/lib.rs
Outdated
| let reader = ProgressReader::new(file, move |read| { | ||
| reporter_clone.on_upload_progress(idx, read as u64); | ||
| }); | ||
| let file_reader = Body::wrap_stream(ReaderStream::new(reader)); |
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.
We need a retry loop here, streaming uploads can't be retried otherwise
| let reserve_request = build_metadata_request( | ||
| &group.raw_filename, | ||
| &reserve_url, | ||
| client, |
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.
That's the client without retries, I think that should be the regular client, plus/minus auth settings?
woodruffw
left a comment
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.
LGTM, one note and one comment but neither is blocking IMO. I'll follow up with some comments on the pyx side too.
| /// Use direct upload to the registry. | ||
| /// | ||
| /// When enabled, the publish command will use a direct two-phase upload protocol | ||
| /// that uploads files directly to storage, bypassing the registry's upload endpoint. | ||
| #[arg(long, hide = true)] | ||
| pub direct: bool, |
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.
Seems reasonable to me, although noting that we could in principle do this without a flag by having uv send an Accept: ... header that indicates support, to which pyx would then respond with the pre-signed URL.
(That might be nice for enabling this implicit/by default in the future, but seems like not worth doing initially!)
| let status = response.status(); | ||
|
|
||
| let reserve_response: ReserveResponse = match status { | ||
| StatusCode::OK => { |
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.
Q, minor: should we use HTTP 409 (Conflict) for this, given that we're not tied to PyPI's response code semantics for our own upload scheme?
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.
Would 409 be for the case in which it already exists and isn't the same file? Or already exists and is the same file? Here I'm using 200 for "already exists and is the same" (and 201 for "created, now proceed to upload").
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.
Oh, good point. 409 would be for "exists but with different content" I believe, 200 is semantically correct for "exists with the same content already."
c42705d to
7a7138d
Compare
7a7138d to
7ea0b02
Compare
7ea0b02 to
fa48051
Compare
| let oidc_client = client_builder | ||
| let unauthenticated_client = client_builder | ||
| .clone() | ||
| .retries(0) |
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.
For the OIDC client, we need retries (GitHub's networking is too unreliable) and ideally the default timeouts, the S3 is separate in that it uses its own retry loop, upload timeouts and no auth middleware.
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.
Fixing.
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 MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.9.22` → `0.9.24` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.9.24`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0924) [Compare Source](astral-sh/uv@0.9.23...0.9.24) Released on 2026-01-09. ##### Bug fixes - Fix handling of `UV_NO_SYNC=1 uv run ...` ([#​17391](astral-sh/uv#17391)) - Rebuild dynamic distribution when version changes with `--no-cache` ([#​17387](astral-sh/uv#17387)) ##### Documentation - Add Rust language classifier ([#​17389](astral-sh/uv#17389)) ### [`v0.9.23`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0923) [Compare Source](astral-sh/uv@0.9.22...0.9.23) Released on 2026-01-09. ##### Enhancements - Only write portable paths in `RECORD` files ([#​17339](astral-sh/uv#17339)) - Support relative paths in `UV_PYTHON_BIN_DIR` and `UV_TOOL_BIN_DIR` ([#​17367](astral-sh/uv#17367)) ##### Preview features - Enable uploads to S3 via pre-signed URLs ([#​17349](astral-sh/uv#17349)) ##### Configuration - Allow setting proxy variables via global / user configuration ([#​16918](astral-sh/uv#16918)) - Manually parse and reconcile Boolean environment variables ([#​17321](astral-sh/uv#17321)) ##### Bug fixes - Avoid broken build artifacts on build failure ([#​17276](astral-sh/uv#17276)) - Fix missing dependencies on synthetic root in SBOM export ([#​17363](astral-sh/uv#17363)) - Recognize `armv8l` as an alias for `armv7l` in platform tag parsing ([#​17384](astral-sh/uv#17384)) - Fix redaction of a URL in a middleware trace log ([#​17346](astral-sh/uv#17346)) ##### Documentation - Add `index.md` suggestion to `llms.txt` ([#​17362](astral-sh/uv#17362)) - Clarify that `uv run` uses inexact syncing by default ([#​17366](astral-sh/uv#17366)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi43NS4xIiwidXBkYXRlZEluVmVyIjoiNDIuNzUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6OnBhdGNoIl19-->
Summary
For pyx, we can allow uploads that bypass the registry and send the file directly to S3. This is an opt-in feature, enabled via the
--directflag.