#749 Bad error handling for IOException while reading incoming requ…

…est body (#1769) * Adding "quick and..." fix to the payload too large exception, adding two acceptance tests for kestrel and http sys. * skipping test if platform is not windows * Moving Payload too large error to HttpExceptionErrorMapper. A review of the exception handling is needed. * Fix issues after merge in Steps.cs * using collection expression here * Code review by @raman-m --------- Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Mar 1, 2024 · 42ac3ca · 42ac3ca
1 parent 319e397
commit 42ac3ca
Show file tree

Hide file tree

Showing 9 changed files with 207 additions and 9 deletions.
diff --git a/src/Ocelot/Errors/OcelotErrorCode.cs b/src/Ocelot/Errors/OcelotErrorCode.cs
@@ -43,5 +43,6 @@ public enum OcelotErrorCode
         ConnectionToDownstreamServiceError = 38,
         CouldNotFindLoadBalancerCreator = 39,
         ErrorInvokingLoadBalancerCreator = 40,
+        PayloadTooLargeError = 41,
     }
 }
diff --git a/src/Ocelot/Middleware/HttpItemsExtensions.cs b/src/Ocelot/Middleware/HttpItemsExtensions.cs
@@ -56,7 +56,7 @@ public static IInternalConfiguration IInternalConfiguration(this IDictionary<obj
         public static List<Error> Errors(this IDictionary<object, object> input)
         {
             var errors = input.Get<List<Error>>("Errors");
-            return errors ?? new List<Error>();
+            return errors ?? [];
         }
 
         public static DownstreamRouteFinder.DownstreamRouteHolder

diff --git a/src/Ocelot/Request/Mapper/PayloadTooLargeError.cs b/src/Ocelot/Request/Mapper/PayloadTooLargeError.cs
@@ -0,0 +1,10 @@
+using Ocelot.Errors;
+
+namespace Ocelot.Request.Mapper;
+
+public class PayloadTooLargeError : Error
+{
+    public PayloadTooLargeError(Exception exception) : base(exception.Message, OcelotErrorCode.PayloadTooLargeError, (int) System.Net.HttpStatusCode.RequestEntityTooLarge)
+    {
+    }
+}
diff --git a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs
@@ -1,6 +1,8 @@
+using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.DependencyInjection;
 using Ocelot.Errors;
 using Ocelot.Errors.QoS;
+using Ocelot.Request.Mapper;
 
 namespace Ocelot.Requester
 {
@@ -9,6 +11,9 @@ public class HttpExceptionToErrorMapper : IExceptionToErrorMapper
         /// <summary>This is a dictionary of custom mappers for exceptions.</summary>
         private readonly Dictionary<Type, Func<Exception, Error>> _mappers;
 
+        /// <summary>413 status.</summary>
+        private const int RequestEntityTooLarge = (int)HttpStatusCode.RequestEntityTooLarge;
+
         public HttpExceptionToErrorMapper(IServiceProvider serviceProvider)
         {
             _mappers = serviceProvider.GetService<Dictionary<Type, Func<Exception, Error>>>();
@@ -39,6 +44,13 @@ public Error Map(Exception exception)
 
             if (type == typeof(HttpRequestException) || type == typeof(TimeoutException))
             {
+                // Inner exception is a BadHttpRequestException, and only this exception exposes the StatusCode property.
+                // We check if the inner exception is a BadHttpRequestException and if the StatusCode is 413, we return a PayloadTooLargeError
+                if (exception.InnerException is BadHttpRequestException { StatusCode: RequestEntityTooLarge })
+                {
+                    return new PayloadTooLargeError(exception);
+                }
+
                 return new ConnectionToDownstreamServiceError(exception);
             }
 

diff --git a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs
@@ -55,6 +55,11 @@ public int Map(List<Error> errors)
                 return 500;
             }
 
+            if (errors.Any(e => e.Code == OcelotErrorCode.PayloadTooLargeError))
+            {
+                return 413;
+            }
+
             return 404;
         }
     }

diff --git a/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj b/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj
@@ -69,6 +69,7 @@
     <PackageReference Include="CacheManager.Microsoft.Extensions.Logging" Version="2.0.0-beta-1629" />
     <PackageReference Include="CacheManager.Serialization.Json" Version="2.0.0-beta-1629" />
     <PackageReference Include="Steeltoe.Discovery.ClientCore" Version="3.2.5" />
+    <PackageReference Include="Xunit.SkippableFact" Version="1.4.13" />
   </ItemGroup>
   <!-- Conditionally obtain references for the net 6.0 target -->
   <ItemGroup Condition=" '$(TargetFramework)' == 'net6.0' ">

diff --git a/test/Ocelot.AcceptanceTests/Requester/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/Requester/PayloadTooLargeTests.cs
@@ -0,0 +1,166 @@
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Hosting;
+using Ocelot.Configuration.File;
+using Ocelot.DependencyInjection;
+using Ocelot.Middleware;
+using System.Runtime.InteropServices;
+using System.Text;
+
+namespace Ocelot.AcceptanceTests.Requester;
+
+public sealed class PayloadTooLargeTests : Steps, IDisposable
+{
+    private readonly ServiceHandler _serviceHandler;
+    private IHost _realServer;
+
+    private const string Payload =
+        "[{\"_id\":\"6540f8ee7beff536c1304e3a\",\"index\":0,\"guid\":\"349307e2-5b1b-4ea9-8e42-d0d26b35059e\",\"isActive\":true,\"balance\":\"$2,458.86\",\"picture\":\"http://placehold.it/32x32\",\"age\":36,\"eyeColor\":\"blue\",\"name\":\"WalshSloan\",\"gender\":\"male\",\"company\":\"ENOMEN\",\"email\":\"[email protected]\",\"phone\":\"+1(818)463-2479\",\"address\":\"863StoneAvenue,Islandia,NewHampshire,7062\",\"about\":\"Exvelitelitutsintlaborisofficialaborisreprehenderittemporsitminim.Exveniamexetesse.Reprehenderitirurealiquipsuntnostrudcillumaliquipsuntvoluptateessenisivoluptatetemporexercitationsint.Laborumexestipsumincididuntvelit.Idnisiproidenttemporelitnonconsequatestnostrudmollit.\\r\\n\",\"registered\":\"2014-11-13T01:53:09-01:00\",\"latitude\":-1.01137,\"longitude\":160.133312,\"tags\":[\"nisi\",\"eu\",\"anim\",\"ipsum\",\"fugiat\",\"excepteur\",\"culpa\"],\"friends\":[{\"id\":0,\"name\":\"MayNoel\"},{\"id\":1,\"name\":\"RichardsDiaz\"},{\"id\":2,\"name\":\"JannieHarvey\"}],\"greeting\":\"Hello,WalshSloan!Youhave6unreadmessages.\",\"favoriteFruit\":\"banana\"},{\"_id\":\"6540f8ee39e04d0ac854b05d\",\"index\":1,\"guid\":\"0f210e11-94a1-45c7-84a4-c2bfcbe0bbfb\",\"isActive\":false,\"balance\":\"$3,371.91\",\"picture\":\"http://placehold.it/32x32\",\"age\":25,\"eyeColor\":\"green\",\"name\":\"FergusonIngram\",\"gender\":\"male\",\"company\":\"DOGSPA\",\"email\":\"[email protected]\",\"phone\":\"+1(804)599-2376\",\"address\":\"130RiverStreet,Bellamy,DistrictOfColumbia,9522\",\"about\":\"Duisvoluptatemollitullamcomollitessedolorvelit.Nonpariaturadipisicingsintdoloranimveniammollitdolorlaborumquisnulla.Ametametametnonlaborevoluptate.Eiusmoddocupidatatveniamirureessequiullamcoincididuntea.\\r\\n\",\"registered\":\"2014-11-01T03:51:36-01:00\",\"latitude\":-57.122954,\"longitude\":-91.22665,\"tags\":[\"nostrud\",\"ipsum\",\"id\",\"cupidatat\",\"consectetur\",\"labore\",\"ullamco\"],\"friends\":[{\"id\":0,\"name\":\"TabithaHuffman\"},{\"id\":1,\"name\":\"LydiaStark\"},{\"id\":2,\"name\":\"FaithStuart\"}],\"greeting\":\"Hello,FergusonIngram!Youhave3unreadmessages.\",\"favoriteFruit\":\"banana\"}]";
+
+    public PayloadTooLargeTests()
+    {
+        _serviceHandler = new ServiceHandler();
+    }
+
+    /// <summary>
+    /// Disposes the instance.
+    /// </summary>
+    /// <remarks>
+    /// Dispose pattern is implemented in the base <see cref="Steps"/> class.
+    /// </remarks>
+    public override void Dispose()
+    {
+        _serviceHandler.Dispose();
+        _realServer?.Dispose();
+        base.Dispose();
+    }
+
+    [Fact]
+    public void Should_throw_payload_too_large_exception_using_kestrel()
+    {
+        var port = PortFinder.GetRandomPort();
+        var route = GivenRoute(port, HttpMethods.Post);
+        var configuration = GivenConfiguration(route);
+
+        this.Given(x => x.GivenThereIsAServiceRunningOn(DownstreamUrl(port)))
+            .And(x => GivenThereIsAConfiguration(configuration))
+            .And(x => GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(1024))
+            .When(x => WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload))))
+            .Then(x => ThenTheStatusCodeShouldBe((int)HttpStatusCode.RequestEntityTooLarge))
+            .BDDfy();
+    }
+
+    [SkippableFact]
+    public void Should_throw_payload_too_large_exception_using_http_sys()
+    {
+        Skip.IfNot(RuntimeInformation.IsOSPlatform(OSPlatform.Windows));
+
+        var port = PortFinder.GetRandomPort();
+        var route = GivenRoute(port, HttpMethods.Post);
+        var configuration = GivenConfiguration(route);
+
+        this.Given(x => x.GivenThereIsAServiceRunningOn(DownstreamUrl(port)))
+            .And(x => GivenThereIsAConfiguration(configuration))
+            .And(x => GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(1024))
+            .When(x => WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload))))
+            .Then(x => ThenTheStatusCodeShouldBe((int)HttpStatusCode.RequestEntityTooLarge))
+            .BDDfy();
+    }
+
+    private static FileRoute GivenRoute(int port, string method = null) => new()
+    {
+        DownstreamPathTemplate = "/",
+        DownstreamHostAndPorts =
+        [
+            new("localhost", port),
+        ],
+        DownstreamScheme = Uri.UriSchemeHttp,
+        UpstreamPathTemplate = "/",
+        UpstreamHttpMethod = [method ?? HttpMethods.Get],
+    };
+
+    private void GivenThereIsAServiceRunningOn(string baseUrl)
+    {
+        _serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, async context =>
+        {
+            context.Response.StatusCode = (int)HttpStatusCode.OK;
+            await context.Response.WriteAsync(string.Empty);
+        });
+    }
+
+    private void GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(long customBodyMaxSize)
+    {
+        _realServer = 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.UseOcelot().Wait();
+                    })
+                    .UseUrls("http://localhost:5001");
+            }).Build();
+        _realServer.Start();
+
+        _ocelotClient = new HttpClient
+        {
+            BaseAddress = new Uri("http://localhost:5001"),
+        };
+    }
+
+    private void GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(long customBodyMaxSize)
+    {
+        _realServer = Host.CreateDefaultBuilder()
+            .ConfigureWebHostDefaults(webBuilder =>
+            {
+#pragma warning disable CA1416 // Validate platform compatibility
+                webBuilder.UseHttpSys(options =>
+                {
+                    options.MaxRequestBodySize = customBodyMaxSize;
+                })
+#pragma warning restore CA1416 // Validate platform compatibility
+                .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.UseOcelot().Wait();
+                })
+                .UseUrls("http://localhost:5001");
+            }).Build();
+        _realServer.Start();
+
+        _ocelotClient = new HttpClient
+        {
+            BaseAddress = new Uri("http://localhost:5001"),
+        };
+    }
+}
diff --git a/test/Ocelot.AcceptanceTests/Steps.cs b/test/Ocelot.AcceptanceTests/Steps.cs
@@ -5,6 +5,7 @@
 using Microsoft.AspNetCore.TestHost;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Newtonsoft.Json;
 using Ocelot.AcceptanceTests.Caching;
@@ -248,13 +249,10 @@ public void GivenOcelotIsRunning()
     /// </summary>
     /// <typeparam name="T">The <see cref="ILoadBalancer"/> type.</typeparam>
     /// <param name="loadBalancerFactoryFunc">The delegate object to load balancer factory.</param>
-    public void GivenOcelotIsRunningWithCustomLoadBalancer<T>(
-        Func<IServiceProvider, DownstreamRoute, IServiceDiscoveryProvider, T> loadBalancerFactoryFunc)
+    public void GivenOcelotIsRunningWithCustomLoadBalancer<T>(Func<IServiceProvider, DownstreamRoute, IServiceDiscoveryProvider, T> loadBalancerFactoryFunc)
         where T : ILoadBalancer
     {
-        _webHostBuilder = new WebHostBuilder();
-
-        _webHostBuilder
+        _webHostBuilder = new WebHostBuilder()
             .ConfigureAppConfiguration((hostingContext, config) =>
             {
                 config.SetBasePath(hostingContext.HostingEnvironment.ContentRootPath);
@@ -272,7 +270,6 @@ public void GivenOcelotIsRunningWithCustomLoadBalancer<T>(
             .Configure(app => { app.UseOcelot().Wait(); });
 
         _ocelotServer = new TestServer(_webHostBuilder);
-
         _ocelotClient = _ocelotServer.CreateClient();
     }
 

diff --git a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs
@@ -81,7 +81,13 @@ public void should_return_bad_gateway_error(OcelotErrorCode errorCode)
         public void should_return_not_found(OcelotErrorCode errorCode)
         {
             ShouldMapErrorToStatusCode(errorCode, HttpStatusCode.NotFound);
-        }
+        }
+
+        [Fact]
+        public void should_return_request_entity_too_large()
+        {
+            ShouldMapErrorsToStatusCode([OcelotErrorCode.PayloadTooLargeError], HttpStatusCode.RequestEntityTooLarge);
+        }
 
         [Fact]
         public void AuthenticationErrorsHaveHighestPriority()
@@ -128,7 +134,7 @@ public void check_we_have_considered_all_errors_in_these_tests()
             // If this test fails then it's because the number of error codes has changed.
             // You should make the appropriate changes to the test cases here to ensure
             // they cover all the error codes, and then modify this assertion.
-            Enum.GetNames(typeof(OcelotErrorCode)).Length.ShouldBe(41, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?");
+            Enum.GetNames(typeof(OcelotErrorCode)).Length.ShouldBe(42, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?");
         }
 
         private void ShouldMapErrorToStatusCode(OcelotErrorCode errorCode, HttpStatusCode expectedHttpStatusCode)