fix(minidump): Streaming uploads from external relays#5977
Conversation
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
jjbayer
changed the title
| headers: HeaderMap, | ||
| Path(upload::LocationPath { project_id, key }): Path<upload::LocationPath>, | ||
| Query(upload::LocationQueryParams { length, signature }): Query<upload::LocationQueryParams>, | ||
| Query(LocationQueryParams { length, signature }): Query<LocationQueryParams<Provisional>>, |
There was a problem hiding this comment.
Introducing the differentiation between Final and Provisional type might seem like a lot of overhead, but I think it's good to encode this difference in the type system.
| Upload::Create(_, tx) => tx.send(Err(Error::LoadShed)), | ||
| Upload::Upload(_, tx) => tx.send(Err(Error::LoadShed)), |
There was a problem hiding this comment.
These messages now have different response types: one is Provisional, the other one Final.
| relay_log::trace!("Validating headers"); | ||
| let upload_length = tus::validate_post_headers(&headers, meta.request_trust().is_trusted()) | ||
| .map_err(Error::from)?; | ||
| let upload_length = tus::validate_post_headers(&headers).map_err(Error::from)?; |
There was a problem hiding this comment.
This is the functional change of this PR: We now always allow the Defer-Length: 1 header (i.e. open-ended uploads for which the length is unknown upfront). Correct rate limiting is guaranteed by the SignedLocation<Final> type.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0fbc612. Configure here.
| /// to validate whether the URI (especially the length) has been tempered with. | ||
| #[derive(Debug)] | ||
| pub struct Location { | ||
| pub struct Location<L: UploadLength> { |
There was a problem hiding this comment.
Not that it matters very much, but is this trait bound necessary?
There was a problem hiding this comment.
Nope. I like how it declares what the L is expected to be, but it's unnecessarily strict so I'll remove it.
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct Helper { |
There was a problem hiding this comment.
It seems like either this Helper struct or the Deserialize bound on UploadLength should be unnecessary. Is there a reason LocationQueryParams can't be straightforwardly Deserialize?
In fact, a quick test seems to suggest that they are both unnecessary.
There was a problem hiding this comment.
Thanks, I think the missing puzzle piece was #[serde(transparent)] on the Provisional type, without it the parser considered the length field to be required.
| impl<L: UploadLength + std::fmt::Debug> SignedLocation<L> | ||
| where | ||
| LocationQueryParams<L>: for<'de> Deserialize<'de>, | ||
| { |
There was a problem hiding this comment.
IMO move both bounds to where.
| impl<L: UploadLength + std::fmt::Debug> SignedLocation<L> | |
| where | |
| LocationQueryParams<L>: for<'de> Deserialize<'de>, | |
| { | |
| impl<L> SignedLocation<L> | |
| where | |
| L: UploadLength + std::fmt::Debug, | |
| LocationQueryParams<L>: for<'de> Deserialize<'de>, | |
| { |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
Approved, subject to the comment from the bot about Final.
2 participants
Out of caution, the upload endpoint rejected uploads for which the length was unknown upfront for untrusted relays. But with recent work on the minidump endpoint, we now can have external managed relays submitting attachment uploads through the upload endpoint.
It is safe to use
Defer-Length: 1on the upload as long as the final placeholder has a validated length that matches the actual bytes uploaded to objectstore, since this is the length used for rate limiting.This PR introduces a strongly typed differentiation between provisional upload locations (as requested by the client), and final upload locations (the location URL containing the number of uploaded bytes as a query parameter).
Fixes INGEST-912