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 to set Host header for Client #1169

Merged
merged 4 commits into from Dec 17, 2021
Merged

Conversation

Tolyar
Copy link
Contributor

@Tolyar Tolyar commented Dec 4, 2021

Allow to change Host header when using Client. Currently only HostClient allows to change Host header.

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 5, 2021

Comments for failed tests. Test TestRequestUpdateURI does r.Header.SetHost("aaa.bbb") but checking header Host: foobar.com This is correct for the old behaviour but not for the new. I do not want to touch tests before PR will be checked.

@erikdubbelboer
Copy link
Collaborator

There was already a discussion about this here: #1164
I think the change I posted there is the only acceptable solution, the change you made now would break for all users who somehow have the Host header set already but depend on it being change to the host of the url that they specify.

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 5, 2021

I've checked your version. It does not works for me with Client. I've get error 'lookup : no such host'. req.URI().SetHost("") allow to change Host header, but breaks connection address. May be possible to add new attribute to Request type for enable to change Host header? This should not break compatibility with old versions with correct defaults.

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 5, 2021

I've pushed new version without tests violation.

http.go Outdated
Comment on lines 60 to 61
// Allow to change Host header with parsedURI == true (E.g. Client for type).
AllowToChangeHostHeader bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think something like this is cleaner?

Suggested change
// Allow to change Host header with parsedURI == true (E.g. Client for type).
AllowToChangeHostHeader bool
// Use Host header (request.Header.SetHost) instead of the host from SetRequestURI, SetHost, or URI().SetHost
UseHostHeader bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good comment. I've rename attribute and add test for it.

@@ -1360,7 +1363,9 @@ func (req *Request) Write(w *bufio.Writer) error {
if len(host) == 0 {
return errRequestHostRequired
}
req.Header.SetHostBytes(host)
if len(req.Header.Host()) == 0 || !req.AllowToChangeHostHeader {
req.Header.SetHostBytes(host)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the if len(host) == 0 { return errRequestHostRequired check then also be inside the if req.AllowToChangeHostHeader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The uri.Host() used as connection address at least for Client type. If host will be empty but Host header is not, connection will be broken. I've checked such code in http.go

	if len(req.Header.Host()) == 0 || !req.UseHostHeader {
		req.Header.SetHostBytes(host)
	} else if len(host) == 0 {
		return errRequestHostRequired
	}

And such usage for this code

	req.UseHostHeader = true
	req.SetHost("")
	req.Header.SetHost(host)

There is result - lookup : no such host

We can use headers for setting uri.Host() but I am not sure that it is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've check code again. It is not important for Client, because error raised inside dialAddr. But I am still not sure that it is correct allow uri.Host() to be empty if Host header presents.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But shouldn't len(host) == 0 be allowed when req.UseHostHeader === true? (when it's used outside of Client). It makes more sense to me to put that inside the if?

Sorry I'm replying bit slow, I'm traveling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how it should work in other cases. But when used in Client len(host) == 0 does not work. Is it ok for you if I change code to this?

if len(req.Header.Host()) == 0 || !req.UseHostHeader {
	req.Header.SetHostBytes(host)
} else if len(host) == 0 {
	return errRequestHostRequired
} 

In my case it does not important at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked logic again and pushed a new version. errRequestHostRequired should be returned only if both uri.Host() and req.Header.Host() are empty. And it does not depend from UseHostHeader. If Header.Host() is empty we always need to set header from uri.Host(). And only when both are not empty we need to check UseHostHeader.

@erikdubbelboer erikdubbelboer merged commit 4517204 into valyala:master Dec 17, 2021
@erikdubbelboer
Copy link
Collaborator

Thanks! this makes sense.

@sikasjc
Copy link

sikasjc commented Dec 22, 2021

Hi, it looks like UseHostHeader is not works. Client.Do uses reqCopy, but Request.copyToSkipBody does not copy the UseHostHeader

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 22, 2021

Hi, it looks like UseHostHeader is not works. Client.Do uses reqCopy, but Request.copyToSkipBody does not copy the UseHostHeader

Could you give me an example where is it does not works? Looks like that it may not works for pipelined client but I am not sure yet. I've using it in Client.Do and it works for me.

@sikasjc
Copy link

sikasjc commented Dec 23, 2021

Could you give me an example where is it does not works? Looks like that it may not works for pipelined client but I am not sure yet. I've using it in Client.Do and it works for me.

I made a mistake, I used Client.DoTimeout() not Client.Do().
Client.Do() is correct, but Client.DoTimeout() does not work. Here is an example:

testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	fmt.Println(r.Host)
	// output:
	// 127.0.0.1:port from Client.DoTimeout 
	// SomeHost       from Client.Do
}))
defer testServer.Close()

client := &fasthttp.Client{}
req := fasthttp.AcquireRequest()
defer fasthttp.ReleaseRequest(req)
resp := fasthttp.AcquireResponse()
defer fasthttp.ReleaseResponse(resp)

req.SetRequestURI(testServer.URL)
req.UseHostHeader = true
req.Header.SetHost("SomeHost")
if err := client.DoTimeout(req, resp, 10*time.Second); err != nil {
	log.Print(err)
}
if err := client.Do(req, resp); err != nil {
	log.Print(err)
}

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 23, 2021

Here is an example:

Thank you. I will check and fix it soon.

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 26, 2021

Hi, it looks like UseHostHeader is not works. Client.Do uses reqCopy, but Request.copyToSkipBody does not copy the UseHostHeader

I've created #1184 that fixes the problem.

@sikasjc
Copy link

sikasjc commented Dec 28, 2021

I've created #1184 that fixes the problem.

Thanks~
But the UseHostHeader was not reset, which may affect subsequent requests.

req := fasthttp.AcquireRequest()
req.UseHostHeader = true
fasthttp.ReleaseRequest(req)

newReq := fasthttp.AcquireRequest()
defer fasthttp.ReleaseRequest(req)
fmt.Println(newReq.UseHostHeader)
// output:
//     true, should be false

@Tolyar
Copy link
Contributor Author

Tolyar commented Dec 28, 2021

But the UseHostHeader was not reset, which may affect subsequent requests.

You are right again. Thanks a lot. There is fix #1185

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.

None yet

3 participants