#749 Bad error handling for IOException while reading incoming request body

[ks1990cn]

Fixes #749

• 749

• Fixing issue for status code 404, when content length is large than body size. will be displaying 413 status code ("Entity Payload too large").
• Considered scenario when application is hosted on Kestrel , IIS or Http.Sys and have different options configured as per host server.

For example:

builder.WebHost.UseHttpSys(options =>
{
    options.MaxRequestBodySize = 60000000;
});

Above code have MaxRequestBodySizeconfigured manually (default is ~28.6MB), so changes will handle this case too.

Proposed Changes

• I have done manual testing, writing test case to fake server host was tough, as per my research on ASP.NET Core there is no exposed way to directly write this fake server test.
• I also faced issue with access level of ToByteArray of RequestMapper, so I could have marked it null and ByteArrayContent constructor have given me error and hence I could have tested it.
• Open for discussion how we can test this functionality :D

[raman-m]

Tanmay, The issue #729 is about sticky sessions! Are you sure your PR is related to #729 ? It seems the number is wrong...

[raman-m]

I found it! The issue is #749 😺

• 749

[ks1990cn]

I found it! The issue is #749 😺

• Bad error handling for IOException while reading incoming request body #749

Sorry! correct its 749. Thank you

[ggnaegi]

@ks1990cn @raman-m Hello, I took now a bit of time to create an acceptance test, and I realized that the implementation is WAAAAAAAAAAAAAY too complicated (and it's impossible to get the KestrelServer from IServer, because you will get KestrelServerImpl first). Kestrel and other servers will return you a BadHttpRequestException with status code 413. So the issue here, is that the exception is wrapped and hidden in the UnmappableRequestError. A rethinking of the exception handling is needed... By the way TestServer doesn't support MaxRequestBodySize.

The issue here can easily be fixed with:

catch (BadHttpRequestException badHttpRequest)
            {
                return badHttpRequest.StatusCode == 413 ? new ErrorResponse<HttpRequestMessage>(new PayloadTooLargeError(badHttpRequest)) : new ErrorResponse<HttpRequestMessage>(new UnmappableRequestError(badHttpRequest));
            }
            catch (Exception ex)
            {
                return new ErrorResponse<HttpRequestMessage>(new UnmappableRequestError(ex));
            }

Again, it's only a patch, imho, the exception should bubble up to a central exception handler. The acceptance test:

[Fact]
        public void should_return_status_413_when_exceeding_body_max_size()
        {
            var port = RandomPortFinder.GetRandomPort();

            var configuration = new FileConfiguration
            {
                Routes = new List<FileRoute>
                {
                    new()
                    {
                        DownstreamPathTemplate = "/",
                        UpstreamPathTemplate = "/",
                        UpstreamHttpMethod = new List<string> { "Post" },
                        DownstreamHostAndPorts = new List<FileHostAndPort>
                        {
                            new()
                            {
                                Host = "localhost",
                                Port = port,
                            },
                        },
                        DownstreamScheme = "http",
                    },
                },
            };

            this.Given(x => x.GivenThereIsAServiceRunningOnNoExceptions($"http://localhost:{port}"))
                .And(x => _steps.GivenThereIsAConfiguration(configuration))
                .And(x => _steps.GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(1024))
                .When(x => _steps.WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(File.ReadAllBytes(@"super_file.txt"))))
                .Then(x => _steps.ThenTheStatusCodeShouldBe(413))
                .BDDfy();
        }

Kestrel Server Implementation

public void GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(long customBodyMaxSize)
        {
             var kestrelServer = Host.CreateDefaultBuilder()
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseKestrel().ConfigureKestrel((_, options) =>
                        {
                            options.Limits.MaxRequestBodySize = customBodyMaxSize;
                        })
                        .ConfigureAppConfiguration((hostingContext, config) =>
                        {
                            config.SetBasePath(hostingContext.HostingEnvironment.ContentRootPath);
                            var env = hostingContext.HostingEnvironment;
                            config.AddJsonFile("appsettings.json", optional: true, reloadOnChange: false)
                                .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: false);
                            config.AddJsonFile(_ocelotConfigFileName, optional: true, reloadOnChange: false);
                            config.AddEnvironmentVariables();
                        })
                        .ConfigureServices(s =>
                        {
                            s.AddOcelot();
                        })
                        .Configure(app =>
                        {
                            app.Use(async (_, next) =>
                            {
                                await next.Invoke();
                            });
                            app.UseOcelot().Wait();
                        })
                        .UseUrls("http://localhost:5001");
                }).Build();
             kestrelServer.Start();

             _ocelotClient = new HttpClient
             {
                 BaseAddress = new Uri("http://localhost:5001"),
             };
        }

[raman-m]

Guys, who is our hero? I'll get back to this funny thread after current release... Keep conversation ongoing! ✌️😁

[ggnaegi]

Guys, who is our hero? I'll get back to this funny thread after current release... Keep conversation ongoing! ✌️😁

Well, not really. I'm just tired of this $%^%&*% error handling. @ks1990cn @raman-m tadaaaaam...

[ks1990cn]

Ah! didn't noticed that it already gives 413 in exception, I remember it was giving me 404 only (Maybe I didn't noticed right). Correct it can be done directly. If you are ready @ggnaegi, you can go ahead. Converting it to draft. Thanks for taking look @ggnaegi .

[raman-m]

Open.. close... open... close... I'm going crazy with you guys 😃 What is a follow up after closing this PR? What is the result? The bug #749 is still open. And I'm late to review 🤣