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

Add basic TLS listener support #611

Closed
wants to merge 1 commit into from

Conversation

ag-TJNII
Copy link

@ag-TJNII ag-TJNII commented Jan 29, 2025

This MR adds basic HTTPS listener support. This was implemented using dedicated config arguments, same as listenAddress, and Go's http.ListenAndServeTLS(). prometheus/exporter-toolkit was not used as the API of that project isn't clear from the documentation and per prometheus/exporter-toolkit#197 the TLS support is slated for removal.

Default options are HTTP so this does not change existing behavior or require any config changes by users.

Resolves #413

Signed-off-by: Tom Noonan II <[email protected]>
Copy link
Contributor

@matthiasr matthiasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for kicking this off!

per prometheus/exporter-toolkit#197 the TLS support is slated for removal

No, that issue is not about removal of the feature, only refactoring how it is implemented.

prometheus/exporter-toolkit was not used as the API of that project isn't clear from the documentation

This is true, but nonetheless we should not diverge from it in this project. It's not just about convenience of writing (and maintaining!) less code, it's also a common user interface across the Prometheus project, and it provides a common way to specify many parameters that do matter to many TLS users.

The Graphite exporter main() is a good starting point that illustrates how to set up the exporter-toolkit HTTP listener. The main steps are to let it add the flags, then start the listener. All the generic flags, including handling the details of which package implements the TLS configuration, are handled between these two.

@ag-TJNII
Copy link
Author

ag-TJNII commented Feb 5, 2025

Ack, that's a reasonable rejection. I'm going to close this and will reopen it later with requested changes.

@ag-TJNII ag-TJNII closed this Feb 5, 2025
@ag-TJNII
Copy link
Author

ag-TJNII commented Feb 6, 2025

The level of refactor requested here is beyond what I can provide at this time. The statsd_exporter uses a mux, whereas graphite does not, and statsd has several endpoints which graphite does not: https://github.com/prometheus/statsd_exporter/blob/master/main.go#L513-L531 I will not be filing a followup PR at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TLS for the web UI + metrics
3 participants