Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

httpRequestForTarget stripping out port when setting Req.Host #365

Closed
robertcankney opened this issue Feb 14, 2020 · 5 comments
Closed

httpRequestForTarget stripping out port when setting Req.Host #365

robertcankney opened this issue Feb 14, 2020 · 5 comments
Assignees
Labels
Milestone

Comments

@robertcankney
Copy link

robertcankney commented Feb 14, 2020

The below probe is trying to connect on port 80 - as evidenced in the below log snippet (internal host names redacted):

probe {
  name: "test-probe"
  type: HTTP
  interval_msec: 15000

  targets {
    # RDS (resource discovery service) targets 
      host_names: "internal_host1,internal_host12,internal_host3,internal_host4"
  }
  
  http_probe {
    resolve_first: true
    relative_url: "/service-bus"
    port: 8080
  }
}

W0214 20:10:26.962758 216 http.go:212] Target:internal_host3, URL:http://internal_ip3:8080/service-bus, http.doHTTPRequest: Get http://internal_host3/service-bus/: dial tcp internal_ip3:80: connect: connection refused

This appears to be due to https://github.com/google/cloudprober/blob/master/probes/http/request.go#L75, which is setting the Req.Host field to the Target.Name (sans port), which is then being used in https://golang.org/src/net/http/client.go#L569 to overwrite the host from the URL, which had the correct port.

@manugarg
Copy link
Contributor

Thanks for reporting the issue and debugging it. It is wrong to skip the custom port in req.Host, but what I find surprising is that this code is not new. We've been using it ourselves for quite some time.

https://golang.org/src/net/http/client.go#L569 seems to suggest that req.Host is used as host, only if there is a redirection. Is there any redirection involved here?

I'll submit a fix for this anyway.

@manugarg manugarg self-assigned this Feb 14, 2020
@manugarg manugarg added the bug label Feb 14, 2020
@manugarg manugarg added this to the v0.10.7 milestone Feb 14, 2020
@manugarg
Copy link
Contributor

I verified that at least for normal case (no redirection), it works fine -- i.e. it uses the port provided. We should still fix req.Host though.

W0214 13:31:23.370350 44895 http.go:220] Target:www.google.com, URL:http://172.217.9.164:8080/, http.doHTTPRequest: timeout error: Get http://172.217.9.164:8080/: context deadline exceeded

@robertcankney
Copy link
Author

Hi Manu, dug in some more and the Tomcat server is performing a redirection to the same path and just adding headers - hadn't caught that initially. Thanks for taking a look at this!

@manugarg
Copy link
Contributor

@robertcankney Thanks for confirming. I'll send a fix soon.

manugarg added a commit that referenced this issue Feb 18, 2020
HTTP spec says that Host header should include port if port is not the default port. Host header's port matters mostly when server redirects the request to another relative URL -- in that case, Go HTTP client uses Host header's value for connection.

Also, add some tests for req.Host behavior.

Fixes: #365
PiperOrigin-RevId: 295666337
@robertcankney
Copy link
Author

Thanks @manugarg!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants