From d2b79f55d9e4d01337bfcd2394098a90ebc376e0 Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Tue, 31 Oct 2023 14:10:12 +0100 Subject: [PATCH 1/6] Adding "quick and..." fix to the payload too large exception, adding two acceptance tests for kestrel and http sys. --- src/Ocelot/Errors/OcelotErrorCode.cs | 1 + .../Request/Mapper/PayloadTooLargeError.cs | 10 ++ .../Responder/ErrorsToHttpStatusCodeMapper.cs | 5 + .../PayloadTooLargeTests.cs | 116 ++++++++++++++++++ test/Ocelot.AcceptanceTests/Steps.cs | 110 ++++++++++++++--- .../ErrorsToHttpStatusCodeMapperTests.cs | 10 +- 6 files changed, 232 insertions(+), 20 deletions(-) create mode 100644 src/Ocelot/Request/Mapper/PayloadTooLargeError.cs create mode 100644 test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs diff --git a/src/Ocelot/Errors/OcelotErrorCode.cs b/src/Ocelot/Errors/OcelotErrorCode.cs index 9063e7142..f74cdace1 100644 --- 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/Request/Mapper/PayloadTooLargeError.cs b/src/Ocelot/Request/Mapper/PayloadTooLargeError.cs new file mode 100644 index 000000000..ba7081982 --- /dev/null +++ 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/Responder/ErrorsToHttpStatusCodeMapper.cs b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs index 2d0e9e8c2..b633e6d9b 100644 --- a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs +++ b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs @@ -55,6 +55,11 @@ public int Map(List errors) return 500; } + if (errors.Any(e => e.Code == OcelotErrorCode.PayloadTooLargeError)) + { + return 413; + } + return 404; } } diff --git a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs new file mode 100644 index 000000000..5cc80c445 --- /dev/null +++ b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs @@ -0,0 +1,116 @@ +using Ocelot.Configuration.File; +using System.Text; +using Microsoft.AspNetCore.Http; + +namespace Ocelot.AcceptanceTests; + +public class PayloadTooLargeTests : IDisposable +{ + private readonly Steps _steps; + private readonly ServiceHandler _serviceHandler; + + 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\":\"walshsloan@enomen.com\",\"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\":\"fergusoningram@dogspa.com\",\"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(); + _steps = new Steps(); + } + + [Fact] + public void should_throw_payload_too_large_exception_using_kestrel() + { + var port = PortFinder.GetRandomPort(); + + var configuration = new FileConfiguration + { + Routes = new List + { + new() + { + DownstreamPathTemplate = "/", + DownstreamHostAndPorts = new List + { + new() + { + Host = "localhost", + Port = port, + }, + }, + DownstreamScheme = "http", + UpstreamPathTemplate = "/", + UpstreamHttpMethod = new List { "POST" }, + }, + }, + }; + + this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) + .And(x => _steps.GivenThereIsAConfiguration(configuration)) + .And(x => _steps.GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(1024)) + .When(x => _steps.WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload)))) + .Then(x => _steps.ThenTheStatusCodeShouldBe((int)HttpStatusCode.RequestEntityTooLarge)) + .BDDfy(); + } + + [Fact] + public void should_throw_payload_too_large_exception_using_http_sys() + { + var port = PortFinder.GetRandomPort(); + + var configuration = new FileConfiguration + { + Routes = new List + { + new() + { + DownstreamPathTemplate = "/", + DownstreamHostAndPorts = new List + { + new() + { + Host = "localhost", + Port = port, + }, + }, + DownstreamScheme = "http", + UpstreamPathTemplate = "/", + UpstreamHttpMethod = new List { "POST" }, + }, + }, + }; + + this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) + .And(x => _steps.GivenThereIsAConfiguration(configuration)) + .And(x => _steps.GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(1024)) + .When(x => _steps.WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload)))) + .Then(x => _steps.ThenTheStatusCodeShouldBe((int)HttpStatusCode.RequestEntityTooLarge)) + .BDDfy(); + } + + private void GivenThereIsAServiceRunningOn(string baseUrl) + { + _serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, async context => + { + context.Response.StatusCode = 200; + await context.Response.WriteAsync(string.Empty); + }); + } + + protected virtual void Dispose(bool disposing) + { + if (!disposing) + { + return; + } + + _serviceHandler?.Dispose(); + _steps?.Dispose(); + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } +} diff --git a/test/Ocelot.AcceptanceTests/Steps.cs b/test/Ocelot.AcceptanceTests/Steps.cs index bac8fbe57..998a31bdd 100644 --- a/test/Ocelot.AcceptanceTests/Steps.cs +++ b/test/Ocelot.AcceptanceTests/Steps.cs @@ -30,6 +30,7 @@ using System.IO.Compression; using System.Net.Http.Headers; using System.Text; +using Microsoft.Extensions.Hosting; using static Ocelot.AcceptanceTests.HttpDelegatingHandlersTests; using ConfigurationBuilder = Microsoft.Extensions.Configuration.ConfigurationBuilder; using CookieHeaderValue = Microsoft.Net.Http.Headers.CookieHeaderValue; @@ -243,16 +244,87 @@ public void GivenOcelotIsRunning() _ocelotClient = _ocelotServer.CreateClient(); } - /// - /// This is annoying cos it should be in the constructor but we need to set up the file before calling startup so its a step. - /// - /// The type. - /// The delegate object to load balancer factory. - public void GivenOcelotIsRunningWithCustomLoadBalancer( - Func loadBalancerFactoryFunc) - where T : ILoadBalancer - { - _webHostBuilder = new WebHostBuilder(); + public 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"), + }; + } + + public void GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(long customBodyMaxSize) + { + _realServer = Host.CreateDefaultBuilder() + .ConfigureWebHostDefaults(webBuilder => + { + webBuilder.UseHttpSys(options => + { + options.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"), + }; + } + + /// + /// This is annoying cos it should be in the constructor but we need to set up the file before calling startup so its a step. + /// + /// The type. + /// The delegate object to load balancer factory. + public void GivenOcelotIsRunningWithCustomLoadBalancer(Func loadBalancerFactoryFunc) + where T : ILoadBalancer + { + _webHostBuilder = new WebHostBuilder(); _webHostBuilder .ConfigureAppConfiguration((hostingContext, config) => @@ -1225,14 +1297,16 @@ protected virtual void Dispose(bool disposing) return; } - if (disposing) - { - _ocelotClient?.Dispose(); - _ocelotServer?.Dispose(); - _ocelotHost?.Dispose(); - DeleteOcelotConfig(); - } + if (disposing) + { + _ocelotClient?.Dispose(); + _ocelotServer?.Dispose(); + _ocelotHost?.Dispose(); + _realServer?.Dispose(); + DeleteOcelotConfig(); + } - _disposedValue = true; + _disposedValue = true; + } } } diff --git a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs index 32c5c8a33..e6c2209e5 100644 --- 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(new List { 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) From 1dde426b2a4df0484643d03e061db05fc5b5f1cd Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Tue, 31 Oct 2023 14:39:45 +0100 Subject: [PATCH 2/6] skipping test if platform is not windows --- test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs index 5cc80c445..0825b6b8d 100644 --- a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs +++ b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs @@ -1,6 +1,7 @@ using Ocelot.Configuration.File; using System.Text; using Microsoft.AspNetCore.Http; +using System.Runtime.InteropServices; namespace Ocelot.AcceptanceTests; @@ -53,9 +54,11 @@ public void should_throw_payload_too_large_exception_using_kestrel() .BDDfy(); } - [Fact] + [SkippableFact] public void should_throw_payload_too_large_exception_using_http_sys() { + Skip.IfNot(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)); + var port = PortFinder.GetRandomPort(); var configuration = new FileConfiguration From df508f968b16dd702eb94f031c44fb03060d33da Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Thu, 14 Dec 2023 01:55:17 +0100 Subject: [PATCH 3/6] Moving Payload too large error to HttpExceptionErrorMapper. A review of the exception handling is needed. --- .../Requester/HttpExceptionToErrorMapper.cs | 10 ++++++ .../Ocelot.AcceptanceTests.csproj | 1 + .../PayloadTooLargeTests.cs | 36 +++++++++---------- test/Ocelot.AcceptanceTests/Steps.cs | 3 +- 4 files changed, 31 insertions(+), 19 deletions(-) diff --git a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs index dad0e856c..fa230ca12 100644 --- 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 { @@ -39,6 +41,14 @@ public Error Map(Exception exception) if (type == typeof(HttpRequestException) || type == typeof(TimeoutException)) { + // the 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: 413 }) + { + return new PayloadTooLargeError(exception); + } + return new ConnectionToDownstreamServiceError(exception); } diff --git a/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj b/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj index 390879fc4..bb1399943 100644 --- a/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj +++ b/test/Ocelot.AcceptanceTests/Ocelot.AcceptanceTests.csproj @@ -69,6 +69,7 @@ + diff --git a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs index 0825b6b8d..5155937c4 100644 --- a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs +++ b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs @@ -26,24 +26,24 @@ public void should_throw_payload_too_large_exception_using_kestrel() var configuration = new FileConfiguration { - Routes = new List - { - new() + Routes = + [ + new FileRoute { DownstreamPathTemplate = "/", - DownstreamHostAndPorts = new List - { - new() + DownstreamHostAndPorts = + [ + new FileHostAndPort { Host = "localhost", Port = port, }, - }, + ], DownstreamScheme = "http", UpstreamPathTemplate = "/", - UpstreamHttpMethod = new List { "POST" }, + UpstreamHttpMethod =["POST"], }, - }, + ], }; this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) @@ -63,24 +63,24 @@ public void should_throw_payload_too_large_exception_using_http_sys() var configuration = new FileConfiguration { - Routes = new List - { - new() + Routes = + [ + new FileRoute { DownstreamPathTemplate = "/", - DownstreamHostAndPorts = new List - { - new() + DownstreamHostAndPorts = + [ + new FileHostAndPort { Host = "localhost", Port = port, }, - }, + ], DownstreamScheme = "http", UpstreamPathTemplate = "/", - UpstreamHttpMethod = new List { "POST" }, + UpstreamHttpMethod =["POST"], }, - }, + ], }; this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) diff --git a/test/Ocelot.AcceptanceTests/Steps.cs b/test/Ocelot.AcceptanceTests/Steps.cs index 998a31bdd..39861a140 100644 --- a/test/Ocelot.AcceptanceTests/Steps.cs +++ b/test/Ocelot.AcceptanceTests/Steps.cs @@ -247,7 +247,8 @@ public void GivenOcelotIsRunning() public void GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(long customBodyMaxSize) { _realServer = Host.CreateDefaultBuilder() - .ConfigureWebHostDefaults(webBuilder => + .ConfigureWebHostDefaults( + webBuilder => { webBuilder.UseKestrel().ConfigureKestrel((_, options) => { From 21113dbebf2dfcb1f1e3ea41049d57de83087c93 Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Fri, 1 Mar 2024 09:53:01 +0100 Subject: [PATCH 4/6] Fix issues after merge in Steps.cs --- test/Ocelot.AcceptanceTests/Steps.cs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/Ocelot.AcceptanceTests/Steps.cs b/test/Ocelot.AcceptanceTests/Steps.cs index 39861a140..3376ee622 100644 --- a/test/Ocelot.AcceptanceTests/Steps.cs +++ b/test/Ocelot.AcceptanceTests/Steps.cs @@ -41,6 +41,7 @@ namespace Ocelot.AcceptanceTests; public class Steps : IDisposable { protected TestServer _ocelotServer; + private IHost _realServer; protected HttpClient _ocelotClient; private HttpResponseMessage _response; private HttpContent _postContent; @@ -1298,16 +1299,15 @@ protected virtual void Dispose(bool disposing) return; } - if (disposing) - { - _ocelotClient?.Dispose(); - _ocelotServer?.Dispose(); - _ocelotHost?.Dispose(); - _realServer?.Dispose(); - DeleteOcelotConfig(); - } - - _disposedValue = true; + if (disposing) + { + _ocelotClient?.Dispose(); + _ocelotServer?.Dispose(); + _ocelotHost?.Dispose(); + _realServer?.Dispose(); + DeleteOcelotConfig(); } + + _disposedValue = true; } } From 7fd5f3f63e392810b454436f44f9647c58dba42b Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Fri, 1 Mar 2024 10:13:35 +0100 Subject: [PATCH 5/6] using collection expression here --- src/Ocelot/Middleware/HttpItemsExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Ocelot/Middleware/HttpItemsExtensions.cs b/src/Ocelot/Middleware/HttpItemsExtensions.cs index 5d2ada0a5..d8d82ef9d 100644 --- a/src/Ocelot/Middleware/HttpItemsExtensions.cs +++ b/src/Ocelot/Middleware/HttpItemsExtensions.cs @@ -56,7 +56,7 @@ public static IInternalConfiguration IInternalConfiguration(this IDictionary Errors(this IDictionary input) { var errors = input.Get>("Errors"); - return errors ?? new List(); + return errors ?? []; } public static DownstreamRouteFinder.DownstreamRouteHolder From 9724c39f9c040c878281ea1f6656bc8c11d6d3f0 Mon Sep 17 00:00:00 2001 From: Raman Maksimchuk Date: Fri, 1 Mar 2024 13:45:15 +0300 Subject: [PATCH 6/6] Code review by @raman-m --- .../Requester/HttpExceptionToErrorMapper.cs | 10 +- .../PayloadTooLargeTests.cs | 119 ------------- .../Requester/PayloadTooLargeTests.cs | 166 ++++++++++++++++++ test/Ocelot.AcceptanceTests/Steps.cs | 98 ++--------- .../ErrorsToHttpStatusCodeMapperTests.cs | 2 +- 5 files changed, 183 insertions(+), 212 deletions(-) delete mode 100644 test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs create mode 100644 test/Ocelot.AcceptanceTests/Requester/PayloadTooLargeTests.cs diff --git a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs index fa230ca12..5c54d39a4 100644 --- a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs +++ b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs @@ -11,6 +11,9 @@ public class HttpExceptionToErrorMapper : IExceptionToErrorMapper /// This is a dictionary of custom mappers for exceptions. private readonly Dictionary> _mappers; + /// 413 status. + private const int RequestEntityTooLarge = (int)HttpStatusCode.RequestEntityTooLarge; + public HttpExceptionToErrorMapper(IServiceProvider serviceProvider) { _mappers = serviceProvider.GetService>>(); @@ -41,10 +44,9 @@ public Error Map(Exception exception) if (type == typeof(HttpRequestException) || type == typeof(TimeoutException)) { - // the 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: 413 }) + // 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); } diff --git a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs deleted file mode 100644 index 5155937c4..000000000 --- a/test/Ocelot.AcceptanceTests/PayloadTooLargeTests.cs +++ /dev/null @@ -1,119 +0,0 @@ -using Ocelot.Configuration.File; -using System.Text; -using Microsoft.AspNetCore.Http; -using System.Runtime.InteropServices; - -namespace Ocelot.AcceptanceTests; - -public class PayloadTooLargeTests : IDisposable -{ - private readonly Steps _steps; - private readonly ServiceHandler _serviceHandler; - - 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\":\"walshsloan@enomen.com\",\"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\":\"fergusoningram@dogspa.com\",\"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(); - _steps = new Steps(); - } - - [Fact] - public void should_throw_payload_too_large_exception_using_kestrel() - { - var port = PortFinder.GetRandomPort(); - - var configuration = new FileConfiguration - { - Routes = - [ - new FileRoute - { - DownstreamPathTemplate = "/", - DownstreamHostAndPorts = - [ - new FileHostAndPort - { - Host = "localhost", - Port = port, - }, - ], - DownstreamScheme = "http", - UpstreamPathTemplate = "/", - UpstreamHttpMethod =["POST"], - }, - ], - }; - - this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) - .And(x => _steps.GivenThereIsAConfiguration(configuration)) - .And(x => _steps.GivenOcelotIsRunningOnKestrelWithCustomBodyMaxSize(1024)) - .When(x => _steps.WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload)))) - .Then(x => _steps.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 configuration = new FileConfiguration - { - Routes = - [ - new FileRoute - { - DownstreamPathTemplate = "/", - DownstreamHostAndPorts = - [ - new FileHostAndPort - { - Host = "localhost", - Port = port, - }, - ], - DownstreamScheme = "http", - UpstreamPathTemplate = "/", - UpstreamHttpMethod =["POST"], - }, - ], - }; - - this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}")) - .And(x => _steps.GivenThereIsAConfiguration(configuration)) - .And(x => _steps.GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(1024)) - .When(x => _steps.WhenIPostUrlOnTheApiGateway("/", new ByteArrayContent(Encoding.UTF8.GetBytes(Payload)))) - .Then(x => _steps.ThenTheStatusCodeShouldBe((int)HttpStatusCode.RequestEntityTooLarge)) - .BDDfy(); - } - - private void GivenThereIsAServiceRunningOn(string baseUrl) - { - _serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, async context => - { - context.Response.StatusCode = 200; - await context.Response.WriteAsync(string.Empty); - }); - } - - protected virtual void Dispose(bool disposing) - { - if (!disposing) - { - return; - } - - _serviceHandler?.Dispose(); - _steps?.Dispose(); - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } -} diff --git a/test/Ocelot.AcceptanceTests/Requester/PayloadTooLargeTests.cs b/test/Ocelot.AcceptanceTests/Requester/PayloadTooLargeTests.cs new file mode 100644 index 000000000..35c789143 --- /dev/null +++ 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\":\"walshsloan@enomen.com\",\"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\":\"fergusoningram@dogspa.com\",\"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(); + } + + /// + /// Disposes the instance. + /// + /// + /// Dispose pattern is implemented in the base class. + /// + 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 index 3376ee622..4e70e95e1 100644 --- 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; @@ -30,7 +31,6 @@ using System.IO.Compression; using System.Net.Http.Headers; using System.Text; -using Microsoft.Extensions.Hosting; using static Ocelot.AcceptanceTests.HttpDelegatingHandlersTests; using ConfigurationBuilder = Microsoft.Extensions.Configuration.ConfigurationBuilder; using CookieHeaderValue = Microsoft.Net.Http.Headers.CookieHeaderValue; @@ -41,7 +41,6 @@ namespace Ocelot.AcceptanceTests; public class Steps : IDisposable { protected TestServer _ocelotServer; - private IHost _realServer; protected HttpClient _ocelotClient; private HttpResponseMessage _response; private HttpContent _postContent; @@ -245,90 +244,15 @@ public void GivenOcelotIsRunning() _ocelotClient = _ocelotServer.CreateClient(); } - public 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"), - }; - } - - public void GivenOcelotIsRunningOnHttpSysWithCustomBodyMaxSize(long customBodyMaxSize) - { - _realServer = Host.CreateDefaultBuilder() - .ConfigureWebHostDefaults(webBuilder => - { - webBuilder.UseHttpSys(options => - { - options.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"), - }; - } - - /// - /// This is annoying cos it should be in the constructor but we need to set up the file before calling startup so its a step. - /// - /// The type. - /// The delegate object to load balancer factory. - public void GivenOcelotIsRunningWithCustomLoadBalancer(Func loadBalancerFactoryFunc) - where T : ILoadBalancer - { - _webHostBuilder = new WebHostBuilder(); - - _webHostBuilder + /// + /// This is annoying cos it should be in the constructor but we need to set up the file before calling startup so its a step. + /// + /// The type. + /// The delegate object to load balancer factory. + public void GivenOcelotIsRunningWithCustomLoadBalancer(Func loadBalancerFactoryFunc) + where T : ILoadBalancer + { + _webHostBuilder = new WebHostBuilder() .ConfigureAppConfiguration((hostingContext, config) => { config.SetBasePath(hostingContext.HostingEnvironment.ContentRootPath); @@ -346,7 +270,6 @@ public void GivenOcelotIsRunningWithCustomLoadBalancer(Func { app.UseOcelot().Wait(); }); _ocelotServer = new TestServer(_webHostBuilder); - _ocelotClient = _ocelotServer.CreateClient(); } @@ -1304,7 +1227,6 @@ protected virtual void Dispose(bool disposing) _ocelotClient?.Dispose(); _ocelotServer?.Dispose(); _ocelotHost?.Dispose(); - _realServer?.Dispose(); DeleteOcelotConfig(); } diff --git a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs index e6c2209e5..e80551117 100644 --- a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs +++ b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs @@ -86,7 +86,7 @@ public void should_return_not_found(OcelotErrorCode errorCode) [Fact] public void should_return_request_entity_too_large() { - ShouldMapErrorsToStatusCode(new List { OcelotErrorCode.PayloadTooLargeError }, HttpStatusCode.RequestEntityTooLarge); + ShouldMapErrorsToStatusCode([OcelotErrorCode.PayloadTooLargeError], HttpStatusCode.RequestEntityTooLarge); } [Fact]