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 HTTPS configuration #693

Merged
merged 4 commits into from
Feb 19, 2021
Merged

Add HTTPS configuration #693

merged 4 commits into from
Feb 19, 2021

Conversation

SuperQ
Copy link
Contributor

@SuperQ SuperQ commented Jan 3, 2021

Add the exporter-toolkit https package to allow configuring TLS and
auth.

Signed-off-by: Ben Kochie [email protected]

@SuperQ SuperQ requested a review from a team as a code owner January 3, 2021 14:56
@SuperQ
Copy link
Contributor Author

SuperQ commented Jan 3, 2021

Simplified version of #680

@carlpett
Copy link
Collaborator

I wonder why the promu logs don't seem to output the actual errors in builds on AppVeyor... Anyway, the build fails because of an undefined variable (logger), which I guess was left as a TODO to integrate?

@replay replay mentioned this pull request Jan 28, 2021
@replay
Copy link

replay commented Jan 28, 2021

the build fails because of an undefined variable (logger), which I guess was left as a TODO to integrate?

I think the issue is that https.Listen() requires a go-kit logger, but the windows exporter currently uses logrus (via github.com/prometheus/common/log).

So we'd have to either replace the logger used by the windows_exporter and make it use go-kit everywhere, or we just instantiate a go-kit logger to pass into https.Listen() which means we'll end up with mixed loggers.

SuperQ and others added 4 commits January 30, 2021 11:53
Add the exporter-toolkit https package to allow configuring TLS and
auth.

Signed-off-by: Ben Kochie <[email protected]>
Signed-off-by: Calle Pettersson <[email protected]>
@carlpett
Copy link
Collaborator

So, PR is updated with working logging. I also took this opportunity to copy common/log into a new package log in the exporter, as discussed previously @breed808.

@replay
Copy link

replay commented Feb 8, 2021

So, PR is updated with working logging. I also took this opportunity to copy common/log into a new package log in the exporter, as discussed previously @breed808.

Thanks a lot @carlpett for implementing that 👍.
I'd be happy to review it, unfortunately my approval won't be sufficient to merge this PR as far as I understand.

@hartman17 hartman17 mentioned this pull request Feb 9, 2021
@breed808
Copy link
Contributor

I've tested this with a self-signed CA and certificate, and it's looking good 👍
Do we want additional testing or review on this change?

@carlpett
Copy link
Collaborator

Whoops, missed the response! Yeah, my testing looked good as well, so I think we can get it out there for some feedback. It's marked as experimental, after all.
I can't approve PRs I've contributed to though, so if you think it looks good I'll need a thumbs up there 🙂

@hartman17
Copy link

We are willing to test this on a larger scale once it's merged and a release is cut. Providing any feedback/issues we encounter.

Copy link
Contributor

@breed808 breed808 left a comment

Choose a reason for hiding this comment

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

I can't approve PRs I've contributed to

I wasn't aware of this limitation, but it makes sense. PR looks good to me 👍

@breed808 breed808 merged commit 5af2a78 into prometheus-community:master Feb 19, 2021
anubhavg-icpl pushed a commit to anubhavg-icpl/windows_exporter that referenced this pull request Sep 22, 2024
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.

5 participants