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

Handle distributed context headers in custom propagator #1533

Merged

Conversation

greenEkatherine
Copy link
Contributor

Resolves #1497

Added ReverseProxyPropagator that handles headers removal. Default inner implementation removes distributed context headers. It can be changed by overriding Propagator property in ForwarderHttpClientFactory which is used to set up handler.ActivityHeadersPropagator.

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the direct-forwarding docs to include changing this property on the SocketsHttpHandler as well:

var httpClient = new HttpMessageInvoker(new SocketsHttpHandler()
{
UseProxy = false,
AllowAutoRedirect = false,
AutomaticDecompression = DecompressionMethods.None,
UseCookies = false
});

As for the breaking-change: anyone using direct forwarding will have to set the ActivityHeadersPropagator property to match existing behavior.

@MihaZupan MihaZupan added this to the YARP 1.1.0 milestone Jan 26, 2022
Katya Sokolova and others added 2 commits January 27, 2022 10:46
@greenEkatherine
Copy link
Contributor Author

We should update the direct-forwarding docs to include changing this property on the SocketsHttpHandler as well:

var httpClient = new HttpMessageInvoker(new SocketsHttpHandler()
{
UseProxy = false,
AllowAutoRedirect = false,
AutomaticDecompression = DecompressionMethods.None,
UseCookies = false
});

I noticed that example there doesn't match with Direct.Sample and not only with this new property. Should I update Direct.Sample as well?

@MihaZupan
Copy link
Member

MihaZupan commented Jan 27, 2022

Yes please.

We should also consider updating https://microsoft.github.io/reverse-proxy/articles/header-guidelines.html#traceparent-request-id-tracestate-baggage-correlation-context to mention you can turn header removal off by calling

.ConfigureHttpHandler((_, handler) => handler.Propagator = null)

(might not be the exact syntax) in Startup

docs/docfx/articles/header-guidelines.md Outdated Show resolved Hide resolved
docs/docfx/articles/direct-forwarding.md Outdated Show resolved Hide resolved
docs/docfx/articles/direct-forwarding.md Outdated Show resolved Hide resolved
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var httpClient = new HttpMessageInvoker(new SocketsHttpHandler()
{
UseProxy = false,
AllowAutoRedirect = false,
AutomaticDecompression = DecompressionMethods.None,
UseCookies = false
});

should include the ActivityHeadersPropagator.

docs/docfx/articles/direct-forwarding.md Outdated Show resolved Hide resolved
src/ReverseProxy/Forwarder/ReverseProxyPropagator.cs Outdated Show resolved Hide resolved
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@greenEkatherine
Copy link
Contributor Author

var httpClient = new HttpMessageInvoker(new SocketsHttpHandler()
{
UseProxy = false,
AllowAutoRedirect = false,
AutomaticDecompression = DecompressionMethods.None,
UseCookies = false
});

should include the ActivityHeadersPropagator.

Oh, forgot this one, adding...

@greenEkatherine greenEkatherine merged commit 33eadc6 into microsoft:main Jan 28, 2022
@MihaZupan MihaZupan modified the milestones: YARP 1.1.0, YARP 1.1.0-RC1 Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YARP is filtering headers (with common names) because of distributed tracing
3 participants