HttpClientFactory Header Propagation MessageHandler

[alefranz]

This PR introduces a MessageHandler for the HttpClientFactory to forward headers received in the current MVC request.
Addresses #526

I believe it is a common use case which deserves to be included in the Extensions namespace, so I'd like to help to land this feature. Its main use case is probably to track distributed transaction which requires the ability to pass through a transaction identifier as well as generating a new one when not present.
The code is loosely based on this gist from @davidfowl with some extra features and a different behaviour as, by default, this PR doesn't add the header if it's already present in the HttpRequestMessage.

Features:

• Forward header from current request MVC context to outgoing HttpClient requests, allowing to use a different header name. The header is forwarded only if present and all values are used.
• Always forward the header to the HttpRequestMessage (AlwaysAdd=true) or only when the header is not already present (default).
• Define a default value(s) to be added to the HttpRequestMessage if the incoming request does not contain the header
• Define a default value(s) generator to be used to add the header value to the HttpRequestMessage if the incoming request does not contain the header. The generator can rely on the HttpRequestMessage or HttpContext to decide the value. (note: in my mind the use case is to generate a new GUID/identifier for a transaction chain if it's not in the incoming request, I'm not sure if having access to the request and context add any value)

The tests should clarify the behaviour if needed.

It definitely needs some polishing on the definition of the configuration as well as probably removing some of the different behaviours, however I believe opening a PR is the best way to move forward the discussion and shape the behaviour.
Probably the discussion on the requirements should stay on the issue to keep the focus of the discussion here on the implementation.

Please Note:

• I didn't have explicit feedback to open the PR following my comment on the issue, however the issue if marked as up-for-grabs
• Part of this PR is based on this gist from @davidfowl , I'm not sure what is the correct way to give attribution.

I really appreciate you taking the time to review this PR.

Thanks,
Alessio

[natemcmaster]

I would not accept this change as is. It introduces a cyclical dependency between Microsoft.AspNetCore packages and Microsoft.Extensions libraries. The layering is intentional - aspnetcore APIs should not be used here.

[mkArtakMSFT]

@rynowak, should this change go into the AspNetCore repo insetad?

[rynowak]

Thanks @alefranz - I think that this is a good start and is on the right track. Thanks for taking this on! It looks valuable.

@natemcmaster is right though - we can't put this in this repo in its current form because we can't add a Microsoft.AspNetCore.* reference. Could I have you close this and create a new PR on the aspnet/AspNetCore repo?

@Tratcher - thoughts on where this should live in that repo? It's not really middleware, but it's at that level of abstraction.

[Tratcher]

@rynowak It's middleware all right, just middleware in a different pipeline. Are you prepared to ship this as a standalone package? It doesn't belong as part of any of our existing packages.

[alefranz]

Thank you all for taking the time to look into this.

@natemcmaster apologies, I still get a bit confused on the scope of the different repositories

I thought a bit about this and, if there is no suitable home for this package, a different approach could be to remove the AspNetCore dependency introducing a level of abstraction, e.g.:

public interface IContextValuesAccessor
{
    IDictionary<string, StringValues> ContextValues { get; }
}

Which will make this no longer explicitly about forwarding headers, but about including headers based of values in a generic context, so not necessary AspNetCore nor in Http request.

Of course, there would still be a need of a package on the AspNetCore ecosystem to make the consumption straightforward, otherwise you would have to register the HttpContextAccessor as well as implementing this interface (and registering it) to forward the values from the context, like

private class AspNetCoreContextAccessor : IContextValuesAccessor
{
    private readonly IHttpContextAccessor _accessor;

    public AspNetCoreContextAccessor(IHttpContextAccessor accessor)
    {
        _accessor = accessor;
    }

    public IDictionary<string, StringValues> ContextValues => _accessor.HttpContext.Request.Headers;
}

[rynowak]

@alefranz - thanks for your patience on this - I'm hopefully going to be in the office tomorrow and I'll talk to some folks to get an agreement on where this code needs to live.

I also considered the idea of splitting this feature into two parts, but I think ultimately we will need something that ties HttpContext and a message handler together. I think it's simpler now to think of the header-propagation as one component until we find a need to break it up.

[Tratcher]

@davidfowl we've run into trouble trying to use IHttpContextAccessor in DelegatingHandlers before haven't we? People would run their HttpClient requests in parallel and cause unsafe mulithreaded access to HttpContext. When ApplicationInsights dealt with a similar problem they concluded the best way to handle it was to collect fields they needed up front and pass them through in a dedicated data structure.

What if instead of a DelegatingHandler we provided a set of request composition helpers on top of HttpClient and HttpRequestMessage?

The biggest downside I can see would be the caller needing to be HttpContext aware, which may or may not work for various libraries.

[alefranz]

Hi Chris,
yes the HttpContext is not thread-safe and it's a really valid point, not sure why I haven't considered sending requests in parallel :(
I should add a HeaderPropagationInitializer similar to the TelemetryInitializer that get the required values from the context and store it in it's own data structure. However this will mean that it needs to be aware of all the headers forwarded by all the HttpClients as we will want to lock on the context only once per MVC request. I'll have to think about it.

About the composition helpers, I'm not sure how that will fit with the HttpClientFactory. Can you elaborate a bit more on what you have in mind?

Btw here is the issue you are referring to:
https://github.com/Microsoft/ApplicationInsights-aspnetcore/issues/373
I remembered he mentioned it on last year talk at NDC :)

[Tratcher]

The composition helpers wouldn't be affiliated with HttpClientFactory, they'd be APIs for directly creating or operating on HttpRequestMessages before calling into HttpClient. They may have extensions on HttpClient to facilitate.

[rynowak]

Yeah, I think the right thing to do here is to make a middleware that initializes state, and a standalone DelegatingHandler that reads that state to modify the outgoing request.

I don't think we need to tie this to the http client factory. In general library code doesn't need to integrate with it directly because wiring up a DelegatingHandler is easy.

[rynowak]

@alefranz - can you please resend this to aspnet/AspNetCore and put it in here: https://github.com/aspnet/AspNetCore/tree/master/src/Middleware

We can discuss this and review in more detail there.

[alefranz]

Moved this to dotnet/aspnetcore#7921 as suggested by @rynowak
Thanks everyone who for the help!