Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HeaderPropagation: propagate incoming request headers to outgoing HTTP requests #7921

Merged
merged 32 commits into from Mar 29, 2019

Conversation

@alefranz
Copy link
Contributor

commented Feb 25, 2019

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

It is a port of aspnet/Extensions#1099 which
Addresses aspnet/Extensions#526

Main feedback on the original PR

@natemcmaster : it can't be added to https://github.com/aspnet/Extensions as it introduce a circulr dependency
@rynowak has suggested to move this to this repo as middleware.
@Tratcher it is not handling concurrency in accessing the Context if multiple HttpClient requests are sent in parallel
@Tratcher @rynowak it should not need to be based on the HttpClientFactory but it could be a DelegatingHandler and a Middleware to initialize the state.

Current Plan

  • Collect feedback on this PR from relevant stake holders (should I open a issue as well?)
  • In the meantime, I'll work on the DelegatingHandler with a initializer middleware, locking the access to the Context if needed, similar to microsoft/ApplicationInsights-aspnetcore#373

Original description

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

@alefranz alefranz requested review from dougbu and Tratcher as code owners Feb 25, 2019

@alefranz alefranz changed the title HeaderPropagation: propagate incoming request headers to outgoing Http requests HeaderPropagation: propagate incoming request headers to outgoing HTTP requests Feb 25, 2019

@Tratcher Tratcher requested a review from rynowak Feb 25, 2019

@alefranz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I've introduced the Middleware to solve the concurrency issue.
It's also not tightly couple with the HttpClientFactory, although it still include an extension method to easily add the DelegeatingHandler to the client setup in the factory, as an opt-in for each client.
The configuration, however, is shared for all the clients. This is to simplify the behavior of the middleware, as it needs to know all the possible headers to be forwarded.

The Middleware take the input headers and put those in the state with the name defined as output, eventually generating/using the default value when not present.
The DelegatingHandler simply add the headers from the state to the outgoing HttpRequestMessage headers, eventually skipping the one already present based on the value of AlwaysAdd.

I'm not convinced about the configuration... the behavior it's probably not really straightforward and can be simplified.

anurse and others added some commits Mar 27, 2019

Update eng/SharedFramework.Local.props
Co-Authored-By: alefranz <alessio@franceschelli.me>
@alefranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Hi @rynowak , thank you for the review and apologies for the delay on my side :)
I should have addressed the feedback.
Let me know what you think about the documentation, specifically the parts I skipped (as it doesn't seems common practice looking at other projects in the solution) and the attempt to explain the logic at https://github.com/aspnet/AspNetCore/pull/7921/files#diff-ceea95213282add8708d3dc7be38d089.

I'll have a look tomorrow at why it's not passing on the CI

@rynowak

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Looks good from my POV - I want to get a :shipit: from @dougbu and @anurse before we merge this bad boy!

@anurse

anurse approved these changes Mar 28, 2019

@dougbu

dougbu approved these changes Mar 28, 2019

Copy link
Member

left a comment

Didn't look at the C# files at all but the rest looks good

@rynowak

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@alefranz - could you rebase? after that I think we're ready to go.

@alefranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Thanks everyone!

@alefranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

ci tests on windows are failing, but it looks unrelated

 [xUnit.net 00:04:51.46]     Templates.Test.RazorPagesTemplateTest.RazorPagesTemplate_IndividualAuthImplAsync(useLocalDB: True) [FAIL]
  Failed   Templates.Test.RazorPagesTemplateTest.RazorPagesTemplate_IndividualAuthImplAsync(useLocalDB: True)
  Error Message:
   System.Net.Http.HttpRequestException : The SSL connection could not be established, see inner exception.
  ---- System.IO.IOException : Authentication failed because the remote party has closed the transport stream.
@dougbu

This comment has been minimized.

@aspnet-hello

This comment has been minimized.

Copy link

commented Mar 29, 2019

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build.

@rynowak rynowak merged commit f28cf2b into aspnet:master Mar 29, 2019

14 checks passed

AspNetCore-ci Build #20190329.3 had test failures
Details
AspNetCore-ci (Build: Linux ARM) Build: Linux ARM succeeded
Details
AspNetCore-ci (Build: Linux ARM64) Build: Linux ARM64 succeeded
Details
AspNetCore-ci (Build: Linux Musl x64) Build: Linux Musl x64 succeeded
Details
AspNetCore-ci (Build: Linux x64) Build: Linux x64 succeeded
Details
AspNetCore-ci (Build: Windows ARM) Build: Windows ARM succeeded
Details
AspNetCore-ci (Build: Windows x64/x86) Build: Windows x64/x86 succeeded
Details
AspNetCore-ci (Build: macOS) Build: macOS succeeded
Details
AspNetCore-ci (Code check) Code check succeeded
Details
AspNetCore-ci (Test: Ubuntu 16.04 x64) Test: Ubuntu 16.04 x64 succeeded
Details
AspNetCore-ci (Test: Windows Server 2016 x64) Test: Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: macOS 10.13) Test: macOS 10.13 succeeded
Details
AspNetCore-helix-test Build #20190329.3 succeeded
Details
license/cla All CLA requirements met.
Details
@rynowak

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Merged ! Thanks @alefranz and everyone else who helped review.

@alefranz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@rynowak What is the policy about back-porting this kind of packages to 2.2?

@rynowak

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@alefranz - we don't backport features to already shipped releases.

If you want a version of this you can use with 2.2, I would suggest grabbing the code and building it from source - it should be straightforward since this package doesn't have tendrils anywhere else.. @Eilon will correct me if I'm wrong but our license allows you to copy as much code as you want (with attribution).

@Eilon

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Yup, what @rynowak said.

@ericsampson

This comment has been minimized.

Copy link

commented Apr 10, 2019

@rynowak I didn't get a chance to review the entire changeset closely, but is it implemented to reduce allocations as much as practical?

@ericsampson

This comment has been minimized.

Copy link

commented Apr 10, 2019

Nice work @alefranz :)

@rynowak

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I didn't get a chance to review the entire changeset closely, but is it implemented to reduce allocations as much as practical?

Our work is never over. If you think it can be improved, collect some data and send a PR 👍

@ericsampson

This comment has been minimized.

Copy link

commented Apr 10, 2019

I didn't get a chance to review the entire changeset closely, but is it implemented to reduce allocations as much as practical?

Our work is never over. If you think it can be improved, collect some data and send a PR 👍

👍 Maybe I'll see if I can nerd-snipe @benaadams into taking a peek first :) Cheers all

_values.Headers.TryGetValue(headerName, out var values) &&
!StringValues.IsNullOrEmpty(values))
{
request.Headers.TryAddWithoutValidation(outputName, (string[])values);

This comment has been minimized.

Copy link
@benaadams

benaadams Apr 10, 2019

Contributor

The cast (string[])values will cause an array allocation for most headers (which are only one)

Better would be to check the count and then switch on overload e.g.

var count = values.Count;
if (count == 1)
{
    request.Headers.TryAddWithoutValidation(outputName, (string)values);
}
else if (count > 1)
{
    request.Headers.TryAddWithoutValidation(outputName, (string[])values);
}

This comment has been minimized.

Copy link
@benaadams

benaadams Apr 10, 2019

Contributor

Also HttpClient has an odd behaviour where some headers are rejected from the Headers collection and instead need to be added to the Content.Headers collection (if there is a content); so might need to be more like:

var hasContent = request.Content != null;

// ...

var count = values.Count;
if (count == 1)
{
    var value = (string)values;
    if (!request.Headers.TryAddWithoutValidation(outputName, value) && hasContent)
    {
        request.Content.Headers.TryAddWithoutValidation(outputName, value);
    }
} 
else ...

from: https://github.com/aspnet/Benchmarks/blob/7b1a5e986f06ae79270b13866e701cc6cb5bb395/src/Proxy/ProxyExtensions.cs#L31-L34

This comment has been minimized.

Copy link
@alefranz

alefranz Apr 11, 2019

Author Contributor

Thanks @benaadams !
I'll raise a PR to address this by the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.