From 6c2902f165e0f96ab11d54c6e7fc2b8705d7aeba Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Fri, 3 May 2019 14:21:01 -0400 Subject: [PATCH 1/9] Add unit tests for libbeat's client proxy settings --- .../elasticsearch/client_proxy_test.go | 223 ++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 libbeat/outputs/elasticsearch/client_proxy_test.go diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go new file mode 100644 index 00000000000..125c3b10409 --- /dev/null +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -0,0 +1,223 @@ +// This file contains tests to confirm that elasticsearch.Client uses proxy +// settings following the intended precedence. + +package elasticsearch + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "os" + "os/exec" + "strings" + "sync" + "testing" + + "github.com/elastic/beats/libbeat/outputs/outil" + "github.com/stretchr/testify/assert" +) + +// TestClientPing is a placeholder test that does nothing on a standard run, +// but starts up a client and sends a ping when the environment variable +// TEST_START_CLIENT is set to 1 as in execClient). +func TestClientPing(t *testing.T) { + // If this is the child process, start up the client, otherwise do nothing. + if os.Getenv("TEST_START_CLIENT") == "1" { + doClientPing(t) + return + } +} + +// TestBaseline makes sure we can have a client process ping the server that +// we start, with no changes to the proxy settings. (This is really a +// meta-test for the helpers that create the servers / client.) +func TestBaseline(t *testing.T) { + listeners := testListeners{} + listeners.start(t) + defer listeners.close() + + // Start a bare client with no proxy settings, pointed at the main server. + _, err := execClient("TEST_SERVER_URL=" + listeners.server.URL) + assert.NoError(t, err) + // We expect one server request and 0 proxy requests + listeners.checkFinalState(t, 1, 0) +} + +// TestClientSettingsProxy confirms that we can control the proxy of a client +// by setting its ClientSettings.Proxy value on creation. (The child process +// uses the TEST_PROXY_URL environment variable to initialize the flag.) +func TestClientSettingsProxy(t *testing.T) { + listeners := testListeners{} + listeners.start(t) + defer listeners.close() + + // Start a client with ClientSettings.Proxy set to the proxy listener. + _, err := execClient( + "TEST_SERVER_URL="+listeners.server.URL, + "TEST_PROXY_URL="+listeners.proxy.URL) + assert.NoError(t, err) + // We expect one proxy request and 0 server requests + listeners.checkFinalState(t, 0, 1) +} + +// TestEnvironmentProxy confirms that we can control the proxy of a client by +// setting the HTTP_PROXY environment variable (see +// https://golang.org/pkg/net/http/#ProxyFromEnvironment). +func TestEnvironmentProxy(t *testing.T) { + listeners := testListeners{} + listeners.start(t) + defer listeners.close() + + // Start a client with HTTP_PROXY set to the proxy listener. + // The server is set to a nonexistent URL because ProxyFromEnvironment + // always returns a nil proxy for local destination URLs. For this case, we + // confirm the intended destination by setting it in the http headers, + // triggered in doClientPing by the TEST_HEADER_URL environment variable. + _, err := execClient( + "TEST_SERVER_URL=http://fakeurl.fake.not-real", + "TEST_HEADER_URL="+listeners.server.URL, + "HTTP_PROXY="+listeners.proxy.URL) + assert.NoError(t, err) + // We expect one proxy request and 0 server requests + listeners.checkFinalState(t, 0, 1) +} + +// TestClientSettingsOverrideEnvironmentProxy confirms that when both +// ClientSettings.Proxy and HTTP_PROXY are set, ClientSettings takes precedence. +func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { + listeners := testListeners{} + listeners.start(t) + defer listeners.close() + + // Start a client with ClientSettings.Proxy set to the proxy listener and + // HTTP_PROXY set to the server listener. We expect that the former will + // override the latter and thus we will only see a ping to the proxy. + // As above, the fake URL is needed to ensure ProxyFromEnvironment gives a + // non-nil result. + _, err := execClient( + "TEST_SERVER_URL=http://fakeurl.fake.not-real", + "TEST_HEADER_URL="+listeners.server.URL, + "TEST_PROXY_URL="+listeners.proxy.URL, + "HTTP_PROXY="+listeners.server.URL) + assert.NoError(t, err) + // We expect one proxy request and 0 server requests + listeners.checkFinalState(t, 0, 1) +} + +// runClientTest executes the current test binary as a child process, +// running only the TestClientPing, and calling it with the environment variable +// TEST_START_CLIENT=1 (so the test can recognize that it is the child process), +// and any additional environment settings specified in env. +// This is helpful for testing proxy settings, since we need to have both a +// proxy / server-side listener and a client that communicates with the server +// using various proxy settings. +func execClient(env ...string) (*bytes.Buffer, error) { + // The child process always runs only the TestClientPing test, which pings + // the server at TEST_SERVER_URL and then terminates. + cmd := exec.Command(os.Args[0], "-test.run=TestClientPing") + cmd.Env = append(append(os.Environ(), + "TEST_START_CLIENT=1"), + env...) + stderr := new(bytes.Buffer) + cmd.Stderr = stderr + + err := cmd.Run() + return stderr, err +} + +func doClientPing(t *testing.T) { + serverURL := os.Getenv("TEST_SERVER_URL") + proxy := os.Getenv("TEST_PROXY_URL") + headerURL := os.Getenv("TEST_HEADER_URL") + clientSettings := ClientSettings{ + URL: serverURL, + Index: outil.MakeSelector(outil.ConstSelectorExpr("test")), + Headers: map[string]string{"X-Test-URL": serverURL}, + } + if proxy != "" { + proxyURL, err := url.Parse(proxy) + assert.NoError(t, err) + clientSettings.Proxy = proxyURL + } + if headerURL != "" { + // Some tests point at an intentionally invalid server because golang's + // helpers never return a proxy for a request to the local host (see + // TestEnvironmentProxy), and in that case we still want to record the real + // server URL in the headers for verification. + clientSettings.Headers["X-Test-URL"] = headerURL + } + client, err := NewClient(clientSettings, nil) + assert.NoError(t, err) + + // This ping won't succeed; we aren't testing end-to-end communication + // (which would require a lot more setup work), we just want to make sure + // the client is pointed at the right server or proxy. + client.Ping() +} + +// testListeners is a wrapper that handles starting up http listeners +// representing an elastic server and an intermediate proxy, confirming that +// requests target the expected (server) URL, and counting how many requests +// arrive at each endpoint. +type testListeners struct { + server *httptest.Server + proxy *httptest.Server + + // T + mutex sync.Mutex + + serverRequestCount int // Requests directly to the server + proxyRequestCount int // Requests via the proxy + + errors []string +} + +func (tl *testListeners) start(t *testing.T) { + tl.server = httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tl.mutex.Lock() + defer tl.mutex.Unlock() + tl._assertEqual("server handler", tl.server.URL, r.Header.Get("X-Test-URL")) + fmt.Fprintln(w, "Hello, client") + tl.serverRequestCount++ + })) + tl.proxy = httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + tl.mutex.Lock() + defer tl.mutex.Unlock() + tl._assertEqual("proxy handler", tl.server.URL, r.Header.Get("X-Test-URL")) + fmt.Fprintln(w, "Hello, client") + tl.proxyRequestCount++ + })) +} + +func (tl *testListeners) close() { + tl.server.Close() + tl.proxy.Close() +} + +func (tl *testListeners) checkFinalState( + t *testing.T, expectedServerCount int, expectedProxyCount int) { + assert.Equal(t, expectedServerCount, tl.serverRequestCount) + assert.Equal(t, expectedProxyCount, tl.proxyRequestCount) + if len(tl.errors) > 0 { + // One or more of the server requests had a problem + assert.Fail(t, + "Error(s) in server requests", + strings.Join(tl.errors, "\n")) + } +} + +// _assertEqual compares the given strings and adds an error to +// s.errors if they aren't equal. The caller must hold s.mutex. +// We do this because testing.T is not thread safe, and we can't make +// testing assertions directly from our http handlers. +func (tl *testListeners) _assertEqual( + source string, expected string, got string) { + if expected != got { + tl.errors = append(tl.errors, fmt.Sprintf( + "Bad value in %v: expected [%v], got [%v]", source, expected, got)) + } +} From f81aa4786f694e32a02796f634a685a3a779a74f Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Fri, 3 May 2019 14:28:43 -0400 Subject: [PATCH 2/9] make update --- .../elasticsearch/client_proxy_test.go | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index 125c3b10409..f6512039e7d 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -1,3 +1,20 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + // This file contains tests to confirm that elasticsearch.Client uses proxy // settings following the intended precedence. @@ -15,8 +32,9 @@ import ( "sync" "testing" - "github.com/elastic/beats/libbeat/outputs/outil" "github.com/stretchr/testify/assert" + + "github.com/elastic/beats/libbeat/outputs/outil" ) // TestClientPing is a placeholder test that does nothing on a standard run, @@ -165,7 +183,9 @@ type testListeners struct { server *httptest.Server proxy *httptest.Server - // T + // This mutex must be held in order to modify the structure. + // Using a mutex for servers that are expected to receive ~1 request in + // their lifetime might be pedantic, but I really dislike race conditions. mutex sync.Mutex serverRequestCount int // Requests directly to the server @@ -174,7 +194,10 @@ type testListeners struct { errors []string } +// start creates and starts the server and proxy listeners. func (tl *testListeners) start(t *testing.T) { + tl.mutex.Lock() + defer tl.mutex.Unlock() tl.server = httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { tl.mutex.Lock() From 99960aaf95e564e0ca9491f341a21e9a33fe264b Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Mon, 6 May 2019 08:21:36 -0400 Subject: [PATCH 3/9] Remove superfluous stderr redirection --- .../outputs/elasticsearch/client_proxy_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index f6512039e7d..9fd9a2c33d1 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -21,7 +21,6 @@ package elasticsearch import ( - "bytes" "fmt" "net/http" "net/http/httptest" @@ -57,7 +56,7 @@ func TestBaseline(t *testing.T) { defer listeners.close() // Start a bare client with no proxy settings, pointed at the main server. - _, err := execClient("TEST_SERVER_URL=" + listeners.server.URL) + err := execClient("TEST_SERVER_URL=" + listeners.server.URL) assert.NoError(t, err) // We expect one server request and 0 proxy requests listeners.checkFinalState(t, 1, 0) @@ -72,7 +71,7 @@ func TestClientSettingsProxy(t *testing.T) { defer listeners.close() // Start a client with ClientSettings.Proxy set to the proxy listener. - _, err := execClient( + err := execClient( "TEST_SERVER_URL="+listeners.server.URL, "TEST_PROXY_URL="+listeners.proxy.URL) assert.NoError(t, err) @@ -93,7 +92,7 @@ func TestEnvironmentProxy(t *testing.T) { // always returns a nil proxy for local destination URLs. For this case, we // confirm the intended destination by setting it in the http headers, // triggered in doClientPing by the TEST_HEADER_URL environment variable. - _, err := execClient( + err := execClient( "TEST_SERVER_URL=http://fakeurl.fake.not-real", "TEST_HEADER_URL="+listeners.server.URL, "HTTP_PROXY="+listeners.proxy.URL) @@ -114,7 +113,7 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // override the latter and thus we will only see a ping to the proxy. // As above, the fake URL is needed to ensure ProxyFromEnvironment gives a // non-nil result. - _, err := execClient( + err := execClient( "TEST_SERVER_URL=http://fakeurl.fake.not-real", "TEST_HEADER_URL="+listeners.server.URL, "TEST_PROXY_URL="+listeners.proxy.URL, @@ -131,18 +130,14 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // This is helpful for testing proxy settings, since we need to have both a // proxy / server-side listener and a client that communicates with the server // using various proxy settings. -func execClient(env ...string) (*bytes.Buffer, error) { +func execClient(env ...string) error { // The child process always runs only the TestClientPing test, which pings // the server at TEST_SERVER_URL and then terminates. cmd := exec.Command(os.Args[0], "-test.run=TestClientPing") cmd.Env = append(append(os.Environ(), "TEST_START_CLIENT=1"), env...) - stderr := new(bytes.Buffer) - cmd.Stderr = stderr - - err := cmd.Run() - return stderr, err + return cmd.Run() } func doClientPing(t *testing.T) { From 87cbe816548438e42ba70de4ab2119efeda3e41b Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Tue, 7 May 2019 14:21:16 -0400 Subject: [PATCH 4/9] Add mutex lock to avoid data race error --- libbeat/outputs/elasticsearch/client_proxy_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index 9fd9a2c33d1..b9a5d9cdd97 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -218,6 +218,11 @@ func (tl *testListeners) close() { func (tl *testListeners) checkFinalState( t *testing.T, expectedServerCount int, expectedProxyCount int) { + // The lock here should be superfluous, since we only call checkFinalState + // after the clients have terminated, but golang still detects it as a data + // race, so we lock here anyway. + tl.mutex.Lock() + defer tl.mutex.Unlock() assert.Equal(t, expectedServerCount, tl.serverRequestCount) assert.Equal(t, expectedProxyCount, tl.proxyRequestCount) if len(tl.errors) > 0 { From cf9f92e9aceadc5a03617db25fd6233b29c696c8 Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Wed, 8 May 2019 11:10:03 -0400 Subject: [PATCH 5/9] Switching to atomic value helpers / removing superfluous synchronization --- .../elasticsearch/client_proxy_test.go | 75 ++++++------------- 1 file changed, 23 insertions(+), 52 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index b9a5d9cdd97..359f98c2d09 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -27,12 +27,12 @@ import ( "net/url" "os" "os/exec" - "strings" - "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/elastic/beats/libbeat/common/atomic" "github.com/elastic/beats/libbeat/outputs/outil" ) @@ -59,7 +59,8 @@ func TestBaseline(t *testing.T) { err := execClient("TEST_SERVER_URL=" + listeners.server.URL) assert.NoError(t, err) // We expect one server request and 0 proxy requests - listeners.checkFinalState(t, 1, 0) + assert.Equal(t, 1, listeners.serverRequestCount()) + assert.Equal(t, 0, listeners.proxyRequestCount()) } // TestClientSettingsProxy confirms that we can control the proxy of a client @@ -76,7 +77,8 @@ func TestClientSettingsProxy(t *testing.T) { "TEST_PROXY_URL="+listeners.proxy.URL) assert.NoError(t, err) // We expect one proxy request and 0 server requests - listeners.checkFinalState(t, 0, 1) + assert.Equal(t, 0, listeners.serverRequestCount()) + assert.Equal(t, 1, listeners.proxyRequestCount()) } // TestEnvironmentProxy confirms that we can control the proxy of a client by @@ -98,7 +100,8 @@ func TestEnvironmentProxy(t *testing.T) { "HTTP_PROXY="+listeners.proxy.URL) assert.NoError(t, err) // We expect one proxy request and 0 server requests - listeners.checkFinalState(t, 0, 1) + assert.Equal(t, 0, listeners.serverRequestCount()) + assert.Equal(t, 1, listeners.proxyRequestCount()) } // TestClientSettingsOverrideEnvironmentProxy confirms that when both @@ -120,7 +123,8 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { "HTTP_PROXY="+listeners.server.URL) assert.NoError(t, err) // We expect one proxy request and 0 server requests - listeners.checkFinalState(t, 0, 1) + assert.Equal(t, 0, listeners.serverRequestCount()) + assert.Equal(t, 1, listeners.proxyRequestCount()) } // runClientTest executes the current test binary as a child process, @@ -151,7 +155,7 @@ func doClientPing(t *testing.T) { } if proxy != "" { proxyURL, err := url.Parse(proxy) - assert.NoError(t, err) + require.NoError(t, err) clientSettings.Proxy = proxyURL } if headerURL != "" { @@ -162,7 +166,7 @@ func doClientPing(t *testing.T) { clientSettings.Headers["X-Test-URL"] = headerURL } client, err := NewClient(clientSettings, nil) - assert.NoError(t, err) + require.NoError(t, err) // This ping won't succeed; we aren't testing end-to-end communication // (which would require a lot more setup work), we just want to make sure @@ -178,36 +182,23 @@ type testListeners struct { server *httptest.Server proxy *httptest.Server - // This mutex must be held in order to modify the structure. - // Using a mutex for servers that are expected to receive ~1 request in - // their lifetime might be pedantic, but I really dislike race conditions. - mutex sync.Mutex - - serverRequestCount int // Requests directly to the server - proxyRequestCount int // Requests via the proxy - - errors []string + _serverRequestCount atomic.Int // Requests directly to the server + _proxyRequestCount atomic.Int // Requests via the proxy } // start creates and starts the server and proxy listeners. func (tl *testListeners) start(t *testing.T) { - tl.mutex.Lock() - defer tl.mutex.Unlock() tl.server = httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tl.mutex.Lock() - defer tl.mutex.Unlock() - tl._assertEqual("server handler", tl.server.URL, r.Header.Get("X-Test-URL")) + assert.Equal(t, tl.server.URL, r.Header.Get("X-Test-URL")) fmt.Fprintln(w, "Hello, client") - tl.serverRequestCount++ + tl._serverRequestCount.Inc() })) tl.proxy = httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tl.mutex.Lock() - defer tl.mutex.Unlock() - tl._assertEqual("proxy handler", tl.server.URL, r.Header.Get("X-Test-URL")) + assert.Equal(t, tl.server.URL, r.Header.Get("X-Test-URL")) fmt.Fprintln(w, "Hello, client") - tl.proxyRequestCount++ + tl._proxyRequestCount.Inc() })) } @@ -216,31 +207,11 @@ func (tl *testListeners) close() { tl.proxy.Close() } -func (tl *testListeners) checkFinalState( - t *testing.T, expectedServerCount int, expectedProxyCount int) { - // The lock here should be superfluous, since we only call checkFinalState - // after the clients have terminated, but golang still detects it as a data - // race, so we lock here anyway. - tl.mutex.Lock() - defer tl.mutex.Unlock() - assert.Equal(t, expectedServerCount, tl.serverRequestCount) - assert.Equal(t, expectedProxyCount, tl.proxyRequestCount) - if len(tl.errors) > 0 { - // One or more of the server requests had a problem - assert.Fail(t, - "Error(s) in server requests", - strings.Join(tl.errors, "\n")) - } +// Convenience functions to unwrap the atomic primitives +func (tl testListeners) serverRequestCount() int { + return tl._serverRequestCount.Load() } -// _assertEqual compares the given strings and adds an error to -// s.errors if they aren't equal. The caller must hold s.mutex. -// We do this because testing.T is not thread safe, and we can't make -// testing assertions directly from our http handlers. -func (tl *testListeners) _assertEqual( - source string, expected string, got string) { - if expected != got { - tl.errors = append(tl.errors, fmt.Sprintf( - "Bad value in %v: expected [%v], got [%v]", source, expected, got)) - } +func (tl testListeners) proxyRequestCount() int { + return tl._proxyRequestCount.Load() } From cfa6ed8dc9add61f9c4ebad5ff23f89aa6071402 Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Wed, 8 May 2019 11:25:19 -0400 Subject: [PATCH 6/9] better error checking / reporting --- .../elasticsearch/client_proxy_test.go | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index 359f98c2d09..0df86786b19 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -21,6 +21,7 @@ package elasticsearch import ( + "bytes" "fmt" "net/http" "net/http/httptest" @@ -56,8 +57,7 @@ func TestBaseline(t *testing.T) { defer listeners.close() // Start a bare client with no proxy settings, pointed at the main server. - err := execClient("TEST_SERVER_URL=" + listeners.server.URL) - assert.NoError(t, err) + execClient(t, "TEST_SERVER_URL="+listeners.server.URL) // We expect one server request and 0 proxy requests assert.Equal(t, 1, listeners.serverRequestCount()) assert.Equal(t, 0, listeners.proxyRequestCount()) @@ -72,10 +72,9 @@ func TestClientSettingsProxy(t *testing.T) { defer listeners.close() // Start a client with ClientSettings.Proxy set to the proxy listener. - err := execClient( + execClient(t, "TEST_SERVER_URL="+listeners.server.URL, "TEST_PROXY_URL="+listeners.proxy.URL) - assert.NoError(t, err) // We expect one proxy request and 0 server requests assert.Equal(t, 0, listeners.serverRequestCount()) assert.Equal(t, 1, listeners.proxyRequestCount()) @@ -94,11 +93,10 @@ func TestEnvironmentProxy(t *testing.T) { // always returns a nil proxy for local destination URLs. For this case, we // confirm the intended destination by setting it in the http headers, // triggered in doClientPing by the TEST_HEADER_URL environment variable. - err := execClient( + execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", "TEST_HEADER_URL="+listeners.server.URL, "HTTP_PROXY="+listeners.proxy.URL) - assert.NoError(t, err) // We expect one proxy request and 0 server requests assert.Equal(t, 0, listeners.serverRequestCount()) assert.Equal(t, 1, listeners.proxyRequestCount()) @@ -116,12 +114,11 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // override the latter and thus we will only see a ping to the proxy. // As above, the fake URL is needed to ensure ProxyFromEnvironment gives a // non-nil result. - err := execClient( + execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", "TEST_HEADER_URL="+listeners.server.URL, "TEST_PROXY_URL="+listeners.proxy.URL, "HTTP_PROXY="+listeners.server.URL) - assert.NoError(t, err) // We expect one proxy request and 0 server requests assert.Equal(t, 0, listeners.serverRequestCount()) assert.Equal(t, 1, listeners.proxyRequestCount()) @@ -134,18 +131,26 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // This is helpful for testing proxy settings, since we need to have both a // proxy / server-side listener and a client that communicates with the server // using various proxy settings. -func execClient(env ...string) error { +func execClient(t *testing.T, env ...string) { // The child process always runs only the TestClientPing test, which pings // the server at TEST_SERVER_URL and then terminates. cmd := exec.Command(os.Args[0], "-test.run=TestClientPing") cmd.Env = append(append(os.Environ(), "TEST_START_CLIENT=1"), env...) - return cmd.Run() + cmdOutput := new(bytes.Buffer) + cmd.Stderr = cmdOutput + cmd.Stdout = cmdOutput + + err := cmd.Run() + if err != nil { + t.Error("Error executing client:\n" + cmdOutput.String()) + } } func doClientPing(t *testing.T) { serverURL := os.Getenv("TEST_SERVER_URL") + require.NotEqual(t, serverURL, "") proxy := os.Getenv("TEST_PROXY_URL") headerURL := os.Getenv("TEST_HEADER_URL") clientSettings := ClientSettings{ From 30cfaa72b44d695e4d4242b94e096e3bba975268 Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Wed, 8 May 2019 11:45:06 -0400 Subject: [PATCH 7/9] refactor per review suggestions --- .../elasticsearch/client_proxy_test.go | 112 +++++++++--------- 1 file changed, 55 insertions(+), 57 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index 0df86786b19..c4c94998900 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -52,41 +52,38 @@ func TestClientPing(t *testing.T) { // we start, with no changes to the proxy settings. (This is really a // meta-test for the helpers that create the servers / client.) func TestBaseline(t *testing.T) { - listeners := testListeners{} - listeners.start(t) - defer listeners.close() + servers, teardown := startServers(t) + defer teardown() // Start a bare client with no proxy settings, pointed at the main server. - execClient(t, "TEST_SERVER_URL="+listeners.server.URL) + execClient(t, "TEST_SERVER_URL="+servers.serverURL) // We expect one server request and 0 proxy requests - assert.Equal(t, 1, listeners.serverRequestCount()) - assert.Equal(t, 0, listeners.proxyRequestCount()) + assert.Equal(t, 1, servers.serverRequestCount()) + assert.Equal(t, 0, servers.proxyRequestCount()) } // TestClientSettingsProxy confirms that we can control the proxy of a client // by setting its ClientSettings.Proxy value on creation. (The child process // uses the TEST_PROXY_URL environment variable to initialize the flag.) func TestClientSettingsProxy(t *testing.T) { - listeners := testListeners{} - listeners.start(t) - defer listeners.close() + servers, teardown := startServers(t) + defer teardown() // Start a client with ClientSettings.Proxy set to the proxy listener. execClient(t, - "TEST_SERVER_URL="+listeners.server.URL, - "TEST_PROXY_URL="+listeners.proxy.URL) + "TEST_SERVER_URL="+servers.serverURL, + "TEST_PROXY_URL="+servers.proxyURL) // We expect one proxy request and 0 server requests - assert.Equal(t, 0, listeners.serverRequestCount()) - assert.Equal(t, 1, listeners.proxyRequestCount()) + assert.Equal(t, 0, servers.serverRequestCount()) + assert.Equal(t, 1, servers.proxyRequestCount()) } // TestEnvironmentProxy confirms that we can control the proxy of a client by // setting the HTTP_PROXY environment variable (see // https://golang.org/pkg/net/http/#ProxyFromEnvironment). func TestEnvironmentProxy(t *testing.T) { - listeners := testListeners{} - listeners.start(t) - defer listeners.close() + servers, teardown := startServers(t) + defer teardown() // Start a client with HTTP_PROXY set to the proxy listener. // The server is set to a nonexistent URL because ProxyFromEnvironment @@ -95,19 +92,18 @@ func TestEnvironmentProxy(t *testing.T) { // triggered in doClientPing by the TEST_HEADER_URL environment variable. execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", - "TEST_HEADER_URL="+listeners.server.URL, - "HTTP_PROXY="+listeners.proxy.URL) + "TEST_HEADER_URL="+servers.serverURL, + "HTTP_PROXY="+servers.proxyURL) // We expect one proxy request and 0 server requests - assert.Equal(t, 0, listeners.serverRequestCount()) - assert.Equal(t, 1, listeners.proxyRequestCount()) + assert.Equal(t, 0, servers.serverRequestCount()) + assert.Equal(t, 1, servers.proxyRequestCount()) } // TestClientSettingsOverrideEnvironmentProxy confirms that when both // ClientSettings.Proxy and HTTP_PROXY are set, ClientSettings takes precedence. func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { - listeners := testListeners{} - listeners.start(t) - defer listeners.close() + servers, teardown := startServers(t) + defer teardown() // Start a client with ClientSettings.Proxy set to the proxy listener and // HTTP_PROXY set to the server listener. We expect that the former will @@ -116,12 +112,12 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // non-nil result. execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", - "TEST_HEADER_URL="+listeners.server.URL, - "TEST_PROXY_URL="+listeners.proxy.URL, - "HTTP_PROXY="+listeners.server.URL) + "TEST_HEADER_URL="+servers.serverURL, + "TEST_PROXY_URL="+servers.proxyURL, + "HTTP_PROXY="+servers.serverURL) // We expect one proxy request and 0 server requests - assert.Equal(t, 0, listeners.serverRequestCount()) - assert.Equal(t, 1, listeners.proxyRequestCount()) + assert.Equal(t, 0, servers.serverRequestCount()) + assert.Equal(t, 1, servers.proxyRequestCount()) } // runClientTest executes the current test binary as a child process, @@ -179,44 +175,46 @@ func doClientPing(t *testing.T) { client.Ping() } -// testListeners is a wrapper that handles starting up http listeners -// representing an elastic server and an intermediate proxy, confirming that -// requests target the expected (server) URL, and counting how many requests -// arrive at each endpoint. -type testListeners struct { - server *httptest.Server - proxy *httptest.Server +// serverState contains the state of the http listeners for proxy tests, +// including the endpoint URLs and the observed request count for each one. +type serverState struct { + serverURL string + proxyURL string _serverRequestCount atomic.Int // Requests directly to the server _proxyRequestCount atomic.Int // Requests via the proxy } -// start creates and starts the server and proxy listeners. -func (tl *testListeners) start(t *testing.T) { - tl.server = httptest.NewServer( +// Convenience functions to unwrap the atomic primitives +func (s serverState) serverRequestCount() int { + return s._serverRequestCount.Load() +} + +func (s serverState) proxyRequestCount() int { + return s._proxyRequestCount.Load() +} + +// startServers starts endpoints representing a backend sefrver and a proxy, +// and returns the corresponding serverState and a teardown function that +// should be called to shut them down at the end of the test. +func startServers(t *testing.T) (*serverState, func()) { + state := serverState{} + server := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, tl.server.URL, r.Header.Get("X-Test-URL")) + assert.Equal(t, state.serverURL, r.Header.Get("X-Test-URL")) fmt.Fprintln(w, "Hello, client") - tl._serverRequestCount.Inc() + state._serverRequestCount.Inc() })) - tl.proxy = httptest.NewServer( + proxy := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, tl.server.URL, r.Header.Get("X-Test-URL")) + assert.Equal(t, state.serverURL, r.Header.Get("X-Test-URL")) fmt.Fprintln(w, "Hello, client") - tl._proxyRequestCount.Inc() + state._proxyRequestCount.Inc() })) -} - -func (tl *testListeners) close() { - tl.server.Close() - tl.proxy.Close() -} - -// Convenience functions to unwrap the atomic primitives -func (tl testListeners) serverRequestCount() int { - return tl._serverRequestCount.Load() -} - -func (tl testListeners) proxyRequestCount() int { - return tl._proxyRequestCount.Load() + state.serverURL = server.URL + state.proxyURL = proxy.URL + return &state, func() { + server.Close() + proxy.Close() + } } From 48aa6e5fcaa4de13ead85f70fc0b3d643a630bf9 Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Wed, 8 May 2019 11:57:18 -0400 Subject: [PATCH 8/9] fix typo in comment --- libbeat/outputs/elasticsearch/client_proxy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index c4c94998900..3aa21ef763a 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -194,7 +194,7 @@ func (s serverState) proxyRequestCount() int { return s._proxyRequestCount.Load() } -// startServers starts endpoints representing a backend sefrver and a proxy, +// startServers starts endpoints representing a backend server and a proxy, // and returns the corresponding serverState and a teardown function that // should be called to shut them down at the end of the test. func startServers(t *testing.T) (*serverState, func()) { From bc17bb6333a100755484f4d2341aaa2d70fb12f4 Mon Sep 17 00:00:00 2001 From: Fae Charlton Date: Wed, 8 May 2019 16:12:53 -0400 Subject: [PATCH 9/9] simplify http test headers / remove data race --- .../elasticsearch/client_proxy_test.go | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/libbeat/outputs/elasticsearch/client_proxy_test.go b/libbeat/outputs/elasticsearch/client_proxy_test.go index 3aa21ef763a..d4a2e0c12eb 100644 --- a/libbeat/outputs/elasticsearch/client_proxy_test.go +++ b/libbeat/outputs/elasticsearch/client_proxy_test.go @@ -37,6 +37,13 @@ import ( "github.com/elastic/beats/libbeat/outputs/outil" ) +// These constants are inserted into client http request headers and confirmed +// by the server listeners. +const ( + headerTestField = "X-Test-Value" + headerTestValue = "client_proxy_test test value" +) + // TestClientPing is a placeholder test that does nothing on a standard run, // but starts up a client and sends a ping when the environment variable // TEST_START_CLIENT is set to 1 as in execClient). @@ -87,12 +94,9 @@ func TestEnvironmentProxy(t *testing.T) { // Start a client with HTTP_PROXY set to the proxy listener. // The server is set to a nonexistent URL because ProxyFromEnvironment - // always returns a nil proxy for local destination URLs. For this case, we - // confirm the intended destination by setting it in the http headers, - // triggered in doClientPing by the TEST_HEADER_URL environment variable. + // always returns a nil proxy for local destination URLs. execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", - "TEST_HEADER_URL="+servers.serverURL, "HTTP_PROXY="+servers.proxyURL) // We expect one proxy request and 0 server requests assert.Equal(t, 0, servers.serverRequestCount()) @@ -112,7 +116,6 @@ func TestClientSettingsOverrideEnvironmentProxy(t *testing.T) { // non-nil result. execClient(t, "TEST_SERVER_URL=http://fakeurl.fake.not-real", - "TEST_HEADER_URL="+servers.serverURL, "TEST_PROXY_URL="+servers.proxyURL, "HTTP_PROXY="+servers.serverURL) // We expect one proxy request and 0 server requests @@ -148,24 +151,16 @@ func doClientPing(t *testing.T) { serverURL := os.Getenv("TEST_SERVER_URL") require.NotEqual(t, serverURL, "") proxy := os.Getenv("TEST_PROXY_URL") - headerURL := os.Getenv("TEST_HEADER_URL") clientSettings := ClientSettings{ URL: serverURL, Index: outil.MakeSelector(outil.ConstSelectorExpr("test")), - Headers: map[string]string{"X-Test-URL": serverURL}, + Headers: map[string]string{headerTestField: headerTestValue}, } if proxy != "" { proxyURL, err := url.Parse(proxy) require.NoError(t, err) clientSettings.Proxy = proxyURL } - if headerURL != "" { - // Some tests point at an intentionally invalid server because golang's - // helpers never return a proxy for a request to the local host (see - // TestEnvironmentProxy), and in that case we still want to record the real - // server URL in the headers for verification. - clientSettings.Headers["X-Test-URL"] = headerURL - } client, err := NewClient(clientSettings, nil) require.NoError(t, err) @@ -201,13 +196,13 @@ func startServers(t *testing.T) (*serverState, func()) { state := serverState{} server := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, state.serverURL, r.Header.Get("X-Test-URL")) + assert.Equal(t, headerTestValue, r.Header.Get(headerTestField)) fmt.Fprintln(w, "Hello, client") state._serverRequestCount.Inc() })) proxy := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, state.serverURL, r.Header.Get("X-Test-URL")) + assert.Equal(t, headerTestValue, r.Header.Get(headerTestField)) fmt.Fprintln(w, "Hello, client") state._proxyRequestCount.Inc() }))