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

AddHeaderPropagation() without providing Action<HeaderPropagationOptions> configureOptions prevents middleware from working correctly #48581

Open
1 task done
jjanuszkiewicz opened this issue Jun 2, 2023 · 6 comments
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware

Comments

@jjanuszkiewicz
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

When trying to configure the header propagation feature using the HeaderPropagationServiceCollectionExtensions.AddHeaderPropagation(this IServiceCollection services) (
link) method, there is no HeaderPropagationOptions instance registered, so the middleware doesn't capture any headers and header propagation won't work, even when header names are specified for a specific HttpClient, e.g.:

services.AddHeaderPropagation();
services.AddHttpClient<FooClient>().AddHeaderPropagation(o => o.Headers.Add("Authorization"));
services.AddHttpClient<BarClient>().AddHeaderPropagation(o => o.Headers.Add("X-Another-Header"));

This results in no headers being forwarded.

Instead, this needs to be done:

services.AddHeaderPropagation(o =>
{
    o.Headers.Add("Authorization");
    o.Headers.Add("X-Another-Header");
});
services.AddHttpClient<FooClient>().AddHeaderPropagation(o => o.Headers.Add("Authorization"));
services.AddHttpClient<BarClient>().AddHeaderPropagation(o => o.Headers.Add("X-Another-Header"));

This is especially problematic if different HttpClients require forwarding different headers - the services.AddHeaderPropagation call needs to list all those headers, which is code duplication and leads to bugs.

I propose to either:

  1. Make AddHeaderPropagation(this IServiceCollection services) internal, as it's useless for users of this library. This way all headers still need to be listed explicitly in the services.AddHeaderPropagation() call, but at least the not working method is hidden.
  2. Change AddHeaderPropagation(Action<HeaderPropagationMessageHandlerOptions>) so that it adds the configured headers to the registered HeaderPropagationOptions (and/or registers it if it's not yet registered).

I can create a PR for either of these (I prefer option 2), but looking for some feedback first.

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 2, 2023
@jjanuszkiewicz
Copy link
Author

jjanuszkiewicz commented Aug 16, 2023

Yes, I register the middleware.

The problem is that:

  • HeaderPropagationMiddleware takes an instance of IOptions<HeaderPropagationOptions> and captures only the headers included in those options
  • but the parameterless IServiceCollection.AddHeaderPropagation() method doesn't register the options,
  • so even if options are later provided for a specific HttpClient via IHttpClientBuilder.AddHeaderPropagation(Action<HeaderPropagationMessageHandlerOptions> configure) it doesn't make the middleware capture any headers, because this method doesn't register or modify the registered instance of HeaderPropagationOptions.

My suggestion 2 would address that:

Change AddHeaderPropagation(Action) so that it adds the configured headers to the registered HeaderPropagationOptions (and/or registers it if it's not yet registered).

@jjanuszkiewicz
Copy link
Author

Like I said, I can create a PR with proposed changes and hopefully also with some tests demonstrating the problem, but I'd like to get some feedback first (e.g. if this is for some reason by design and the PR won't be considered for merging?)

@davidfowl
Copy link
Member

API changes require an API proposal and review and number 1 is a non-starter because it's a binary breaking change.

@rmja
Copy link
Contributor

rmja commented Dec 20, 2023

I am seeing this as well. I was expecting services.AddHttpClient<FooClient>().AddHeaderPropagation(o => o.Headers.Add("Authorization")); to automatically add Authorization to the set of headers being captured by the middleware.

@jjanuszkiewicz
Copy link
Author

If I have some spare time during holiday, I'll create a MR with tests showing the problem and maybe a possible solution (#2 from my original post). If time permits.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware
Projects
None yet
Development

No branches or pull requests

6 participants