Repository: prometheus/blackbox_exporter
Pull request #515 (2019-09-04 16:57:45)

Adding web.external-url and web.route-prefix flags
Hi,
First all, let's make it clear that this is my first contribution within the Prometheus ecosystem, but hopefully not the last one.

This PR aims to fix https://github.com/prometheus/blackbox_exporter/issues/475 making several assumptions and having different feelings on the way.

- Keep consistency with `alertmanager` and `prometheus` which seems to implement the same flags, I essentially copied the same logic adapting for the specifics of this component.
- I'm not forcing absolute url when `--web.external-url` is set, since all links are anyway relative reusing the original domain, I guess in other components more complex such as prometheus or alertmanager this might be necessary. I'm happy to change the behavior if forcing absolute path is what we want.

Since this component is so simple I've coded this PR using the naive approach, but I feel it probably makes sense to move the following [function](https://github.com/prometheus/blackbox_exporter/compare/master...snebel29:feat/add-support-for-web-route-prefix-external-url?expand=1#diff-7ddfb3e035b42cd70649cc33393fe32cR397-R429) with its [test case](https://github.com/prometheus/blackbox_exporter/compare/master...snebel29:feat/add-support-for-web-route-prefix-external-url?expand=1#diff-53618cae32e23ae47e1c35f10da92654R144-R218) to https://github.com/prometheus/common since the same function is used in the alertmanager as well (In fact It was literally copied from there)

May be creating something like https://github.com/prometheus/common/tree/master/promlog even trying to get the flags packed there, then implement in both `blackbox_exporter` and `alertmanger` and take a look to see if those flags are implemented anywhere else, but I would appreciate feedback on this point before doing anything.





snebel29: Sorry for bothering again, but let's see if I understood correctly

> Let's go with consistency.

The only places were we could be using the absolute url value coming from `web.external-url` are

- ServeMux pattern of `Handle` and `HandleFunc` functions: But this would be inconsistent with other projects from the ecosystem, and kind of weird
- The html code returned by `/` path pattern matching: We already agreed on using relative path there 

It's therefore my understanding that we are already consistent and ok on this point.

> Probably best to do that with the TLS stuff when it's moved over to common. It's also in the pushgateway.

I'm not sure what you mean by `TLS` in the context of `alertmanager` and `pushgateway`, I can't see any obvious TLS stuff in their main files to move to common on them.

In any case my understanding is that you would want to address moving `extURL` together with "the TLS stuff" in a different PR.

Is this PR then ok to be merged?

brian-brazil: > The only places were we could be using the absolute url value coming from web.external-url are

I meant consistent in the checks it performs. Having the semantics different across different servers would be confusing.

> I'm not sure what you mean by TLS in the context of alertmanager and pushgateway

It's in progress in the node exporter currently, and it'd make sense for all this code to be together at some future point.

snebel29: > I meant consistent in the checks it performs. Having the semantics different across different servers would be confusing.

I agree, I took the logic from the `alertmanager`, therefore the same parameters across servers of the ecosystem should produce the same result
snebel29: I will double-check `prometheus`, `pushgateway` and `alertmanager` behaviour for those two flags, to ensure the semantics are exactly the same (as it was intended).

Here is a recently implemented example https://github.com/prometheus/pushgateway/issues/190 which code I could reuse I guess, allowing to be moved on to common later on.
brian-brazil: The logic in https://github.com/prometheus/prometheus/blob/d98d4a9bf04217f5b36fd3248cbe88d3046e84e4/cmd/prometheus/main.go#L791 is a bit different
snebel29: @brian-brazil I reviewed prometheus behavior for `--web.external-url` and `--web.route-prefix` and I think I've matched it accordingly, note that alertmanager from where I originally "took" the logic is slightly different, see [here](https://github.com/prometheus/alertmanager/blob/d5f6f8892824a485663301b9a542c6b6dff91c27/cmd/alertmanager/main.go#L521-L553) so may be as an intermediary step towards leveraging common would be good to migrate the function in the alertmanager as well, at first glance the function signature is different and this [check](https://github.com/prometheus/alertmanager/blob/d5f6f8892824a485663301b9a542c6b6dff91c27/cmd/alertmanager/main.go#L542-L544) is not performed in prometheus.

From what I can see, only one behaviour will remain different between prometheus <> blackbox_exporter after merging this PR, currently blackbox_exporter matches any non registered path to the landing page, see [here](https://github.com/prometheus/blackbox_exporter/blob/ce8e9952a074768995cb67de0bc31bb43c1ec7c1/main.go#L265), while prometheus return 404s when a path wasn't explicitly registered, but this the case already before this PR.

This is hard to match due to using [ServeMux](https://golang.org/pkg/net/http/#ServeMux) in blackbox_exporter vs `github.com/prometheus/common/route` by leveraging [julienschmidt/httprouter](https://github.com/julienschmidt/httprouter) which matches exact patterns only.

I guess in the future, migrating over `github.com/prometheus/common/route` would make things easier if the intention is to behave 100% the same over time, also easier to maintain.

I hope this makes sense now, but please let me know otherwise.

Cheers


Patch:
diff --git a/go.mod b/go.mod
index bfae6ec..b60028b 100644
--- a/go.mod
+++ b/go.mod
@@ -6,7 +6,7 @@ require (
 	github.com/go-kit/kit v0.8.0
 	github.com/kr/pretty v0.1.0 // indirect
 	github.com/miekg/dns v1.1.14
-	github.com/pkg/errors v0.8.1 // indirect
+	github.com/pkg/errors v0.8.1
 	github.com/prometheus/client_golang v1.0.0
 	github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90
 	github.com/prometheus/common v0.6.0
diff --git a/history.go b/history.go
index 52e02c0..95b686d 100644
--- a/history.go
+++ b/history.go
@@ -25,11 +25,15 @@ type result struct {
 	success     bool
 }
 
+// resultHistory contains two history slices: `results` contains most recent `maxResults` results.
+// After they expire out of `results`, failures will be saved in `preservedFailedResults`. This
+// ensures that we are always able to see debug information about recent failures.
 type resultHistory struct {
-	mu         sync.Mutex
-	nextId     int64
-	results    []*result
-	maxResults uint
+	mu                     sync.Mutex
+	nextId                 int64
+	results                []*result
+	preservedFailedResults []*result
+	maxResults             uint
 }
 
 // Add a result to the history.
@@ -48,6 +52,16 @@ func (rh *resultHistory) Add(moduleName, target, debugOutput string, success boo
 
 	rh.results = append(rh.results, r)
 	if uint(len(rh.results)) > rh.maxResults {
+		// If we are about to remove a failure, add it to the failed result history, then
+		// remove the oldest failed result, if needed.
+		if !rh.results[0].success {
+			rh.preservedFailedResults = append(rh.preservedFailedResults, rh.results[0])
+			if uint(len(rh.preservedFailedResults)) > rh.maxResults {
+				preservedFailedResults := make([]*result, len(rh.preservedFailedResults)-1)
+				copy(preservedFailedResults, rh.preservedFailedResults[1:])
+				rh.preservedFailedResults = preservedFailedResults
+			}
+		}
 		results := make([]*result, len(rh.results)-1)
 		copy(results, rh.results[1:])
 		rh.results = results
@@ -59,7 +73,8 @@ func (rh *resultHistory) List() []*result {
 	rh.mu.Lock()
 	defer rh.mu.Unlock()
 
-	return rh.results[:]
+	// Results in each slice are disjoint. We can simply concatenate the results.
+	return append(rh.preservedFailedResults[:], rh.results...)
 }
 
 // Get returns a given result.
@@ -67,6 +82,11 @@ func (rh *resultHistory) Get(id int64) *result {
 	rh.mu.Lock()
 	defer rh.mu.Unlock()
 
+	for _, r := range rh.preservedFailedResults {
+		if r.id == id {
+			return r
+		}
+	}
 	for _, r := range rh.results {
 		if r.id == id {
 			return r
diff --git a/main.go b/main.go
index e4f9b75..3d95971 100644
--- a/main.go
+++ b/main.go
@@ -18,16 +18,21 @@ import (
 	"context"
 	"fmt"
 	"html"
+	"net"
 	"net/http"
 	_ "net/http/pprof"
+	"net/url"
 	"os"
 	"os/signal"
+	"path"
 	"strconv"
+	"strings"
 	"syscall"
 	"time"
 
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/log/level"
+	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"github.com/prometheus/common/expfmt"
@@ -51,6 +56,8 @@ var (
 	timeoutOffset = kingpin.Flag("timeout-offset", "Offset to subtract from timeout in seconds.").Default("0.5").Float64()
 	configCheck   = kingpin.Flag("config.check", "If true validate the config file and then exit.").Default().Bool()
 	historyLimit  = kingpin.Flag("history.limit", "The maximum amount of items to keep in the history.").Default("100").Uint()
+	externalURL   = kingpin.Flag("web.external-url", "The URL under which Blackbox exporter is externally reachable (for example, if Blackbox exporter is served via a reverse proxy). Used for generating relative and absolute links back to Blackbox exporter itself. If the URL has a path portion, it will be used to prefix all HTTP endpoints served by Blackbox exporter. If omitted, relevant URL components will be derived automatically.").PlaceHolder("<url>").String()
+	routePrefix   = kingpin.Flag("web.route-prefix", "Prefix for the internal routes of web endpoints. Defaults to path of --web.external-url.").PlaceHolder("<path>").String()
 
 	Probers = map[string]prober.ProbeFn{
 		"http": prober.ProbeHTTP,
@@ -217,6 +224,28 @@ func run() int {
 
 	level.Info(logger).Log("msg", "Loaded config file")
 
+	// Infer or set Blackbox exporter externalURL
+	beURL, err := computeExternalURL(*externalURL, *listenAddress)
+	if err != nil {
+		level.Error(logger).Log("msg", "failed to determine external URL", "err", err)
+		return 1
+	}
+	level.Debug(logger).Log("externalURL", beURL.String())
+
+	// Default -web.route-prefix to path of -web.external-url.
+	if *routePrefix == "" {
+		*routePrefix = beURL.Path
+	}
+
+	// routePrefix must always be at least '/'.
+	*routePrefix = "/" + strings.Trim(*routePrefix, "/")
+	// routePrefix requires path to have trailing "/" in order
+	// for browsers to interpret the path-relative path correctly, instead of stripping it.
+	if *routePrefix != "/" {
+		*routePrefix = *routePrefix + "/"
+	}
+	level.Debug(logger).Log("routePrefix", *routePrefix)
+
 	hup := make(chan os.Signal, 1)
 	reloadCh := make(chan chan error)
 	signal.Notify(hup, syscall.SIGHUP)
@@ -241,7 +270,19 @@ func run() int {
 		}
 	}()
 
-	http.HandleFunc("/-/reload",
+	// Match Prometheus behaviour and redirect over externalURL for root path only
+	// if routePrefix is different than "/"
+	if *routePrefix != "/" {
+		http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path != "/" {
+				http.NotFound(w, r)
+				return
+			}
+			http.Redirect(w, r, beURL.String(), http.StatusFound)
+		})
+	}
+
+	http.HandleFunc(path.Join(*routePrefix, "/-/reload"),
 		func(w http.ResponseWriter, r *http.Request) {
 			if r.Method != "POST" {
 				w.WriteHeader(http.StatusMethodNotAllowed)
@@ -255,23 +296,23 @@ func run() int {
 				http.Error(w, fmt.Sprintf("failed to reload config: %s", err), http.StatusInternalServerError)
 			}
 		})
-	http.Handle("/metrics", promhttp.Handler())
-	http.HandleFunc("/probe", func(w http.ResponseWriter, r *http.Request) {
+	http.Handle(path.Join(*routePrefix, "/metrics"), promhttp.Handler())
+	http.HandleFunc(path.Join(*routePrefix, "/probe"), func(w http.ResponseWriter, r *http.Request) {
 		sc.Lock()
 		conf := sc.C
 		sc.Unlock()
 		probeHandler(w, r, conf, logger, rh)
 	})
-	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+	http.HandleFunc(*routePrefix, func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "text/html")
 		w.Write([]byte(`<html>
     <head><title>Blackbox Exporter</title></head>
     <body>
     <h1>Blackbox Exporter</h1>
-    <p><a href="/probe?target=prometheus.io&module=http_2xx">Probe prometheus.io for http_2xx</a></p>
-    <p><a href="/probe?target=prometheus.io&module=http_2xx&debug=true">Debug probe prometheus.io for http_2xx</a></p>
-    <p><a href="/metrics">Metrics</a></p>
-    <p><a href="/config">Configuration</a></p>
+    <p><a href="probe?target=prometheus.io&module=http_2xx">Probe prometheus.io for http_2xx</a></p>
+    <p><a href="probe?target=prometheus.io&module=http_2xx&debug=true">Debug probe prometheus.io for http_2xx</a></p>
+    <p><a href="metrics">Metrics</a></p>
+    <p><a href="config">Configuration</a></p>
     <h2>Recent Probes</h2>
     <table border='1'><tr><th>Module</th><th>Target</th><th>Result</th><th>Debug</th>`))
 
@@ -283,7 +324,7 @@ func run() int {
 			if !r.success {
 				success = "<strong>Failure</strong>"
 			}
-			fmt.Fprintf(w, "<tr><td>%s</td><td>%s</td><td>%s</td><td><a href='logs?id=%d'>Logs</a></td></td>",
+			fmt.Fprintf(w, "<tr><td>%s</td><td>%s</td><td>%s</td><td><a href='"+path.Join(*routePrefix, "/logs")+"?id=%d'>Logs</a></td></td>",
 				html.EscapeString(r.moduleName), html.EscapeString(r.target), success, r.id)
 		}
 
@@ -291,7 +332,7 @@ func run() int {
     </html>`))
 	})
 
-	http.HandleFunc("/logs", func(w http.ResponseWriter, r *http.Request) {
+	http.HandleFunc(path.Join(*routePrefix, "/logs"), func(w http.ResponseWriter, r *http.Request) {
 		id, err := strconv.ParseInt(r.URL.Query().Get("id"), 10, 64)
 		if err != nil {
 			http.Error(w, "Invalid probe id", 500)
@@ -306,7 +347,7 @@ func run() int {
 		w.Write([]byte(result.debugOutput))
 	})
 
-	http.HandleFunc("/config", func(w http.ResponseWriter, r *http.Request) {
+	http.HandleFunc(path.Join(*routePrefix, "/config"), func(w http.ResponseWriter, r *http.Request) {
 		sc.RLock()
 		c, err := yaml.Marshal(sc.C)
 		sc.RUnlock()
@@ -366,3 +407,41 @@ func getTimeout(r *http.Request, module config.Module, offset float64) (timeoutS
 
 	return timeoutSeconds, nil
 }
+
+func startsOrEndsWithQuote(s string) bool {
+	return strings.HasPrefix(s, "\"") || strings.HasPrefix(s, "'") ||
+		strings.HasSuffix(s, "\"") || strings.HasSuffix(s, "'")
+}
+
+// computeExternalURL computes a sanitized external URL from a raw input. It infers unset
+// URL parts from the OS and the given listen address.
+func computeExternalURL(u, listenAddr string) (*url.URL, error) {
+	if u == "" {
+		hostname, err := os.Hostname()
+		if err != nil {
+			return nil, err
+		}
+		_, port, err := net.SplitHostPort(listenAddr)
+		if err != nil {
+			return nil, err
+		}
+		u = fmt.Sprintf("http://%s:%s/", hostname, port)
+	}
+
+	if startsOrEndsWithQuote(u) {
+		return nil, errors.New("URL must not begin or end with quotes")
+	}
+
+	eu, err := url.Parse(u)
+	if err != nil {
+		return nil, err
+	}
+
+	ppref := strings.TrimRight(eu.Path, "/")
+	if ppref != "" && !strings.HasPrefix(ppref, "/") {
+		ppref = "/" + ppref
+	}
+	eu.Path = ppref
+
+	return eu, nil
+}


Test patch:
diff --git a/history_test.go b/history_test.go
new file mode 100644
index 0000000..3fb3c49
--- /dev/null
+++ b/history_test.go
@@ -0,0 +1,81 @@
+// Copyright 2017 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestHistoryKeepsLatestResults(t *testing.T) {
+	history := &resultHistory{maxResults: 3}
+	for i := 0; i < 4; i++ {
+		history.Add("module", "target", fmt.Sprintf("result %d", i), true)
+	}
+
+	savedResults := history.List()
+	for i := 0; i < len(savedResults); i++ {
+		if savedResults[i].debugOutput != fmt.Sprintf("result %d", i+1) {
+			t.Errorf("History contained the wrong result at index %d", i)
+		}
+	}
+}
+
+func FillHistoryWithMaxSuccesses(h *resultHistory) {
+	for i := uint(0); i < h.maxResults; i++ {
+		h.Add("module", "target", fmt.Sprintf("result %d", h.nextId), true)
+	}
+}
+
+func FillHistoryWithMaxPreservedFailures(h *resultHistory) {
+	for i := uint(0); i < h.maxResults; i++ {
+		h.Add("module", "target", fmt.Sprintf("result %d", h.nextId), false)
+	}
+}
+
+func TestHistoryPreservesExpiredFailedResults(t *testing.T) {
+	history := &resultHistory{maxResults: 3}
+
+	// Success are expired, no failues are expired
+	FillHistoryWithMaxSuccesses(history)
+	FillHistoryWithMaxPreservedFailures(history)
+	savedResults := history.List()
+	for i := uint(0); i < uint(len(savedResults)); i++ {
+		expectedDebugOutput := fmt.Sprintf("result %d", i+history.maxResults)
+		if savedResults[i].debugOutput != expectedDebugOutput {
+			t.Errorf("History contained the wrong result at index %d. Expected: %s, Actual: %s", i, expectedDebugOutput, savedResults[i].debugOutput)
+		}
+	}
+
+	// Failures are expired, should all be preserved
+	FillHistoryWithMaxPreservedFailures(history)
+	savedResults = history.List()
+	for i := uint(0); i < uint(len(savedResults)); i++ {
+		expectedDebugOutput := fmt.Sprintf("result %d", i+history.maxResults)
+		if savedResults[i].debugOutput != expectedDebugOutput {
+			t.Errorf("History contained the wrong result at index %d. Expected: %s, Actual: %s", i, expectedDebugOutput, savedResults[i].debugOutput)
+		}
+	}
+
+	// New expired failures are preserved, new success are not expired
+	FillHistoryWithMaxPreservedFailures(history)
+	FillHistoryWithMaxSuccesses(history)
+	savedResults = history.List()
+	for i := uint(0); i < uint(len(savedResults)); i++ {
+		expectedDebugOutput := fmt.Sprintf("result %d", i+history.maxResults*3)
+		if savedResults[i].debugOutput != expectedDebugOutput {
+			t.Errorf("History contained the wrong result at index %d. Expected: %s, Actual: %s", i, expectedDebugOutput, savedResults[i].debugOutput)
+		}
+	}
+}
diff --git a/main_test.go b/main_test.go
index c2341ec..f9d536b 100644
--- a/main_test.go
+++ b/main_test.go
@@ -137,3 +137,56 @@ func TestTimeoutIsSetCorrectly(t *testing.T) {
 		}
 	}
 }
+
+func TestComputeExternalURL(t *testing.T) {
+	tests := []struct {
+		input string
+		valid bool
+	}{
+		{
+			input: "",
+			valid: true,
+		},
+		{
+			input: "http://proxy.com/prometheus",
+			valid: true,
+		},
+		{
+			input: "'https://url/prometheus'",
+			valid: false,
+		},
+		{
+			input: "'relative/path/with/quotes'",
+			valid: false,
+		},
+		{
+			input: "http://alertmanager.company.com",
+			valid: true,
+		},
+		{
+			input: "https://double--dash.de",
+			valid: true,
+		},
+		{
+			input: "'http://starts/with/quote",
+			valid: false,
+		},
+		{
+			input: "ends/with/quote\"",
+			valid: false,
+		},
+	}
+
+	for _, test := range tests {
+		_, err := computeExternalURL(test.input, "0.0.0.0:9090")
+		if test.valid {
+			if err != nil {
+				t.Errorf("unexpected error %v", err)
+			}
+		} else {
+			if err == nil {
+				t.Errorf("expected error computing %s got none", test.input)
+			}
+		}
+	}
+}