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

http probe: skip DNS lookup if using proxy #338

Closed
wants to merge 11 commits into from

Conversation

veksh
Copy link

@veksh veksh commented Jun 29, 2018

This PR implements simple work-around to skip DNS lookup for target if proxy_url is defined in module config. It seems to be popular request for monitoring availability of external hosts if only proxied access to Internet is available (see #303 and #264).

@brian-brazil
Copy link
Contributor

This would break the functionality of choosing v4 vs v6. This probe is not intended to be used with a proxy, and changing this will likely break other uses cases.

@veksh
Copy link
Author

veksh commented Jul 2, 2018

"v4 vs v6" is fixable by probing proxy address instead of target, I could implement that in PR.

As for using probe with proxy: well, with proxy_url parameter (and fixes from this PR) it works pretty well for probing availability and latency of external dependencies. Putting probe to DMZ for direct access will measure different metrics, less relevant to internal application. What are other use cases that will be broken by fixing proxy functionality? Maybe they are fixable too?

@brian-brazil
Copy link
Contributor

It's a general complexity concern, you're second guessing the user by doing magic under the covers. That's bound to cause confusion at some point.

@veksh
Copy link
Author

veksh commented Jul 2, 2018

Automated "v4 vs v6" choice is already a bit "magical" (in a good sense), but for proxied case semantic is different: proxy could be accessible with v4 while accessing host with v6. Maybe mentioning that in exporter documentation will clear potential confusion?

Proxied probing is already confusing (not to say "broken") enough: it seems to work OK, but actually do so only if target is resolvable with DNS, which is completely unnecessary with proxied access. This PR will not change probe functionality if noproxy_url is enabled, so nobody will be negatively affected.

@brian-brazil
Copy link
Contributor

As I've indicated elsewhere, the proxy_url is only there because the library we use for http configuration includes it. Even with a proxy, we still should be sending the same HTTP request to it so that we know what IP we're talking to - which is important for debugging failed probes.

The purpose of the http probe is to test real http targets, not to test proxies.

@veksh
Copy link
Author

veksh commented Jul 2, 2018

... the proxy_url is only there because the library we use for http configuration includes it.

Ok, and it is a great feature to have, thanks to that http library :)

Even with a proxy, we still should be sending the same HTTP request to it so that we know what IP we're talking to - which is important for debugging failed probes.

If "it" means "proxy" in this context, we could query it with HTTP to get back (local) v4 vs v6 auto-detection and (local) DNS timing (and mb document that in blackbox_exporter README).

The purpose of the http probe is to test real http targets, not to test proxies.

Goal is not to test proxy, but to check accessibility of external dependency. Like, our internal app depends on external API (like Google Maps for some geo-spatial calculations), so latency and alerting must be correlated to (proxied) http metrics for external host. Current blackbox_exporter could do that just fine, if only not for DNS lookup requirements. Actually I could proxy limited DNS access to internal host (and for plain http just adding "tgt_hostname" to /etc/hosts seem to do the trick), but it makes little sense outside of making blackbox_exporter happy.

@brian-brazil
Copy link
Contributor

we could query it with HTTP to get back (local) v4 vs v6 auto-detection and (local) DNS timing (and mb document that in blackbox_exporter README).

I don't see how that would work, and that'd be getting a bit involved.

Actually I could proxy limited DNS access to internal host (and for plain http just adding "tgt_hostname" to /etc/hosts seem to do the trick), but it makes little sense outside of making blackbox_exporter happy.

It sounds like you have a non-trivial setup that includes some form of split-DNS setup, I don't think that's something we can sanely support given all the other features we currently do support I'm afraid.

@veksh
Copy link
Author

veksh commented Jul 2, 2018

I don't see how that would work, and that'd be getting a bit involved.

Maybe just resoling proxy address instead of target would do?

My setup is simple: application could access Internet only via proxy, DNS is local-only so external resources are accessible, but not directly resolvable. Testing proxy itself is easy (just http connect to it). Checking external resources must follow the same path that application use to access them.

@brian-brazil
Copy link
Contributor

Per previous discussions looking at a way to test proxy servers is the current plan. It's not determined yet how that would look config wise, as it could be added onto the http probe or be a new probe type.

@veksh
Copy link
Author

veksh commented Jul 2, 2018

Testing proxy itself is easy: in case of squid it could be implemented with either http_probe or just connecting to proxy port (3128 by default) and issuing "GET cache_object://mycache.example.com/info HTTP/1.0". Checking remote host accessibility via proxy is another matter.

@brian-brazil
Copy link
Contributor

Think more how the dns probe is to test dns servers rather than dns resolution generally.

@alexeiaguiar
Copy link

alexeiaguiar commented Sep 28, 2018

Our security department is implemented a policy that all services in production must pass through an outbound proxy to reach external services. We are the first team to implement that. We configured tens of applications to use the proxy and all of them worked, including many that were developed in Go. blackbox_exporter is the only one that doesn't work because it sends the ip address instead of sending the hostname like all other applications do. I did a fix that is pretty similar to this one and I was about to send it when I found this PR.
I think this fix is not only acceptable, but the right thing to do. I hope it will be accepted so that we can go on and keep blackbox_exporter as planned.

@dabear
Copy link

dabear commented Mar 19, 2019

@alexeiaguiar agreed, we do have the same problem at my workspace currently. I've added a comment to #303 but that issue seems to be closed. Makes it kinda hard to monitor sites that changes ips frequently, when you have to pass through a proxy for outgoing connections to the internet.

@dabear
Copy link

dabear commented Mar 22, 2019

@brian-brazil @veksh Any chance to get this PR built against latest master and mainlined?

@DeviaVir
Copy link

@dabear feel free to grab https://github.com/deviavir/blackbox_exporter, in a Dockerfile:

FROM golang:alpine

RUN apk add --no-cache git
RUN go get github.com/DeviaVir/blackbox_exporter \
    && GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build github.com/DeviaVir/blackbox_exporter

FROM alpine:latest
COPY --from=0 /go/bin/blackbox_exporter /bin/blackbox_exporter

EXPOSE      9115
ENTRYPOINT  [ "/bin/blackbox_exporter" ]
CMD         [ "--config.file=/etc/blackbox_exporter/config.yml" ]

@SuperQ
Copy link
Member

SuperQ commented Apr 1, 2019

@DeviaVir Would you be able to cherrypick and publish a new PR to replace this one?

@DeviaVir
Copy link

DeviaVir commented Apr 1, 2019

@SuperQ I think @brian-brazil was clear in that the PR in its current form will not be merged. Feel free to use my fork.

@raypettersen
Copy link

Voicing additional concern, blackbox must be able to function through a proxy, so we can safe-guard the applications and detect anomalies in proxy-only environments.

@veksh
Copy link
Author

veksh commented Apr 3, 2019

I've (force-)rebased this PR against current master.

@veksh veksh reopened this Apr 3, 2019
@brian-brazil
Copy link
Contributor

As indicated above, this change will not be accepted. However there is the path of making the http probe suitable for probing proxies, which would also cover your use case.

@veksh
Copy link
Author

veksh commented Apr 3, 2019

I've also updated binary

@veksh
Copy link
Author

veksh commented Apr 3, 2019

@brian-brazil once again: this PR is not for probing proxies, but for probing http servers via proxy.

@veksh veksh closed this Apr 3, 2019
@veksh veksh reopened this Apr 3, 2019
@brian-brazil
Copy link
Contributor

I am aware, and that is not in line with the rest of the exporter.

@DeviaVir
Copy link

DeviaVir commented Apr 8, 2019

@brian-brazil what is the recommended way of healthchecking tor addresses using the exporter?

@dnascimento
Copy link

dnascimento commented Jul 5, 2019

Same use case here, proxy is the only way to reach outside.
How can we make changes so that this does not break ipv4/ipv6 but supports via proxy?
@DeviaVir your configuration works well

@brian-brazil
Copy link
Contributor

A thought: Why not put the blackbox exporter on the other side of your proxy?

@okushchenko
Copy link

A thought: Why not put the blackbox exporter on the other side of your proxy?

@brian-brazil this is not possible in some environment that mandate usage of HTTP proxies for all outgoing traffic as an InfoSec practice.

@brian-brazil
Copy link
Contributor

You'd still be going through the proxy, the blackbox exporter would live on the other side of it.

@raypettersen
Copy link

This is becoming a bit silly. It's clear that Brian doesn't want this feature at all in the exporter, and we're forced to use forked versions for getting this basic feature as PRs are just left to rot. If it was a wanted feature then I would expect a more constructive tone than this.

@brian-brazil
Copy link
Contributor

Constructive in this context means helping solve the problem where possible and not leading you on, on which front I've made several suggestions and made clear what won't be accepted. Constructive does not mean agreeing with everything someone else says.

As this is going nowhere, I'm going to close and lock this. You're always free to discuss this on prometheus-developers.

@prometheus prometheus locked and limited conversation to collaborators Jul 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants