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 3ac29c636c6b85418945a08f49b2d92233390b5b..2b73d86a628da81fc70cf64db8f348acbb0f0060 100644 (file)
--- 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 41182d67c2cb3a0c7dee789058e40667f6a362eb..500c92b560864dd3f41c8ca105b84fa61821c429 100644 (file)
--- 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 08390991429562b2db2cc56c88c56d72a8e8cbf5..ec7106ae6d699673c7cd83b02b5a63cbc2f1432a 100644 (file)
--- 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 bf21dc7954f4d4e927f14e3f03bdf3b651738b87..a395ccce4b3077360a83f43b1322bc1a424f4256 100644 (file)
--- 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{}
                                        }
                                }