Closed thomasgl-orange closed 2 years ago
This is a breaking change and not how we usually do things in the Prometheus ecosystem. I think it would be preferable to add a proxy URL to the URL parameters, or to a specific file (to avoid leaking secrets).
This is a breaking change
It is a breaking change for someone who runs haproxy_exporter
with an $HTTP(S)_PROXY
var set, but no (proper) $NO_PROXY
var, and who relies on these proxy settings being ignored. I think it is a very unlikely situation, with straightforward solutions.
not how we usually do things in the Prometheus ecosystem
This PR simply restores what would otherwise be the default behaviour of http.Client
, if the http.Transport
had not been customized to allow disabling TLS check in #86. $HTTP_PROXY
and friends were indeed properly taken into account before that (just tested with the 0.8.0 binary), so the current state looks more like an accident than a conscious decision (which is fine, I understand that having to go through an HTTP proxy from the exporter is not a common requirement, and that this "regression" was overlooked back then).
I think it would be preferable to add a proxy URL to the URL parameters, or to a specific file (to avoid leaking secrets).
I disagree, I don't like having every single binary invent its own way to get proxy settings. I like the standardized env-based way golang promotes in the http
lib. Just my opinion of course.
====
On a side note, I realize that my PR could be much shorter:
diff --git a/haproxy_exporter.go b/haproxy_exporter.go
index 54b4cbb..7b44548 100644
--- a/haproxy_exporter.go
+++ b/haproxy_exporter.go
@@ -332,7 +332,10 @@ func (e *Exporter) Collect(ch chan<- prometheus.Metric) {
}
func fetchHTTP(uri string, sslVerify bool, timeout time.Duration) func() (io.ReadCloser, error) {
- tr := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: !sslVerify}}
+ tr := &http.Transport{
+ Proxy: http.ProxyFromEnvironment,
+ TLSClientConfig: &tls.Config{InsecureSkipVerify: !sslVerify},
+ }
client := http.Client{
Timeout: timeout,
Transport: tr,
I didn't want to push-force anything now that you've already viewed the initial version, but I'm happy to do it if you don't mind.
Wheter or not it is a breaking change is not really subject of question. Someone could have a global proxy set but targetting haproxy locally which should not be proxied.
In the whole Prometheus ecosystem, we do not set proxy based on environment variables. I would avoid having one exporter behaving differently than others, but I do recognize that users should be able to set proxies. What about a boolean flag --http.proxy-from-env maybe?
What about a boolean flag --http.proxy-from-env maybe?
Yes, it sounds good. I've pushed a new version with this flag.
Thanks! There are linting issues.
Can we keep it http.proxy-from-env? that way we could reuse it for other exporters that might need this. It also clarifies that it does not concern HAProxy when used with unix sockets.
Oops, sorry for haproxy.proxy-from-env
vs http.proxy-from-env
, I will change that tomorrow, and also have a look on the linting errors.
The CLI flag is now renamed http.proxy-from-env
.
Regarding linting error, I don't get any with golangci-lint
latest version (1.45.2), which is what I had installed here (hence no error for me on make
). Downgrading to 1.42.0, I've been able to reproduce (that's with go 1.18.1). It's not related to this PR, I get the same on master.
Anyway, I propose bumping golangci-lint
to 1.45.2 (Makefile and GitHub actions). I've added a commit for that, so you can see it fixes the GitHub Action. But I can get rid of this commit if you prefer a separate PR.
For what it's worth (just scratching an itch), a git bisect
on golangci-lint
from 1.43.0 to 1.44.0 (first version where linting errors disappear when running on haproxy_exporter
) leads to golangci/golangci-lint#2373 (bump to gofumpt v0.2.0) as the commit which fixed these errors.
Thanks for the review and suggestions; I've applied them all, but forgot Signed-off-by
because I did it in GitHub web UI (todogroup/gh-issues#50), and so DCO not happy. I will rewrite that when rebasing on an updated master (with golangci-lint bump).
done :)
Thanks, rebased :)
Thanks!
I need to run the
haproxy_exporter
from an environment which can only reach the target haproxy stats API by going through an HTTP proxy. The wayhaproxy_exporter
currently executes its GET requests (with a customizedhttp.Transport
to allow disabling TLS validation) ignores the proxy settings defined in the traditional environment variables ($HTTP(S)_PROXY
,$NO_PROXY
). This PR fixes that, by adding the proxy fromhttp.ProxyFromEnvironment
to thehttp.Transport
. If there is no proxy env vars, or the target haproxy domain is in$NO_PROXY
, the proxy will benil
, and nothing changes in the way requests are executed.It can be tested, for instance, by running a local mitmproxy service, and running
haproxy_exporter
with aHTTP_PROXY=http://localhost:8080
(or$HTTPS_PROXY
) env var.CC @grobie, @roidelapluie