From ba1a92f77e39655a2e43fef8e60329072e2e5245 Mon Sep 17 00:00:00 2001
From: Andrew Cholakian <andrew@andrewvc.com>
Date: Thu, 30 Jan 2020 08:05:36 -0600
Subject: [PATCH] [Heartbeat] Fix accumulation/location of redirects (#15944)

1. Fixes: #15941 . Each invoked check using redirects would add to the previous list of
2. The redirects should be in the `http.response.redirects` field, not
the `http.redirects` field  where they reside today. The docs and
mapping both indicate that `http.response.redirects` is the correct
place already.
redirects from the last check, resulting in huge arrays.
---
 heartbeat/monitors/active/http/http_test.go | 42 +++++++++++----------
 heartbeat/monitors/active/http/task.go      | 31 +++++++--------
 2 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/heartbeat/monitors/active/http/http_test.go b/heartbeat/monitors/active/http/http_test.go
index 8d64dbd40257..bcb372361080 100644
--- a/heartbeat/monitors/active/http/http_test.go
+++ b/heartbeat/monitors/active/http/http_test.go
@@ -479,25 +479,29 @@ func TestRedirect(t *testing.T) {
 	sched, _ := schedule.Parse("@every 1s")
 	job := wrappers.WrapCommon(jobs, "test", "", "http", sched, time.Duration(0))[0]
 
-	event := &beat.Event{}
-	_, err = job(event)
-	require.NoError(t, err)
-
-	testslike.Test(
-		t,
-		lookslike.Strict(lookslike.Compose(
-			hbtest.BaseChecks("", "up", "http"),
-			hbtest.SummaryChecks(1, 0),
-			minimalRespondingHTTPChecks(testURL, 200),
-			lookslike.MustCompile(map[string]interface{}{
-				"http.redirects": []string{
-					server.URL + redirectingPaths["/redirect_one"],
-					server.URL + redirectingPaths["/redirect_two"],
-				},
-			}),
-		)),
-		event.Fields,
-	)
+	// Run this test multiple times since in the past we had an issue where the redirects
+	// list was added onto by each request. See https://github.com/elastic/beats/pull/15944
+	for i := 0; i < 10; i++ {
+		event := &beat.Event{}
+		_, err = job(event)
+		require.NoError(t, err)
+
+		testslike.Test(
+			t,
+			lookslike.Strict(lookslike.Compose(
+				hbtest.BaseChecks("", "up", "http"),
+				hbtest.SummaryChecks(1, 0),
+				minimalRespondingHTTPChecks(testURL, 200),
+				lookslike.MustCompile(map[string]interface{}{
+					"http.response.redirects": []string{
+						server.URL + redirectingPaths["/redirect_one"],
+						server.URL + redirectingPaths["/redirect_two"],
+					},
+				}),
+			)),
+			event.Fields,
+		)
+	}
 }
 
 func TestNewRoundTripper(t *testing.T) {
diff --git a/heartbeat/monitors/active/http/task.go b/heartbeat/monitors/active/http/task.go
index 295da85912d8..31fb279fbcdf 100644
--- a/heartbeat/monitors/active/http/task.go
+++ b/heartbeat/monitors/active/http/task.go
@@ -53,14 +53,6 @@ func newHTTPMonitorHostJob(
 	validator multiValidator,
 ) (jobs.Job, error) {
 
-	// Trace visited URLs when redirects occur
-	var redirects []string
-	client := &http.Client{
-		CheckRedirect: makeCheckRedirect(config.MaxRedirects, &redirects),
-		Transport:     transport,
-		Timeout:       config.Timeout,
-	}
-
 	request, err := buildRequest(addr, config, enc)
 	if err != nil {
 		return nil, err
@@ -69,7 +61,17 @@ func newHTTPMonitorHostJob(
 	timeout := config.Timeout
 
 	return jobs.MakeSimpleJob(func(event *beat.Event) error {
-		_, _, err := execPing(event, client, request, body, timeout, validator, config.Response, &redirects)
+		var redirects []string
+		client := &http.Client{
+			// Trace visited URLs when redirects occur
+			CheckRedirect: makeCheckRedirect(config.MaxRedirects, &redirects),
+			Transport:     transport,
+			Timeout:       config.Timeout,
+		}
+		_, _, err := execPing(event, client, request, body, timeout, validator, config.Response)
+		if len(redirects) > 0 {
+			event.PutValue("http.response.redirects", redirects)
+		}
 		return err
 	}), nil
 }
@@ -111,7 +113,6 @@ func createPingFactory(
 ) func(*net.IPAddr) jobs.Job {
 	timeout := config.Timeout
 	isTLS := request.URL.Scheme == "https"
-	checkRedirect := makeCheckRedirect(config.MaxRedirects, nil)
 
 	return monitors.MakePingIPFactory(func(event *beat.Event, ip *net.IPAddr) error {
 		addr := net.JoinHostPort(ip.String(), strconv.Itoa(int(port)))
@@ -137,6 +138,10 @@ func createPingFactory(
 		// It seems they can be invoked still sometime after the request is done
 		cbMutex := sync.Mutex{}
 
+		// We don't support redirects for IP jobs, so this effectively just
+		// prevents following redirects in this case, we know that
+		// config.MaxRedirects must be zero to even be here
+		checkRedirect := makeCheckRedirect(0, nil)
 		client := &http.Client{
 			CheckRedirect: checkRedirect,
 			Timeout:       timeout,
@@ -160,7 +165,7 @@ func createPingFactory(
 			},
 		}
 
-		_, end, err := execPing(event, client, request, body, timeout, validator, config.Response, nil)
+		_, end, err := execPing(event, client, request, body, timeout, validator, config.Response)
 		cbMutex.Lock()
 		defer cbMutex.Unlock()
 
@@ -221,7 +226,6 @@ func execPing(
 	timeout time.Duration,
 	validator multiValidator,
 	responseConfig responseConfig,
-	redirects *[]string,
 ) (start, end time.Time, err reason.Reason) {
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
@@ -248,9 +252,6 @@ func execPing(
 
 	httpFields := common.MapStr{"response": responseFields}
 
-	if redirects != nil && len(*redirects) > 0 {
-		httpFields["redirects"] = redirects
-	}
 	eventext.MergeEventFields(event, common.MapStr{"http": httpFields})
 
 	// Mark the end time as now, since we've finished downloading