Kestrel reloadable endpoint config #21072

halter73 · 2020-04-21T21:05:10Z

This PR allows Kestrel to observe changes to the config passed to KestrelServerOptions.Configure(IConfiguration), and bind/unbind/rebind to endpoints as necessary. This gives Kestrel the new ability to gracefully start and stop a subset of endpoints necessitating the new TransportManager and TransportConnectionManager.

If an endpoint is defined in code, even using the methods on KestrelConfigurationLoader, they will never be unbound or rebound due to configuration changes. Any code-based configuration that modifies endpoints defined in config, like callbacks passed to ConfigureEndpointDefaults, ConfigureHttpsDefaults and callbacks to configure modified named endpoints will be rerun.

Addresses #19376

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

analogrelay

Smithed some words in the comments

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

src/Servers/Kestrel/Kestrel/test/KestrelConfigurationBuilderTests.cs

davidfowl · 2020-04-21T21:42:54Z

If an endpoint is defined in code, even using the methods on KestrelConfigurationLoader, they will never be unbound or rebound due to configuration changes. Any code-based configuration that modifies endpoints defined in config, like callbacks passed to ConfigureEndpointDefaults, ConfigureHttpsDefaults and to configure named endpoints will be rerun.

Any plans to add an API that can do the same or is it purely configuration based?

halter73 · 2020-04-21T21:57:57Z

Any plans to add an API that can do the same or is it purely configuration based?

It would be nice to have. @Tratcher suggested this too, but there are ~~now~~ no immediate plans for a code-based API.

That said, this PR takes care of of a lot of the inner plumbing necessary to make it work. I think we could easily expose KestrelServerOptions.Listen-style APIs on a singleton.

Designing the API to unbind is a bit trickier. We could create a new API to query existing endpoints. We could also leverage IServerAddressFeature and make the address the key for unbinding.

src/Servers/Kestrel/Core/src/KestrelServer.cs

Tratcher

Yeah, please file a separate issue for code based rebinding. I expect the larger proxy customers will be doing primarily code based setup rather than config based.

src/Servers/Kestrel/Core/src/Internal/AddressBindContext.cs

src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

src/Servers/Kestrel/Core/src/KestrelServer.cs

Tratcher · 2020-04-21T23:04:54Z

src/Servers/Kestrel/Core/src/KestrelServer.cs

+                    using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(5));
+                    using var combinedCts = CancellationTokenSource.CreateLinkedTokenSource(_stopCts.Token, timeoutCts.Token);
+
+                    // TODO: It would be nice to start binding to new endpoints immediately and reconfigured endpoints as soon


Except there may be conflicts.

Ofc. We would have to write the algorithm to sequence unbinding and binding only for endpoints that are being reconfigured instead of newly added. If it was a simple as binding and unbinding in parallel. I would have already done it.

That's probably not worth the effort. What might still be worth doing is adding a TransportManager method that just unbinds endpoints. We could unbind first, then bind in parallel to draining the old connections.

We're going to get asked if non-destructive updates are possible. E.g. if all I did was change the cert file, why do I have to drop the socket? This will make even more sense when we get to config based SNI, if all I do is add one more host/cert to the list why should everything be torn down.

Not saying this is something we have to address right now, but we should at least think about if all changes are equal.

src/Servers/Kestrel/Core/src/KestrelServer.cs

Tratcher · 2020-04-21T23:14:17Z

src/Servers/Kestrel/samples/SampleApp/Startup.cs

-                    config.AddJsonFile("appsettings.json", optional: true)
-                          .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true);
+                    config.AddJsonFile("appsettings.json", optional: true, reloadOnChange: true)
+                          .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: true);
                })
                .UseKestrel((context, options) =>


Can you show setting kestrel's reloadOnChange option, even it it's to the default?

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

src/Servers/Kestrel/Core/src/Internal/ServerAddressesCollection.cs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

davidfowl · 2020-04-26T15:15:11Z

Looking at this logic I don't know we didn't use IOptions<T> for this originally... Seems like this should be IOptionsMonitor<T> would be the replacement instead of doing what this is doing.

Also https://github.com/dotnet/aspnetcore/pull/21072/files#diff-b2d7bf739c9e417a24fca24244fb808dR265

We have a helper that does reload token management and change notification that may be better to use https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.changetoken.onchange?view=dotnet-plat-ext-3.1. Of course, if you use the IOptionsMonitor this happens for you 😄

Tratcher · 2020-04-26T15:27:15Z

Yeah, at the time I didn't know the config system well enough to know if it could handle this. In hindsight I probably could have made it work. You'd still have to diff and debounce though.

davidfowl · 2020-04-26T15:40:34Z

Debouncing needs to be built into the system and shouldnt be something that everyone needs to handle (e.g. #10620)

ghost · 2020-04-28T17:22:03Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

Co-Authored-By: Andrew Stanton-Nurse <[email protected]>

Co-Authored-By: Chris Ross <[email protected]>

analogrelay · 2020-04-30T19:20:20Z

API seems minimal and looks good to me.

halter73 requested review from Tratcher and jkotalik April 21, 2020 21:05

halter73 requested a review from analogrelay as a code owner April 21, 2020 21:05

ghost added the area-servers label Apr 21, 2020

halter73 commented Apr 21, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs Outdated Show resolved Hide resolved

halter73 force-pushed the halter73/19376 branch 2 times, most recently from b2fe55d to ba11a58 Compare April 21, 2020 21:30

analogrelay reviewed Apr 21, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs Outdated Show resolved Hide resolved

davidfowl self-requested a review April 21, 2020 21:38

davidfowl reviewed Apr 21, 2020

View reviewed changes

src/Servers/Kestrel/Kestrel/test/KestrelConfigurationBuilderTests.cs Outdated Show resolved Hide resolved

halter73 commented Apr 21, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServer.cs Show resolved Hide resolved

Tratcher reviewed Apr 21, 2020

View reviewed changes

halter73 force-pushed the halter73/19376 branch 8 times, most recently from e953190 to 97577a4 Compare April 22, 2020 02:03

Tratcher reviewed Apr 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/ServerAddressesCollection.cs Outdated Show resolved Hide resolved

halter73 force-pushed the halter73/19376 branch 2 times, most recently from 1f05613 to bac3966 Compare April 24, 2020 20:43

Tratcher approved these changes Apr 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Outdated Show resolved Hide resolved

halter73 force-pushed the halter73/19376 branch from bac3966 to 860bf97 Compare April 27, 2020 21:04

halter73 force-pushed the halter73/19376 branch from 860bf97 to 39fa43e Compare April 28, 2020 19:41

halter73 and others added 16 commits April 29, 2020 11:24

Kestrel reloadable endpoint config

5f7a97a

Apply suggestions from code review

231b306

Co-Authored-By: Andrew Stanton-Nurse <[email protected]>

Fix spacing

409288b

Consolidate server and transport shutdown logic

d30f047

Apply ConnectionLimitMiddleware to HTTP/3 connections

e4cca8d

Make ServerAddressFeature thread safe

fea46ff

Compare complete EndpointConfig sections

e2b848a

More review feedback.

9720e0d

Update reference source

638b9ba

Cleanup

37d2725

Add integration test

bc9356c

Update src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

f6a7d9a

Co-Authored-By: Chris Ross <[email protected]>

Nit

4b9f2bc

Make ReloadsOnConfigurationChangeByDefault more reliable

03b68a7

Default config reloadOnChange to false

432029d

Don't require dev cert for integration test

f80f222

halter73 force-pushed the halter73/19376 branch from 72c3acb to f80f222 Compare April 29, 2020 18:24

analogrelay added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 30, 2020

halter73 merged commit 100823a into master May 1, 2020

halter73 deleted the halter73/19376 branch May 1, 2020 03:42

JamesNK mentioned this pull request May 2, 2020

Kestrel ListenAnyIP error #21423

Closed

halter73 mentioned this pull request Jun 4, 2020

Enable Kestrel config reload in ConfigureWebHostDefaults #22528

Merged

halter73 mentioned this pull request Sep 21, 2020

Document Kestrel breaking changes, new API and new config in 5.0 #26141

Closed

JamesNK mentioned this pull request Sep 23, 2020

5.0 docs: Kestrel config reload #26241

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kestrel reloadable endpoint config #21072

Kestrel reloadable endpoint config #21072

halter73 commented Apr 21, 2020 •

edited

Loading

analogrelay left a comment

davidfowl commented Apr 21, 2020

halter73 commented Apr 21, 2020 •

edited

Loading

Tratcher left a comment

Tratcher Apr 21, 2020

halter73 Apr 22, 2020

Tratcher Apr 22, 2020

Tratcher Apr 21, 2020

davidfowl commented Apr 26, 2020

Tratcher commented Apr 26, 2020

davidfowl commented Apr 26, 2020

ghost commented Apr 28, 2020

analogrelay commented Apr 30, 2020

Kestrel reloadable endpoint config #21072

Kestrel reloadable endpoint config #21072

Conversation

halter73 commented Apr 21, 2020 • edited Loading

analogrelay left a comment

Choose a reason for hiding this comment

davidfowl commented Apr 21, 2020

halter73 commented Apr 21, 2020 • edited Loading

Tratcher left a comment

Choose a reason for hiding this comment

Tratcher Apr 21, 2020

Choose a reason for hiding this comment

halter73 Apr 22, 2020

Choose a reason for hiding this comment

Tratcher Apr 22, 2020

Choose a reason for hiding this comment

Tratcher Apr 21, 2020

Choose a reason for hiding this comment

davidfowl commented Apr 26, 2020

Tratcher commented Apr 26, 2020

davidfowl commented Apr 26, 2020

ghost commented Apr 28, 2020

analogrelay commented Apr 30, 2020

halter73 commented Apr 21, 2020 •

edited

Loading

halter73 commented Apr 21, 2020 •

edited

Loading