diff --git a/docs/config.md b/docs/config.md index eefdb0195..c2b9dcf8e 100644 --- a/docs/config.md +++ b/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 diff --git a/reverse-proxy.sln b/reverse-proxy.sln index b36dcad16..7e158fe5c 100644 --- a/reverse-proxy.sln +++ b/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 diff --git a/samples/IslandGateway.Sample/appsettings.Development.json b/samples/IslandGateway.Sample/appsettings.Development.json index 45fe774a9..4f30a00f8 100644 --- a/samples/IslandGateway.Sample/appsettings.Development.json +++ b/samples/IslandGateway.Sample/appsettings.Development.json @@ -1,9 +1,9 @@ { "Logging": { "LogLevel": { - "Default": "Information", + "Default": "Debug", "Microsoft": "Warning", "Microsoft.Hosting.Lifetime": "Information" } } -} \ No newline at end of file +} diff --git a/samples/SampleServer/Startup.cs b/samples/SampleServer/Startup.cs index 62814f04f..c3a4754f3 100644 --- a/samples/SampleServer/Startup.cs +++ b/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) /// 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(); diff --git a/samples/SampleServer/appsettings.Development.json b/samples/SampleServer/appsettings.Development.json index 45fe774a9..63ed6e146 100644 --- a/samples/SampleServer/appsettings.Development.json +++ b/samples/SampleServer/appsettings.Development.json @@ -2,8 +2,8 @@ "Logging": { "LogLevel": { "Default": "Information", - "Microsoft": "Warning", + "Microsoft": "Information", "Microsoft.Hosting.Lifetime": "Information" } } -} \ No newline at end of file +} diff --git a/src/IslandGateway.Core/Service/Management/IslandGatewayConfigManager.cs b/src/IslandGateway.Core/Service/Management/IslandGatewayConfigManager.cs index 4ab0f180d..0d9c30dc2 100644 --- a/src/IslandGateway.Core/Service/Management/IslandGatewayConfigManager.cs +++ b/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 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 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; }