Skip to content

Commit

Permalink
[Heartbeat] Fix accumulation/location of redirects (#15944) (#15959)
Browse files Browse the repository at this point in the history
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.

(cherry picked from commit ba1a92f)
  • Loading branch information
andrewvc authored Jan 30, 2020
1 parent 238cc7d commit b1b319d
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 34 deletions.
42 changes: 23 additions & 19 deletions heartbeat/monitors/active/http/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
31 changes: 16 additions & 15 deletions heartbeat/monitors/active/http/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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)))
Expand All @@ -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,
Expand All @@ -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()

Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down

0 comments on commit b1b319d

Please sign in to comment.