From d16ede358a2ec049bda28bf37e79c9cfcaa64c29 Mon Sep 17 00:00:00 2001
From: Dave Henderson <dhenderson@gmail.com>
Date: Thu, 17 Sep 2020 23:46:24 -0400
Subject: [PATCH] metrics: Fix hidden panic while observing with bad exemplars
(#3733)
* metrics: Fixing panic while observing with bad exemplars
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
* Minor cleanup
The server is already added to the context. So, we can simply use that
to get the server name, which is a field on the server.
* Add integration test for auto HTTP->HTTPS redirects
A test like this would have caught the problem in the first place
Co-authored-by: Matthew Holt <mholt@users.noreply.github.com>
---
caddytest/integration/autohttps_test.go | 22 +++++++++++
modules/caddyhttp/app.go | 1 +
modules/caddyhttp/metrics.go | 52 +++++++++----------------
modules/caddyhttp/metrics_test.go | 17 +++++++-
modules/caddyhttp/routes.go | 9 ++---
modules/caddyhttp/server.go | 2 +
6 files changed, 62 insertions(+), 41 deletions(-)
create mode 100644 caddytest/integration/autohttps_test.go
diff --git a/caddytest/integration/autohttps_test.go b/caddytest/integration/autohttps_test.go
new file mode 100644
index 000000000..62f172d2b
--- /dev/null
+++ b/caddytest/integration/autohttps_test.go
@@ -0,0 +1,22 @@
+package integration
+
+import (
+ "net/http"
+ "testing"
+
+ "github.com/caddyserver/caddy/v2/caddytest"
+)
+
+func TestAutoHTTPtoHTTPSRedirects(t *testing.T) {
+ tester := caddytest.NewTester(t)
+ tester.InitServer(`
+ {
+ http_port 9080
+ https_port 9443
+ }
+ localhost:9443
+ respond "Yahaha! You found me!"
+ `, "caddyfile")
+
+ tester.AssertRedirect("http://localhost:9080/", "https://localhost/", http.StatusPermanentRedirect)
+}
diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go
index 375ca4d9b..41820ea6c 100644
--- a/modules/caddyhttp/app.go
+++ b/modules/caddyhttp/app.go
@@ -155,6 +155,7 @@ func (app *App) Provision(ctx caddy.Context) error {
// prepare each server
for srvName, srv := range app.Servers {
+ srv.name = srvName
srv.tlsApp = app.tlsApp
srv.logger = app.logger.Named("log")
srv.errorLogger = app.logger.Named("log.error")
diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go
index 2b3b7c755..74be135f1 100644
--- a/modules/caddyhttp/metrics.go
+++ b/modules/caddyhttp/metrics.go
@@ -82,44 +82,38 @@ func initHTTPMetrics() {
}, httpLabels)
}
-type ctxKeyServerName struct{}
-
// serverNameFromContext extracts the current server name from the context.
-// Returns "UNKNOWN" if none is available (should probably never happen?)
+// Returns "UNKNOWN" if none is available (should probably never happen).
func serverNameFromContext(ctx context.Context) string {
- srvName, ok := ctx.Value(ctxKeyServerName{}).(string)
- if !ok {
+ srv, ok := ctx.Value(ServerCtxKey).(*Server)
+ if !ok || srv == nil || srv.name == "" {
return "UNKNOWN"
}
- return srvName
+ return srv.name
}
type metricsInstrumentedHandler struct {
- labels prometheus.Labels
- statusLabels prometheus.Labels
- mh MiddlewareHandler
+ handler string
+ mh MiddlewareHandler
}
-func newMetricsInstrumentedHandler(server, handler string, mh MiddlewareHandler) *metricsInstrumentedHandler {
+func newMetricsInstrumentedHandler(handler string, mh MiddlewareHandler) *metricsInstrumentedHandler {
httpMetrics.init.Do(func() {
initHTTPMetrics()
})
- labels := prometheus.Labels{"server": server, "handler": handler}
- statusLabels := prometheus.Labels{"server": server, "handler": handler, "code": "", "method": ""}
- return &metricsInstrumentedHandler{labels, statusLabels, mh}
+ return &metricsInstrumentedHandler{handler, mh}
}
func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Request, next Handler) error {
- inFlight := httpMetrics.requestInFlight.With(h.labels)
+ server := serverNameFromContext(r.Context())
+ labels := prometheus.Labels{"server": server, "handler": h.handler}
+ statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": r.Method}
+
+ inFlight := httpMetrics.requestInFlight.With(labels)
inFlight.Inc()
defer inFlight.Dec()
- statusLabels := prometheus.Labels{"method": r.Method}
- for k, v := range h.labels {
- statusLabels[k] = v
- }
-
start := time.Now()
// This is a _bit_ of a hack - it depends on the ShouldBufferFunc always
@@ -128,35 +122,25 @@ func (h *metricsInstrumentedHandler) ServeHTTP(w http.ResponseWriter, r *http.Re
writeHeaderRecorder := ShouldBufferFunc(func(status int, header http.Header) bool {
statusLabels["code"] = sanitizeCode(status)
ttfb := time.Since(start).Seconds()
- observeWithExemplar(statusLabels, httpMetrics.responseDuration, ttfb)
+ httpMetrics.responseDuration.With(statusLabels).Observe(ttfb)
return false
})
wrec := NewResponseRecorder(w, nil, writeHeaderRecorder)
err := h.mh.ServeHTTP(wrec, r, next)
dur := time.Since(start).Seconds()
- httpMetrics.requestCount.With(h.labels).Inc()
+ httpMetrics.requestCount.With(labels).Inc()
if err != nil {
- httpMetrics.requestErrors.With(h.labels).Inc()
+ httpMetrics.requestErrors.With(labels).Inc()
return err
}
- observeWithExemplar(statusLabels, httpMetrics.requestDuration, dur)
- observeWithExemplar(statusLabels, httpMetrics.requestSize, float64(computeApproximateRequestSize(r)))
+ httpMetrics.requestDuration.With(statusLabels).Observe(dur)
+ httpMetrics.requestSize.With(statusLabels).Observe(float64(computeApproximateRequestSize(r)))
httpMetrics.responseSize.With(statusLabels).Observe(float64(wrec.Size()))
return nil
}
-func observeWithExemplar(l prometheus.Labels, o *prometheus.HistogramVec, value float64) {
- obs := o.With(l)
- if oe, ok := obs.(prometheus.ExemplarObserver); ok {
- oe.ObserveWithExemplar(value, l)
- return
- }
- // _should_ be a noop, but here just in case...
- obs.Observe(value)
-}
-
func sanitizeCode(code int) string {
if code == 0 {
return "200"
diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go
index 14248a389..6ac591ff6 100644
--- a/modules/caddyhttp/metrics_test.go
+++ b/modules/caddyhttp/metrics_test.go
@@ -1,6 +1,7 @@
package caddyhttp
import (
+ "context"
"errors"
"net/http"
"net/http/httptest"
@@ -9,6 +10,20 @@ import (
"github.com/prometheus/client_golang/prometheus/testutil"
)
+func TestServerNameFromContext(t *testing.T) {
+ ctx := context.Background()
+ expected := "UNKNOWN"
+ if actual := serverNameFromContext(ctx); actual != expected {
+ t.Errorf("Not equal: expected %q, but got %q", expected, actual)
+ }
+
+ in := "foo"
+ ctx = context.WithValue(ctx, ServerCtxKey, &Server{name: in})
+ if actual := serverNameFromContext(ctx); actual != in {
+ t.Errorf("Not equal: expected %q, but got %q", in, actual)
+ }
+}
+
func TestMetricsInstrumentedHandler(t *testing.T) {
handlerErr := errors.New("oh noes")
response := []byte("hello world!")
@@ -26,7 +41,7 @@ func TestMetricsInstrumentedHandler(t *testing.T) {
return h.ServeHTTP(w, r)
})
- ih := newMetricsInstrumentedHandler("foo", "bar", mh)
+ ih := newMetricsInstrumentedHandler("bar", mh)
r := httptest.NewRequest("GET", "/", nil)
w := httptest.NewRecorder()
diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index be23d39cd..83e635406 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -243,12 +243,9 @@ func wrapRoute(route Route) Middleware {
// pointer into its own stack frame to preserve it so it
// won't be overwritten in future loop iterations.
func wrapMiddleware(ctx caddy.Context, mh MiddlewareHandler) Middleware {
- // first, wrap the middleware with metrics instrumentation
- metricsHandler := newMetricsInstrumentedHandler(
- serverNameFromContext(ctx.Context),
- caddy.GetModuleName(mh),
- mh,
- )
+ // wrap the middleware with metrics instrumentation
+ metricsHandler := newMetricsInstrumentedHandler(caddy.GetModuleName(mh), mh)
+
return func(next Handler) Handler {
// copy the next handler (it's an interface, so it's
// just a very lightweight copy of a pointer); this
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index 6851e986d..4f198e5df 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -122,6 +122,8 @@ type Server struct {
// ⚠️ Experimental feature; subject to change or removal.
AllowH2C bool `json:"allow_h2c,omitempty"`
+ name string
+
primaryHandlerChain Handler
errorHandlerChain Handler
listenerWrappers []caddy.ListenerWrapper