Support configuring the MaxRequestBodySize for the upload endpoint

[alexmg]

Motivation and Context (Why the change? What's the scenario?)

We have some important documents that need to be imported to Kermel Memory that are larger than the current ASP.NET Core default maximum request body size limit of 30 MB (~28.6 MiB).

aspnet/Announcements#267

It is not currently possible to set the MaxRequestBodySize from configuration making it impossible to override this limit when running Kernel Memory from the Docker container image.

dotnet/aspnetcore#4765

High level description (Approach, Design)

This PR adds a new MaxUploadRequestBodySize property to the ServiceConfig class that defaults to null.

• When the setting is null the default ASP.NET Core MaxRequestBodySize continues to be applied as is currently the case
• When a value is provided it will be configured on the upload endpoint using the IHttpMaxRequestBodySizeFeature feature

This allows for an explicit limit to be configured that may be less than or greater than the default ASP.NET Core limit.

[alexmg]

Thanks for taking a look at the PR @dluc!

I noticed in your commit that the minimum of the configured value and 10 is being used as the multiplier in the GetMaxUploadSizeInBytes method. That is then used for the various server options and the IHttpMaxRequestBodySizeFeature feature. This looks like it would reduce the allowed maximum size to no greater than 10 MB, which is lower than the default limit of ASP.NET Core.

public long? GetMaxUploadSizeInBytes()
{
    if (this.MaxUploadSizeMb.HasValue)
    {
        return Math.Min(10, this.MaxUploadSizeMb.Value) * 1024 * 1024;
    }

    return null;
}

Was the intent to limit the configurable upload size to a value lower than the out-of-the-box default? I was hoping to support increasing the maximum to support the uploading of larger documents. I can appreciate wanting to put a limit on how large the configured value could be if there are downstream limitations but allowing a value greater than the ASP.NET Core defaults would make the setting more useful.

[alexmg]

Hi @dluc. Can you provide feedback on the above issue related to this PR? It is currently only possible to reduce the maximum size of the request body to a value lower than the default. If you want to enforce a maximum configurable value, would it be better to enforce that through validation of the setting during startup?

[marcominerva]

return Math.Min(10, this.MaxUploadSizeMb.Value) * 1024 * 1024;

I too think that this should be Math.Max, or we could use Math.Clamp if we want to enforce both a minimum and a maximum value at the same time.

Another point is related to the fact that the value is expressed in Megabytes. Is this the right choice? Typically this kind of property is always in bytes (like in the ASP.NET Core IHttpMaxRequestBodySizeFeature.MaxRequestBodySize property) to give total control on the value.

[alexmg]

I'm hoping this was a typo and the value was intended to be something greater than 10. @dluc Are you able to confirm if this was the case? I have not been able to test this change because it is not possible to increase the limit.

I had the value as bytes in the original commit to align with the server options that users would be used to setting. Using bytes certainly does provide the finest granularity possible. Because the value is in a configuration file you don't get the benefit of using a multiplier like * 1024 * 1024 to improve readability. I suspect that was the reason for the choice but still lean towards using bytes and aligning with the underlying options.

[marcominerva]

@alexmg @dluc I have made a PR with the suggested fix: #893.

[dluc]

Confirming it's a typo - it should indeed be .Max.

I still prefer managing the configuration in MBs because it aligns better with realistic usage, offers better readability, and sets more accurate expectations. Specifically, there are two different values at play:

1. Max file size: while it could be expressed in bytes, there's rarely a practical case where N bytes would be accepted but N+1 would be denied.
2. Max HTTP payload size: this is best expressed in MBs, as the value is typically an estimate and relates to factors like VM disk space and RAM capacity.

[marcominerva]

Confirming it's a typo - it should indeed be .Max.

I think that, in this case, we should use 1 as a lower bound, to handle all the scenarios.

[marcominerva]

@alexmg This is the final PR that been merged: #896.