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

Allow bypassing of x-forwarded header removal #2728

Merged
merged 5 commits into from
Feb 14, 2025

Conversation

rkargMsft
Copy link
Contributor

@rkargMsft rkargMsft commented Jan 27, 2025

Addresses #2727

@rkargMsft
Copy link
Contributor Author

Looks like AddXForwarded similarly removes the Forwarded headers and needs similar treatment.

@adityamandaleeka
Copy link
Member

Haha, I did some digging and found this: #1070 (comment)

@MihaZupan
Copy link
Member

I agree that having the extra argument be removeAllXForwardedHeaders instead of suppressRemoveAllXForwardedHeaders is better.

The annoying part is that the current logic doesn't always remove the other header. AddForwarded does it conditionally.

if (action == ForwardedTransformActions.Off)
{
return context;
}
if (byFormat != NodeFormat.None || forFormat != NodeFormat.None || useHost || useProto)
{
var random = context.Services.GetRequiredService<IRandomFactory>();
context.RequestTransforms.Add(new RequestHeaderForwardedTransform(random,
forFormat, byFormat, useHost, useProto, action));
// Remove the X-Forwarded headers when an Forwarded transform is enabled
TransformHelpers.RemoveAllXForwardedHeaders(context, ForwardedTransformFactory.DefaultXForwardedPrefix);
}

I'm not actually sure why someone would want to add a no-op Forwarded transform like that.
I'd be okay with making a small breaking change here and have the new default be that the other header gets removed unconditionally (well, conditioned on the new argument only).

@rkargMsft
Copy link
Contributor Author

I can swap the parameter naming.

I'm not sure what the best way to handle action == ForwardedTransformActions.Off is because, as you say, it doesn't make sense to call AddForwarded with that action. If it were a new method then I'd propose validation that would throw if it was Off because, just don't call AddForwarded. I think that since it exists with the current behavior (do nothing if .Off) that it be kept as is. I think it would be odd to have the desired behavior be that .AddForwarded(..., action: ForwardedTransformActions.Off) be to do nothing with Forwarded headers but add transforms to remove the XForwarded headers.

@MihaZupan MihaZupan added this to the YARP 2.3 milestone Feb 11, 2025
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.

Thanks.

Mind adding a few tests for these?

@rkargMsft
Copy link
Contributor Author

Minor pain point is that targeting the alpha SDK does add some friction to contributing

@MihaZupan
Copy link
Member

Thank you.

Did you run into SDK issues even after running build.cmd from the root?
(it's not that common of a step in general, but is a thing for most dotnet/* repos)

@rkargMsft
Copy link
Contributor Author

rkargMsft commented Feb 14, 2025

I was not aware of that. Running build.bat and startvs.bat worked great.

Someone should put that in the Readme, maybe somewhere near the top in a Build section or something. Wait a minute...

Narrator: The Readme already tells you exactly how to do this.

@MihaZupan MihaZupan merged commit f52bfb9 into dotnet:main Feb 14, 2025
7 checks passed
@rkargMsft rkargMsft deleted the forwarded branch February 14, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants