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

Use web configuration from exporter-toolkit #461

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Jul 20, 2023

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

@lucacome lucacome requested a review from a team as a code owner July 20, 2023 03:33
@lucacome lucacome self-assigned this Jul 20, 2023
@github-actions 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
@n-rodriguez
Copy link

n-rodriguez commented Jul 20, 2023

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 Outdated Show resolved Hide resolved
@lucacome
Copy link
Member Author

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
Copy link

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

yes, please 👍

@lucacome
Copy link
Member Author

Added SIGTERM to the list.

Copy link

@n-rodriguez n-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM!

@n-rodriguez
Copy link

n-rodriguez commented Jul 21, 2023

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
Copy link

github-actions bot commented Jul 26, 2023

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

PackageVersionLicenseIssue Type
golang.org/x/crypto0.8.0NullUnknown License
golang.org/x/net0.10.0NullUnknown License
golang.org/x/oauth20.8.0NullUnknown License
golang.org/x/sync0.2.0NullUnknown License
golang.org/x/text0.9.0NullUnknown License
gopkg.in/yaml.v22.4.0NullUnknown 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/v22@22.5.0
  • github.com/jpillora/backoff@1.0.0
  • github.com/kr/text@0.2.0
  • github.com/mwitkow/go-conntrack@0.0.0-20190716064945-2f068394615f
  • github.com/prometheus/exporter-toolkit@0.10.0
  • golang.org/x/crypto@0.8.0
  • golang.org/x/net@0.10.0
  • golang.org/x/oauth2@0.8.0
  • golang.org/x/sync@0.2.0
  • golang.org/x/text@0.9.0
  • google.golang.org/appengine@1.6.7
  • gopkg.in/yaml.v2@2.4.0

Replaces manual implementation of TLS and adds Basic Auth.
@lucacome lucacome enabled auto-merge (squash) July 26, 2023 00:38
@lucacome lucacome merged commit f45b6fd into main Jul 26, 2023
10 checks passed
@lucacome lucacome deleted the change/exporter-toolkit branch July 26, 2023 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change dependencies Pull requests that update a dependency file documentation Pull requests/issues for documentation
Projects
Archived in project
4 participants