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

explicitly set hostname at the proxy server #1

Closed
wants to merge 0 commits into from

Conversation

spaparaju
Copy link
Contributor

@spaparaju spaparaju commented Jan 22, 2021

At ReverseProxy, HTTP Header value for Host is not set properly (instead contains the hostname of the token-refresher).
With this problem, The local tests would pass as both the source and host to proxy are both 'localhost'.
This problem can be reproduced when the host to proxy to is different from the the host the token-refresher runs.
This fix require explicitly setting the hostname as part of HttpRequest at the ReverseProxy.

Checkout reference to this problem here

Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

the NewSIngleHostReverseProxy function only creates a director function and returns a ReverseProxy struct, so calling the function and then overriding the director with a new one is funny because it's the same as not calling the function.

Better would be to return a ReverseProxy{Director: func(){//scome code here}}

@spaparaju
Copy link
Contributor Author

Makes sense. Switched to directly creating ReverseProxy.

Copy link
Member

@squat squat left a comment

Choose a reason for hiding this comment

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

great 🚀

@squat squat closed this Jan 25, 2021
@squat squat reopened this Jan 25, 2021
@spaparaju spaparaju closed this Jan 26, 2021
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.

2 participants