HostFilteringMiddleware. It is returning "The host '{Host}' does not match an allowed host." time to time.

[charytech]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

In HostFilteringMiddleware we are using IOptionsManager. During reloading the configuration, it clears the state used to evaluate the host. In edge cases we may end up with an inconsistent state when calling the main Middleware method, the state can be cleared during the execution of the "Invoke" method.

Expected Behavior

Middleware shouldn't return us badrequest when allowed host are properly configured.

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

We shouldn't clear state but re-evaluate it during OnChange action.

[Tratcher]

This change notification code should probably re-calculate the list inline and swap out atomically. _allowAnyNonEmptyHost would also need to be made part of an atomic swap object. https://github.com/dotnet/aspnetcore/blob/da1d588a9afe7d1ae5591b45265045abf270515e/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs#L49-L55

Also call Configure in the constructor and remove EnsureConfigured. https://github.com/dotnet/aspnetcore/blob/da1d588a9afe7d1ae5591b45265045abf270515e/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs#L76C34-L76C52

[charytech]

I agree with the above comment. Proposal solution for #51960 is to reevaluate _allowAnyNonEmptyHost and _allowedHosts instead of resetting them to empty values. https://github.com/dotnet/aspnetcore/blob/da1d588a9afe7d1ae5591b45265045abf270515e/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs#L49-L55 The method will be similar to the already existing "Configure()". https://github.com/dotnet/aspnetcore/blob/da1d588a9afe7d1ae5591b45265045abf270515e/src/Middleware/HostFiltering/src/HostFilteringMiddleware.cs#L100-L123 We should also handle reseteting the _allowAnyNonEmptyHost state when it is not top level wildcard and allow _allowedHosts to be adjusted to empty values instead of throwing an exception.

[Tratcher]

Thanks, do you want to try it and send a PR?

Note, _allowedHosts and _allowAnyNonEmptyHost will likely need to be combine into a type & field that can be swapped out atomically. It's not safe to update them separately.

[charytech]

Hey, @Tratcher are you planning to patch previous versions with the above fix?

[Tratcher]

What's the impact on you? E.g. how often does the config change, and how many requests are disrupted in the meantime. Issues have to have significant impact before they're considered for backport.

[charytech]

In a couple of microservices config is refreshing a couple of times per minute. We have a problem with this every day depends on the traffic, day and luck as it is nondeterministic behavior, but even hundreds of errors per month. From my perspective, it is a significant impact as the error happens all the time day after day.

[Tratcher]

@adityamandaleeka for servicing consideration.

[charytech]

@adityamandaleeka any updates on the above?

[adityamandaleeka]

@charytech Are you looking to get this in .NET 8 servicing?

[charytech]

hey @adityamandaleeka I think both .NET 6 and .NET 8 should be patched as they are still under LTS. We are still using NET 6, but now we are starting migration to .NET 8. When do you plan to add it? It's already been a few months since the PR was merged.