Implement dynamic probes

[jkroepke]

This is useful on platforms where a team offers a observability Stack and users could use them, e,g define custom fail reges matches without creating a dedicated module for each use case. In combination with the Probe CRD, it would be an powerful option.

• Fixes [Feature]probe support params of module  #986

[jurgenhaas]

Very nice, that's exactly what I was looking for. Hope this gets accepted and merged soon.

[dswarbrick]

I would recommend drawing attention to possibility that this could be abused by malicious actors. Currently, all aspects of a probe are predefined by a (presumably) responsible sysadmin. As such, blackbox_exporters may be used to probe targets that would not normally be accessible (e.g. behind some network perimiter).

This PR would open up the blackbox_exporter to injecting all manner of arbitrary headers / body in requests to arbitrary targets, and this really accentuates the need to adequately secure the blackbox_exporter from unauthorised use.

[jurgenhaas]

this really accentuates the need to adequately secure the blackbox_exporter from unauthorised use.

Isn't that what should be done in any case, even without this enhancement?

[dswarbrick]

Isn't that what should be done in any case, even without this enhancement?

Arguably, yes. But if the blackbox_exporter has only been configured with a concise set of known-innocent probes, probably a lot of instances are not secured (albeit hopefully not accessible from public networks).

If this new functionality is enabled and available by default, a lot of unsecured blackbox_exporters would suddenly become a much bigger security risk, and this should really be communicated in big bold letters.

[jkroepke]

I can understand the points from @dswarbrick and I guess this feature should be an opt-in by default. Which means, an sys admin has explicit enabled this feature.

Then, this should not introduce new issues by default, and targets behind blackbox_exporter remains secured.

@dswarbrick do you think, this might be enough?

[dswarbrick]

@jkroepke I certainly think that the feature should be opt-in.

[SuperQ]

Yes, we already get enough security reports that the blackbox_exporter is allowing the target param to make it a proxy.

[jkroepke]

There is always the caveat between unsecured instances of blackbox exporter vs. an well secured environment.

@SuperQ To support both sides, could be a opt-in toggle a compromise?

[jkroepke]

@electron0zero Do you have an opinion, here?

Since this is opt-in be default, it should fine. For additional network restrictions, I would recommend NetworkPolicies.

[electron0zero]

@electron0zero Do you have an opinion, here?

Since this is opt-in be default, it should fine. For additional network restrictions, I would recommend NetworkPolicies.

this looks like a valid use-case to me, but since it's a fairly big change I would like to hear what the other maintainers have to say, and ideally have a discussion about the feature, and how we want to implement it :)

cc @mem @roidelapluie

[jkroepke]

@mem @roidelapluie do you have an opinion here?

[jkroepke]

@electron0zero it seems like, other maintainers does not have any opinion here.

How we want to proceed here?

[jc36]

I have already tested and use these dynamic probes. Thank you very much for the work done!
I also added query:"timeout" here as it is necessary for some probes. (units are nanoseconds, so 5s = 5000000000)

[jkroepke]

@jc36 the blackbox exporter should respect the scrape_timeout from Prometheus.

blackbox_exporter/prober/handler.go

Line 207 in c908cba

func getTimeout(r *http.Request, module config.Module, offset float64) (timeoutSeconds float64, err error) {

You can test this behavior via curl, by using curl -H "X-Prometheus-Scrape-Timeout-Seconds: 10"

Do you think an explicit timeout query is still useful?

[jkroepke]

@electron0zero do you have an opionion here how we can proceed here

[jc36]

I didn't really understand where I should add the header "X-Prometheus-Scrape-Timeout-Seconds" in the scrape-config job settings so that blackbox-exporter waits for a response from the target for no more than the specified time.

  scrape_configs:
        - job_name: blackbox_exporter_tcp
          params:
            prober: ['tcp']
            timeout: ['5000000000'] # timeout to wait response from target
            tcp.preferred_ip_protocol: ['ip4']
          scrape_interval: 30s
          scrape_timeout: 10s  # timeout to wait response from blackbox
          metrics_path: /probe/dynamic
          scheme: http
          static_configs:
            - targets:
                - some-target.com:443
          relabel_configs:
            - source_labels: [__address__]
              target_label: __param_target
            - target_label: __address__
              replacement: extmon-blackbox-exporter:9115

In any case, the timeout specified together with other parameters will be clearer.

[jkroepke]

It will be set by prometheus based on the scrape_timeout configuration.

https://github.com/prometheus/prometheus/blob/4419399e4e831052bc9b8b4f26a8e7cf337091b0/scrape/scrape.go#L826

[jkroepke]

@SuperQ @electron0zero @mem @roidelapluie Can I assume that we have a common sense here?

[GuridMa]

When will this feature be released approximately?
What version to publish to？

[mem]

Hi,

Thank you for taking the time to contribute to blackbox_exporter.

I have been looking at this code. It seems mostly OK. The goal of the change is not clear, though. It's hard to give you a good code review without knowing what the goal is.

The bit added to README.md tells the user how to use the feature, but not why they would want to use the feature, e.g. when it's appropriate to use one configuration method over the other. I find the example confusing since that's something that you can do with the regular configuration file. Other than embedding the configuration in the Prometheus configuration, I don't see much advantage with this method. In fact, things that are simple using two configuration files become a little awkward (because params is a sequence of key/value pairs, so while it still looks like you are writing YAML, you are in fact not, at least not what you think you are writing, e.g. prober: [http] has to be written like that instead of just prober: http; I was a bit confused as to why that worked until I remembered how it's being parsed).

Lacking a description of the goal of this change, my guess is that you have some highly dynamic system and you are obtaining the configuration parameters from some other system. If that's the case, I still don't see why it's not possible to generate the regular configuration file with modules and pass the parameters as usual and then issue a reload when the configuration changes.

[mem]

Also, I'm not following how this addresses #1050.

[mem]

Also, I'm not following how this addresses #1050.

I should be more verbose.

That issue is talking about not storing the API key in the configuration file.

While I do see that this change would address that in the sense that the configuration file is not required, you would still need to write this in the Prometheus configuration file, as shown in the example.

That issue is basically asking for a secret to be read from a different location (e.g. vault) and then passed to BBE in some way. This change implements "some way". But if you want to scrape BBE using Prometheus, you would still need to write that information to the configuration file, wouldn't you?

[jkroepke]

Goal: As part of the platform team, I want a provide a generic probe service which can be use by any team. Each team can setup probes without having the requirement of preregister probes on the probe service. With the PR, Teams can use the Prometheus Operator Scrape CR to configure any probes which are supported by blackbox exporter. Including expecting specific strings, set headers, expect status codes

But if you want to scrape BBE using Prometheus, you would still need to write that information to the configuration file, wouldn't you?

Yes, but in with an Prometheus Operator eco system, it can be securely abstracted. The values can be alternativly delivered by auto discovery.

If users are running a standalone/static Prometheus, its may have no benefit.

[SuperQ]

With the PR, Teams can use the Prometheus Operator Scrape CR to configure any probes which are supported by blackbox exporter. Including expecting specific strings, set headers, expect status codes

I agree with @mem, this seems like an XY Problem.

It sounds like what you need is "ProbeConfig" CRD that allows teams to dynamically configure the exporter. This way a matching Probe would be simpler to maintain.

[jkroepke]

The an another goal of the PR is to provide an generic request interface can eliminate the concept additional abstraction of probes. I would like to have one place where I can configure probe targets and probe options. Like the nagios check check_http in the old days. The configuration of probes can be fully shifted to the Prometheus configuration.

It sounds like what you need is "ProbeConfig" CRD

And with this PR, an ProbeConfig CRD is not longer needed, since the configuration could be included into Probe CRs. Of cause, an blackbox exporter operator would be one solution, but I'm favorite to remove the abstraction of pre-defined probe configs which eliminate the required of such an Operator.

The alternative to Prometheus Operator would the classical annotation based service discovery. Annotation could be use to configure additional probe settings, e.g. expected status code.

[jkroepke]

I resolved the merge conflicts.

One more use-case: Since blackbox exporter is embedded into grafana/agent, it would be helpful, too.

Instead having the requirements of maintain 2 distinct configs (probes + probe target), the PR aims to avoids this.

@SuperQ @electron0zero @mem How we can proceed here? Are we comfortable to merge this?

[SuperQ]

No, sorry, I don't think we want this feature.

[electron0zero]

I am in agreement with @SuperQ and @mem on this :)

[jkroepke]

It make me unhappy to read this.

It removes a lot of complexity on a lot of setups by shifting the probe configs into prometheus (or upstream SD).

Feature like http.fail_if_body_matches_regexp are mostly valid for single endpoints and worst case I have to configure a module for each probe to observe (In cases where I have to check more than status code).

@SuperQ I would like to appreciate one time more here to understand to user point of view here. We have the Prometheus Operator Eco system where someone can define probes via Kubernetes Custom Resources, but we do not have a corresponding blackbox_exporter Operator for Kubernetes and I expect in the next 12-24 months, there wont be one.

Looking at exporters like https://github.com/webdevops/azure-metrics-exporter#default-template , they allows each input as GET parameters, which perfectly integrate into the existing Prometheus Operator eco system.

[SuperQ]

We have the Prometheus Operator Eco system where someone can define probes via Kubernetes Custom Resources, but we do not have a corresponding blackbox_exporter Operator for Kubernetes and I expect in the next 12-24 months, there wont be one.

Maybe you can contribute them.

[SuperQ]

It removes a lot of complexity on a lot of setups by shifting the probe configs into prometheus (or upstream SD).

That's just shifting complexity around. It doesn't actually solve the underlying problem and makes it worse for everyone since there's now a much larger abuse potential.

The XY Problem here is that you need to make the probes self-service. That's a problem with the blackbox_exporter config, not Prometheus.

I recommend checking the Prometheus Operator issue tracker and filing an issue there first.

[SuperQ]

Since all of the maintainers agree that we can not accept this, I am going to close this PR.