From: Marcelo E. Magallon <marcelo.magallon@grafana.com>
Date: Wed, 3 Feb 2021 20:43:07 +0000 (-0600)
Subject: makefile: update Makefile.common with newer version
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=cee8181c4a84ee0e25b65fb67805a7872000aa27;p=blackbox_exporter.git

makefile: update Makefile.common with newer version

This updates golangci-lint from version 1.18.0 to 1.36.0. Newer versions
flag a possible nil pointer dereference, so rework some of the logic to
make sure that's not the case.

Add a test that covers the case of the changed code.

Closes #738

Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
---

diff --git a/Makefile.common b/Makefile.common
index 3ac29c6..2b73d86 100644
--- a/Makefile.common
+++ b/Makefile.common
@@ -83,7 +83,7 @@ PROMU_URL     := https://github.com/prometheus/promu/releases/download/v$(PROMU_
 
 GOLANGCI_LINT :=
 GOLANGCI_LINT_OPTS ?=
-GOLANGCI_LINT_VERSION ?= v1.18.0
+GOLANGCI_LINT_VERSION ?= v1.36.0
 # golangci-lint only supports linux, darwin and windows platforms on i386/amd64.
 # windows isn't included here because of the path separator being different.
 ifeq ($(GOHOSTOS),$(filter $(GOHOSTOS),linux darwin))
diff --git a/prober/http.go b/prober/http.go
index 41182d6..500c92b 100644
--- a/prober/http.go
+++ b/prober/http.go
@@ -415,9 +415,16 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
 	request = request.WithContext(httptrace.WithClientTrace(request.Context(), trace))
 
 	resp, err := client.Do(request)
-	// Err won't be nil if redirects were turned off. See https://github.com/golang/go/issues/3795
-	if err != nil && resp == nil {
-		level.Error(logger).Log("msg", "Error for HTTP request", "err", err)
+	// This is different from the usual err != nil you'd expect here because err won't be nil if redirects were
+	// turned off. See https://github.com/golang/go/issues/3795
+	//
+	// If err == nil there should never be a case where resp is also nil, but better be safe than sorry, so check if
+	// resp == nil first, and then check if there was an error.
+	if resp == nil {
+		resp = &http.Response{}
+		if err != nil {
+			level.Error(logger).Log("msg", "Error for HTTP request", "err", err)
+		}
 	} else {
 		requestErrored := (err != nil)
 
@@ -459,7 +466,7 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
 			}
 		}
 
-		if resp != nil && !requestErrored {
+		if !requestErrored {
 			_, err = io.Copy(ioutil.Discard, byteCounter)
 			if err != nil {
 				level.Info(logger).Log("msg", "Failed to read HTTP response body", "err", err)
@@ -504,12 +511,8 @@ func ProbeHTTP(ctx context.Context, target string, module config.Module, registr
 				success = false
 			}
 		}
-
 	}
 
-	if resp == nil {
-		resp = &http.Response{}
-	}
 	tt.mu.Lock()
 	defer tt.mu.Unlock()
 	for i, trace := range tt.traces {
diff --git a/prober/http_test.go b/prober/http_test.go
index 0839099..ec7106a 100644
--- a/prober/http_test.go
+++ b/prober/http_test.go
@@ -24,6 +24,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -241,6 +242,68 @@ func TestRedirectNotFollowed(t *testing.T) {
 
 }
 
+// TestRedirectionLimit verifies that the probe stops following
+// redirects after some limit
+func TestRedirectionLimit(t *testing.T) {
+	const redirectLimit = 11
+
+	tooManyRedirects := false
+
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.URL.Path == fmt.Sprintf("/redirect-%d", redirectLimit+1):
+			// the client should never hit this path
+			// because they should stop at the previous one.
+			w.WriteHeader(http.StatusTooManyRequests)
+			tooManyRedirects = true
+			return
+
+		case strings.HasPrefix(r.URL.Path, "/redirect-"):
+			n, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/redirect-"))
+			if err != nil {
+				w.WriteHeader(http.StatusInternalServerError)
+				fmt.Fprintf(w, "failed to extract redirect number from %s", r.URL.Path)
+				return
+			}
+			http.Redirect(w, r, fmt.Sprintf("/redirect-%d", n+1), http.StatusFound)
+
+		default:
+			http.Redirect(w, r, "/redirect-1", http.StatusFound)
+		}
+	}))
+	defer ts.Close()
+
+	// Follow redirect, should eventually fail with 302
+	registry := prometheus.NewRegistry()
+	testCTX, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	result := ProbeHTTP(
+		testCTX,
+		ts.URL,
+		config.Module{Timeout: time.Second, HTTP: config.HTTPProbe{IPProtocolFallback: true}},
+		registry,
+		log.NewNopLogger())
+	if result {
+		t.Fatalf("Probe suceeded unexpectedly")
+	}
+
+	if tooManyRedirects {
+		t.Fatalf("Probe followed too many redirects")
+	}
+
+	mfs, err := registry.Gather()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expectedResults := map[string]float64{
+		"probe_http_redirects":   redirectLimit,    // should stop here
+		"probe_http_status_code": http.StatusFound, // final code should be Found
+	}
+	checkRegistryResults(expectedResults, mfs, t)
+}
+
 func TestPost(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != "POST" {
diff --git a/prober/utils_test.go b/prober/utils_test.go
index bf21dc7..a395ccc 100644
--- a/prober/utils_test.go
+++ b/prober/utils_test.go
@@ -209,7 +209,7 @@ func checkMetrics(expected map[string]map[string]map[string]struct{}, mfs []*dto
 				var lv labelValidation
 				if values != nil {
 					lv.values = map[string]valueValidation{}
-					for vname, _ := range values {
+					for vname := range values {
 						lv.values[vname] = valueValidation{}
 					}
 				}