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 85a9842b0d8..e442c3c92e8 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 cd8626c7fc6..992c773eaac 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 7a5740d1498..47ac18f9692 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 cc47121fa21..f30d7db23bf 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 55f3494eb9a..aecd56ce3b3 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 b702404967e..e753a967f01 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 47ac18f9692..b96a10037fd 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 0a7b0ad163e..00657648ece 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 fefe20c1c02..afb2b9e1923 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 e88c8cb9379..a0c5c50c2b1 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 c634055e66d..26cd157b153 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 480a9492b4a..9a7b105f9a7 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 26cd157b153..686a8c362b3 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)