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

feat: Promote "Host" header if present. Fixes #12690 #12691

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

philBrown
Copy link
Contributor

Fixes #12690

Motivation

When communicating with a proxy or load balancer that does not do Host header rewriting, it may be required to override the default request Host header.

Go's HTTP request does not do this by default when passed a Host header value.

Modifications

Checks for any Host header in the HTTP Facade and promotes it to the request Host field if found.

Verification

This is a very simple change and since there were no existing headers tests, it didn't seem necessary.

@philBrown philBrown marked this pull request as ready for review February 22, 2024 03:49
Promotes any `Host` header to the `http.Request.Host` field.

Relates to golang/go#29865

Signed-off-by: Phil Brown <pbrown2@atlassian.com>
@agilgur5
Copy link
Member

What was your intent for this -- did you mean to modify the CLI? Because the apiclient is used for a few things (an SDK, the CLI, some internal API calls).

It may make sense to limit this change to only the CLI if that was your intent

@philBrown
Copy link
Contributor Author

@agilgur5 the intent was for the CLI.

I didn't see anywhere else where this could possibly be done though. The request construction is completely opaque outside the Facade

@agilgur5 agilgur5 added the area/cli The `argo` CLI label Feb 23, 2024
Trim whitespace from any detected "Host" header value

Signed-off-by: Phil Brown <pbrown2@atlassian.com>
@philBrown
Copy link
Contributor Author

Unit test failure doesn't seem related and I cannot reproduce it locally. Counting files in /tmp seems flakey at best

@philBrown
Copy link
Contributor Author

FYI we're using my fork and it's working great for us. It would be fantastic if this could be incorporated into the official release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli The `argo` CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Promote "Host" header to Request.Host
2 participants