[5/n] [dropshot] add support for per-endpoint size limits #1171

sunshowers · 2024-11-12T10:31:29Z

For some endpoints like uploading update and support bundles, we would like to
have a larger request limit. Add support for that.

This is the other half of RFD 353, after #617. (A previous attempt was #618,
which I've abandoned.)

The PR consists of:

The implementation.
A small tweak to the API generation code for better span attribution.
Tests.

Depends on:

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

sunshowers · 2024-11-12T22:44:43Z

~~I'm going to try splitting up this PR a bit.~~ Done, this PR is almost all additions.

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers · 2024-11-15T19:35:53Z

I've rebased the stack on to main, resolving conflicts with #1115.

ahl

I assume this would be ... fine?

#[endpoint {
    request_body_max_bytes = "100000".try_into().unwrap(),
    ....
}

Yeah?

This all looks good

ahl · 2024-11-15T21:50:29Z

dropshot_endpoint/src/lib.rs

+///     // Optional maximum request body size in bytes
+///     request_body_max_bytes = 1 * 1024 * 1024,


I know tags indicates that it's optional, but all these are optional. I would suggest:

/// #[endpoint { /// // Required fields /// method = { DELETE | HEAD | GET | OPTIONS | PATCH | POST | PUT }, /// path = "/path/name/with/{named}/{variables}", /// /// // Optional fields /// // Tags to be included in the OpenAPI document /// tags = [ "all", "your", "OpenAPI", "tags" ], /// // Specifies the media type used to encode the request body /// content_type = { "application/json" | "application/x-www-form-urlencoded" | "multipart/form-data" } /// // A value of `true` marks the operation as deprecated /// deprecated = { true | false }, /// // A value of `true` causes the operation to be omitted from the API description /// unpublished = { true | false }, /// // Maximum request body size in bytes; may be any rust expression including a constant. /// request_body_max_bytes = 1 * 1024 * 1024,

Small suggestion for clarity of the value, but I'm not in love with what I wrote...

I'll try and make this clearer, good catch.

sunshowers · 2024-11-15T22:51:22Z

I assume this would be ... fine?

Yeah, any expression that evaluates to a usize would work (doesn't need to be a const expression specifically).

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

ahl · 2024-11-16T02:28:09Z

dropshot_endpoint/src/lib.rs

 ///     unpublished = { true | false },
-///     // Optional maximum request body size in bytes
+///     // Maximum request body size in bytes (default: server config default_request_body_max_bytes)


this is good, but I think it would be nice to let people know that a CONSTANT would be fine here (the fact that any expression works is fine, but I think the interesting thing is that a constant works).

I was trying to keep the comment short to be honest. I think the best thing to do here would be to use Markdown for all the configuration parameters, similar to the tag configuration section below.

I've expanded the docs and added a clearer hint about being able to use constants.

…#1173) In upcoming work (#1171), we're going to add optional per-endpoint size limits. To make it clear that per-endpoint limits always override whatever's specified in the configuration, rename this field. (Without the rename, it can be confusing to determine what happens if the per-endpoint limit is smaller than the configuration limit.) Also add a deserialize-time check to fail if `request_body_max_bytes` is specified, guiding users to the new name.

This is part of `RequestContext`, and returns metadata that's stored on the endpoint. In upcoming work (#1171) we're going to add per-endpoint size limits to this struct.

Created using spr 1.3.6-beta.1

davepacheco

Nice! Nothing major here, just a few doc requests and a suggestion.

CHANGELOG.adoc

dropshot/src/handler.rs

davepacheco · 2024-11-20T20:56:45Z

dropshot/src/handler.rs

@@ -218,6 +224,9 @@ pub struct RequestEndpointMetadata {

    /// The expected request body MIME type.
    pub body_content_type: ApiEndpointBodyContentType,
+
+    /// The maximum number of bytes allowed in the request body, if overridden.
+    pub request_body_max_bytes: Option<usize>,


What do you think of calling this request_body_max_bytes_override? (I'm honestly not sure. Sometimes I find that suffix clarifies that what might be here is an override of some other value that comes from some place else. Otherwise, you might think that if this were None that there was no limit.)

Hmm, not sure -- personally I feel like if you're using this you're likely also aware of the default in the config, maybe? I generally lean towards shorter names over spelling things out completely, especially when there's a clear chain of implication.

davepacheco · 2024-11-20T20:58:30Z

dropshot/src/lib.rs

@@ -707,7 +707,7 @@
 //!


This comment belongs at L309, but, you know, thanks GitHub.

I think you want to add this to the synopsis at L309 and also add a note about it in that section (#[endpoint { ... }] attribute parameters) around L322).

Hmm, the synopsis doesn't cover all the arguments today so it would be a bigger change. I can make this change but it would be a substantial one that I would do separately.

(This is where I feel like not having everything in the lib.rs documentation, but instead having an external documentation site, might be useful.)

Oh, so I just realized that versions is specified in dropshot/src/lib.rs but not in the dropshot_endpoint::endpoint documentation. There are also several parameters described in endpoint but not in dropshot/src/lib.rs.

Yikes -- I think the list of arguments should only be specified in one spot!

#1181 does that. I'd like to not block on it if that's ok, I think that's a bigger fix that should be done in a followup.

Created using spr 1.3.6-beta.1

In the review for #1171 I realized that we were specifying the endpoint attribute params in two places, neither of which had a complete list. Put it all in one place: on documentation for the proc macro. Also move the documentation to `dropshot/src/lib.rs` to allow doctests to work.

sunshowers added 2 commits November 12, 2024 10:31

[spr] initial version

b97472e

Created using spr 1.3.6-beta.1

[spr] changes to main this commit is based on

a8cfa4d

Created using spr 1.3.6-beta.1 [skip ci]

sunshowers mentioned this pull request Nov 12, 2024

add support for setting a larger request body for some endpoints #618

Closed

sunshowers changed the title ~~[2/2] [dropshot] add support for per-endpoint size limits~~ [2/n] [dropshot] add support for per-endpoint size limits Nov 12, 2024

sunshowers changed the title ~~[2/n] [dropshot] add support for per-endpoint size limits~~ [3/n] [dropshot] add support for per-endpoint size limits Nov 12, 2024

sunshowers added 2 commits November 12, 2024 10:57

rebase on 2/n

bd693e0

Created using spr 1.3.6-beta.1

[spr] changes introduced through rebase

d33aff7

Created using spr 1.3.6-beta.1 [skip ci]

This was referenced Nov 12, 2024

[3/n] rename request_body_max_bytes to default_request_body_max_bytes #1173

Merged

[4/n] refactor, add RequestEndpointMetadata #1174

Merged

sunshowers added 2 commits November 12, 2024 23:23

rebase

633d208

Created using spr 1.3.6-beta.1

[spr] changes introduced through rebase

9b02b18

Created using spr 1.3.6-beta.1 [skip ci]

sunshowers changed the title ~~[3/n] [dropshot] add support for per-endpoint size limits~~ [5/n] [dropshot] add support for per-endpoint size limits Nov 12, 2024

update copyright

45e00a5

Created using spr 1.3.6-beta.1

sunshowers requested review from ahl, davepacheco and iliana November 12, 2024 23:29

sunshowers added 2 commits November 15, 2024 04:40

[spr] changes introduced through rebase

09cb551

Created using spr 1.3.6-beta.1 [skip ci]

rebase

2ec7ac4

Created using spr 1.3.6-beta.1

ahl approved these changes Nov 15, 2024

View reviewed changes

sunshowers added 2 commits November 16, 2024 01:00

[spr] changes introduced through rebase

6fb01a4

Created using spr 1.3.6-beta.1 [skip ci]

review comments

65d0e69

Created using spr 1.3.6-beta.1

ahl reviewed Nov 16, 2024

View reviewed changes

sunshowers changed the base branch from sunshowers/spr/main.22-dropshot-add-support-for-per-endpoint-size-limits to main November 16, 2024 20:56

rebase on main, expand comment

55fc9ad

Created using spr 1.3.6-beta.1

davepacheco reviewed Nov 20, 2024

View reviewed changes

some review comments

088aa70

Created using spr 1.3.6-beta.1

sunshowers mentioned this pull request Nov 20, 2024

[dropshot] improve doc organization #1181

Merged

rebase on main

176b43d

Created using spr 1.3.6-beta.1

sunshowers enabled auto-merge (squash) November 21, 2024 03:20

sunshowers merged commit d5874eb into main Nov 21, 2024
10 checks passed

sunshowers deleted the sunshowers/spr/22-dropshot-add-support-for-per-endpoint-size-limits branch November 21, 2024 03:27

This was referenced Nov 21, 2024

[repo depot 3/n] nexus background task to replicate TUF artifacts across sleds oxidecomputer/omicron#7129

Open

adjust body limits for Nexus external system_update_put_repository and Sled Agent artifact_put oxidecomputer/omicron#7134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5/n] [dropshot] add support for per-endpoint size limits #1171

[5/n] [dropshot] add support for per-endpoint size limits #1171

sunshowers commented Nov 12, 2024 •

edited

Loading

sunshowers commented Nov 12, 2024 •

edited

Loading

sunshowers commented Nov 15, 2024

ahl left a comment

ahl Nov 15, 2024

sunshowers Nov 15, 2024

sunshowers commented Nov 15, 2024

ahl Nov 16, 2024

sunshowers Nov 16, 2024

sunshowers Nov 16, 2024

davepacheco left a comment

davepacheco Nov 20, 2024

sunshowers Nov 20, 2024

davepacheco Nov 20, 2024

sunshowers Nov 20, 2024 •

edited

Loading

sunshowers Nov 20, 2024 •

edited

Loading

sunshowers Nov 20, 2024

		/// // Optional maximum request body size in bytes
		/// request_body_max_bytes = 1 * 1024 * 1024,

[5/n] [dropshot] add support for per-endpoint size limits #1171

[5/n] [dropshot] add support for per-endpoint size limits #1171

Conversation

sunshowers commented Nov 12, 2024 • edited Loading

sunshowers commented Nov 12, 2024 • edited Loading

sunshowers commented Nov 15, 2024

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepacheco 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

sunshowers Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

sunshowers Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers commented Nov 12, 2024 •

edited

Loading

sunshowers commented Nov 12, 2024 •

edited

Loading

sunshowers Nov 20, 2024 •

edited

Loading

sunshowers Nov 20, 2024 •

edited

Loading