Add support of Host header
#9411
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9411 +/- ##
=========================================
+ Coverage 0 90.40% +90.40%
=========================================
Files 0 346 +346
Lines 0 18112 +18112
=========================================
+ Hits 0 16374 +16374
- Misses 0 1405 +1405
- Partials 0 333 +333 ☔ View full report in Codecov by Sentry. |
| if found && hostHeader != "" { | ||
| // `Host` field should be set to override default `Host` header value which is Endpoint | ||
| req.Host = string(hostHeader) | ||
| } | ||
| for k, v := range interceptor.headers { | ||
| req.Header.Set(k, string(v)) |
There was a problem hiding this comment.
would Host still be available at this point?
There was a problem hiding this comment.
hi, yes! It works in test that I wrote and test is broken without this change. I also just tested in my environment and it works.
There was a problem hiding this comment.
I took a closer look at Go's documentation, and there's one mismatch here compared to that. Go's stdlib precedence is:
- req.Host
- req.URL.Host (from HTTPServerConfig.Endpoint.Host)
- req.Headers["Host"] (from HTTPServerConfig.Headers["Host"])
Right now, we don't have an equivalent to req.Host in confighttp, so this PR introduces the following precedence order:
- HTTPServerConfig.Headers["Host"]
- HTTPServerConfig.Endpoint.Host
While I don't expect this to be a breaking change affecting any users at all given that this header must be explicitly set as part of the headers field, I would prefer if we'd introduce a HostHeader field, to be consistent with Go ("path of least surprise" type of thing).
But I'm not 100% convinced myself. I'd appreciate other approvers to give their opinions.
cc @open-telemetry/collector-approvers
There was a problem hiding this comment.
The http.Request doesn't use the Headers["Host"].
// For client requests, Host optionally overrides the Host
// header to send. If empty, the Request.Write method uses
// the value of URL.Host. Host may contain an international
// domain name.
Host string
There was a problem hiding this comment.
I'm not 100% sure this is the way to move with this PR. Go tries to be smart by promoting the Host header to req.Host field, but at the HTTP level, it's all headers...
I'm fine with both the approach this PR takes and with adding a new field to the config specifically for the host (like Go does with the Request object).
| if found && hostHeader != "" { | ||
| // `Host` field should be set to override default `Host` header value which is Endpoint | ||
| req.Host = string(hostHeader) | ||
| } | ||
| for k, v := range interceptor.headers { | ||
| req.Header.Set(k, string(v)) |
There was a problem hiding this comment.
I took a closer look at Go's documentation, and there's one mismatch here compared to that. Go's stdlib precedence is:
- req.Host
- req.URL.Host (from HTTPServerConfig.Endpoint.Host)
- req.Headers["Host"] (from HTTPServerConfig.Headers["Host"])
Right now, we don't have an equivalent to req.Host in confighttp, so this PR introduces the following precedence order:
- HTTPServerConfig.Headers["Host"]
- HTTPServerConfig.Endpoint.Host
While I don't expect this to be a breaking change affecting any users at all given that this header must be explicitly set as part of the headers field, I would prefer if we'd introduce a HostHeader field, to be consistent with Go ("path of least surprise" type of thing).
But I'm not 100% convinced myself. I'd appreciate other approvers to give their opinions.
cc @open-telemetry/collector-approvers
|
I don't mind adding a separate config, whatever community decides |
|
I added a topic to discuss today during the SIG meeting. Feel free to join us! https://docs.google.com/document/d/1r2JC5MB7GupCE7N32EwGEXs9V_YIsPgoFiLP4VWVMkE/edit |
Closes #9395