From 2f0bbbca562b0cd80a54ead193beb91a4fe12424 Mon Sep 17 00:00:00 2001 From: Guillaume Gnaegi <58469901+ggnaegi@users.noreply.github.com> Date: Sat, 4 Nov 2023 09:27:51 +0100 Subject: [PATCH] #1550 #1706 Addressing the QoS options ExceptionsAllowedBeforeBreaking issue (#1753) * When using the QoS option "ExceptionsAllowedBeforeBreaking" the circuit breaker never opens the circuit. * merge issue, PortFinder * some code improvements, using httpresponsemessage status codes as a base for circuit breaker * Adding more unit tests, and trying to mitigate the test issues with the method "GivenThereIsAPossiblyBrokenServiceRunningOn" * fixing some test issues * setting timeout value to 5000 to avoid side effects * again timing issues * timing issues again * ok, first one ok * Revert "ok, first one ok" This reverts commit 2e4a673c28894a39f7e057907badb448247ff9d7. * inline method * putting back logging for http request exception * removing logger configuration, back to default * adding a bit more tests to check the policy wrap * Removing TimeoutStrategy from parameters, it's set by default to pessimistic, at least one policy will be returned, so using First() in circuit breaker and removing the branch Policy == null from delegating handler. * Fix StyleCop warnings * Format parameters * Sort usings * since we might have two policies wrapped, timeout and circuit breaker, we can't use the name CircuitBreaker for polly qos provider, it's not right. Using PollyPolicyWrapper and AsnycPollyPolicy instead. * modifying circuit breaker delegating handler name, usin Polly policies instead * renaming CircuitBreakerFactory to PolicyWrapperFactory in tests * DRY for FileConfiguration, using FileConfigurationFactory * Add copy constructor * Refactor setup * Use expression body for method * Fix acceptance test * IDE1006 Naming rule violation: These words must begin with upper case characters * CA1816 Change ReturnsErrorTests.Dispose() to call GC.SuppressFinalize(object) * Sort usings * Use expression body for method * Return back named arguments --------- Co-authored-by: raman-m --- .../OcelotBuilderExtensions.cs | 2 +- .../PollyPoliciesDelegatingHandler.cs | 19 +++- .../PollyPolicyWrapper.cs | 1 + .../Configuration/File/FileCacheOptions.cs | 8 +- test/Ocelot.AcceptanceTests/PollyQoSTests.cs | 87 +++++++++---------- .../ReturnsErrorTests.cs | 2 +- .../FileConfigurationFluentValidatorTests.cs | 64 +++++++------- .../Polly/PollyQoSProviderTests.cs | 2 +- 8 files changed, 99 insertions(+), 86 deletions(-) diff --git a/src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs b/src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs index a12633683..933e7e07b 100644 --- a/src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs +++ b/src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs @@ -9,7 +9,7 @@ using Ocelot.Requester; using Polly.CircuitBreaker; using Polly.Timeout; - + namespace Ocelot.Provider.Polly; public static class OcelotBuilderExtensions diff --git a/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs b/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs index 03be86495..ff8de7057 100644 --- a/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs +++ b/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs @@ -43,10 +43,21 @@ protected override async Task SendAsync(HttpRequestMessage var qoSProvider = GetQoSProvider(); // At least one policy (timeout) will be returned - // AsyncPollyPolicy can't be null - // AsyncPollyPolicy constructor will throw if no policy is provided - var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy; + // AsyncPollyPolicy can't be null + // AsyncPollyPolicy constructor will throw if no policy is provided + var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy; - return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken)); + return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken)); + } + catch (BrokenCircuitException ex) + { + _logger.LogError("Reached to allowed number of exceptions. Circuit is open", ex); + throw; + } + catch (HttpRequestException ex) + { + _logger.LogError($"Error in {nameof(PollyPoliciesDelegatingHandler)}.{nameof(SendAsync)}", ex); + throw; + } } } diff --git a/src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs b/src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs index 0b3058cbb..6aaf1249a 100644 --- a/src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs +++ b/src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs @@ -11,6 +11,7 @@ public class PollyPolicyWrapper public PollyPolicyWrapper(params IAsyncPolicy[] policies) { var allPolicies = policies.Where(p => p != null).ToArray(); + AsyncPollyPolicy = allPolicies.First(); AsyncPollyPolicy = allPolicies.Length > 1 ? Policy.WrapAsync(allPolicies) : diff --git a/src/Ocelot/Configuration/File/FileCacheOptions.cs b/src/Ocelot/Configuration/File/FileCacheOptions.cs index a1b1deed5..40b74a1fe 100644 --- a/src/Ocelot/Configuration/File/FileCacheOptions.cs +++ b/src/Ocelot/Configuration/File/FileCacheOptions.cs @@ -4,18 +4,20 @@ public class FileCacheOptions { public FileCacheOptions() { + Header = string.Empty; Region = string.Empty; TtlSeconds = 0; } public FileCacheOptions(FileCacheOptions from) { + Header = from.Header; Region = from.Region; TtlSeconds = from.TtlSeconds; } - - public int TtlSeconds { get; set; } - public string Region { get; set; } + public string Header { get; set; } + public string Region { get; set; } + public int TtlSeconds { get; set; } } } diff --git a/test/Ocelot.AcceptanceTests/PollyQoSTests.cs b/test/Ocelot.AcceptanceTests/PollyQoSTests.cs index fda947c81..880614aed 100644 --- a/test/Ocelot.AcceptanceTests/PollyQoSTests.cs +++ b/test/Ocelot.AcceptanceTests/PollyQoSTests.cs @@ -1,6 +1,6 @@ -using Microsoft.AspNetCore.Http; -using Ocelot.Configuration; -using Ocelot.Configuration.File; +using Microsoft.AspNetCore.Http; +using Ocelot.Configuration; +using Ocelot.Configuration.File; namespace Ocelot.AcceptanceTests { @@ -9,11 +9,11 @@ public class PollyQoSTests : IDisposable private readonly Steps _steps; private readonly ServiceHandler _serviceHandler; - public PollyQoSTests() - { - _serviceHandler = new ServiceHandler(); - _steps = new Steps(); - } + public PollyQoSTests() + { + _serviceHandler = new ServiceHandler(); + _steps = new Steps(); + } private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, string httpMethod = nameof(HttpMethods.Get)) => new() @@ -87,24 +87,24 @@ public void Should_open_circuit_breaker_then_close() var port = PortFinder.GetRandomPort(); var configuration = FileConfigurationFactory(port, new QoSOptions(1, 500, 1000, null)); - this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port}", "Hello from Laura")) - .Given(x => _steps.GivenThereIsAConfiguration(configuration)) - .Given(x => _steps.GivenOcelotIsRunningWithPolly()) - .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) - .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) - .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) - .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) - .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) - .Given(x => GivenIWaitMilliseconds(3000)) - .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) - .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) - .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) - .BDDfy(); - } + this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port}", "Hello from Laura")) + .Given(x => _steps.GivenThereIsAConfiguration(configuration)) + .Given(x => _steps.GivenOcelotIsRunningWithPolly()) + .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) + .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) + .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) + .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) + .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) + .Given(x => GivenIWaitMilliseconds(3000)) + .When(x => _steps.WhenIGetUrlOnTheApiGateway("/")) + .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) + .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) + .BDDfy(); + } [Fact] public void Open_circuit_should_not_effect_different_route() @@ -156,7 +156,7 @@ public void Should_timeout_per_default_after_90_seconds() .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable)) .BDDfy(); } - + private static void GivenIWaitMilliseconds(int ms) => Thread.Sleep(ms); private void GivenThereIsABrokenServiceRunningOn(string url) @@ -184,21 +184,20 @@ private void GivenThereIsAPossiblyBrokenServiceRunningOn(string url, string resp }); } - private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout) - { - _serviceHandler.GivenThereIsAServiceRunningOn(url, async context => - { - Thread.Sleep(timeout); - context.Response.StatusCode = statusCode; - await context.Response.WriteAsync(responseBody); - }); - } - - public void Dispose() - { - _serviceHandler?.Dispose(); - _steps.Dispose(); - GC.SuppressFinalize(this); - } - } + private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout) + { + _serviceHandler.GivenThereIsAServiceRunningOn(url, async context => + { + Thread.Sleep(timeout); + context.Response.StatusCode = statusCode; + await context.Response.WriteAsync(responseBody); + }); + } + + public void Dispose() + { + _serviceHandler?.Dispose(); + _steps.Dispose(); + GC.SuppressFinalize(this); + } } diff --git a/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs b/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs index c2157e24d..42da5f474 100644 --- a/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs +++ b/test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs @@ -123,7 +123,7 @@ private void GivenThereIsAServiceRunningOn(string url) public void Dispose() { _serviceHandler?.Dispose(); - _steps.Dispose(); + _steps.Dispose(); GC.SuppressFinalize(this); } } diff --git a/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs b/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs index 77d2bf791..a5d9c00e2 100644 --- a/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs +++ b/test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs @@ -14,7 +14,7 @@ using Ocelot.UnitTests.Requester; using Ocelot.Values; using System.Security.Claims; -using System.Text.Encodings.Web; +using System.Text.Encodings.Web; namespace Ocelot.UnitTests.Configuration.Validation { @@ -25,10 +25,10 @@ public class FileConfigurationFluentValidatorTests private Response _result; private IServiceProvider _provider; private readonly ServiceCollection _services; - private readonly Mock _authProvider; + private readonly Mock _authProvider; public FileConfigurationFluentValidatorTests() - { + { _services = new ServiceCollection(); _authProvider = new Mock(); _provider = _services.BuildServiceProvider(); @@ -39,7 +39,7 @@ public FileConfigurationFluentValidatorTests() [Fact] public void configuration_is_valid_if_service_discovery_options_specified_and_has_service_fabric_as_option() - { + { var route = GivenServiceDiscoveryRoute(); var configuration = GivenAConfiguration(route); configuration.GlobalConfiguration.ServiceDiscoveryProvider = GivenDefaultServiceDiscoveryProvider(); @@ -107,7 +107,7 @@ public void configuration_is_invalid_if_service_discovery_options_specified_dyna [Fact] public void configuration_is_invalid_if_service_discovery_options_specified_but_no_service_discovery_handler_with_matching_name() - { + { var route = GivenServiceDiscoveryRoute(); var configuration = GivenAConfiguration(route); configuration.GlobalConfiguration.ServiceDiscoveryProvider = GivenDefaultServiceDiscoveryProvider(); @@ -123,11 +123,11 @@ public void configuration_is_invalid_if_service_discovery_options_specified_but_ [Fact] public void configuration_is_valid_if_qos_options_specified_and_has_qos_handler() - { + { var route = GivenDefaultRoute("/laura", "/"); route.Key = "Laura"; route.QoSOptions = new FileQoSOptions - { + { TimeoutValue = 1, ExceptionsAllowedBeforeBreaking = 1, }; @@ -145,7 +145,7 @@ public void configuration_is_valid_if_qos_options_specified_globally_and_has_qos route.Key = "Laura"; var configuration = GivenAConfiguration(route); configuration.GlobalConfiguration.QoSOptions = new FileQoSOptions - { + { TimeoutValue = 1, ExceptionsAllowedBeforeBreaking = 1, }; @@ -162,7 +162,7 @@ public void configuration_is_invalid_if_qos_options_specified_but_no_qos_handler var route = GivenDefaultRoute("/laura", "/"); route.Key = "Laura"; route.QoSOptions = new FileQoSOptions - { + { TimeoutValue = 1, ExceptionsAllowedBeforeBreaking = 1, }; @@ -181,7 +181,7 @@ public void configuration_is_invalid_if_qos_options_specified_globally_but_no_qo route.Key = "Laura"; var configuration = GivenAConfiguration(route); configuration.GlobalConfiguration.QoSOptions = new FileQoSOptions - { + { TimeoutValue = 1, ExceptionsAllowedBeforeBreaking = 1, }; @@ -213,7 +213,7 @@ public void configuration_is_valid_if_aggregates_are_valid() "Laura", ], }, - }; + }; this.Given(x => x.GivenAConfiguration(configuration)) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsValid()) @@ -241,7 +241,7 @@ public void configuration_is_invalid_if_aggregates_are_duplicate_of_routes() "Laura", ], }, - }; + }; this.Given(x => x.GivenAConfiguration(configuration)) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsNotValid()) @@ -270,7 +270,7 @@ public void configuration_is_valid_if_aggregates_are_not_duplicate_of_routes() "Laura", ], }, - }; + }; this.Given(x => x.GivenAConfiguration(configuration)) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsValid()) @@ -279,7 +279,7 @@ public void configuration_is_valid_if_aggregates_are_not_duplicate_of_routes() [Fact] public void configuration_is_invalid_if_aggregates_are_duplicate_of_aggregates() - { + { var route = GivenDefaultRoute("/laura", "/"); route.Key = "Laura"; var route2 = GivenDefaultRoute("/lol", "/"); @@ -307,7 +307,7 @@ public void configuration_is_invalid_if_aggregates_are_duplicate_of_aggregates() "Laura", ], }, - }; + }; this.Given(x => x.GivenAConfiguration(configuration)) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsNotValid()) @@ -362,7 +362,7 @@ public void configuration_is_invalid_if_aggregate_has_routes_with_specific_reque "Laura", ], }, - }; + }; this.Given(x => x.GivenAConfiguration(configuration)) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsNotValid()) @@ -408,7 +408,7 @@ public void configuration_is_invalid_without_slash_prefix_downstream_path_templa [Fact] public void configuration_is_invalid_without_slash_prefix_upstream_path_template() - { + { this.Given(x => x.GivenAConfiguration(GivenDefaultRoute("api/prod/", "/api/products/"))) .When(x => x.WhenIValidateTheConfiguration()) .Then(x => x.ThenTheResultIsNotValid()) @@ -438,7 +438,7 @@ public void configuration_is_invalid_if_downstream_url_contains_forward_slash_th [Fact] public void configuration_is_valid_with_valid_authentication_provider() - { + { var route = GivenDefaultRoute(); route.AuthenticationOptions.AuthenticationProviderKey = "Test"; this.Given(x => x.GivenAConfiguration(route)) @@ -466,7 +466,7 @@ public void configuration_is_invalid_with_invalid_authentication_provider() [Fact] public void configuration_is_not_valid_with_duplicate_routes_all_verbs() - { + { var route = GivenDefaultRoute(); var duplicate = GivenDefaultRoute(); duplicate.DownstreamPathTemplate = "/www/test/"; @@ -608,14 +608,14 @@ public void configuration_is_valid_with_using_service_discovery_and_service_name .Then(x => x.ThenTheResultIsValid()) .BDDfy(); } - + private const string Empty = ""; [Theory] [InlineData(null)] [InlineData(Empty)] public void configuration_is_invalid_when_not_using_service_discovery_and_host(string downstreamHost) - { + { var route = GivenDefaultRoute(); route.DownstreamHostAndPorts[0].Host = downstreamHost; this.Given(x => x.GivenAConfiguration(route)) @@ -623,8 +623,8 @@ public void configuration_is_invalid_when_not_using_service_discovery_and_host(s .Then(x => x.ThenTheResultIsNotValid()) .And(x => x.ThenTheErrorMessageAtPositionIs(0, "When not using service discovery Host must be set on DownstreamHostAndPorts if you are not using Route.Host or Ocelot cannot find your service!")) .BDDfy(); - } - + } + [Theory] [InlineData(null, true)] [InlineData(Empty, true)] @@ -651,14 +651,14 @@ public void HaveServiceDiscoveryProviderRegistered_RouteServiceName_Validated(st [InlineData(true, "type", false)] [InlineData(true, "servicefabric", true)] public void HaveServiceDiscoveryProviderRegistered_ServiceDiscoveryProvider_Validated(bool create, string type, bool valid) - { + { // Arrange var route = GivenServiceDiscoveryRoute(); var config = GivenAConfiguration(route); var provider = create ? GivenDefaultServiceDiscoveryProvider() : null; config.GlobalConfiguration.ServiceDiscoveryProvider = provider; if (create && provider != null) - { + { provider.Type = type; } @@ -757,8 +757,8 @@ public void configuration_is_invalid_when_placeholder_is_used_twice_in_upstream_ .Then(x => x.ThenTheResultIsNotValid()) .And(x => x.ThenTheErrorMessageAtPositionIs(0, "route /foo/bar/{everything}/{everything} has duplicated placeholder")) .BDDfy(); - } - + } + private FileRoute GivenDefaultRoute() => GivenDefaultRoute(null, null); private FileRoute GivenDefaultRoute(string upstreamPathTemplate, string downstreamPathTemplate) => new() @@ -784,7 +784,7 @@ public void configuration_is_invalid_when_placeholder_is_used_twice_in_upstream_ private void GivenAConfiguration(FileConfiguration fileConfiguration) { _fileConfiguration = fileConfiguration; - } + } private FileConfiguration GivenAConfiguration(params FileRoute[] routes) { @@ -793,7 +793,7 @@ private FileConfiguration GivenAConfiguration(params FileRoute[] routes) _fileConfiguration = config; return config; } - + private FileServiceDiscoveryProvider GivenDefaultServiceDiscoveryProvider() => new FileServiceDiscoveryProvider { Scheme = "https", @@ -859,7 +859,7 @@ private class FakeServiceDiscoveryProvider : IServiceDiscoveryProvider private class TestOptions : AuthenticationSchemeOptions { } private class TestHandler : AuthenticationHandler - { + { // https://learn.microsoft.com/en-us/dotnet/core/compatibility/aspnet-core/8.0/isystemclock-obsolete // .NET 8.0: TimeProvider is now a settable property on the Options classes for the authentication and identity components. // It can be set directly or by registering a provider in the dependency injection container. @@ -870,9 +870,9 @@ public TestHandler(IOptionsMonitor options, ILoggerFactory logger, #else public TestHandler(IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock) : base(options, logger, encoder, clock) { - } + } #endif - + protected override Task HandleAuthenticateAsync() { var principal = new ClaimsPrincipal(); diff --git a/test/Ocelot.UnitTests/Polly/PollyQoSProviderTests.cs b/test/Ocelot.UnitTests/Polly/PollyQoSProviderTests.cs index 6ebfeee93..e7892b684 100644 --- a/test/Ocelot.UnitTests/Polly/PollyQoSProviderTests.cs +++ b/test/Ocelot.UnitTests/Polly/PollyQoSProviderTests.cs @@ -240,5 +240,5 @@ private static PollyPolicyWrapper PolicyWrapperFactory(stri pollyPolicyWrapper.AsyncPollyPolicy.ShouldNotBeNull(); return pollyPolicyWrapper; - } + } }