Skip to content

Commit

Permalink
[Heartbeat] Fix accumulation/location of redirects
Browse files Browse the repository at this point in the history
This patch fixes two issues:

1. Fixes: elastic#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.
  • Loading branch information
andrewvc committed Jan 29, 2020
1 parent 486d78b commit 0524e27
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 34 deletions.
40 changes: 21 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,27 @@ 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,
)
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
38 changes: 23 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 All @@ -183,6 +188,10 @@ func createPingFactory(
})
}

func makeIPClient() {

}

func buildRequest(addr string, config *Config, enc contentEncoder) (*http.Request, error) {
method := strings.ToUpper(config.Check.Request.Method)
request, err := http.NewRequest(method, addr, nil)
Expand Down Expand Up @@ -221,7 +230,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 +256,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 Expand Up @@ -321,6 +326,9 @@ func makeCheckRedirect(max int, redirects *[]string) func(*http.Request, []*http
}

return func(r *http.Request, via []*http.Request) error {
if via == nil {
redirects = nil
}
if redirects != nil {
*redirects = append(*redirects, r.URL.String())
}
Expand Down

0 comments on commit 0524e27

Please sign in to comment.