Introduce KestrelServerOptions.CheckDefaultCertificate #48057
Conversation
...as a way to determine whether a certificate is available before invoking `KestrelServerOptions.Listen` and possibly failing in `ListenOptions.UseHttps`. Fixes dotnet#28120
amcasey
requested review from
Tratcher,
halter73,
BrennanConroy,
JamesNK and
mgravell
as code owners
ghost
added
the
amcasey
added
blocked
|
Blocked on API approval: #48055 |
| /// </summary> | ||
| /// <returns>True if there is a default or development server certificate, false otherwise.</returns> | ||
| /// <exception cref="InvalidOperationException">If there is a configuration and it has not been loaded.</exception> | ||
| public bool HasDefaultOrDevelopmentCertificate |
There was a problem hiding this comment.
Not super enthusiastic about a property getter having side effects.
There was a problem hiding this comment.
Yeah, make it a method.
bool CheckForDefaultCertificate()
The development certificate counts as a default and is explained in the docs.
There was a problem hiding this comment.
What side-effect are we worried about? Enabling https config?
There was a problem hiding this comment.
I'm not familiar with the bool CheckFoo() naming convention - personally, I'd expect that method to throw if the condition weren't satisfied. I take it we use that elsewhere?
There was a problem hiding this comment.
Renamed and made it a method, but I'd still like to understand more about side-effects.
There was a problem hiding this comment.
Incidentally, now that it doesn't actually return the cert, I would be fine with this throwing if https configuration isn't enabled.
There was a problem hiding this comment.
The point of the method is to avoid throwing for known, common conditions. Exceptions should only be used for unexpected conditions.
Sorry, I'm not sure which part of the thread you're responding to. If it was about my expectation that CheckFoo would throw, that was an intuition about the naming convention and not a comment on how this member (whatever name and shape it ends up taking) should behave.
There was a problem hiding this comment.
This is a friendly guard method, letting you check the state in a controlled way to avoid an exception later. It should not throw an exception.
There was a problem hiding this comment.
@halter73 Asked me to have it throw in that case. Would you like me to work up a version that loads eagerly and then compensates if the configuration changes?
There was a problem hiding this comment.
If you're getting conflicting advice we should hold off until we can discuss it in a meeting, API review or otherwise.
| /// <exception cref="InvalidOperationException">If there is a configuration and it has not been loaded.</exception> | ||
| public bool CheckDefaultCertificate() | ||
| { | ||
| if (ConfigurationLoader is { IsLoaded: false }) |
There was a problem hiding this comment.
Can this call ConfigurationLoader.Load() instead of throwing?
There was a problem hiding this comment.
@halter73 Suggested doing it this way because invoking Load ourselves would put the burden on us to reload on change.
There was a problem hiding this comment.
We'd have to reload if CheckDefaultCertificate were called again? That seems reasonable.
There was a problem hiding this comment.
I understood him to say that config changes between the first call to Load and bind-time would be lost since bind wouldn't attempt to reload and already loaded configuration.
There was a problem hiding this comment.
(i.e. other consumers of the configuration might see stale values.)
There was a problem hiding this comment.
@halter73 Suggested doing it this way because invoking
Loadourselves would put the burden on us to reload on change.
We should capture the reload token during the first call to Load regardless. If we start calling Load ourselves in more places, we should fix the bug at the same time. Otherwise, we risk not picking up new endpoints or certa added to Kestrel config between the manual call to Load and a later call to StartAsync. We could always call Reload during StarAsync, but it'd be more efficient to only reload if the configuration has actually changed.
That said, this is a pretty narrow edge case and an existing big.
| /// </summary> | ||
| /// <returns>True if there is a default or development server certificate, false otherwise.</returns> | ||
| /// <exception cref="InvalidOperationException">If there is a configuration and it has not been loaded.</exception> | ||
| public bool HasDefaultOrDevelopmentCertificate |
There was a problem hiding this comment.
This is a friendly guard method, letting you check the state in a controlled way to avoid an exception later. It should not throw an exception.
amcasey
changed the title
|
We're going to try to fix things without a new API. |
Introduce KestrelServerOptions.HasDefaultOrDevelopmentCertificate
Description
Introduce
KestrelServerOptions.HasDefaultOrDevelopmentCertificateas a way to determine whether a certificate is available before invokingKestrelServerOptions.Listenand possibly failing inListenOptions.UseHttps.Replaces #48054
Fixes #28120