From 81ec33c560dccef3cd1f2983d6641ee701491dd8 Mon Sep 17 00:00:00 2001 From: Daniel Nelson Date: Tue, 30 Jun 2020 14:19:54 -0700 Subject: [PATCH] Fix data race in phpfpm initializing http client (#7738) --- plugins/inputs/phpfpm/phpfpm.go | 55 ++++++++++++++-------------- plugins/inputs/phpfpm/phpfpm_test.go | 32 ++++++++++++++-- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/plugins/inputs/phpfpm/phpfpm.go b/plugins/inputs/phpfpm/phpfpm.go index 38f8e320133c2..d6b3681209272 100644 --- a/plugins/inputs/phpfpm/phpfpm.go +++ b/plugins/inputs/phpfpm/phpfpm.go @@ -79,24 +79,39 @@ var sampleConfig = ` # insecure_skip_verify = false ` -func (r *phpfpm) SampleConfig() string { +func (p *phpfpm) SampleConfig() string { return sampleConfig } -func (r *phpfpm) Description() string { +func (p *phpfpm) Description() string { return "Read metrics of phpfpm, via HTTP status page or socket" } +func (p *phpfpm) Init() error { + tlsCfg, err := p.ClientConfig.TLSConfig() + if err != nil { + return err + } + + p.client = &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: tlsCfg, + }, + Timeout: p.Timeout.Duration, + } + return nil +} + // Reads stats from all configured servers accumulates stats. // Returns one of the errors encountered while gather stats (if any). -func (g *phpfpm) Gather(acc telegraf.Accumulator) error { - if len(g.Urls) == 0 { - return g.gatherServer("http://127.0.0.1/status", acc) +func (p *phpfpm) Gather(acc telegraf.Accumulator) error { + if len(p.Urls) == 0 { + return p.gatherServer("http://127.0.0.1/status", acc) } var wg sync.WaitGroup - urls, err := expandUrls(g.Urls) + urls, err := expandUrls(p.Urls) if err != nil { return err } @@ -105,7 +120,7 @@ func (g *phpfpm) Gather(acc telegraf.Accumulator) error { wg.Add(1) go func(serv string) { defer wg.Done() - acc.AddError(g.gatherServer(serv, acc)) + acc.AddError(p.gatherServer(serv, acc)) }(serv) } @@ -115,23 +130,9 @@ func (g *phpfpm) Gather(acc telegraf.Accumulator) error { } // Request status page to get stat raw data and import it -func (g *phpfpm) gatherServer(addr string, acc telegraf.Accumulator) error { - if g.client == nil { - tlsCfg, err := g.ClientConfig.TLSConfig() - if err != nil { - return err - } - tr := &http.Transport{ - TLSClientConfig: tlsCfg, - } - g.client = &http.Client{ - Transport: tr, - Timeout: g.Timeout.Duration, - } - } - +func (p *phpfpm) gatherServer(addr string, acc telegraf.Accumulator) error { if strings.HasPrefix(addr, "http://") || strings.HasPrefix(addr, "https://") { - return g.gatherHttp(addr, acc) + return p.gatherHttp(addr, acc) } var ( @@ -170,11 +171,11 @@ func (g *phpfpm) gatherServer(addr string, acc telegraf.Accumulator) error { return err } - return g.gatherFcgi(fcgi, statusPath, acc, addr) + return p.gatherFcgi(fcgi, statusPath, acc, addr) } // Gather stat using fcgi protocol -func (g *phpfpm) gatherFcgi(fcgi *conn, statusPath string, acc telegraf.Accumulator, addr string) error { +func (p *phpfpm) gatherFcgi(fcgi *conn, statusPath string, acc telegraf.Accumulator, addr string) error { fpmOutput, fpmErr, err := fcgi.Request(map[string]string{ "SCRIPT_NAME": "/" + statusPath, "SCRIPT_FILENAME": statusPath, @@ -194,7 +195,7 @@ func (g *phpfpm) gatherFcgi(fcgi *conn, statusPath string, acc telegraf.Accumula } // Gather stat using http protocol -func (g *phpfpm) gatherHttp(addr string, acc telegraf.Accumulator) error { +func (p *phpfpm) gatherHttp(addr string, acc telegraf.Accumulator) error { u, err := url.Parse(addr) if err != nil { return fmt.Errorf("Unable parse server address '%s': %s", addr, err) @@ -202,7 +203,7 @@ func (g *phpfpm) gatherHttp(addr string, acc telegraf.Accumulator) error { req, err := http.NewRequest("GET", fmt.Sprintf("%s://%s%s", u.Scheme, u.Host, u.Path), nil) - res, err := g.client.Do(req) + res, err := p.client.Do(req) if err != nil { return fmt.Errorf("Unable to connect to phpfpm status page '%s': %v", addr, err) diff --git a/plugins/inputs/phpfpm/phpfpm_test.go b/plugins/inputs/phpfpm/phpfpm_test.go index e7e36c36065e0..5f68b07f5dbae 100644 --- a/plugins/inputs/phpfpm/phpfpm_test.go +++ b/plugins/inputs/phpfpm/phpfpm_test.go @@ -33,9 +33,12 @@ func TestPhpFpmGeneratesMetrics_From_Http(t *testing.T) { Urls: []string{ts.URL}, } + err := r.Init() + require.NoError(t, err) + var acc testutil.Accumulator - err := acc.GatherError(r.Gather) + err = acc.GatherError(r.Gather) require.NoError(t, err) tags := map[string]string{ @@ -76,6 +79,9 @@ func TestPhpFpmGeneratesMetrics_From_Fcgi(t *testing.T) { Urls: []string{"fcgi://" + tcp.Addr().String() + "/status"}, } + err = r.Init() + require.NoError(t, err) + var acc testutil.Accumulator err = acc.GatherError(r.Gather) require.NoError(t, err) @@ -121,6 +127,9 @@ func TestPhpFpmGeneratesMetrics_From_Socket(t *testing.T) { Urls: []string{tcp.Addr().String()}, } + err = r.Init() + require.NoError(t, err) + var acc testutil.Accumulator err = acc.GatherError(r.Gather) @@ -177,6 +186,9 @@ func TestPhpFpmGeneratesMetrics_From_Multiple_Sockets_With_Glob(t *testing.T) { Urls: []string{"/tmp/test-fpm[\\-0-9]*.sock"}, } + err = r.Init() + require.NoError(t, err) + var acc1, acc2 testutil.Accumulator err = acc1.GatherError(r.Gather) @@ -232,6 +244,9 @@ func TestPhpFpmGeneratesMetrics_From_Socket_Custom_Status_Path(t *testing.T) { Urls: []string{tcp.Addr().String() + ":custom-status-path"}, } + err = r.Init() + require.NoError(t, err) + var acc testutil.Accumulator err = acc.GatherError(r.Gather) @@ -264,9 +279,12 @@ func TestPhpFpmGeneratesMetrics_From_Socket_Custom_Status_Path(t *testing.T) { func TestPhpFpmDefaultGetFromLocalhost(t *testing.T) { r := &phpfpm{} + err := r.Init() + require.NoError(t, err) + var acc testutil.Accumulator - err := acc.GatherError(r.Gather) + err = acc.GatherError(r.Gather) require.Error(t, err) assert.Contains(t, err.Error(), "127.0.0.1/status") } @@ -276,9 +294,12 @@ func TestPhpFpmGeneratesMetrics_Throw_Error_When_Fpm_Status_Is_Not_Responding(t Urls: []string{"http://aninvalidone"}, } + err := r.Init() + require.NoError(t, err) + var acc testutil.Accumulator - err := acc.GatherError(r.Gather) + err = acc.GatherError(r.Gather) require.Error(t, err) assert.Contains(t, err.Error(), `Unable to connect to phpfpm status page 'http://aninvalidone'`) assert.Contains(t, err.Error(), `lookup aninvalidone`) @@ -289,9 +310,12 @@ func TestPhpFpmGeneratesMetrics_Throw_Error_When_Socket_Path_Is_Invalid(t *testi Urls: []string{"/tmp/invalid.sock"}, } + err := r.Init() + require.NoError(t, err) + var acc testutil.Accumulator - err := acc.GatherError(r.Gather) + err = acc.GatherError(r.Gather) require.Error(t, err) assert.Equal(t, `dial unix /tmp/invalid.sock: connect: no such file or directory`, err.Error())