#2119 Review load balancing (2nd round) and redesign DefaultConsulServiceBuilder with ConsulProviderFactory refactoring to make it thread safe and friendly

ReleaseNotes.md

@@ -1 +1,8 @@
 Technical release, version {0}
+
+### Breaking changes
+
+- The `ILoadBalancer` interface: The `Lease` method was renamed to `LeaseAsync`.
+  Interface FQN: `Ocelot.LoadBalancer.LoadBalancers.ILoadBalancer`
+  Method FQN:  `Ocelot.LoadBalancer.LoadBalancers.ILoadBalancer.LeaseAsync`
+- TO BE Written

docs/features/servicediscovery.rst

@@ -246,10 +246,12 @@ However, the quickest and most streamlined approach is to inherit directly from
 
     public class MyConsulServiceBuilder : DefaultConsulServiceBuilder
     {
-        public MyConsulServiceBuilder(Func<ConsulRegistryConfiguration> configurationFactory, IConsulClientFactory clientFactory, IOcelotLoggerFactory loggerFactory)
-            : base(configurationFactory, clientFactory, loggerFactory) { }
+        public MyConsulServiceBuilder(IHttpContextAccessor contextAccessor, IConsulClientFactory clientFactory, IOcelotLoggerFactory loggerFactory)
+            : base(contextAccessor, clientFactory, loggerFactory) { }
+
         // I want to use the agent service IP address as the downstream hostname
-        protected override string GetDownstreamHost(ServiceEntry entry, Node node) => entry.Service.Address;
+        protected override string GetDownstreamHost(ServiceEntry entry, Node node)
+            => entry.Service.Address;
     }
 
 **Second**, we must inject the new behavior into DI, as demonstrated in the Ocelot versus Consul setup:
@@ -543,7 +545,7 @@ But you can leave this ``Type`` option for compatibility between both designs.
 .. _KV Store: https://developer.hashicorp.com/consul/docs/dynamic-app-config/kv
 .. _3 seconds TTL: https://github.com/search?q=repo%3AThreeMammals%2FOcelot+TimeSpan.FromSeconds%283%29&type=code
 .. _catalog nodes: https://developer.hashicorp.com/consul/api-docs/catalog#list-nodes
-.. _the acceptance test: https://github.com/search?q=repo%3AThreeMammals%2FOcelot+Should_return_service_address_by_overridden_service_builder_when_there_is_a_node&type=code
+.. _the acceptance test: https://github.com/search?q=repo%3AThreeMammals%2FOcelot+ShouldReturnServiceAddressByOverriddenServiceBuilderWhenThereIsANode&type=code
 .. _346: https://github.com/ThreeMammals/Ocelot/issues/346
 .. _909: https://github.com/ThreeMammals/Ocelot/pull/909
 .. _954: https://github.com/ThreeMammals/Ocelot/issues/954

src/Ocelot.Provider.Consul/Consul.cs

@@ -33,21 +33,16 @@ public virtual async Task<List<Service>> GetAsync()
 
         var entries = entriesTask.Result.Response ?? Array.Empty<ServiceEntry>();
         var nodes = nodesTask.Result.Response ?? Array.Empty<Node>();
-        var services = new List<Service>();
-
-        if (entries.Length != 0)
-        {
-            _logger.LogDebug(() => $"{nameof(Consul)} Provider: Found total {entries.Length} service entries for '{_configuration.KeyOfServiceInConsul}' service.");
-            _logger.LogDebug(() => $"{nameof(Consul)} Provider: Found total {nodes.Length} catalog nodes.");
-            var collection = BuildServices(entries, nodes);
-            services.AddRange(collection);
-        }
-        else
+        if (entries.Length == 0)
         {
             _logger.LogWarning(() => $"{nameof(Consul)} Provider: No service entries found for '{_configuration.KeyOfServiceInConsul}' service!");
+            return new();
         }
 
-        return services;
+        _logger.LogDebug(() => $"{nameof(Consul)} Provider: Found total {entries.Length} service entries for '{_configuration.KeyOfServiceInConsul}' service.");
+        _logger.LogDebug(() => $"{nameof(Consul)} Provider: Found total {nodes.Length} catalog nodes.");
+        return BuildServices(entries, nodes)
+            .ToList();
     }
 
     protected virtual IEnumerable<Service> BuildServices(ServiceEntry[] entries, Node[] nodes)

src/Ocelot.Provider.Consul/ConsulClientFactory.cs

@@ -4,10 +4,15 @@ namespace Ocelot.Provider.Consul;
 
 public class ConsulClientFactory : IConsulClientFactory
 {
+    // TODO We need this overloaded method -> 
+    //public IConsulClient Get(ServiceProviderConfiguration config)
     public IConsulClient Get(ConsulRegistryConfiguration config)
         => new ConsulClient(c => OverrideConfig(c, config));
 
-    private static void OverrideConfig(ConsulClientConfiguration to, ConsulRegistryConfiguration from)
+    // TODO ->
+    //private static void OverrideConfig(ConsulClientConfiguration to, ServiceProviderConfiguration from)
+    // Factory which consumes concrete types is a bad factory! A more abstract types are required
+    private static void OverrideConfig(ConsulClientConfiguration to, ConsulRegistryConfiguration from) // TODO Why ConsulRegistryConfiguration? We use ServiceProviderConfiguration props only! :)
     {
         to.Address = new Uri($"{from.Scheme}://{from.Host}:{from.Port}");
 

src/Ocelot.Provider.Consul/ConsulProviderFactory.cs

@@ -1,41 +1,43 @@
-using Microsoft.Extensions.DependencyInjection;
+using Microsoft.AspNetCore.Http;
+using Microsoft.Extensions.DependencyInjection;
 using Ocelot.Configuration;
 using Ocelot.Logging;
 using Ocelot.Provider.Consul.Interfaces;
 using Ocelot.ServiceDiscovery.Providers;
 
 namespace Ocelot.Provider.Consul;
 
-public static class ConsulProviderFactory
+/// <summary>
+/// TODO It must be refactored converting to real factory-class and add to DI.
+/// </summary>
+/// <remarks>
+/// Must inherit from <see cref="IServiceDiscoveryProviderFactory"/> interface.
+/// Also the <see cref="ServiceDiscoveryFinderDelegate"/> must be removed from the design.
+/// </remarks>
+public static class ConsulProviderFactory // TODO : IServiceDiscoveryProviderFactory
 {
-    /// <summary>
-    /// String constant used for provider type definition.
-    /// </summary>
+    /// <summary>String constant used for provider type definition.</summary>
     public const string PollConsul = nameof(Provider.Consul.PollConsul);
 
-    private static readonly List<PollConsul> ServiceDiscoveryProviders = new();
-    private static readonly object LockObject = new();
+    private static readonly List<PollConsul> ServiceDiscoveryProviders = new(); // TODO It must be scoped service in DI-container
+    private static readonly object SyncRoot = new();
 
     public static ServiceDiscoveryFinderDelegate Get { get; } = CreateProvider;
-
-    private static ConsulRegistryConfiguration configuration;
-    private static ConsulRegistryConfiguration ConfigurationGetter() => configuration;
-    public static Func<ConsulRegistryConfiguration> GetConfiguration { get; } = ConfigurationGetter;
-
-    private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provider,
-        ServiceProviderConfiguration config, DownstreamRoute route)
+    private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provider, ServiceProviderConfiguration config, DownstreamRoute route)
     {
         var factory = provider.GetService<IOcelotLoggerFactory>();
         var consulFactory = provider.GetService<IConsulClientFactory>();
+        var contextAccessor = provider.GetService<IHttpContextAccessor>();
 
-        configuration = new ConsulRegistryConfiguration(config.Scheme, config.Host, config.Port, route.ServiceName, config.Token);
-        var serviceBuilder = provider.GetService<IConsulServiceBuilder>();
+        var configuration = new ConsulRegistryConfiguration(config.Scheme, config.Host, config.Port, route.ServiceName, config.Token); // TODO Why not to pass 2 args only: config, route? LoL
+        contextAccessor.HttpContext.Items[nameof(ConsulRegistryConfiguration)] = configuration; // initialize data
+        var serviceBuilder = provider.GetService<IConsulServiceBuilder>(); // consume data in default/custom builder
 
-        var consulProvider = new Consul(configuration, factory, consulFactory, serviceBuilder);
+        var consulProvider = new Consul(configuration, factory, consulFactory, serviceBuilder); // TODO It must be added to DI-container!
 
         if (PollConsul.Equals(config.Type, StringComparison.OrdinalIgnoreCase))
         {
-            lock (LockObject)
+            lock (SyncRoot)
             {
                 var discoveryProvider = ServiceDiscoveryProviders.FirstOrDefault(x => x.ServiceName == route.ServiceName);
                 if (discoveryProvider != null)

src/Ocelot.Provider.Consul/ConsulRegistryConfiguration.cs

@@ -1,6 +1,6 @@
 namespace Ocelot.Provider.Consul;
 
-public class ConsulRegistryConfiguration
+public class ConsulRegistryConfiguration // TODO Inherit from ServiceProviderConfiguration ?
 {
     /// <summary>
     /// Consul HTTP client default port.
@@ -12,6 +12,7 @@ public class ConsulRegistryConfiguration
 
     public ConsulRegistryConfiguration(string scheme, string host, int port, string keyOfServiceInConsul, string token)
     {
+        // TODO Why not to encapsulate this biz logic right in ConsulProviderFactory? LoL
         Host = string.IsNullOrEmpty(host) ? "localhost" : host;
         Port = port > 0 ? port : DefaultHttpPort;
         Scheme = string.IsNullOrEmpty(scheme) ? Uri.UriSchemeHttp : scheme;

src/Ocelot.Provider.Consul/DefaultConsulServiceBuilder.cs

@@ -1,4 +1,5 @@
-using Ocelot.Infrastructure.Extensions;
+using Microsoft.AspNetCore.Http;
+using Ocelot.Infrastructure.Extensions;
 using Ocelot.Logging;
 using Ocelot.Provider.Consul.Interfaces;
 using Ocelot.Values;
@@ -7,23 +8,31 @@ namespace Ocelot.Provider.Consul;
 
 public class DefaultConsulServiceBuilder : IConsulServiceBuilder
 {
-    private readonly ConsulRegistryConfiguration _configuration;
-    private readonly IConsulClient _client;
-    private readonly IOcelotLogger _logger;
+    private readonly HttpContext _context;
+    private readonly IConsulClientFactory _clientFactory;
+    private readonly IOcelotLoggerFactory _loggerFactory;
+
+    private ConsulRegistryConfiguration _configuration;
+    private IConsulClient _client;
+    private IOcelotLogger _logger;
 
     public DefaultConsulServiceBuilder(
-        Func<ConsulRegistryConfiguration> configurationFactory,
+        IHttpContextAccessor contextAccessor,
         IConsulClientFactory clientFactory,
         IOcelotLoggerFactory loggerFactory)
     {
-        _configuration = configurationFactory.Invoke();
-        _client = clientFactory.Get(_configuration);
-        _logger = loggerFactory.CreateLogger<DefaultConsulServiceBuilder>();
+        _context = contextAccessor.HttpContext;
+        _clientFactory = clientFactory;
+        _loggerFactory = loggerFactory;
     }
 
-    public ConsulRegistryConfiguration Configuration => _configuration;
-    protected IConsulClient Client => _client;
-    protected IOcelotLogger Logger => _logger;
+    // TODO See comment in the interface about the privacy. The goal is to eliminate IBC!
+    // So, we need more abstract type, and ServiceProviderConfiguration is a good choice. The rest of props can be obtained from HttpContext
+    protected /*public*/ ConsulRegistryConfiguration Configuration => _configuration
+        ??= _context.Items.TryGetValue(nameof(ConsulRegistryConfiguration), out var value)
+            ? value as ConsulRegistryConfiguration : default;
+    protected IConsulClient Client => _client ??= _clientFactory.Get(Configuration);
+    protected IOcelotLogger Logger => _logger ??= _loggerFactory.CreateLogger<DefaultConsulServiceBuilder>();
 
     public virtual bool IsValid(ServiceEntry entry)
     {
@@ -36,7 +45,7 @@ public virtual bool IsValid(ServiceEntry entry)
 
         if (!valid)
         {
-            _logger.LogWarning(
+            Logger.LogWarning(
                 () => $"Unable to use service address: '{service.Address}' and port: {service.Port} as it is invalid for the service: '{service.Service}'. Address must contain host only e.g. 'localhost', and port must be greater than 0.");
         }
 

src/Ocelot.Provider.Consul/Interfaces/IConsulServiceBuilder.cs

@@ -4,7 +4,8 @@ namespace Ocelot.Provider.Consul.Interfaces;
 
 public interface IConsulServiceBuilder
 {
-    ConsulRegistryConfiguration Configuration { get; }
+    // Keep config private (deep encapsulation) until an architectural decision is made.
+    // ConsulRegistryConfiguration Configuration { get; }
     bool IsValid(ServiceEntry entry);
     IEnumerable<Service> BuildServices(ServiceEntry[] entries, Node[] nodes);
     Service CreateService(ServiceEntry serviceEntry, Node serviceNode);

src/Ocelot.Provider.Consul/OcelotBuilderExtensions.cs

@@ -24,9 +24,8 @@ public static IOcelotBuilder AddConsul(this IOcelotBuilder builder)
     {
         builder.Services
             .AddSingleton(ConsulProviderFactory.Get)
-            .AddSingleton(ConsulProviderFactory.GetConfiguration)
             .AddSingleton<IConsulClientFactory, ConsulClientFactory>()
-            .AddSingleton<IConsulServiceBuilder, DefaultConsulServiceBuilder>()
+            .AddScoped<IConsulServiceBuilder, DefaultConsulServiceBuilder>()
             .RemoveAll(typeof(IFileConfigurationPollerOptions))
             .AddSingleton<IFileConfigurationPollerOptions, ConsulFileConfigurationPollerOption>();
         return builder;
@@ -49,7 +48,7 @@ public static IOcelotBuilder AddConsul<TServiceBuilder>(this IOcelotBuilder buil
     {
         AddConsul(builder).Services
             .RemoveAll<IConsulServiceBuilder>()
-            .AddSingleton(typeof(IConsulServiceBuilder), typeof(TServiceBuilder));
+            .AddScoped(typeof(IConsulServiceBuilder), typeof(TServiceBuilder));
         return builder;
     }