provide TLS feature using Prometheus community exporter-toolkit web #231
Conversation
brianehlert
requested review from
lucacome,
pleshakov,
soneillf5 and
tomasohaodha
|
Will it bring both TLS and Basic Auth support? |
|
it will bring everything included in the exporter-toolkit/web |
|
Hi @lucian-vanghele thanks for the PR and sorry for the long delay. Would you mind rebasing your branch (and fixing the conflicts) so I can start testing this? Thanks! |
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Signed-off-by: lucian <lucian.vanghele@ing.com>
Signed-off-by: lucian <lucian.vanghele@ing.com> rebase fix conflicts
Signed-off-by: lucian <lucian.vanghele@ing.com> conflicts
Signed-off-by: lucian <lucian.vanghele@ing.com> conflicts
@lucacome sorry for delay. can you please check? |
There was a problem hiding this comment.
@lucian-vanghele I was checking https://github.com/prometheus/exporter-toolkit and it still says
This repository is currently WIP and experimental.
So I'm not entirely sure if we should use yet...I see that a few exporter started using it...do you know if it's still considered experimental?
In the meantime I left a couple of comments
| var listener net.Listener | ||
| var err error | ||
|
|
||
| if strings.HasPrefix(listenAddress, "unix:") { |
There was a problem hiding this comment.
We want to keep the option to use a unix socket. You can still use this logic in combination with the TLS config
| log.Fatalf("Could not create listener: %v", err) | ||
| } | ||
|
|
||
| if *securedMetrics { |
There was a problem hiding this comment.
related to the other comment we can keep this logic and check if a path to the config was provided and use web.ListenAndServe instead of srv.Serve since the former doesn't support unix sockets.
There was a problem hiding this comment.
does it make sense if we are doing it like it's now done in the last node_exporter version? not long ago they added socket option directly in the exporter-toolkit.
not so easy it seems; this exporter is using flag while exporter-toolkit needs kingpin and migrating to this hits the custom implementations of Value.
can you maybe take a look into this?
There was a problem hiding this comment.
@lucacome can you please take a look at my previous comment? thanks
There was a problem hiding this comment.
@lucian-vanghele sorry for the long wait, I've finally found some time to work on this.
If you want to take a look at #240 🙂
There was a problem hiding this comment.
Just realized I linked the wrong PR, it was #420 😅
|
thanks for the comments. I'll take a look at the other comments. |
|
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Hi @lucian-vanghele now that #420 is merged let me know if you want to update your PR. If not I should have time to take a look at implementing this feature next week. Thanks again anyway for bringing this to my attention in the first place 🙏 |
|
Closing in favor of #461 Thanks again @lucian-vanghele ! |
Proposed changes
Have a unified way to provide tls configuration by using exporter-toolkit from Prometheus community.(https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md). For this exporter is beneficial to rely on a big community. For teams using multiple exporters it helps to do things same way.
Checklist
Before creating a PR, run through this checklist and mark each as complete.