ribbybibby / ssl_exporter

Exports Prometheus metrics for TLS certificates
Apache License 2.0
525 stars 99 forks source link

allow module and target be configured via flags #68

Closed hroongtanew closed 2 years ago

hroongtanew commented 3 years ago

Currently the probe target needs to be passed module and target via query params. However for some systems where the scrapper is hard to change from /metrics it becomes impossible to use this.

As a result add 2 flags web.default-module and web.target which can be used in the probe handler to default (in case query param is empty)

ribbybibby commented 3 years ago

What are you scraping the exporter with that can't configure query params?

hroongtanew commented 3 years ago

@ribbybibby We have our own central scrapper which is dynamically configured and only takes port/host.. In other words assumes /metrics and no params can be passed in. This change allows more flexibility to configure, without changing any default you had previously.

perlun commented 3 years ago

@hroongtanew Would it be possible to implement this in your central scraper instead? Given that Prometheus has that functionality built-in, it feels like a reasonable feature to support. You might be running into similar issues with other exporters as well (for example, the Envoy metrics are by default exposed at /stats/prometheus). While this can be worked around, I strongly suggest bringing this up with the developer(s) of the central scraper you're using instead, to make it support more flexible setups.

hroongtanew commented 3 years ago

@perlun I see your your that promethus can be configured to pass params. Unfortunately the layer above the generic scrapper in our case only accepts port and host. Is it possible to change it? yes with lot of pain.

Is it unreasonable to add more configuration options in exporter for defaults without any changes to the current behaviour or the functionality? (I do agree with @ribbybibby 's comment to move it to config file instead of flags)

perlun commented 3 years ago

@perlun I see your your that promethus can be configured to pass params.

Yes. More details about this can be found here, the params keyword: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config

Unfortunately the layer above the generic scrapper in our case only accepts port and host. Is it possible to change it? yes with lot of pain.

Too bad for you.

Sorry to be (quite) a bit obnoxious here, I just want to understand your circumstances a bit better.

Is it unreasonable to add more configuration options in exporter for defaults without any changes to the current behaviour or the functionality?

@ribbybibby will have to answer whether he finds it reasonable or not; I'm not in charge of this project. I just think that it's pretty fine for a Prometheus exporter to be optimized for being consumed by Prometheus. If the exporter is consumed by something else, well, sorry, but you are a bit "on your own" already.

Having that said: I looked briefly at your PR and the complexity it adds seems manageable. Rob already implied that he would support a PR handling this in the config file, so... if's he fine with it, why not. :smile:

ribbybibby commented 3 years ago

I agree that this exporter shouldn't introduce changes just to make life easier for people trying to use it in obscure ways.

That being said, I think that being able to set the module+target on the exporter rather than in Prometheus could be desirable in situations where the exporter is running 1-to-1 with the target it's scraping.

If you're probing a list of URLs with the tcp prober it makes sense for the targets to live in the Prometheus scrape config because ultimately the URL is the instance that we're scraping. That's why the instance is replaced with the value of target.

However, if you're running an ssl_exporter per-node with the file prober, scraping a target like /etc/tls/**.crt, then I don't think that target necessarily needs to be in Prometheus. The instance in that case is the exporter itself and the targets it exports are probably better defined closer to the exporter, in the same way that you can enable/disable collectors in the node-exporter.

perlun commented 3 years ago

If you're probing a list of URLs with the tcp prober it makes sense for the targets to live in the Prometheus scrape config because ultimately the URL is the instance that we're scraping. That's why the instance is replaced with the value of target.

However, if you're running an ssl_exporter per-node with the file prober, scraping a target like /etc/tls/**.crt, then I don't think that target necessarily needs to be in Prometheus. The instance in that case is the exporter itself and the targets it exports are probably better defined closer to the exporter, in the same way that you can enable/disable collectors in the node-exporter.

Thanks for sharing this. I'll admit I'm very new in this project (I deployed the exporter to our in-house Prometheus server yesterday :see_no_evil:) but I wonder what the major benefits would be to deploy the ssl_exporter like that? Isn't typically it much easier (with less moving parts) to just have a single ssl_exporter instance and let it probe an arbitrary number of SSL endpoints (TCP, HTTPS, etc)? Just like you describe in the README.md file.

One thing I can imagine: firewall rules, but... it's not like it's more likely that port 9219 is open to a random set of hosts than port 443. :smirk: But maybe there's some use cases here that I fail to see at the moment.

ribbybibby commented 3 years ago

Isn't typically it much easier (with less moving parts) to just have a single ssl_exporter instance and let it probe an arbitrary number of SSL endpoints (TCP, HTTPS, etc)?

That's true for the tcp and https probers, yes. The file prober however exports metrics local to the exporter, so if you want to export metrics for certificates on different nodes in this way, you'd need an exporter per-node.

ribbybibby commented 2 years ago

Added here: https://github.com/ribbybibby/ssl_exporter/commit/02d61835e831b0fe48bd8859a0c5b69bd74a7243