Add support library for returning Dropshot range requests #6963

smklein · 2024-10-30T21:10:36Z

This library is used as a part of #1599 , I'm pulling it out of #6782 to help make the diff smaller.

This PR adds a range-requests crate for dropshot-based range requests.

Heavily inspired by @jclulow 's work in https://github.com/oxidecomputer/buildomat/blob/main/download/src/lib.rs

hawkw

Nice! I had some fairly nitpicky feedback about cases where I think we ought to be returning 419 status codes, and some equally nitpicky suggestions for allocation and header-value-validation golfing. None of this matters all that much, but I figured it was worth mentioning...

hawkw · 2024-11-01T16:39:29Z

range-requests/Cargo.toml

Out of curiosity, any particular motivation for including this crate in the omicron repo, rather than in the dropshot repo or in its own repo? It seems general-purpose enough that it might eventually be useful in other projects, and there are no Omicron-specific dependencies beyond omicron-workspace-hack (which I assume could be dropped if this library lived elsewhere)...

Totally, I want to leave this door open, but I figured I'd just start locally to iterate quickly. It's already a separate crate, so if we keep the number of dependencies low, it should be easy to move out.

I haven't actually used this much from the client side yet, but I'd like to also validate that half works before exporting it out of Omicron, if that's cool?

hawkw · 2024-11-01T16:41:17Z

range-requests/src/lib.rs

+    #[error("Failed to parse range")]
+    Parse(http_range::HttpRangeParseError),


nit: should this format the underlying parse error as well, rather than potentially discarding it? note that the inner error it isn't annotated with #[source], so it won't be part of the source chain when formatting this error.

perhaps this:

Suggested change

#[error("Failed to parse range")]

Parse(http_range::HttpRangeParseError),

#[error("Failed to parse range: {0}")]

Parse(http_range::HttpRangeParseError),

and/or this:

Suggested change

#[error("Failed to parse range")]

Parse(http_range::HttpRangeParseError),

#[error("Failed to parse range")]

Parse(#[source] http_range::HttpRangeParseError),

Updated -- can't use source, because tragically HttpRangeParseError doesn't implement error, but I'll show the debug representation at least.

hawkw · 2024-11-01T16:49:02Z

range-requests/src/lib.rs

+            Error::MultipleRangesUnsupported | Error::Parse(_) => {
+                HttpError::for_bad_request(None, err.to_string())
+            }


If I understand the semantics of 419 Range Not Satisfiable correctly, I think Error::Parse(HttpRangeParseError::NoOverlap) should return a Range Not Satisfiable error, given that it indicates that the first byte position is out of range.

I'm not sure whether HttpRangeParseError::InvalidRange indicates a condition that should get a 419 or a 400. The documentation doesn't make it clear what "invalid range" means --- if it indicates that a syntactically valid range was parsed but the range didn't make sense (e.g. start greater than end), that should probably also be a 419?

Updated to 416 for these cases!

hawkw · 2024-11-01T16:51:43Z

range-requests/src/lib.rs

+fn make_response_common(
+    range: Option<SingleRange>,
+    file_length: u64,
+    content_type: Option<&str>,


nit, take it or leave it: we might want to consider taking the content-type header as an http::HeaderValue (or impl Into<http::HeaderValue>, rather than as a &str. i believe using an &str as a header value will always allocate, but a http::HeaderValue can also be constructed from an &'static str without allocating, using HeaderValue::from_static. If we took that here, we could avoid the allocation in the &'static str case...

Updated to impl Into

hawkw · 2024-11-01T16:56:54Z

range-requests/src/lib.rs

+    content_type: Option<&str>,
+) -> hyper::http::response::Builder {
+    let mut res = Response::builder();
+    res = res.header(ACCEPT_RANGES, "bytes");


nit, take it or leave it: as i mentioned above, we might consider using a const HeaderValue for the value of our accept-ranges header?

hawkw · 2024-11-01T17:04:08Z

range-requests/src/lib.rs

+            .unwrap()
+            .checked_sub(1)
+            .unwrap()


nitpicky, goofy: perhaps these unwraps ought to be expects? i think both are more or less guaranteed unreachable, but it might be nice to have the expects document why?

+1 on making these expects

hawkw · 2024-11-01T17:04:28Z

range-requests/src/lib.rs

+
+    /// Generate the Content-Range header for inclusion in a HTTP 206 partial
+    /// content response using this range.
+    pub fn to_content_range(&self) -> String {


nit, take it or leave it: it might be nice if this function returned an http::HeaderValue?

hawkw · 2024-11-01T17:04:42Z

range-requests/src/lib.rs

+
+    /// Generate a Range header for inclusion in another HTTP request; e.g.,
+    /// to a backend object store.
+    pub fn to_range(&self) -> String {


similarly, this could perhaps return a HeaderValue?

hawkw · 2024-11-01T17:05:02Z

range-requests/src/lib.rs

+    pub fn content_length(&self) -> u64 {
+        assert!(self.range.length > 0);


maybe this should return a NonZeroU64, since we promise that it is one?

hawkw · 2024-11-01T17:06:02Z

range-requests/src/lib.rs

+    use tokio_util::io::ReaderStream;
+
+    #[test]
+    fn parse_range_valid() {


not a blocker, but this feels like it could potentially be a nice use-case for property tests eventually?

Heh I checked to see if omicron pulled in quick check so that I could suggest the same thing and then I realized the crate I actually wanted to suggest was proptest.

I went ahead and added a couple proptest tests -- this is my first real usage of it, so I welcome extra scrutiny here.

papertigers · 2024-11-01T20:13:58Z

I think this largely looks good to me. I came here to say that this would be useful as a a crate outside of omicron so that it could be used elsewhere.

There are no consumers of this crate yet as of this PR - do you plan on merging it ahead of the other PR or roughly at the same time as that in progress PR?

hawkw · 2024-11-02T18:39:52Z

range-requests/src/lib.rs

+            ),
+            Error::Parse(_) => HttpError::for_client_error(
+                None,
+                http::StatusCode::RANGE_NOT_SATISFIABLE,


Sorry to continue nitpicking about this, but I thought that only parseable-but-nonsensical range requests were supposed to return this status, and one that's syntactically malformed should get Bad Request instead?

good feedback, I'll change this case to "bad request".

hawkw · 2024-11-02T18:48:06Z

range-requests/src/lib.rs

+            | Error::RangeOverflow
+            | Error::EmptyRange => HttpError::for_client_error(
+                None,
+                http::StatusCode::RANGE_NOT_SATISFIABLE,


Per MDN, when returning a 419, the server is also supposed to add a Content-Range header indicating the actual length of the resource:

The 416 response message should contain a Content-Range indicating an unsatisfied range (that is a '*') followed by a '/' and the current length of the resource, e.g., Content-Range: bytes */12777

(see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/416)

I don't think it's practical to actually do that in this PR, since Dropshot doesn't presently have a nice way to add headers to error responses (see oxidecomputer/dropshot#801, oxidecomputer/dropshot#39). But, it might be worth adding a comment or something to remember to return such a header if/when Dropshot adds a way to do so. We might also want to go ahead and add a field for the actual length of the resource to these error variants, so that it can be used to generate the response headers later — for now, we could also include the actual length in the formatted representation of the errors?

(I think I might go take a crack at implementing oxidecomputer/dropshot#39 in the near future, because I've also needed it for very different reasons and it seems like it could be fun to do...)

I think we're doing this in this PR -- see bad_range_response, where we do attach these headers. If we try to parse a SingleRange from a PotentialRange and fail with this error, we'll attach the necessary headers.

I refactored this in 6c2562d to make this harder to mis-use - I've removed the conversion to HttpError that automatically happens, and ensured that the headers get supplied in-line.

That said, we'd still greatly benefit from oxidecomputer/dropshot#39. I'm returning a Response<Body> directly, just to be able to supply these headers, and I'd really prefer not to do that.

smklein · 2024-11-15T00:05:46Z

Heyo, so my plan was to merge this before #6782, in an attempt to make that PR smaller. Would y'all be okay taking another look here?

Just to make sure we're on the same page:

I'm totally on-board with moving this out of Omicron -- just wanted to keep it in here for the short-term to iterate quickly
I'm also on-board with refactoring this around could use customisation of error presentation dropshot#39 , to return something more strongly-typed than simply Response<Body> -- but I also don't think that needs to hold-up subsequent integration

papertigers

I am happy with this as is unless @hawkw had feedback that she wanted addressed. Also I think leaving this in omicron for now while it gets some milage before pulling it out into a standalone crate is a good idea!

+1

smklein added 7 commits October 30, 2024 14:08

Add support library for returning Dropshot range requests

70cb256

lockfile and clippy, classic combo

6b1b556

And fmt, how could i forget

536117c

one more hakari swing

d175e8c

Merge branch 'main' into range-requests

6aae655

Body checking too

c8421fb

Merge branch 'main' into range-requests

6f1773e

smklein mentioned this pull request Oct 31, 2024

[sled-agent] Support bundle storage api #6782

Merged

smklein requested review from jclulow and papertigers October 31, 2024 22:30

hawkw reviewed Nov 1, 2024

View reviewed changes

smklein added 3 commits November 1, 2024 14:40

Merge branch 'main' into range-requests

9588978

Feedback

9bf3f5c

nonzero

c195b03

hawkw reviewed Nov 2, 2024

View reviewed changes

smklein added 2 commits November 4, 2024 15:02

Merge branch 'main' into range-requests

2c66da4

Make it harder to see 'not satisfiable' without corresponding headers

6c2562d

papertigers approved these changes Nov 20, 2024

View reviewed changes

smklein merged commit bb3536f into main Nov 21, 2024
18 checks passed

smklein deleted the range-requests branch November 21, 2024 20:09

iliana mentioned this pull request Nov 21, 2024

fix up Cargo.lock #7133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support library for returning Dropshot range requests #6963

Add support library for returning Dropshot range requests #6963

smklein commented Oct 30, 2024

hawkw left a comment

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

papertigers Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

smklein Nov 1, 2024

hawkw Nov 1, 2024

papertigers Nov 1, 2024

smklein Nov 1, 2024

papertigers commented Nov 1, 2024

hawkw Nov 2, 2024

smklein Nov 4, 2024

hawkw Nov 2, 2024

hawkw Nov 2, 2024

smklein Nov 4, 2024

smklein commented Nov 15, 2024

papertigers left a comment

		#[error("Failed to parse range")]
		Parse(http_range::HttpRangeParseError),

		pub fn content_length(&self) -> u64 {
		assert!(self.range.length > 0);

Add support library for returning Dropshot range requests #6963

Add support library for returning Dropshot range requests #6963

Conversation

smklein commented Oct 30, 2024

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papertigers commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smklein commented Nov 15, 2024

papertigers left a comment

Choose a reason for hiding this comment