From cee8181c4a84ee0e25b65fb67805a7872000aa27 Mon Sep 17 00:00:00 2001 From: "Marcelo E. Magallon" Date: Wed, 3 Feb 2021 14:43:07 -0600 Subject: [PATCH] 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 --- Makefile.common | 2 +- prober/http.go | 19 +++++++------ prober/http_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++++ prober/utils_test.go | 2 +- 4 files changed, 76 insertions(+), 10 deletions(-) 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{} } } -- 2.25.1