-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add hostname parameter #823
Add hostname parameter #823
Conversation
Signed-off-by: anemyte <anemyte@gmail.com>
77107e4
to
76678d2
Compare
@roidelapluie please look |
Yes sorry, I am a bit behind but will followup shortly. |
My suggestion is to use the following method:
In this way, the consistency of the use mode, avoid parameter conflicts, misoperation, and expandability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have tested this and added my comments.
prober/http.go
Outdated
} | ||
if !changed { | ||
// Otherwise use the hostname of the target. | ||
httpClientConfig.TLSConfig.ServerName = targetHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could set it first so we do not need !changed
main.go
Outdated
@@ -120,6 +120,23 @@ func probeHandler(w http.ResponseWriter, r *http.Request, c *config.Config, logg | |||
return | |||
} | |||
|
|||
if module.Prober == "http" { | |||
paramHost := params.Get("hostname") | |||
if paramHost != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really find this code elegant, maybe moving it to a dedicated function could help making it nicer.
main.go
Outdated
} | ||
} | ||
} | ||
module.HTTP.Headers["Host"] = paramHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mutating the actual module, so all subsequent calls will get the same host header, if the hostname parameter is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review! I agree with the comments and I'll push a fix soon. However this one puzzles me because I cannot achieve the described behavior. I'll look into this anyway but it will be nice if you help me to fall into the problem. I tried myself but my tests failed to prove that parameter mutated the module. Here is a pastebin with the requests that I made; the exporter was not restarted during those tests and both debug output and server responses show that the module has not been changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reproduce, please add an unrelated HTTP header to the config
Signed-off-by: anemyte <anemyte@gmail.com>
9df2326
to
0d9bfa3
Compare
@roidelapluie I've added fixes, hope it looks better now. Can you please look again? |
main.go
Outdated
@@ -150,6 +159,23 @@ func probeHandler(w http.ResponseWriter, r *http.Request, c *config.Config, logg | |||
h.ServeHTTP(w, r) | |||
} | |||
|
|||
func setHttpHost(hostname string, module *config.Module) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func setHttpHost(hostname string, module *config.Module) error { | |
func setHTTPHost(hostname string, module *config.Module) error { |
Thanks! Just one nit. |
Signed-off-by: anemyte <anemyte@gmail.com>
Thanks! |
Thank you for the review :) |
This adds the ability to set the TLS server name for TCP probes using the hostname parameter, just like #823 did for HTTP probes * add hostname parameter for tcp probe * only add servername if TLS is true * add even if TLS isn't true in case of STARTTLS * added test hostname parameter with TCP probe * remove unnecessary function and inline assignment --------- Signed-off-by: Lyas Spiehler <lspiehler@gmail.com> Co-authored-by: Lyas Spiehler <lyas.spiehler@sapphirehealth.org>
This is refactoring of #816
As per the design doc, the exporter now accept
hostname
parameter, which setsHost
header for HTTP probes. It also sets TLS server name (if it is not already defined by module configuration) to avoid TLS handshake problems whentarget
is an IP address. SettingHost
with both module configuration andhostname
parameter results in 400 Bad Request.