Skip to content

promtool: Add TLSClientConfig to query subcmd#8082

Closed
mrueg wants to merge 1 commit intoprometheus:mainfrom
mrueg:tls-promtool
Closed

promtool: Add TLSClientConfig to query subcmd#8082
mrueg wants to merge 1 commit intoprometheus:mainfrom
mrueg:tls-promtool

Conversation

@mrueg
Copy link
Contributor

@mrueg mrueg commented Oct 19, 2020

This adds client cert support to promtool query subcmd, allowing queries against a prometheus server.

Usage:

./promtool query --cacert chain.pem --cert client.pem --key key.pem instant $SERVER $QUERY

usage: promtool query instant [<flags>] <server> <expr>

Run instant query.

Flags:
  -h, --help           Show context-sensitive help (also try --help-long and --help-man).
      --version        Show application version.
      --cacert=CACERT  Path to CA Certificate File
      --cert=CERT      Path to Client Certificate File
      --key=KEY        Path to Client Key File
  -o, --format=promql  Output format of the query.
      --time=TIME      Query evaluation time (RFC3339 or Unix timestamp).

Args:
  <server>  Prometheus server to query.
  <expr>    PromQL query expression.

Fixes: #8081

@mrueg mrueg changed the title promtool: Add TLS support for query subcmd promtool: Add TLSClientConfig to query subcmd Oct 19, 2020
@roidelapluie
Copy link
Member

roidelapluie commented Oct 19, 2020

Thanks for your pull request.

I think that instead of supporting passwords on the command line, promtool should take a yaml config file as input, with the same TLS client config options as Prometheus.

@mrueg
Copy link
Contributor Author

mrueg commented Oct 19, 2020

Just to make sure that I understand correctly, which passwords are you referring to?
The PR adds three arguments, which are file paths to key-file, cert-file and cacert-file.

The difference between prometheus and promtool is also: prometheus being a service (and thus it makes sense to have a config) while the promtool is meant to be a cli (where I feel a config-only solution might make it more difficult to use).

@roidelapluie
Copy link
Member

roidelapluie commented Oct 19, 2020

Just to make sure that I understand correctly, which passwords are you referring to?
The PR adds three arguments, which are file paths to key-file, cert-file and cacert-file.

The difference between prometheus and promtool is also: prometheus being a service (and thus it makes sense to have a config) while the promtool is meant to be a cli (where I feel a config-only solution might make it more difficult to use). Reusing the same (known) config file fields as Prometheus does not add a lot of complexity I think?

I don't follow you: certificates are already files and difficult to use. I think it does not make sense to only support certificates, and not basic auth, proxy, etc...

But maybe we can have more opinions than mine. @simonpasquier is the actual maintainer of Promtool and shall have the last word on this.

@brian-brazil
Copy link
Contributor

brian-brazil commented Nov 2, 2020

We should also consider amtool, we should try to avoid duplicating security-related code.

@roidelapluie
Copy link
Member

roidelapluie commented Nov 8, 2020

prometheus/alertmanager#2391

@stale stale bot added the stale label Jan 7, 2021
Base automatically changed from master to main February 23, 2021 19:36
@stale stale bot removed the stale label Feb 23, 2021
@stale stale bot added the stale label Apr 24, 2021
@parinapatel
Copy link

parinapatel commented Nov 30, 2021

@mrueg This is really neat feature that would allow to support prometheus that requires mtls. can you take a look at the conflicts when you have few moments ?

@stale stale bot removed the stale label Nov 30, 2021
@mrueg
Copy link
Contributor Author

mrueg commented Nov 30, 2021 •
edited
Loading

@mrueg This is really neat feature that would allow to support prometheus that requires mtls. can you take a look at the conflicts when you have few moments ?

Unfortunately won't have time to work on it or test it in the next months.
I would suggest to reuse the solution provided in prometheus/alertmanager#2391 and open a new PR and close this one.

@mrueg mrueg force-pushed the tls-promtool branch 4 times, most recently from 300ce3e to 12acdba Compare January 29, 2022 22:15
@mrueg
Copy link
Contributor Author

mrueg commented Jan 29, 2022

@parinapatel found some time to rebase on latest main.

@mrueg mrueg force-pushed the tls-promtool branch 4 times, most recently from 1518792 to 043d1b9 Compare January 31, 2022 13:00
@mrueg
promtool: Add TLSClientConfig for query subcmd
233b104 
Signed-off-by: Manuel Rüger <manuel@rueg.eu>
@FUSAKLA FUSAKLA mentioned this pull request Oct 24, 2022
Merged
@roidelapluie
Copy link
Member

roidelapluie commented Sep 5, 2023

We have looked at this pull request during our bug scrub.

promtool now supports --http.config.file= where you can configure the TLS configuration (#11487). This pull request is therefore implemented in another way and we have decided to close it.

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgl dgl Awaiting requested review from dgl dgl is a code owner

@JessicaGreben JessicaGreben Awaiting requested review from JessicaGreben

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add TLSClientConfig to Promtool query subcmds

4 participants

@mrueg @roidelapluie @brian-brazil @parinapatel