fix review comments

microsoft · Nov 16, 2023 · a910dcf · a910dcf
1 parent 2a19fc7
commit a910dcf
Show file tree

Hide file tree

Showing 14 changed files with 46 additions and 96 deletions.
diff --git a/src/ReverseProxy/Configuration/ClusterValidators/DestinationValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/DestinationValidator.cs
@@ -5,22 +5,19 @@ namespace Yarp.ReverseProxy.Configuration.ClusterValidators;
 
 internal sealed class DestinationValidator : IClusterValidator
 {
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         if (cluster.Destinations is null)
         {
-            return Array.Empty<Exception>();
+            return;
         }
 
-        var errors = new List<Exception>();
         foreach (var (name, destination) in cluster.Destinations)
         {
             if (string.IsNullOrEmpty(destination.Address))
             {
                 errors.Add(new ArgumentException($"No address found for destination '{name}' on cluster '{cluster.ClusterId}'."));
             }
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/ClusterValidators/HealthCheckValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/HealthCheckValidator.cs
@@ -20,7 +20,7 @@ public HealthCheckValidator(IEnumerable<IAvailableDestinationsPolicy> availableD
         _passiveHealthCheckPolicies = passiveHealthCheckPolicies?.ToDictionaryByUniqueId(p => p.Name) ?? throw new ArgumentNullException(nameof(availableDestinationsPolicies));
     }
 
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         var availableDestinationsPolicy = cluster.HealthCheck?.AvailableDestinationsPolicy;
         if (string.IsNullOrEmpty(availableDestinationsPolicy))
@@ -29,19 +29,16 @@ public IList<Exception> Validate(ClusterConfig cluster)
             availableDestinationsPolicy = HealthCheckConstants.AvailableDestinations.HealthyOrPanic;
         }
 
-        var errors = new List<Exception>();
         if (!_availableDestinationsPolicies.ContainsKey(availableDestinationsPolicy))
         {
             errors.Add(new ArgumentException($"No matching {nameof(IAvailableDestinationsPolicy)} found for the available destinations policy '{availableDestinationsPolicy}' set on the cluster.'{cluster.ClusterId}'."));
         }
 
-        ValidateActiveHealthCheck(errors, cluster);
-        ValidatePassiveHealthCheck(errors, cluster);
-
-        return errors;
+        ValidateActiveHealthCheck(cluster, errors);
+        ValidatePassiveHealthCheck(cluster, errors);
     }
 
-    private void ValidateActiveHealthCheck(IList<Exception> errors, ClusterConfig cluster)
+    private void ValidateActiveHealthCheck(ClusterConfig cluster, IList<Exception> errors)
     {
         if (!(cluster.HealthCheck?.Active?.Enabled ?? false))
         {
@@ -72,7 +69,7 @@ private void ValidateActiveHealthCheck(IList<Exception> errors, ClusterConfig cl
         }
     }
 
-    private void ValidatePassiveHealthCheck(IList<Exception> errors, ClusterConfig cluster)
+    private void ValidatePassiveHealthCheck(ClusterConfig cluster, IList<Exception> errors)
     {
         if (!(cluster.HealthCheck?.Passive?.Enabled ?? false))
         {

diff --git a/src/ReverseProxy/Configuration/ClusterValidators/IClusterValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/IClusterValidator.cs
@@ -3,13 +3,7 @@
 
 namespace Yarp.ReverseProxy.Configuration.ClusterValidators;
 
-internal interface IClusterValidator
+public interface IClusterValidator
 {
-    protected IList<Exception> Validate(ClusterConfig cluster);
-
-    public bool IsValid(ClusterConfig cluster, out IList<Exception> errors)
-    {
-        errors = Validate(cluster);
-        return errors.Count == 0;
-    }
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors);
 }
diff --git a/src/ReverseProxy/Configuration/ClusterValidators/LoadBalancingValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/LoadBalancingValidator.cs
@@ -14,7 +14,7 @@ public LoadBalancingValidator(IEnumerable<ILoadBalancingPolicy> loadBalancingPol
         _loadBalancingPolicies = loadBalancingPolicies?.ToDictionaryByUniqueId(p => p.Name) ?? throw new ArgumentNullException(nameof(loadBalancingPolicies));
     }
 
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         var loadBalancingPolicy = cluster.LoadBalancingPolicy;
         if (string.IsNullOrEmpty(loadBalancingPolicy))
@@ -23,12 +23,9 @@ public IList<Exception> Validate(ClusterConfig cluster)
             loadBalancingPolicy = LoadBalancingPolicies.PowerOfTwoChoices;
         }
 
-        var errors = new List<Exception>();
         if (!_loadBalancingPolicies.ContainsKey(loadBalancingPolicy))
         {
             errors.Add(new ArgumentException($"No matching {nameof(ILoadBalancingPolicy)} found for the load balancing policy '{loadBalancingPolicy}' set on the cluster '{cluster.ClusterId}'."));
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/ClusterValidators/ProxyHttpClientValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/ProxyHttpClientValidator.cs
@@ -6,15 +6,14 @@ namespace Yarp.ReverseProxy.Configuration.ClusterValidators;
 
 internal sealed class ProxyHttpClientValidator : IClusterValidator
 {
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         if (cluster.HttpClient is null)
         {
             // Proxy http client options are not set.
-            return Array.Empty<Exception>();
+            return;
         }
 
-        var errors = new List<Exception>();
         if (cluster.HttpClient.MaxConnectionsPerServer is not null && cluster.HttpClient.MaxConnectionsPerServer <= 0)
         {
             errors.Add(new ArgumentException($"Max connections per server limit set on the cluster '{cluster.ClusterId}' must be positive."));
@@ -34,18 +33,18 @@ public IList<Exception> Validate(ClusterConfig cluster)
         }
 
         var responseHeaderEncoding = cluster.HttpClient.ResponseHeaderEncoding;
-        if (responseHeaderEncoding is not null)
+        if (responseHeaderEncoding is null)
         {
-            try
-            {
-                Encoding.GetEncoding(responseHeaderEncoding);
-            }
-            catch (ArgumentException aex)
-            {
-                errors.Add(new ArgumentException($"Invalid response header encoding '{responseHeaderEncoding}'.", aex));
-            }
+            return;
         }
 
-        return errors;
+        try
+        {
+            Encoding.GetEncoding(responseHeaderEncoding);
+        }
+        catch (ArgumentException aex)
+        {
+            errors.Add(new ArgumentException($"Invalid response header encoding '{responseHeaderEncoding}'.", aex));
+        }
     }
 }
diff --git a/src/ReverseProxy/Configuration/ClusterValidators/ProxyHttpRequestValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/ProxyHttpRequestValidator.cs
@@ -14,16 +14,14 @@ public ProxyHttpRequestValidator(ILogger<ProxyHttpRequestValidator> logger)
         _logger = logger;
     }
 
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         if (cluster.HttpRequest is null)
         {
             // Proxy http request options are not set.
-            return Array.Empty<Exception>();
+            return;
         }
 
-        var errors = new List<Exception>();
-
         if (cluster.HttpRequest.Version is not null &&
             cluster.HttpRequest.Version != HttpVersion.Version10 &&
             cluster.HttpRequest.Version != HttpVersion.Version11 &&
@@ -37,8 +35,6 @@ public IList<Exception> Validate(ClusterConfig cluster)
         {
             Log.Http10Version(_logger);
         }
-
-        return errors;
     }
 
     private static class Log

diff --git a/src/ReverseProxy/Configuration/ClusterValidators/SessionAffinityValidator.cs b/src/ReverseProxy/Configuration/ClusterValidators/SessionAffinityValidator.cs
@@ -15,12 +15,12 @@ public SessionAffinityValidator(IEnumerable<IAffinityFailurePolicy> affinityFail
         _affinityFailurePolicies = affinityFailurePolicies?.ToDictionaryByUniqueId(p => p.Name) ?? throw new ArgumentNullException(nameof(affinityFailurePolicies));
     }
 
-    public IList<Exception> Validate(ClusterConfig cluster)
+    public void AddValidationErrors(ClusterConfig cluster, IList<Exception> errors)
     {
         if (!(cluster.SessionAffinity?.Enabled ?? false))
         {
             // Session affinity is disabled
-            return Array.Empty<Exception>();
+            return;
         }
 
         // Note some affinity validation takes place in AffinitizeTransformProvider.ValidateCluster.
@@ -32,7 +32,6 @@ public IList<Exception> Validate(ClusterConfig cluster)
             affinityFailurePolicy = SessionAffinityConstants.FailurePolicies.Redistribute;
         }
 
-        var errors = new List<Exception>();
         if (!_affinityFailurePolicies.ContainsKey(affinityFailurePolicy))
         {
             errors.Add(new ArgumentException($"No matching {nameof(IAffinityFailurePolicy)} found for the affinity failure policy name '{affinityFailurePolicy}' set on the cluster '{cluster.ClusterId}'."));
@@ -47,7 +46,7 @@ public IList<Exception> Validate(ClusterConfig cluster)
 
         if (cookieConfig is null)
         {
-            return errors;
+            return;
         }
 
         if (cookieConfig.Expiration is not null && cookieConfig.Expiration <= TimeSpan.Zero)
@@ -59,7 +58,5 @@ public IList<Exception> Validate(ClusterConfig cluster)
         {
             errors.Add(new ArgumentException($"Session affinity cookie max-age must be positive or null."));
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/ConfigValidator.cs b/src/ReverseProxy/Configuration/ConfigValidator.cs
@@ -69,10 +69,7 @@ public async ValueTask<IList<Exception>> ValidateRouteAsync(RouteConfig route)
 
         foreach (var routeValidator in _routeValidators)
         {
-            if (!routeValidator.IsValid(route.Match, route.RouteId, out var validationErrors))
-            {
-                errors.AddRange(validationErrors);
-            }
+            routeValidator.AddValidationErrors(route.Match, route.RouteId, errors);
         }
 
         return errors;
@@ -93,10 +90,7 @@ public ValueTask<IList<Exception>> ValidateClusterAsync(ClusterConfig cluster)
 
         foreach (var clusterValidator in _clusterValidators)
         {
-            if (!clusterValidator.IsValid(cluster, out var validationErrors))
-            {
-                errors.AddRange(validationErrors);
-            }
+            clusterValidator.AddValidationErrors(cluster, errors);
         }
 
         return new ValueTask<IList<Exception>>(errors);

diff --git a/src/ReverseProxy/Configuration/RouteValidators/HeadersValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/HeadersValidator.cs
@@ -6,15 +6,14 @@ namespace Yarp.ReverseProxy.Configuration.RouteValidators;
 
 internal sealed class HeadersValidator : IRouteValidator
 {
-    public IList<Exception> Validate(RouteMatch route, string routeId)
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors)
     {
-        // Headers are optional
         if (route.Headers is null)
         {
-            return Array.Empty<Exception>();
+            // Headers are optional
+            return;
         }
 
-        var errors = new List<Exception>();
         foreach (var header in route.Headers)
         {
             if (header is null)
@@ -39,7 +38,5 @@ public IList<Exception> Validate(RouteMatch route, string routeId)
                 errors.Add(new ArgumentException($"Header values were set when using mode '{header.Mode}' on route header '{header.Name}' for route '{routeId}'."));
             }
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/RouteValidators/HostValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/HostValidator.cs
@@ -6,15 +6,14 @@ namespace Yarp.ReverseProxy.Configuration.RouteValidators;
 
 internal sealed class HostValidator : IRouteValidator
 {
-    public IList<Exception> Validate(RouteMatch route, string routeId)
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors)
     {
-        // Host is optional when Path is specified
         if (route.Hosts is null || route.Hosts.Count == 0)
         {
-            return Array.Empty<Exception>();
+            // Host is optional when Path is specified
+            return;
         }
 
-        var errors = new List<Exception>();
         foreach (var host in route.Hosts)
         {
             if (string.IsNullOrEmpty(host))
@@ -26,7 +25,5 @@ public IList<Exception> Validate(RouteMatch route, string routeId)
                 errors.Add(new ArgumentException($"Punycode host name '{host}' has been set for route '{routeId}'. Use the unicode host name instead."));
             }
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/RouteValidators/IRouteValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/IRouteValidator.cs
@@ -3,13 +3,7 @@
 
 namespace Yarp.ReverseProxy.Configuration.RouteValidators;
 
-internal interface IRouteValidator
+public interface IRouteValidator
 {
-    protected IList<Exception> Validate(RouteMatch route, string routeId);
-
-    public bool IsValid(RouteMatch route, string routeId, out IList<Exception> errors)
-    {
-       errors = Validate(route, routeId);
-       return errors.Count == 0;
-    }
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors);
 }
diff --git a/src/ReverseProxy/Configuration/RouteValidators/MethodsValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/MethodsValidator.cs
@@ -11,15 +11,14 @@ internal sealed class MethodsValidator : IRouteValidator
         "HEAD", "OPTIONS", "GET", "PUT", "POST", "PATCH", "DELETE", "TRACE",
     };
 
-    public IList<Exception> Validate(RouteMatch route, string routeId)
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors)
     {
-        // Methods are optional
         if (route.Methods is null)
         {
-            return Array.Empty<Exception>();
+            // Methods are optional
+            return;
         }
 
-        var errors = new List<Exception>();
         var seenMethods = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
         foreach (var method in route.Methods)
         {
@@ -34,7 +33,5 @@ public IList<Exception> Validate(RouteMatch route, string routeId)
                 errors.Add(new ArgumentException($"Unsupported HTTP method '{method}' has been set for route '{routeId}'."));
             }
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/RouteValidators/PathValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/PathValidator.cs
@@ -7,15 +7,14 @@ namespace Yarp.ReverseProxy.Configuration.RouteValidators;
 
 internal sealed class PathValidator : IRouteValidator
 {
-    public IList<Exception> Validate(RouteMatch route, string routeId)
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors)
     {
-        // Path is optional when Host is specified
         if (string.IsNullOrEmpty(route.Path))
         {
-            return ImmutableList<Exception>.Empty;
+            // Path is optional when Host is specified
+            return;
         }
 
-        var errors = new List<Exception>();
         try
         {
             RoutePatternFactory.Parse(route.Path);
@@ -24,7 +23,5 @@ public IList<Exception> Validate(RouteMatch route, string routeId)
         {
             errors.Add(new ArgumentException($"Invalid path '{route.Path}' for route '{routeId}'.", ex));
         }
-
-        return errors;
     }
 }
diff --git a/src/ReverseProxy/Configuration/RouteValidators/QueryParametersValidator.cs b/src/ReverseProxy/Configuration/RouteValidators/QueryParametersValidator.cs
@@ -5,15 +5,14 @@ namespace Yarp.ReverseProxy.Configuration.RouteValidators;
 
 internal sealed class QueryParametersValidator : IRouteValidator
 {
-    public IList<Exception> Validate(RouteMatch route, string routeId)
+    public void AddValidationErrors(RouteMatch route, string routeId, IList<Exception> errors)
     {
-        // Query Parameters are optional
         if (route.QueryParameters is null)
         {
-            return Array.Empty<Exception>();
+            // Query Parameters are optional
+            return;
         }
 
-        var errors = new List<Exception>();
         foreach (var queryParameter in route.QueryParameters)
         {
             if (queryParameter is null)
@@ -38,7 +37,5 @@ public IList<Exception> Validate(RouteMatch route, string routeId)
                 errors.Add(new ArgumentException($"Query parameter values where set when using mode '{nameof(QueryParameterMatchMode.Exists)}' on route query parameter '{queryParameter.Name}' for route '{routeId}'."));
             }
         }
-
-        return errors;
     }
 }