Add Caddyfile support for request_body #3859
Conversation
```
request_body {
max_size 10000000
}
```
|
|
|
@mholt |
| rb := new(RequestBody) | ||
|
|
||
| for h.Next() { | ||
| // if not, they should be in a block |
There was a problem hiding this comment.
This comment doesn't really make sense because the only thing being parsed is max_size.
You should reject unknown fields (you can use a switch statement with a default that returns an error). You should also reject any additional args on the same line (if we decide to only configure this via the block option, see below)
I'm not convinced that the only way to configure this should be via a block, because it only has one option. For example, the directive might be max_request_body_size <size> instead.
But on the other hand, aligning it with the JSON config isn't a bad idea either. It's just that we might never have any other options than just max_size, so it feels kinda strange to require a block.
I'm of two minds about it.
Anyways, the actual syntax as written would be:
request_body [<matcher>] {
max_size <size>
}
There was a problem hiding this comment.
yes, exactly. Should I change to max_request_body_size or do we keep it this way?
There was a problem hiding this comment.
I'm not sure. I'll let @mholt chime in. I'm leaning towards keeping it as-is but I wanted to bring up the point of discussion.
There was a problem hiding this comment.
I think this is a less-commonly-used feature, so we're probably fine with the more verbose syntax for now. It also lets us add more options like replacements (etc.) in the future.
|
Also please don't forget to sign the CLA. If you already did but it's not recognizing it, it might be that you committed have a different email address than you have attached to your GitHub account. Please add that email to your GitHub account if so. |
|
Oh - also please add an adapt test, see here: https://github.com/caddyserver/caddy/tree/master/caddytest/integration/caddyfile_adapt just make a new file that has a minimal Caddyfile, then a separator with 10 |
|
@francislavoie 👍 , done. |
There was a problem hiding this comment.
Thank you! This actually looks pretty good. And it's a simple improvement, which we like. Happy to accept this.
Thanks for your contribution! If you have a moment, would appreciate some help updating the docs for this new directive for our website too: https://github.com/caddyserver/website
|
@mholt thanks for merging, |
See: https://caddy.community/t/file-upload-size-limits/8419