Use web configuration from exporter-toolkit #461

lucacome · 2023-07-20T03:33:51Z

Proposed changes

Replaces manual implementation of TLS and Unix socket support with Web Configuration
Adds Basic Auth support.

Thanks @lucian-vanghele for opening #231 and bringing this to my attention.

Closes #232
Closes #393
Closes #425

n-rodriguez · 2023-07-20T17:28:24Z

LGTM! (almost ^^)

SIGINT :

nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ ./nginx-prometheus-exporter
level=info ts=2023-07-20T17:27:44.004Z caller=exporter.go:137 msg="Starting nginx-prometheus-exporter" version="(version=0.11.0, branch=, revision=c722e30e951cd1fae380543e6ac993b73e6a6d63)"
level=info ts=2023-07-20T17:27:44.004Z caller=exporter.go:138 msg="Build context" build_context="(go=go1.20.3, platform=darwin/arm64, user=, date=, tags=unknown)"
level=info ts=2023-07-20T17:27:44.006Z caller=tls_config.go:274 msg="Listening on" address=[::]:9113
level=info ts=2023-07-20T17:27:44.006Z caller=tls_config.go:277 msg="TLS is disabled." http2=false address=[::]:9113
^Clevel=info ts=2023-07-20T17:27:44.830Z caller=exporter.go:259 msg="Shutting down"
level=info ts=2023-07-20T17:27:44.830Z caller=exporter.go:250 msg="HTTP server closed"
nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ echo $?
0

SIGTERM :

nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ ./nginx-prometheus-exporter
level=info ts=2023-07-20T17:30:39.388Z caller=exporter.go:137 msg="Starting nginx-prometheus-exporter" version="(version=0.11.0, branch=, revision=c722e30e951cd1fae380543e6ac993b73e6a6d63)"
level=info ts=2023-07-20T17:30:39.388Z caller=exporter.go:138 msg="Build context" build_context="(go=go1.20.3, platform=darwin/arm64, user=, date=, tags=unknown)"
level=info ts=2023-07-20T17:30:39.390Z caller=tls_config.go:274 msg="Listening on" address=[::]:9113
level=info ts=2023-07-20T17:30:39.390Z caller=tls_config.go:277 msg="TLS is disabled." http2=false address=[::]:9113
Terminated: 15
nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ echo $?
143

There is an issue with SIGTERM : the signal is not caught and thus "Shutting down"is not showing in the logs.

It works with this patch :

diff --git a/exporter.go b/exporter.go
index 6c81376..2de83a5 100644
--- a/exporter.go
+++ b/exporter.go
@@ -11,6 +11,7 @@ import (
        "os"
        "os/signal"
        "strings"
+       "syscall"
        "time"

        plusclient "github.com/nginxinc/nginx-plus-go-client/client"
@@ -237,7 +238,7 @@ func main() {
                http.Handle("/", landingPage)
        }

-       ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, os.Kill)
+       ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
        defer cancel()

        srv := &http.Server{

nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ ./nginx-prometheus-exporter
level=info ts=2023-07-20T17:44:25.176Z caller=exporter.go:138 msg="Starting nginx-prometheus-exporter" version="(version=0.11.0, branch=, revision=c722e30e951cd1fae380543e6ac993b73e6a6d63-modified)"
level=info ts=2023-07-20T17:44:25.176Z caller=exporter.go:139 msg="Build context" build_context="(go=go1.20.3, platform=darwin/arm64, user=, date=, tags=unknown)"
level=info ts=2023-07-20T17:44:25.178Z caller=tls_config.go:274 msg="Listening on" address=[::]:9113
level=info ts=2023-07-20T17:44:25.178Z caller=tls_config.go:277 msg="TLS is disabled." http2=false address=[::]:9113
level=info ts=2023-07-20T17:44:33.863Z caller=exporter.go:260 msg="Shutting down"
level=info ts=2023-07-20T17:44:33.863Z caller=exporter.go:251 msg="HTTP server closed"
nicolas@MacBook-Pro-de-Nicolas:~/go/src/github.com/n-rodriguez/nginx-prometheus-exporter$ echo $?
0

exporter.go

lucacome · 2023-07-20T19:30:57Z

Thanks for reviewing @n-rodriguez

From:

https://github.com/golang/go/blob/master/src/os/exec_posix.go#L16-L24

// The only signal values guaranteed to be present in the os package on all
// systems are os.Interrupt (send the process an interrupt) and os.Kill (force
// the process to exit). On Windows, sending os.Interrupt to a process with
// os.Process.Signal is not implemented; it will return an error instead of
// sending a signal.
var (
        Interrupt Signal = syscall.SIGINT
        Kill      Signal = syscall.SIGKILL
)

so os.Kill and os.Interrupt are syscall.SIGKILL and syscall.SIGINT and are the only two that are guaranteed to be present. But for example k8s sends a SIGTERM to the container, so we should probably add it?

Like this I think:

signal.NotifyContext(context.Background(), os.Interrupt, os.Kill, syscall.SIGTERM)

To be honest I'm not entirely sure I see the value of waiting for a shutdown in the first place, we're not serving traffic or anything like that, since it's just exposing metrics, but I guess it doesn't hurt.

n-rodriguez · 2023-07-20T19:46:11Z

But for example k8s sends a SIGTERM to the container, so we should probably add it?

yes, please 👍

lucacome · 2023-07-20T22:56:47Z

Added SIGTERM to the list.

n-rodriguez

LGTM!

n-rodriguez · 2023-07-21T18:11:02Z

we're not serving traffic or anything like that, since it's just exposing metrics

basically it's the same thing : an http request => in all cases you don't want clients to receive a truncated response from the server. (https://pkg.go.dev/net/http#Server.Shutdown)

github-actions · 2023-07-26T00:32:26Z

Dependency Review

The following issues were found:

✅ 0 vulnerable package(s)
✅ 0 package(s) with incompatible licenses
✅ 0 package(s) with invalid SPDX license definitions
⚠️ 6 package(s) with unknown licenses.

See the Details below.

License Issues

go.mod

Package	Version	License	Issue Type
golang.org/x/crypto	0.8.0	Null	Unknown License
golang.org/x/net	0.10.0	Null	Unknown License
golang.org/x/oauth2	0.8.0	Null	Unknown License
golang.org/x/sync	0.2.0	Null	Unknown License
golang.org/x/text	0.9.0	Null	Unknown License
gopkg.in/yaml.v2	2.4.0	Null	Unknown License

Allowed Licenses: Apache-1.1, Apache-2.0, BSD-2-Clause, BSD-3-Clause, BSL-1.0, ISC, MIT, NCSA, OpenSSL, Python-2.0, X11

Scanned Manifest Files

go.mod

github.com/coreos/go-systemd/[email protected]
github.com/jpillora/[email protected]
github.com/kr/[email protected]
github.com/mwitkow/[email protected]
github.com/prometheus/[email protected]
golang.org/x/[email protected]
golang.org/x/[email protected]
golang.org/x/[email protected]
golang.org/x/[email protected]
golang.org/x/[email protected]
google.golang.org/[email protected]
gopkg.in/[email protected]

Replaces manual implementation of TLS and adds Basic Auth.

lucacome requested a review from a team as a code owner July 20, 2023 03:33

lucacome self-assigned this Jul 20, 2023

github-actions bot added change Pull requests that introduce a change dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation labels Jul 20, 2023

This was referenced Jul 20, 2023

provide TLS feature using Prometheus community exporter-toolkit web #231

Closed

Fix wrong exit status (#393) #403

Closed

lucacome force-pushed the change/exporter-toolkit branch from b7531cb to 22a1ce9 Compare July 20, 2023 03:41

jjngx approved these changes Jul 20, 2023

View reviewed changes

vepatel requested review from haywoodsh and shaun-nx July 20, 2023 08:59

n-rodriguez suggested changes Jul 20, 2023

View reviewed changes

exporter.go Outdated Show resolved Hide resolved

lucacome requested a review from n-rodriguez July 20, 2023 22:56

n-rodriguez approved these changes Jul 21, 2023

View reviewed changes

vepatel approved these changes Jul 25, 2023

View reviewed changes

Use web configuration from exporter-toolkit

72ef862

Replaces manual implementation of TLS and adds Basic Auth.

lucacome force-pushed the change/exporter-toolkit branch from a5b34db to 72ef862 Compare July 26, 2023 00:36

lucacome enabled auto-merge (squash) July 26, 2023 00:38

lucacome merged commit f45b6fd into main Jul 26, 2023

lucacome deleted the change/exporter-toolkit branch July 26, 2023 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use web configuration from exporter-toolkit #461

Use web configuration from exporter-toolkit #461

lucacome commented Jul 20, 2023 •

edited

Loading

n-rodriguez commented Jul 20, 2023 •

edited

Loading

lucacome commented Jul 20, 2023

n-rodriguez commented Jul 20, 2023

lucacome commented Jul 20, 2023

n-rodriguez left a comment

n-rodriguez commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading

Use web configuration from exporter-toolkit #461

Use web configuration from exporter-toolkit #461

Conversation

lucacome commented Jul 20, 2023 • edited Loading

Proposed changes

n-rodriguez commented Jul 20, 2023 • edited Loading

lucacome commented Jul 20, 2023

n-rodriguez commented Jul 20, 2023

lucacome commented Jul 20, 2023

n-rodriguez left a comment

Choose a reason for hiding this comment

n-rodriguez commented Jul 21, 2023 • edited Loading

github-actions bot commented Jul 26, 2023 • edited Loading

Dependency Review

License Issues

go.mod

Scanned Manifest Files

lucacome commented Jul 20, 2023 •

edited

Loading

n-rodriguez commented Jul 20, 2023 •

edited

Loading

n-rodriguez commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 26, 2023 •

edited

Loading