From f5c26814abebc4f5808e049552615933488f04bd Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Thu, 13 May 2021 09:09:32 +0200 Subject: [PATCH 1/5] Loggers to catch the e2e flake. --- client/pkg/fileutil/fileutil.go | 5 ++++- client/pkg/transport/listener.go | 16 ++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/client/pkg/fileutil/fileutil.go b/client/pkg/fileutil/fileutil.go index 85a9842b0..e442c3c92 100644 --- a/client/pkg/fileutil/fileutil.go +++ b/client/pkg/fileutil/fileutil.go @@ -32,7 +32,10 @@ const ( // IsDirWriteable checks if dir is writable by writing and removing a file // to dir. It returns nil if dir is writable. func IsDirWriteable(dir string) error { - f := filepath.Join(dir, ".touch") + f, err := filepath.Abs(filepath.Join(dir, ".touch")) + if err != nil { + return err + } if err := ioutil.WriteFile(f, []byte(""), PrivateFileMode); err != nil { return err } diff --git a/client/pkg/transport/listener.go b/client/pkg/transport/listener.go index cd8626c7f..992c773ea 100644 --- a/client/pkg/transport/listener.go +++ b/client/pkg/transport/listener.go @@ -203,8 +203,14 @@ func SelfCert(lg *zap.Logger, dirpath string, hosts []string, selfSignedCertVali return } - certPath := filepath.Join(dirpath, "cert.pem") - keyPath := filepath.Join(dirpath, "key.pem") + certPath, err := filepath.Abs(filepath.Join(dirpath, "cert.pem")) + if err != nil { + return + } + keyPath, err := filepath.Abs(filepath.Join(dirpath, "key.pem")) + if err != nil { + return + } _, errcert := os.Stat(certPath) _, errkey := os.Stat(keyPath) if errcert == nil && errkey == nil { @@ -468,6 +474,10 @@ func (info TLSInfo) ServerConfig() (*tls.Config, error) { return nil, err } + if info.Logger == nil { + info.Logger = zap.NewNop() + } + cfg.ClientAuth = tls.NoClientCert if info.TrustedCAFile != "" || info.ClientCertAuth { cfg.ClientAuth = tls.RequireAndVerifyClientCert @@ -475,6 +485,8 @@ func (info TLSInfo) ServerConfig() (*tls.Config, error) { cs := info.cafiles() if len(cs) > 0 { + info.Logger.Info("Loading cert pool", zap.Strings("cs", cs), + zap.Any("tlsinfo", info)) cp, err := tlsutil.NewCertPool(cs) if err != nil { return nil, err From 582d02e7f52734bb96f4fcfa23c455e3792bc8a7 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 14 May 2021 05:54:15 +0200 Subject: [PATCH 2/5] E2E tests should log commandlines used to spawn etcd or etcd proxy binaries. --- tests/e2e/cluster_proxy_test.go | 6 +++++- tests/e2e/cluster_test.go | 4 ++++ tests/e2e/etcd_process.go | 4 +++- tests/e2e/etcd_spawn_nocov.go | 11 +++++++++++ 4 files changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/e2e/cluster_proxy_test.go b/tests/e2e/cluster_proxy_test.go index 7a5740d14..47ac18f96 100644 --- a/tests/e2e/cluster_proxy_test.go +++ b/tests/e2e/cluster_proxy_test.go @@ -27,6 +27,7 @@ import ( "strings" "go.etcd.io/etcd/pkg/v3/expect" + "go.uber.org/zap" ) type proxyEtcdProcess struct { @@ -115,6 +116,7 @@ func (p *proxyEtcdProcess) WithStopSignal(sig os.Signal) os.Signal { } type proxyProc struct { + lg *zap.Logger execPath string args []string ep string @@ -130,7 +132,7 @@ func (pp *proxyProc) start() error { if pp.proc != nil { panic("already started") } - proc, err := spawnCmd(append([]string{pp.execPath}, pp.args...)) + proc, err := spawnCmdWithLogger(pp.lg, append([]string{pp.execPath}, pp.args...)) if err != nil { return err } @@ -192,6 +194,7 @@ func newProxyV2Proc(cfg *etcdServerProcessConfig) *proxyV2Proc { } return &proxyV2Proc{ proxyProc{ + lg: cfg.lg, execPath: cfg.execPath, args: append(args, cfg.tlsArgs...), ep: listenAddr, @@ -276,6 +279,7 @@ func newProxyV3Proc(cfg *etcdServerProcessConfig) *proxyV3Proc { } return &proxyV3Proc{ proxyProc{ + lg: cfg.lg, execPath: cfg.execPath, args: append(args, tlsArgs...), ep: listenAddr, diff --git a/tests/e2e/cluster_test.go b/tests/e2e/cluster_test.go index cc47121fa..f30d7db23 100644 --- a/tests/e2e/cluster_test.go +++ b/tests/e2e/cluster_test.go @@ -25,6 +25,7 @@ import ( "go.etcd.io/etcd/server/v3/etcdserver" "go.etcd.io/etcd/tests/v3/integration" + "go.uber.org/zap/zaptest" ) const etcdProcessBasePort = 20000 @@ -225,6 +226,8 @@ func (cfg *etcdProcessClusterConfig) peerScheme() string { } func (cfg *etcdProcessClusterConfig) etcdServerProcessConfigs(tb testing.TB) []*etcdServerProcessConfig { + lg := zaptest.NewLogger(tb) + if cfg.basePort == 0 { cfg.basePort = etcdProcessBasePort } @@ -309,6 +312,7 @@ func (cfg *etcdProcessClusterConfig) etcdServerProcessConfigs(tb testing.TB) []* } etcdCfgs[i] = &etcdServerProcessConfig{ + lg: lg, execPath: cfg.execPath, args: args, tlsArgs: cfg.tlsArgs(), diff --git a/tests/e2e/etcd_process.go b/tests/e2e/etcd_process.go index 55f3494eb..aecd56ce3 100644 --- a/tests/e2e/etcd_process.go +++ b/tests/e2e/etcd_process.go @@ -21,6 +21,7 @@ import ( "go.etcd.io/etcd/client/pkg/v3/fileutil" "go.etcd.io/etcd/pkg/v3/expect" + "go.uber.org/zap" ) var ( @@ -50,6 +51,7 @@ type etcdServerProcess struct { } type etcdServerProcessConfig struct { + lg *zap.Logger execPath string args []string tlsArgs []string @@ -88,7 +90,7 @@ func (ep *etcdServerProcess) Start() error { if ep.proc != nil { panic("already started") } - proc, err := spawnCmd(append([]string{ep.cfg.execPath}, ep.cfg.args...)) + proc, err := spawnCmdWithLogger(ep.cfg.lg, append([]string{ep.cfg.execPath}, ep.cfg.args...)) if err != nil { return err } diff --git a/tests/e2e/etcd_spawn_nocov.go b/tests/e2e/etcd_spawn_nocov.go index b70240496..e753a967f 100644 --- a/tests/e2e/etcd_spawn_nocov.go +++ b/tests/e2e/etcd_spawn_nocov.go @@ -21,14 +21,25 @@ import ( "os" "go.etcd.io/etcd/pkg/v3/expect" + "go.uber.org/zap" ) const noOutputLineCount = 0 // regular binaries emit no extra lines func spawnCmd(args []string) (*expect.ExpectProcess, error) { + return spawnCmdWithLogger(zap.NewNop(), args) +} + +func spawnCmdWithLogger(lg *zap.Logger, args []string) (*expect.ExpectProcess, error) { + wd, err := os.Getwd() + if err != nil { + return nil, err + } if args[0] == ctlBinPath+"3" { env := append(os.Environ(), "ETCDCTL_API=3") + lg.Info("spawning process with ETCDCTL_API=3", zap.Strings("args", args), zap.String("working-dir", wd)) return expect.NewExpectWithEnv(ctlBinPath, args[1:], env) } + lg.Info("spawning process", zap.Strings("args", args), zap.String("working-dir", wd)) return expect.NewExpect(args[0], args[1:]...) } From c18010cf42f265cb247f924c54bf157f08a17626 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 14 May 2021 06:16:36 +0200 Subject: [PATCH 3/5] etcdproxy e2e tests should run in dedicated directories. So far all proxies were sharing the same (current) directory, leading to tests flakes, e.g. due to certificates being overriden in autoTLS mode. --- tests/e2e/cluster_proxy_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/e2e/cluster_proxy_test.go b/tests/e2e/cluster_proxy_test.go index 47ac18f96..b96a10037 100644 --- a/tests/e2e/cluster_proxy_test.go +++ b/tests/e2e/cluster_proxy_test.go @@ -186,21 +186,23 @@ func proxyListenURL(cfg *etcdServerProcessConfig, portOffset int) string { func newProxyV2Proc(cfg *etcdServerProcessConfig) *proxyV2Proc { listenAddr := proxyListenURL(cfg, 2) name := fmt.Sprintf("testname-proxy-%p", cfg) + dataDir := path.Join(cfg.dataDirPath, name+".etcd") args := []string{ "--name", name, "--proxy", "on", "--listen-client-urls", listenAddr, "--initial-cluster", cfg.name + "=" + cfg.purl.String(), + "--data-dir", dataDir, } return &proxyV2Proc{ - proxyProc{ + proxyProc: proxyProc{ lg: cfg.lg, execPath: cfg.execPath, args: append(args, cfg.tlsArgs...), ep: listenAddr, donec: make(chan struct{}), }, - name + ".etcd", + dataDir: dataDir, } } @@ -242,6 +244,7 @@ func newProxyV3Proc(cfg *etcdServerProcessConfig) *proxyV3Proc { "--endpoints", cfg.acurl, // pass-through member RPCs "--advertise-client-url", "", + "--data-dir", cfg.dataDirPath, } murl := "" if cfg.murl != "" { From 8981afb6f5b7568a827b93d159676fd1bb26d18c Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 14 May 2021 07:12:52 +0200 Subject: [PATCH 4/5] Fix unit tests logging config. --- client/pkg/transport/listener_test.go | 6 +- pkg/go.mod | 1 + pkg/go.sum | 8 ++- pkg/proxy/server_test.go | 90 ++++++++++++++------------- 4 files changed, 60 insertions(+), 45 deletions(-) diff --git a/client/pkg/transport/listener_test.go b/client/pkg/transport/listener_test.go index 0a7b0ad16..00657648e 100644 --- a/client/pkg/transport/listener_test.go +++ b/client/pkg/transport/listener_test.go @@ -26,6 +26,7 @@ import ( "time" "go.uber.org/zap" + "go.uber.org/zap/zaptest" ) func createSelfCert(hosts ...string) (*TLSInfo, func(), error) { @@ -473,6 +474,7 @@ func TestTLSInfoParseFuncError(t *testing.T) { } func TestTLSInfoConfigFuncs(t *testing.T) { + ln := zaptest.NewLogger(t) tlsinfo, del, err := createSelfCert() if err != nil { t.Fatalf("unable to create cert: %v", err) @@ -485,13 +487,13 @@ func TestTLSInfoConfigFuncs(t *testing.T) { wantCAs bool }{ { - info: TLSInfo{CertFile: tlsinfo.CertFile, KeyFile: tlsinfo.KeyFile}, + info: TLSInfo{CertFile: tlsinfo.CertFile, KeyFile: tlsinfo.KeyFile, Logger: ln}, clientAuth: tls.NoClientCert, wantCAs: false, }, { - info: TLSInfo{CertFile: tlsinfo.CertFile, KeyFile: tlsinfo.KeyFile, TrustedCAFile: tlsinfo.CertFile}, + info: TLSInfo{CertFile: tlsinfo.CertFile, KeyFile: tlsinfo.KeyFile, TrustedCAFile: tlsinfo.CertFile, Logger: ln}, clientAuth: tls.RequireAndVerifyClientCert, wantCAs: true, }, diff --git a/pkg/go.mod b/pkg/go.mod index fefe20c1c..afb2b9e19 100644 --- a/pkg/go.mod +++ b/pkg/go.mod @@ -8,6 +8,7 @@ require ( github.com/golang/protobuf v1.5.1 // indirect github.com/spf13/cobra v1.1.3 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.7.0 go.etcd.io/etcd/client/pkg/v3 v3.5.0-alpha.0 go.uber.org/zap v1.16.1-0.20210329175301-c23abee72d19 google.golang.org/grpc v1.37.0 diff --git a/pkg/go.sum b/pkg/go.sum index e88c8cb93..a0c5c50c2 100644 --- a/pkg/go.sum +++ b/pkg/go.sum @@ -128,8 +128,10 @@ github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvW github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/kr/logfmt v0.0.0-20140226030751-b84e30acd515/go.mod h1:+0opPa2QZZtGFBFZlji/RkVcI2GknAs/DXo4wKdlNEc= +github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= +github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/magiconair/properties v1.8.1/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ= github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= @@ -191,8 +193,9 @@ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= -github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4= github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= +github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= @@ -338,6 +341,7 @@ google.golang.org/protobuf v1.26.0 h1:bxAC2xTBsZGibn2RTntX0oH50xLsqy1OxA9tTL3p/l google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/ini.v1 v1.51.0/go.mod h1:pNLf8WUiyNEtQjuu5G5vTm06TEv9tsIgeAvK8hOrP4k= @@ -348,6 +352,8 @@ gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= diff --git a/pkg/proxy/server_test.go b/pkg/proxy/server_test.go index c634055e6..26cd157b1 100644 --- a/pkg/proxy/server_test.go +++ b/pkg/proxy/server_test.go @@ -30,21 +30,13 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "go.etcd.io/etcd/client/pkg/v3/transport" + "go.uber.org/zap/zaptest" "go.uber.org/zap" ) -// enable DebugLevel -var testLogger = zap.NewExample() - -var testTLSInfo = transport.TLSInfo{ - KeyFile: "../../tests/fixtures/server.key.insecure", - CertFile: "../../tests/fixtures/server.crt", - TrustedCAFile: "../../tests/fixtures/ca.crt", - ClientCertAuth: true, -} - func TestServer_Unix_Insecure(t *testing.T) { testServer(t, "unix", false, false) } func TestServer_TCP_Insecure(t *testing.T) { testServer(t, "tcp", false, false) } func TestServer_Unix_Secure(t *testing.T) { testServer(t, "unix", true, false) } @@ -55,6 +47,7 @@ func TestServer_Unix_Secure_DelayTx(t *testing.T) { testServer(t, "unix", true func TestServer_TCP_Secure_DelayTx(t *testing.T) { testServer(t, "tcp", true, true) } func testServer(t *testing.T, scheme string, secure bool, delayTx bool) { + lg := zaptest.NewLogger(t) srcAddr, dstAddr := newUnixAddr(), newUnixAddr() if scheme == "tcp" { ln1, ln2 := listen(t, "tcp", "localhost:0", transport.TLSInfo{}), listen(t, "tcp", "localhost:0", transport.TLSInfo{}) @@ -67,20 +60,17 @@ func testServer(t *testing.T, scheme string, secure bool, delayTx bool) { os.RemoveAll(dstAddr) }() } - tlsInfo := testTLSInfo - if !secure { - tlsInfo = transport.TLSInfo{} - } + tlsInfo := createTLSInfo(lg, secure) ln := listen(t, scheme, dstAddr, tlsInfo) defer ln.Close() cfg := ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, } if secure { - cfg.TLSInfo = testTLSInfo + cfg.TLSInfo = tlsInfo } p := NewServer(cfg) <-p.Ready() @@ -167,29 +157,40 @@ func testServer(t *testing.T, scheme string, secure bool, delayTx bool) { } } +func createTLSInfo(lg *zap.Logger, secure bool) transport.TLSInfo { + if secure { + return transport.TLSInfo{ + KeyFile: "../../tests/fixtures/server.key.insecure", + CertFile: "../../tests/fixtures/server.crt", + TrustedCAFile: "../../tests/fixtures/ca.crt", + ClientCertAuth: true, + Logger: lg, + } + } + return transport.TLSInfo{Logger: lg} +} + func TestServer_Unix_Insecure_DelayAccept(t *testing.T) { testServerDelayAccept(t, false) } func TestServer_Unix_Secure_DelayAccept(t *testing.T) { testServerDelayAccept(t, true) } func testServerDelayAccept(t *testing.T, secure bool) { + lg := zaptest.NewLogger(t) srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { os.RemoveAll(srcAddr) os.RemoveAll(dstAddr) }() - tlsInfo := testTLSInfo - if !secure { - tlsInfo = transport.TLSInfo{} - } + tlsInfo := createTLSInfo(lg, secure) scheme := "unix" ln := listen(t, scheme, dstAddr, tlsInfo) defer ln.Close() cfg := ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, } if secure { - cfg.TLSInfo = testTLSInfo + cfg.TLSInfo = tlsInfo } p := NewServer(cfg) <-p.Ready() @@ -227,6 +228,7 @@ func testServerDelayAccept(t *testing.T, secure bool) { } func TestServer_PauseTx(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -237,7 +239,7 @@ func TestServer_PauseTx(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -273,6 +275,7 @@ func TestServer_PauseTx(t *testing.T) { } func TestServer_ModifyTx_corrupt(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -283,7 +286,7 @@ func TestServer_ModifyTx_corrupt(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -308,6 +311,7 @@ func TestServer_ModifyTx_corrupt(t *testing.T) { } func TestServer_ModifyTx_packet_loss(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -318,7 +322,7 @@ func TestServer_ModifyTx_packet_loss(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -344,6 +348,7 @@ func TestServer_ModifyTx_packet_loss(t *testing.T) { } func TestServer_BlackholeTx(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -354,7 +359,7 @@ func TestServer_BlackholeTx(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -394,6 +399,7 @@ func TestServer_BlackholeTx(t *testing.T) { } func TestServer_Shutdown(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -404,7 +410,7 @@ func TestServer_Shutdown(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -423,6 +429,7 @@ func TestServer_Shutdown(t *testing.T) { } func TestServer_ShutdownListener(t *testing.T) { + lg := zaptest.NewLogger(t) scheme := "unix" srcAddr, dstAddr := newUnixAddr(), newUnixAddr() defer func() { @@ -434,7 +441,7 @@ func TestServer_ShutdownListener(t *testing.T) { defer ln.Close() p := NewServer(ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, }) @@ -460,6 +467,7 @@ func TestServerHTTP_Secure_DelayTx(t *testing.T) { testServerHTTP(t, true, tru func TestServerHTTP_Insecure_DelayRx(t *testing.T) { testServerHTTP(t, false, false) } func TestServerHTTP_Secure_DelayRx(t *testing.T) { testServerHTTP(t, true, false) } func testServerHTTP(t *testing.T, secure, delayTx bool) { + lg := zaptest.NewLogger(t) scheme := "tcp" ln1, ln2 := listen(t, scheme, "localhost:0", transport.TLSInfo{}), listen(t, scheme, "localhost:0", transport.TLSInfo{}) srcAddr, dstAddr := ln1.Addr().String(), ln2.Addr().String() @@ -476,10 +484,10 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { t.Fatal(err) } }) + tlsInfo := createTLSInfo(lg, secure) var tlsConfig *tls.Config - var err error if secure { - tlsConfig, err = testTLSInfo.ServerConfig() + _, err := tlsInfo.ServerConfig() if err != nil { t.Fatal(err) } @@ -501,18 +509,19 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { if !secure { srv.ListenAndServe() } else { - srv.ListenAndServeTLS(testTLSInfo.CertFile, testTLSInfo.KeyFile) + srv.ListenAndServeTLS(tlsInfo.CertFile, tlsInfo.KeyFile) } + defer srv.Close() }() time.Sleep(200 * time.Millisecond) cfg := ServerConfig{ - Logger: testLogger, + Logger: lg, From: url.URL{Scheme: scheme, Host: srcAddr}, To: url.URL{Scheme: scheme, Host: dstAddr}, } if secure { - cfg.TLSInfo = testTLSInfo + cfg.TLSInfo = tlsInfo } p := NewServer(cfg) <-p.Ready() @@ -520,21 +529,18 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { data := "Hello World!" - now := time.Now() var resp *http.Response + var err error + now := time.Now() if secure { - tp, terr := transport.NewTransport(testTLSInfo, 3*time.Second) - if terr != nil { - t.Fatal(terr) - } + tp, terr := transport.NewTransport(tlsInfo, 3*time.Second) + assert.NoError(t, terr) cli := &http.Client{Transport: tp} resp, err = cli.Post("https://"+srcAddr+"/hello", "", strings.NewReader(data)) } else { resp, err = http.Post("http://"+srcAddr+"/hello", "", strings.NewReader(data)) } - if err != nil { - t.Fatal(err) - } + assert.NoError(t, err) d, err := ioutil.ReadAll(resp.Body) if err != nil { t.Fatal(err) @@ -559,7 +565,7 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { now = time.Now() if secure { - tp, terr := transport.NewTransport(testTLSInfo, 3*time.Second) + tp, terr := transport.NewTransport(tlsInfo, 3*time.Second) if terr != nil { t.Fatal(terr) } From d8550deb7f1f72d632f4eaeb10149abaca3db79d Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 14 May 2021 10:20:38 +0200 Subject: [PATCH 5/5] Fix pkg/proxy tests such that they don't leek goroutines and do close transports. --- pkg/proxy/server.go | 3 +++ pkg/proxy/server_test.go | 18 +++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/server.go b/pkg/proxy/server.go index 480a9492b..9a7b105f9 100644 --- a/pkg/proxy/server.go +++ b/pkg/proxy/server.go @@ -401,13 +401,16 @@ func (s *server) listenAndServe() { continue } + s.closeWg.Add(2) go func() { + defer s.closeWg.Done() // read incoming bytes from listener, dispatch to outgoing connection s.transmit(out, in) out.Close() in.Close() }() go func() { + defer s.closeWg.Done() // read response from outgoing connection, write back to listener s.receive(in, out) in.Close() diff --git a/pkg/proxy/server_test.go b/pkg/proxy/server_test.go index 26cd157b1..686a8c362 100644 --- a/pkg/proxy/server_test.go +++ b/pkg/proxy/server_test.go @@ -477,6 +477,7 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { mux := http.NewServeMux() mux.HandleFunc("/hello", func(w http.ResponseWriter, req *http.Request) { d, err := ioutil.ReadAll(req.Body) + req.Body.Close() if err != nil { t.Fatal(err) } @@ -505,13 +506,12 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { <-donec }() go func() { - defer close(donec) if !secure { srv.ListenAndServe() } else { srv.ListenAndServeTLS(tlsInfo.CertFile, tlsInfo.KeyFile) } - defer srv.Close() + defer close(donec) }() time.Sleep(200 * time.Millisecond) @@ -525,7 +525,11 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { } p := NewServer(cfg) <-p.Ready() - defer p.Close() + defer func() { + lg.Info("closing Proxy server...") + p.Close() + lg.Info("closed Proxy server.") + }() data := "Hello World!" @@ -537,14 +541,18 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { assert.NoError(t, terr) cli := &http.Client{Transport: tp} resp, err = cli.Post("https://"+srcAddr+"/hello", "", strings.NewReader(data)) + defer cli.CloseIdleConnections() + defer tp.CloseIdleConnections() } else { resp, err = http.Post("http://"+srcAddr+"/hello", "", strings.NewReader(data)) + defer http.DefaultClient.CloseIdleConnections() } assert.NoError(t, err) d, err := ioutil.ReadAll(resp.Body) if err != nil { t.Fatal(err) } + resp.Body.Close() took1 := time.Since(now) t.Logf("took %v with no latency", took1) @@ -571,8 +579,11 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { } cli := &http.Client{Transport: tp} resp, err = cli.Post("https://"+srcAddr+"/hello", "", strings.NewReader(data)) + defer cli.CloseIdleConnections() + defer tp.CloseIdleConnections() } else { resp, err = http.Post("http://"+srcAddr+"/hello", "", strings.NewReader(data)) + defer http.DefaultClient.CloseIdleConnections() } if err != nil { t.Fatal(err) @@ -581,6 +592,7 @@ func testServerHTTP(t *testing.T, secure, delayTx bool) { if err != nil { t.Fatal(err) } + resp.Body.Close() took2 := time.Since(now) t.Logf("took %v with latency %v±%v", took2, lat, rv)