#1550 #1706 Addressing the QoS options ExceptionsAllowedBeforeBreakin…

…g 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 2e4a673.

* 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 <[email protected]>

.gitignore

@@ -255,3 +255,4 @@ _templates/
 
 # Test Results
 *.trx
+/Ocelot.sln.DotSettings

src/Ocelot.Provider.Polly/Interfaces/IPollyQoSProvider.cs

@@ -1,6 +1,9 @@
-namespace Ocelot.Provider.Polly.Interfaces;
+using Ocelot.Configuration;
 
-public interface IPollyQoSProvider
+namespace Ocelot.Provider.Polly.Interfaces;
+
+public interface IPollyQoSProvider<TResult>
+    where TResult : class
 {
-    CircuitBreaker CircuitBreaker { get; }
+    PollyPolicyWrapper<TResult> GetPollyPolicyWrapper(DownstreamRoute route);
 }

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

@@ -1,32 +1,35 @@
-using Microsoft.Extensions.DependencyInjection;
+using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.DependencyInjection;
 using Ocelot.Configuration;
 using Ocelot.DependencyInjection;
 using Ocelot.Errors;
 using Ocelot.Logging;
+using Ocelot.Provider.Polly.Interfaces;
 using Ocelot.Requester;
 using Polly.CircuitBreaker;
 using Polly.Timeout;
-
-namespace Ocelot.Provider.Polly
-{
-    public static class OcelotBuilderExtensions
-    {
-        public static IOcelotBuilder AddPolly(this IOcelotBuilder builder)
-        {
-            var errorMapping = new Dictionary<Type, Func<Exception, Error>>
-            {
-                {typeof(TaskCanceledException), e => new RequestTimedOutError(e)},
-                {typeof(TimeoutRejectedException), e => new RequestTimedOutError(e)},
-                {typeof(BrokenCircuitException), e => new RequestTimedOutError(e)},
-            };
-
-            builder.Services
-                .AddSingleton(errorMapping)
-                .AddSingleton<QosDelegatingHandlerDelegate>(GetDelegatingHandler);
-            return builder;
-        }
 
-        private static DelegatingHandler GetDelegatingHandler(DownstreamRoute route, IOcelotLoggerFactory logger)
-            => new PollyCircuitBreakingDelegatingHandler(new PollyQoSProvider(route, logger), logger);
-    }
+namespace Ocelot.Provider.Polly;
+
+public static class OcelotBuilderExtensions
+{
+    public static IOcelotBuilder AddPolly(this IOcelotBuilder builder)
+    {
+        var errorMapping = new Dictionary<Type, Func<Exception, Error>>
+        {
+            { typeof(TaskCanceledException), e => new RequestTimedOutError(e) },
+            { typeof(TimeoutRejectedException), e => new RequestTimedOutError(e) },
+            { typeof(BrokenCircuitException), e => new RequestTimedOutError(e) },
+            { typeof(BrokenCircuitException<HttpResponseMessage>), e => new RequestTimedOutError(e) },
+        };
+
+        builder.Services
+            .AddSingleton(errorMapping)
+            .AddSingleton<IPollyQoSProvider<HttpResponseMessage>, PollyQoSProvider>()
+            .AddSingleton<QosDelegatingHandlerDelegate>(GetDelegatingHandler);
+        return builder;
+    }
+
+    private static DelegatingHandler GetDelegatingHandler(DownstreamRoute route, IHttpContextAccessor contextAccessor, IOcelotLoggerFactory loggerFactory) 
+        => new PollyPoliciesDelegatingHandler(route, contextAccessor, loggerFactory);
 }

src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs

@@ -0,0 +1,56 @@
+using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.DependencyInjection;
+using Ocelot.Configuration;
+using Ocelot.Logging;
+using Ocelot.Provider.Polly.Interfaces;
+using Polly.CircuitBreaker;
+using System.Diagnostics;
+
+namespace Ocelot.Provider.Polly;
+
+public class PollyPoliciesDelegatingHandler : DelegatingHandler
+{
+    private readonly DownstreamRoute _route;
+    private readonly IHttpContextAccessor _contextAccessor;
+    private readonly IOcelotLogger _logger;
+
+    public PollyPoliciesDelegatingHandler(
+        DownstreamRoute route,
+        IHttpContextAccessor contextAccessor,
+        IOcelotLoggerFactory loggerFactory)
+    {
+        _route = route;
+        _contextAccessor = contextAccessor;
+        _logger = loggerFactory.CreateLogger<PollyPoliciesDelegatingHandler>();
+    }
+
+    private IPollyQoSProvider<HttpResponseMessage> GetQoSProvider()
+    {
+        Debug.Assert(_contextAccessor.HttpContext != null, "_contextAccessor.HttpContext != null");
+        return _contextAccessor.HttpContext.RequestServices.GetService<IPollyQoSProvider<HttpResponseMessage>>();
+    }
+
+    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
+        CancellationToken cancellationToken)
+    {
+        var qoSProvider = GetQoSProvider();
+        try
+        {
+            // 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;
+            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;
+        }
+    }
+}

src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs

@@ -0,0 +1,23 @@
+namespace Ocelot.Provider.Polly;
+
+public class PollyPolicyWrapper<TResult>
+    where TResult : class
+{
+    /// <summary>
+    /// Initializes a new instance of the <see cref="PollyPolicyWrapper{TResult}"/> class.
+    /// We expect at least one policy to be passed in, default can't be null.
+    /// </summary>
+    /// <param name="policies">The policies with at least a <see cref="Policy.Timeout(int)"/> policy.</param>
+    public PollyPolicyWrapper(params IAsyncPolicy<TResult>[] policies)
+    {
+        var allPolicies = policies.Where(p => p != null).ToArray();
+        AsyncPollyPolicy = allPolicies.First();
+
+        if (allPolicies.Length > 1)
+        {
+            AsyncPollyPolicy = Policy.WrapAsync(allPolicies);
+        }
+    }
+
+    public IAsyncPolicy<TResult> AsyncPollyPolicy { get; }
+}

src/Ocelot.Provider.Polly/PollyQoSProvider.cs

@@ -1,54 +1,78 @@
-using Ocelot.Configuration;
-using Ocelot.Logging;
-using Ocelot.Provider.Polly.Interfaces;
-using Polly.CircuitBreaker;
-using Polly.Timeout;
-
-namespace Ocelot.Provider.Polly
-{
-    public class PollyQoSProvider : IPollyQoSProvider
-    {
-        public PollyQoSProvider(DownstreamRoute route, IOcelotLoggerFactory loggerFactory)
-        {
-            AsyncCircuitBreakerPolicy circuitBreakerPolicy = null;
-            if (route.QosOptions.ExceptionsAllowedBeforeBreaking > 0)
+using Ocelot.Configuration;
+using Ocelot.Logging;
+using Ocelot.Provider.Polly.Interfaces;
+using Polly.CircuitBreaker;
+using Polly.Timeout;
+using System.Net;
+
+namespace Ocelot.Provider.Polly;
+
+public class PollyQoSProvider : IPollyQoSProvider<HttpResponseMessage>
+{
+    private readonly Dictionary<string, PollyPolicyWrapper<HttpResponseMessage>> _policyWrappers = new();
+    private readonly object _lockObject = new();
+    private readonly IOcelotLogger _logger;
+
+    private readonly HashSet<HttpStatusCode> _serverErrorCodes = new()
+    {
+        HttpStatusCode.InternalServerError,
+        HttpStatusCode.NotImplemented,
+        HttpStatusCode.BadGateway,
+        HttpStatusCode.ServiceUnavailable,
+        HttpStatusCode.GatewayTimeout,
+        HttpStatusCode.HttpVersionNotSupported,
+        HttpStatusCode.VariantAlsoNegotiates,
+        HttpStatusCode.InsufficientStorage,
+        HttpStatusCode.LoopDetected,
+    };
+
+    public PollyQoSProvider(IOcelotLoggerFactory loggerFactory)
+    {
+        _logger = loggerFactory.CreateLogger<PollyQoSProvider>();
+    }
+
+    private static string GetRouteName(DownstreamRoute route)
+        => string.IsNullOrWhiteSpace(route.ServiceName)
+            ? route.UpstreamPathTemplate?.Template ?? route.DownstreamPathTemplate?.Value ?? string.Empty
+            : route.ServiceName;
+
+    public PollyPolicyWrapper<HttpResponseMessage> GetPollyPolicyWrapper(DownstreamRoute route)
+    {
+        lock (_lockObject)
+        {
+            var currentRouteName = GetRouteName(route);
+            if (!_policyWrappers.ContainsKey(currentRouteName))
             {
-                var info = $"Route: {GetRouteName(route)}; Breaker logging in {nameof(PollyQoSProvider)}: ";
-                var logger = loggerFactory.CreateLogger<PollyQoSProvider>();
-                circuitBreakerPolicy = Policy
-                    .Handle<HttpRequestException>()
-                    .Or<TimeoutRejectedException>()
-                    .Or<TimeoutException>()
-                    .CircuitBreakerAsync(
-                        exceptionsAllowedBeforeBreaking: route.QosOptions.ExceptionsAllowedBeforeBreaking,
-                        durationOfBreak: TimeSpan.FromMilliseconds(route.QosOptions.DurationOfBreak),
-                        onBreak: (ex, breakDelay) =>
-                            logger.LogError(info + $"Breaking the circuit for {breakDelay.TotalMilliseconds} ms!", ex),
-                        onReset: () =>
-                            logger.LogDebug(info + "Call OK! Closed the circuit again."),
-                        onHalfOpen: () =>
-                            logger.LogDebug(info + "Half-open; Next call is a trial.")
-                    );
-            }
-
-            _ = Enum.TryParse(route.QosOptions.TimeoutStrategy, out TimeoutStrategy strategy);
-            var timeoutPolicy = Policy.TimeoutAsync(TimeSpan.FromMilliseconds(route.QosOptions.TimeoutValue), strategy);
-            CircuitBreaker = new CircuitBreaker(circuitBreakerPolicy, timeoutPolicy);
+                _policyWrappers.Add(currentRouteName, PollyPolicyWrapperFactory(route));
+            }
+
+            return _policyWrappers[currentRouteName];
         }
+    }
+
+    private PollyPolicyWrapper<HttpResponseMessage> PollyPolicyWrapperFactory(DownstreamRoute route)
+    {
+        AsyncCircuitBreakerPolicy<HttpResponseMessage> exceptionsAllowedBeforeBreakingPolicy = null;
+        if (route.QosOptions.ExceptionsAllowedBeforeBreaking > 0)
+        {
+            var info = $"Route: {GetRouteName(route)}; Breaker logging in {nameof(PollyQoSProvider)}: ";
+
+            exceptionsAllowedBeforeBreakingPolicy = Policy
+                .HandleResult<HttpResponseMessage>(r => _serverErrorCodes.Contains(r.StatusCode))
+                .Or<TimeoutRejectedException>()
+                .Or<TimeoutException>()
+                .CircuitBreakerAsync(route.QosOptions.ExceptionsAllowedBeforeBreaking,
+                    durationOfBreak: TimeSpan.FromMilliseconds(route.QosOptions.DurationOfBreak),
+                    onBreak: (ex, breakDelay) => _logger.LogError(info + $"Breaking the circuit for {breakDelay.TotalMilliseconds} ms!", ex.Exception),
+                    onReset: () => _logger.LogDebug(info + "Call OK! Closed the circuit again."),
+                    onHalfOpen: () => _logger.LogDebug(info + "Half-open; Next call is a trial."));
+        }
+
+        var timeoutPolicy = Policy
+            .TimeoutAsync<HttpResponseMessage>(
+                TimeSpan.FromMilliseconds(route.QosOptions.TimeoutValue), 
+                TimeoutStrategy.Pessimistic);
 
-        private const string ObsoleteConstructorMessage = $"Use the constructor {nameof(PollyQoSProvider)}({nameof(DownstreamRoute)} route, {nameof(IOcelotLoggerFactory)} loggerFactory)!";
-
-        [Obsolete(ObsoleteConstructorMessage)]
-        public PollyQoSProvider(AsyncCircuitBreakerPolicy circuitBreakerPolicy, AsyncTimeoutPolicy timeoutPolicy, IOcelotLogger logger)
-        {
-            throw new NotSupportedException(ObsoleteConstructorMessage);
-        }
-
-        public CircuitBreaker CircuitBreaker { get; }
-
-        private static string GetRouteName(DownstreamRoute route)
-            => string.IsNullOrWhiteSpace(route.ServiceName)
-                ? route.UpstreamPathTemplate?.Template ?? route.DownstreamPathTemplate?.Value ?? string.Empty
-                : route.ServiceName;
-    }
-}
+        return new PollyPolicyWrapper<HttpResponseMessage>(exceptionsAllowedBeforeBreakingPolicy, timeoutPolicy);
+    }
+}

src/Ocelot/Configuration/File/FileAuthenticationOptions.cs

@@ -6,6 +6,12 @@ public FileAuthenticationOptions()
         {
             AllowedScopes = new List<string>();
         }
+
+        public FileAuthenticationOptions(FileAuthenticationOptions from)
+        {
+            AllowedScopes = new(from.AllowedScopes);
+            AuthenticationProviderKey = from.AuthenticationProviderKey;
+        }
 
         public string AuthenticationProviderKey { get; set; }
         public List<string> AllowedScopes { get; set; }

src/Ocelot/Configuration/File/FileCacheOptions.cs

@@ -1,9 +1,23 @@
 namespace Ocelot.Configuration.File
 {
     public class FileCacheOptions
-    {
-        public int TtlSeconds { get; set; }
-        public string Region { get; set; }
+    {
+        public FileCacheOptions()
+        {
+            Header = string.Empty;
+            Region = string.Empty;
+            TtlSeconds = 0;
+        }
+
+        public FileCacheOptions(FileCacheOptions from)
+        {
+            Header = from.Header;
+            Region = from.Region;
+            TtlSeconds = from.TtlSeconds;
+        }
+
         public string Header { get; set; }
+        public string Region { get; set; }
+        public int TtlSeconds { get; set; }
     }
 }