From e8efb3cfee71a05b638be387ab06937eef88736b Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 6 Jan 2022 22:46:49 -0600 Subject: [PATCH] [Heartbeat] Separate http req per task (#29697) (#29732) This is an attempt to fix the race reported in #29580 by instantiating a separate http request per HTTP task. The theory being that the HTTP library modifies the headers and that the req object is not safe to share. This has passed manual testing using mode: all against endpoints with multiple A records. Tests are not included here due to the tricky nature of testing here, but we will do so in a follow-up (cherry picked from commit 27f2d00521f9a4bc51723caf9ea8cc0f73d5dac0) Co-authored-by: Andrew Cholakian --- CHANGELOG.next.asciidoc | 2 + heartbeat/monitors/active/http/task.go | 47 +++++++++++++-------- heartbeat/monitors/active/http/task_test.go | 2 +- 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 29af9b1a6c3..27e8cb1ca8a 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -40,6 +40,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d *Heartbeat* +- Fix broken monitors with newer versions of image relying on dup3. {pull}28938[pull] +- Fix race condition in http monitors using `mode:all` that can cause crashes. {pull}29697[pull] *Metricbeat* diff --git a/heartbeat/monitors/active/http/task.go b/heartbeat/monitors/active/http/task.go index f273b0a4cc7..9ee5f3fa1e8 100644 --- a/heartbeat/monitors/active/http/task.go +++ b/heartbeat/monitors/active/http/task.go @@ -45,6 +45,8 @@ import ( "github.com/elastic/beats/v7/libbeat/common/transport/tlscommon" ) +type requestFactory func() (*http.Request, error) + func newHTTPMonitorHostJob( addr string, config *Config, @@ -54,10 +56,7 @@ func newHTTPMonitorHostJob( validator multiValidator, ) (jobs.Job, error) { - request, err := buildRequest(addr, config, enc) - if err != nil { - return nil, err - } + var reqFactory requestFactory = func() (*http.Request, error) { return buildRequest(addr, config, enc) } return jobs.MakeSimpleJob(func(event *beat.Event) error { var redirects []string @@ -67,7 +66,13 @@ func newHTTPMonitorHostJob( Transport: transport, Timeout: config.Transport.Timeout, } - _, _, err := execPing(event, client, request, body, config.Transport.Timeout, validator, config.Response) + + req, err := reqFactory() + if err != nil { + return fmt.Errorf("could not make http request: %w", err) + } + + _, _, err = execPing(event, client, req, body, config.Transport.Timeout, validator, config.Response) if len(redirects) > 0 { event.PutValue("http.response.redirects", redirects) } @@ -84,17 +89,14 @@ func newHTTPMonitorIPsJob( validator multiValidator, ) (jobs.Job, error) { - req, err := buildRequest(addr, config, enc) - if err != nil { - return nil, err - } + var reqFactory requestFactory = func() (*http.Request, error) { return buildRequest(addr, config, enc) } - hostname, port, err := splitHostnamePort(req) + hostname, port, err := splitHostnamePort(addr) if err != nil { return nil, err } - pingFactory := createPingFactory(config, port, tls, req, body, validator) + pingFactory := createPingFactory(config, port, tls, reqFactory, body, validator) job, err := monitors.MakeByHostJob(hostname, config.Mode, monitors.NewStdResolver(), pingFactory) return job, err @@ -104,14 +106,19 @@ func createPingFactory( config *Config, port uint16, tls *tlscommon.TLSConfig, - request *http.Request, + reqFactory requestFactory, body []byte, validator multiValidator, ) func(*net.IPAddr) jobs.Job { timeout := config.Transport.Timeout - isTLS := request.URL.Scheme == "https" return monitors.MakePingIPFactory(func(event *beat.Event, ip *net.IPAddr) error { + req, err := reqFactory() + if err != nil { + return fmt.Errorf("could not create http request: %w", err) + } + isTLS := req.URL.Scheme == "https" + addr := net.JoinHostPort(ip.String(), strconv.Itoa(int(port))) d := &dialchain.DialerChain{ Net: dialchain.MakeConstAddrDialer(addr, dialchain.TCPDialer(timeout)), @@ -163,7 +170,7 @@ func createPingFactory( Transport: httpcommon.HeaderRoundTripper(transport, map[string]string{"User-Agent": userAgent}), } - _, end, err := execPing(event, client, request, body, timeout, validator, config.Response) + _, end, err := execPing(event, client, req, body, timeout, validator, config.Response) cbMutex.Lock() defer cbMutex.Unlock() @@ -313,11 +320,15 @@ func execRequest(client *http.Client, req *http.Request) (start time.Time, resp return start, resp, nil } -func splitHostnamePort(requ *http.Request) (string, uint16, error) { - host := requ.URL.Host +func splitHostnamePort(addr string) (string, uint16, error) { + u, err := url.Parse(addr) + if err != nil { + return "", 0, err + } + host := u.Host // Try to add a default port if needed if strings.LastIndex(host, ":") == -1 { - switch requ.URL.Scheme { + switch u.Scheme { case urlSchemaHTTP: host += ":80" case urlSchemaHTTPS: @@ -330,7 +341,7 @@ func splitHostnamePort(requ *http.Request) (string, uint16, error) { } p, err := strconv.ParseUint(port, 10, 16) if err != nil { - return "", 0, fmt.Errorf("'%v' is no valid port number in '%v'", port, requ.URL.Host) + return "", 0, fmt.Errorf("'%v' is no valid port number in '%v'", port, u.Host) } return host, uint16(p), nil } diff --git a/heartbeat/monitors/active/http/task_test.go b/heartbeat/monitors/active/http/task_test.go index 342dfdec1ad..d6dbe0bd52b 100644 --- a/heartbeat/monitors/active/http/task_test.go +++ b/heartbeat/monitors/active/http/task_test.go @@ -106,7 +106,7 @@ func TestSplitHostnamePort(t *testing.T) { request := &http.Request{ URL: url, } - host, port, err := splitHostnamePort(request) + host, port, err := splitHostnamePort(request.URL.String()) if err != nil { if test.expectedError == nil {