Config spec updates, more logging (#26)

docs/config.md

@@ -40,6 +40,21 @@ This basic structure is useful though the "Rule" [system](https://github.com/mic
 
 The GatewayRoute.Metadata dictionary may be able to be replaced or supplemented by giving direct access to the config node for that route. Compare to Kestrel's [EndpointConfig.ConfigSection](https://github.com/dotnet/aspnetcore/blob/f4d81e3af2b969744a57d76d4d622036ac514a6a/src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs#L168-L175) property. That would allow for augmenting an endpoint with additional complex custom entries that the app code can reference for additional config actions.
 
+Update: The custom rule system was modified by [#24](https://github.com/microsoft/reverse-proxy/pull/24) so that the config now looks like this:
+```
+       "Routes": [
+         {
+           "RouteId": "backend1/route1",
+           "BackendId": "backend1",
+           "Match": {
+             "Methods": [ "GET", "POST" ],
+             "Host": "localhost",
+             "Path": "/{**catchall}"
+           }
+         }
+       ]
+```
+
 ## Backend configuration
 
 The proxy/gateway code defines the types [Backend](https://github.com/microsoft/reverse-proxy/blob/b2cf5bdddf7962a720672a75f2e93913d16dfee7/src/IslandGateway.Core/Abstractions/BackendDiscovery/Contract/Backend.cs) and [BackendEndpoint](https://github.com/microsoft/reverse-proxy/blob/b2cf5bdddf7962a720672a75f2e93913d16dfee7/src/IslandGateway.Core/Abstractions/BackendEndpointDiscovery/Contract/BackendEndpoint.cs) and allows these to be defined via config and referenced by name from routes.
@@ -48,6 +63,77 @@ A BackendEndpoint defines a specific service instance with an id, address, and a
 
 A Backend is a collection of one or more BackendEndpoints and a set of policies for choosing which endpoint to rout each request to (load balancing, circuit breakers, health checks, affinities, etc.). This seems a bit monolithic compared to our initial design explorations. We anticipate wanting to break these policies up into distinct steps in a pipeline to make them more replaceable. That said, we'll still need a config model for the default set of components and it may look very much like what's already here.
 
+Question: Why are the backends and the endpoints listed separately in config rather than nested? Object model links endpoints 1:1 with backends, so there doesn't seem to be a reason to list them separately.
+
+Existing:
+```
+      "Backends": [
+        {
+          "BackendId": "backend1"
+        },
+        {
+          "BackendId": "backend2"
+        }
+      ],
+      "Endpoints": {
+        "backend1": [
+          {
+            "EndpointId": "backend1/endpoint1",
+            "Address": "https://localhost:10000/"
+          }
+        ],
+        "backend2": [
+          {
+            "EndpointId": "backend2/endpoint1",
+            "Address": "https://localhost:10001/"
+          }
+        ]
+      },
+```
+Nested:
+```
+      "Backends": [
+        {
+          "BackendId": "backend1",
+          "Endpoints": [
+            {
+              "EndpointId": "backend1/endpoint1",
+              "Address": "https://localhost:10000/"
+            }
+          ],
+        },
+        {
+          "BackendId": "backend2"
+          "Endpoints": [
+            {
+              "EndpointId": "backend2/endpoint1",
+              "Address": "https://localhost:10001/"
+            }
+          ],
+        }
+      ],
+```
+Additional feedback: Why is it using arrays instead of objects? These items are not order sensitive, and they already have id properties anyways.
+```
+      "Backends": {
+        "backend1" : {
+          "Endpoints": [
+            "endpoint1": {
+              "Address": "https://localhost:10000/"
+            }
+          },
+        },
+        "backend2": {
+          "Endpoints": {
+            "endpoint1": {
+              "Address": "https://localhost:10001/"
+            }
+          },
+        }
+      },
+```
+
+
 ## Config reloading
 
 Config reloading is not yet a blocking requirement but we do expect to need it in the future. This design needs to factor in how reloading might work when it does get added.
@@ -62,7 +148,15 @@ Kestrel support for reloading config is tracked by https://github.com/dotnet/asp
 
 Reloading proxy config will need to happen atomically and avoid disrupting requests already in flight. We may need to rebuild portions of the app pipeline and swap them out for new requests, drain the old requests, and clean up the old pipelines. We also want to avoid a full reset for small config changes where possible. E.g. if only one route changes then ideally we'd only rebuild that route.
 
-Reloading should be something you can opt into or out of.
+Reloading should be something you can opt into or out of. Right now this is only possible at the config level by opting in or out for a config source, but that affects the whole app.
+
+Updates:
+
+Config reload for gateway routes, backends, and endpoints already works. You edit appsettings.json and it automatically reloads and reconfigures the routes. Note the config change notification usually gets fired twice and logs "Applying gateway configs" each time, but the change diff logic prevents an unnecessary update the second time. We may still want to do some debounce detection to prevent extra config diffs, but that's lower priority.
+
+@halter73 raised the question of how much effort we put in to make the kestrel reload atomic with the routing reload? Conceptually it makes sense to keep the two in sync, but pragmatically it's quite difficult as there's no connection between the two systems. They'd be reacting to the same change notification event in serial and requests in flight may see one set of changes without the other. We discussed this in the weekly sync and decided that since kestrel endpoint changes will be a less common scenario we won't initually worry about the atomicity here until we have customer feedback that demonstrates issues.
+
+Also, when a kestrel endpoint is modified or removed should existing connections on that endpoint be eagerly drained and closed, or should they be allowed to run a normal lifecycle? Kestrel does not currently track active connection per endpoint so additional tracking would be needed if we wanted to shut them down.
 
 ## Augmenting config via code
 

reverse-proxy.sln

@@ -42,6 +42,11 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "test", "test", "{0631147E-3
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{6CBE18D4-64E9-492B-BB02-58CD57126C10}"
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "docs", "docs", "{BC344A50-8F81-4762-9F4B-12714693144B}"
+	ProjectSection(SolutionItems) = preProject
+		docs\config.md = docs\config.md
+	EndProjectSection
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU

samples/IslandGateway.Sample/appsettings.Development.json

@@ -1,9 +1,9 @@
 {
   "Logging": {
     "LogLevel": {
-      "Default": "Information",
+      "Default": "Debug",
       "Microsoft": "Warning",
       "Microsoft.Hosting.Lifetime": "Information"
     }
   }
-}
+}

samples/SampleServer/Startup.cs

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
 using Microsoft.AspNetCore.Builder;
@@ -36,7 +36,9 @@ public void ConfigureServices(IServiceCollection services)
         /// </summary>
         public void Configure(IApplicationBuilder app)
         {
-            app.UseHttpsRedirection();
+            // Disabling https redirection behind the proxy. Forwarders are not currently set up so we can't tell if the external connection used https.
+            // Nor do we know the correct port to redirect to.
+            // app.UseHttpsRedirection();
 
             app.UseWebSockets();
 

samples/SampleServer/appsettings.Development.json

@@ -2,8 +2,8 @@
   "Logging": {
     "LogLevel": {
       "Default": "Information",
-      "Microsoft": "Warning",
+      "Microsoft": "Information",
       "Microsoft.Hosting.Lifetime": "Information"
     }
   }
-}
+}

src/IslandGateway.Core/Service/Management/IslandGatewayConfigManager.cs

@@ -104,6 +104,15 @@ private void UpdateRuntimeBackends(DynamicConfigRoot config)
                             currentBackendConfig.HealthCheckOptions.Port != newConfig.HealthCheckOptions.Port ||
                             currentBackendConfig.HealthCheckOptions.Path != newConfig.HealthCheckOptions.Path)
                         {
+                            if (currentBackendConfig == null)
+                            {
+                                _logger.LogDebug("Backend {backendId} has been added.", configBackend.BackendId);
+                            }
+                            else
+                            {
+                                _logger.LogDebug("Backend {backendId} has changed.", configBackend.BackendId);
+                            }
+
                             // Config changed, so update runtime backend
                             backend.Config.Value = newConfig;
                         }
@@ -120,6 +129,7 @@ private void UpdateRuntimeBackends(DynamicConfigRoot config)
                     // NOTE 2: Removing the backend from `IBackendManager` is safe and existing
                     // ASP .NET Core endpoints will continue to work with their existing behavior (until those endpoints are updated)
                     // and the Garbage Collector won't destroy this backend object while it's referenced elsewhere.
+                    _logger.LogDebug("Backend {backendId} has been removed.", existingBackend.BackendId);
                     _backendManager.TryRemoveItem(existingBackend.BackendId);
                 }
             }
@@ -137,6 +147,14 @@ private void UpdateRuntimeEndpoints(IList<BackendEndpoint> configEndpoints, IEnd
                     {
                         if (endpoint.Config.Value?.Address != configEndpoint.Address)
                         {
+                            if (endpoint.Config.Value == null)
+                            {
+                                _logger.LogDebug("Endpoint {endpointId} has been added.", configEndpoint.EndpointId);
+                            }
+                            else
+                            {
+                                _logger.LogDebug("Endpoint {endpointId} has changed.", configEndpoint.EndpointId);
+                            }
                             endpoint.Config.Value = new EndpointConfig(configEndpoint.Address);
                         }
                     });
@@ -152,6 +170,7 @@ private void UpdateRuntimeEndpoints(IList<BackendEndpoint> configEndpoints, IEnd
                     // NOTE 2: Removing the endpoint from `IEndpointManager` is safe and existing
                     // backends will continue to work with their existing behavior (until those backends are updated)
                     // and the Garbage Collector won't destroy this backend object while it's referenced elsewhere.
+                    _logger.LogDebug("Endpoint {endpointId} has been removed.", existingEndpoint.EndpointId);
                     endpointManager.TryRemoveItem(existingEndpoint.EndpointId);
                 }
             }
@@ -181,6 +200,14 @@ private void UpdateRuntimeRoutes(DynamicConfigRoot config)
                         {
                             // Config changed, so update runtime route
                             changed = true;
+                            if (currentRouteConfig == null)
+                            {
+                                _logger.LogDebug("Route {routeId} has been added.", configRoute.RouteId);
+                            }
+                            else
+                            {
+                                _logger.LogDebug("Route {routeId} has changed.", configRoute.RouteId);
+                            }
 
                             var newConfig = _routeEndpointBuilder.Build(configRoute, backendOrNull, route);
                             route.Config.Value = newConfig;
@@ -198,6 +225,7 @@ private void UpdateRuntimeRoutes(DynamicConfigRoot config)
                     // NOTE 2: Removing the route from `IRouteManager` is safe and existing
                     // ASP .NET Core endpoints will continue to work with their existing behavior since
                     // their copy of `RouteConfig` is immutable and remains operational in whichever state is was in.
+                    _logger.LogDebug("Route {routeId} has been removed.", existingRoute.RouteId);
                     _routeManager.TryRemoveItem(existingRoute.RouteId);
                     changed = true;
                 }