#849 #1496 Extend the route key format used for load balancing making…

… it unique (#1944)

* Fix the route key format used for load balancing

The old key format did not contain enough information to disambiguate routes based on an UpstreamHost. This was especially problematic when a ServiceName was used in conjuction with Service Discovery, instead of DownstreamHostAndPorts configuration.

Resolves #1496

* Update tests

* Amend test

* Remove empty key parts

Co-authored-by: Raman Maksimchuk <[email protected]>

* Amend

* Make route keys uniform, keep empty parts

* Fix wrong usage of dictionary TryGetValue

* Coalesce empty strings or white space, use fallback values

* Add back host and ports

* Remove redundant load balancer type check

* Fix TryGetValue with null argument

* Revert removal of balancer type check, still relevant

* Improve helper local functions

* Fix outdated comment

* Add acceptance test

* Refactor RouteKeyCreator class

* Add developer's XML docs

* Add TODO

* Convert fact to theory

* Review unit tests

* Fix incorrect usages of ServiceHandler, make sure all are correctly Disposed

* Fix services responding to wrong path

* Add Consul counter

* Reduce summary length

* Rename OkResponse->MapGet

* Refactor `LoadBalancerHouse` class because of DRY principle

---------

Co-authored-by: Raman Maksimchuk <[email protected]>

src/Ocelot/Configuration/Creator/RouteKeyCreator.cs

@@ -1,17 +1,86 @@
 using Ocelot.Configuration.File;
-using Ocelot.LoadBalancer.LoadBalancers;
+using Ocelot.LoadBalancer.LoadBalancers;
 
-namespace Ocelot.Configuration.Creator
-{
-    public class RouteKeyCreator : IRouteKeyCreator
+namespace Ocelot.Configuration.Creator;
+
+public class RouteKeyCreator : IRouteKeyCreator
+{
+    /// <summary>
+    /// Creates the unique <see langword="string"/> key based on the route properties for load balancing etc.
+    /// </summary>
+    /// <remarks>
+    /// Key template:
+    /// <list type="bullet">
+    /// <item>UpstreamHttpMethod|UpstreamPathTemplate|UpstreamHost|DownstreamHostAndPorts|ServiceNamespace|ServiceName|LoadBalancerType|LoadBalancerKey</item>
+    /// </list>
+    /// </remarks>
+    /// <param name="fileRoute">The route object.</param>
+    /// <returns>A <see langword="string"/> object containing the key.</returns>
+    public string Create(FileRoute fileRoute)
     {
-        public string Create(FileRoute fileRoute) => IsStickySession(fileRoute)
-            ? $"{nameof(CookieStickySessions)}:{fileRoute.LoadBalancerOptions.Key}"
-            : $"{fileRoute.UpstreamPathTemplate}|{string.Join(',', fileRoute.UpstreamHttpMethod)}|{string.Join(',', fileRoute.DownstreamHostAndPorts.Select(x => $"{x.Host}:{x.Port}"))}";
+        var isStickySession = fileRoute.LoadBalancerOptions is
+        {
+            Type: nameof(CookieStickySessions),
+            Key.Length: > 0
+        };
+
+        if (isStickySession)
+        {
+            return $"{nameof(CookieStickySessions)}:{fileRoute.LoadBalancerOptions.Key}";
+        }
+
+        var upstreamHttpMethods = Csv(fileRoute.UpstreamHttpMethod);
+        var downstreamHostAndPorts = Csv(fileRoute.DownstreamHostAndPorts.Select(downstream => $"{downstream.Host}:{downstream.Port}"));
+
+        var keyBuilder = new StringBuilder()
+
+            // UpstreamHttpMethod and UpstreamPathTemplate are required
+            .AppendNext(upstreamHttpMethods)
+            .AppendNext(fileRoute.UpstreamPathTemplate)
+
+            // Other properties are optional, replace undefined values with defaults to aid debugging
+            .AppendNext(Coalesce(fileRoute.UpstreamHost, "no-host"))
+
+            .AppendNext(Coalesce(downstreamHostAndPorts, "no-host-and-port"))
+            .AppendNext(Coalesce(fileRoute.ServiceNamespace, "no-svc-ns"))
+            .AppendNext(Coalesce(fileRoute.ServiceName, "no-svc-name"))
+            .AppendNext(Coalesce(fileRoute.LoadBalancerOptions.Type, "no-lb-type"))
+            .AppendNext(Coalesce(fileRoute.LoadBalancerOptions.Key, "no-lb-key"));
 
-        private static bool IsStickySession(FileRoute fileRoute) =>
-            !string.IsNullOrEmpty(fileRoute.LoadBalancerOptions.Type)
-            && !string.IsNullOrEmpty(fileRoute.LoadBalancerOptions.Key)
-            && fileRoute.LoadBalancerOptions.Type == nameof(CookieStickySessions);
-    }
+        return keyBuilder.ToString();
+    }
+
+    /// <summary>
+    /// Helper function to convert multiple strings into a comma-separated string.
+    /// </summary>
+    /// <param name="values">The collection of strings to join by comma separator.</param>
+    /// <returns>A <see langword="string"/> in the comma-separated format.</returns>
+    private static string Csv(IEnumerable<string> values) => string.Join(',', values);
+
+    /// <summary>
+    /// Helper function to return the first non-null-or-whitespace string.
+    /// </summary>
+    /// <param name="first">The 1st string to check.</param>
+    /// <param name="second">The 2nd string to check.</param>
+    /// <returns>A <see langword="string"/> which is not empty.</returns>
+    private static string Coalesce(string first, string second) => string.IsNullOrWhiteSpace(first) ? second : first;
+}
+
+internal static class RouteKeyCreatorHelpers
+{
+    /// <summary>
+    /// Helper function to append a string to the key builder, separated by a pipe.
+    /// </summary>
+    /// <param name="builder">The builder of the key.</param>
+    /// <param name="next">The next word to add.</param>
+    /// <returns>The reference to the builder.</returns>
+    public static StringBuilder AppendNext(this StringBuilder builder, string next)
+    {
+        if (builder.Length > 0)
+        {
+            builder.Append('|');
+        }
+
+        return builder.Append(next);
+    }
 }

src/Ocelot/LoadBalancer/LoadBalancers/LoadBalancerHouse.cs

@@ -1,5 +1,5 @@
 using Ocelot.Configuration;
-using Ocelot.Responses;
+using Ocelot.Responses;
 
 namespace Ocelot.LoadBalancer.LoadBalancers
 {
@@ -18,45 +18,40 @@ public Response<ILoadBalancer> Get(DownstreamRoute route, ServiceProviderConfigu
         {
             try
             {
-                Response<ILoadBalancer> result;
-
                 if (_loadBalancers.TryGetValue(route.LoadBalancerKey, out var loadBalancer))
-                {
-                    loadBalancer = _loadBalancers[route.LoadBalancerKey];
-
+                {
+                    // TODO Fix ugly reflection issue of dymanic detection in favor of static type property
                     if (route.LoadBalancerOptions.Type != loadBalancer.GetType().Name)
-                    {
-                        result = _factory.Get(route, config);
-                        if (result.IsError)
-                        {
-                            return new ErrorResponse<ILoadBalancer>(result.Errors);
-                        }
-
-                        loadBalancer = result.Data;
-                        AddLoadBalancer(route.LoadBalancerKey, loadBalancer);
+                    {
+                        return GetResponse(route, config);
                     }
 
                     return new OkResponse<ILoadBalancer>(loadBalancer);
                 }
 
-                result = _factory.Get(route, config);
-
-                if (result.IsError)
-                {
-                    return new ErrorResponse<ILoadBalancer>(result.Errors);
-                }
-
-                loadBalancer = result.Data;
-                AddLoadBalancer(route.LoadBalancerKey, loadBalancer);
-                return new OkResponse<ILoadBalancer>(loadBalancer);
+                return GetResponse(route, config);
             }
             catch (Exception ex)
             {
-                return new ErrorResponse<ILoadBalancer>(new List<Errors.Error>
-                {
-                    new UnableToFindLoadBalancerError($"unabe to find load balancer for {route.LoadBalancerKey} exception is {ex}"),
-                });
+                return new ErrorResponse<ILoadBalancer>(
+                [
+                    new UnableToFindLoadBalancerError($"Unable to find load balancer for '{route.LoadBalancerKey}'. Exception: {ex};"),
+                ]);
             }
+        }
+
+        private Response<ILoadBalancer> GetResponse(DownstreamRoute route, ServiceProviderConfiguration config)
+        {
+            var result = _factory.Get(route, config);
+
+            if (result.IsError)
+            {
+                return new ErrorResponse<ILoadBalancer>(result.Errors);
+            }
+
+            var loadBalancer = result.Data;
+            AddLoadBalancer(route.LoadBalancerKey, loadBalancer);
+            return new OkResponse<ILoadBalancer>(loadBalancer);
         }
 
         private void AddLoadBalancer(string key, ILoadBalancer loadBalancer)