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

update host.ProxyHandler to compatiable with different hosts in target url and request host #1703

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

tuhao1020
Copy link
Contributor

@tuhao1020 tuhao1020 requested a review from kataras as a code owner January 11, 2021 07:31
@CLAassistant
Copy link

CLAassistant commented Jan 11, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

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

Hello @tuhao1020, how are you?

I think we must not introduce changes to that function, as it may be used by many applications and it was not updated for the past versions. I think it's better to introduce a new function for that e.g. ProxyHandlerRemote , please do it and don't forget to use gofmt :P

@tuhao1020
Copy link
Contributor Author

@kataras I have added ProxyHandlerRemote and NewProxyRemote, and code has formatted :)

@tuhao1020 tuhao1020 requested a review from kataras January 16, 2021 13:19
Copy link
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tuhao1020 (although there is space for improvements there, e.g. remove the code duplication on the ProxyHandlerRemote, the 95% of the code is identical to the existing ProxyHandler one, but I will fix it later on)

@kataras kataras merged commit e990b23 into kataras:master Jan 21, 2021
@tuhao1020
Copy link
Contributor Author

iris/core/host/proxy.go

Lines 40 to 55 in 6c823e4

func modifyProxiedRequest(req *http.Request, target *url.URL) {
req.URL.Scheme = target.Scheme
req.URL.Host = target.Host
req.Host = target.Host
if target.RawQuery == "" || req.URL.RawQuery == "" {
req.URL.RawQuery = target.RawQuery + req.URL.RawQuery
} else {
req.URL.RawQuery = target.RawQuery + "&" + req.URL.RawQuery
}
if _, ok := req.Header["User-Agent"]; !ok {
// explicitly disable User-Agent so it's not set to default value
req.Header.Set("User-Agent", "")
}
}

Line 43: req.Host = target.Host
this line will make below code useless

iris/core/host/proxy.go

Lines 70 to 72 in 6c823e4

if req.Host != target.Host {
req.URL.Path = target.Path
} else {

req.Host = target.Host should be placed after above code.

@kataras
Copy link
Owner

kataras commented Jan 23, 2021

Oh you are totally right, merging the 2nd one now @tuhao1020 .

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.

3 participants