From 7b3f4703dc0b6a3e1b1fb565855635b4abd7557c Mon Sep 17 00:00:00 2001 From: aermakov-zalando Date: Mon, 11 Mar 2019 18:08:21 +0100 Subject: [PATCH] shortcheck: run with -race (#970) * shortcheck: run with -race Signed-off-by: Alexey Ermakov * clone routes in the file watch data client as it is storing the same instances that it passes to the routing Signed-off-by: Arpad Ryszka * add missing read lock and wait for the refresh goroutine to finish before test assertion Signed-off-by: Arpad Ryszka * fix race in test code with legacy table tests Signed-off-by: Arpad Ryszka * fix tests for audit logging during upgrade requests and connection checking during roundtripper retry Signed-off-by: Arpad Ryszka * fix access log time sensitivity during testing with race Signed-off-by: Arpad Ryszka * update timeout values for breaker tests for race testing Signed-off-by: Arpad Ryszka * remove some time sensitive tests from under -race testing Signed-off-by: Arpad Ryszka * fix race condition in test data client Signed-off-by: Arpad Ryszka * adjust or disable some remaining time sensitive tests Signed-off-by: Arpad Ryszka * exclude test writing to global var from race detection tests Signed-off-by: Arpad Ryszka * split more time sensitive tests from race detection Signed-off-by: Arpad Ryszka * split -race testing from shortcheck because some of the tests are not executed now with -race enabled Signed-off-by: Arpad Ryszka --- Makefile | 14 ++- circuit/breaker_test.go | 2 +- circuit/registry_test.go | 153 +---------------------- circuit/registryrace_test.go | 157 ++++++++++++++++++++++++ dataclients/routestring/example_test.go | 2 + eskip/eskip.go | 67 ++++++++++ eskip/eskip_test.go | 24 ++++ eskipfile/watch.go | 17 ++- eskipfile/watch_test.go | 2 +- etcd/etcd_test.go | 54 -------- etcd/etcdtime_test.go | 50 ++++++++ filters/auth/encrypter.go | 7 ++ filters/auth/encrypter_test.go | 6 +- filters/builtin/compress_test.go | 96 ++++++++------- filters/serve/serve_test.go | 108 ++++++++-------- metrics/codahalenorace_test.go | 33 +++++ metrics/codehale_test.go | 22 ---- plugins_test.go | 32 +++++ proxy/breaker_test.go | 2 + proxy/proxy.go | 2 + proxy/proxy_test.go | 35 ++++-- proxy/upgrade.go | 8 ++ proxy/upgrade_test.go | 9 +- ratelimit/cluster_test.go | 2 + routing/datasource.go | 41 ++++--- routing/example_test.go | 7 +- routing/testdataclient/dataclient.go | 46 +++---- skipper_test.go | 19 --- 28 files changed, 602 insertions(+), 415 deletions(-) create mode 100644 circuit/registryrace_test.go create mode 100644 etcd/etcdtime_test.go create mode 100644 metrics/codahalenorace_test.go create mode 100644 plugins_test.go diff --git a/Makefile b/Makefile index 8bc13e8241..d358833f96 100644 --- a/Makefile +++ b/Makefile @@ -56,6 +56,14 @@ shortcheck: build check-plugins # for p in $(PACKAGES); do GO111MODULE=on go test -test.short -run ^Test $$p || break -1; done +check-race: build + # go test -race -test.short -run ^Test $(PACKAGES) + # + # due to vendoring and how go test ./... is not the same as go test ./a/... ./b/... + # probably can be reverted once etcd is fully mocked away for tests + # + for p in $(PACKAGES); do GO111MODULE=on go test -race -test.short -run ^Test $$p || break -1; done + check-plugins: $(TEST_PLUGINS) GO111MODULE=on go test -run LoadPlugins @@ -106,9 +114,9 @@ fmt: $(SOURCES) check-fmt: $(SOURCES) @if [ "$$(gofmt -d $(SOURCES))" != "" ]; then false; else true; fi -precommit: fmt build vet staticcheck shortcheck +precommit: fmt build vet staticcheck check-race shortcheck -check-precommit: check-fmt build vet staticcheck shortcheck +check-precommit: check-fmt build vet staticcheck check-race shortcheck .coverprofile-all: $(SOURCES) $(TEST_PLUGINS) # go list -f \ @@ -169,5 +177,5 @@ else ifeq ($(TRAVIS_BRANCH)_$(TRAVIS_PULL_REQUEST), master_false) else ifeq ($(TRAVIS_BRANCH), master) make deps check-precommit else - make deps shortcheck check-plugins + make deps check-race shortcheck check-plugins endif diff --git a/circuit/breaker_test.go b/circuit/breaker_test.go index f8bc2693d0..fc7f32444f 100644 --- a/circuit/breaker_test.go +++ b/circuit/breaker_test.go @@ -50,7 +50,7 @@ func TestConsecutiveFailures(t *testing.T) { Type: ConsecutiveFailures, Failures: 3, HalfOpenRequests: 3, - Timeout: 3 * time.Millisecond, + Timeout: 15 * time.Millisecond, } waitTimeout := func() { diff --git a/circuit/registry_test.go b/circuit/registry_test.go index d2888aa335..1fdb1e0e03 100644 --- a/circuit/registry_test.go +++ b/circuit/registry_test.go @@ -1,7 +1,8 @@ +/* +build !race */ + package circuit import ( - "math/rand" "testing" "time" ) @@ -319,153 +320,3 @@ func TestIndividualIdle(t *testing.T) { shouldBeClosed(t, "foo") }) } - -func TestRegistryFuzzy(t *testing.T) { - if testing.Short() { - t.Skip() - } - - const ( - hostCount = 1200 - customSettingsCount = 120 - concurrentRequests = 2048 - requestDurationMean = 120 * time.Microsecond - requestDurationDeviation = 60 * time.Microsecond - idleTTL = time.Second - duration = 3 * time.Second - ) - - genHost := func() string { - const ( - minHostLength = 12 - maxHostLength = 36 - ) - - h := make([]byte, minHostLength+rand.Intn(maxHostLength-minHostLength)) - for i := range h { - h[i] = 'a' + byte(rand.Intn(int('z'+1-'a'))) - } - - return string(h) - } - - hosts := make([]string, hostCount) - for i := range hosts { - hosts[i] = genHost() - } - - settings := []BreakerSettings{{IdleTTL: idleTTL}} - - settingsMap := make(map[string]BreakerSettings) - for _, h := range hosts { - s := BreakerSettings{ - Host: h, - Type: ConsecutiveFailures, - Failures: 5, - IdleTTL: idleTTL, - } - settings = append(settings, s) - settingsMap[h] = s - } - - r := NewRegistry(settings...) - - // the first customSettingsCount hosts can have corresponding custom settings - customSettings := make(map[string]BreakerSettings) - for _, h := range hosts[:customSettingsCount] { - s := settingsMap[h] - s.Failures = 15 - s.IdleTTL = idleTTL - customSettings[h] = s - } - - var syncToken struct{} - sync := make(chan struct{}, 1) - sync <- syncToken - synced := func(f func()) { - t := <-sync - f() - sync <- t - } - - replaceHostSettings := func(settings map[string]BreakerSettings, old, nu string) { - if s, ok := settings[old]; ok { - delete(settings, old) - s.Host = nu - settings[nu] = s - } - } - - replaceHost := func() { - synced(func() { - i := rand.Intn(len(hosts)) - old := hosts[i] - nu := genHost() - hosts[i] = nu - replaceHostSettings(settingsMap, old, nu) - replaceHostSettings(customSettings, old, nu) - }) - } - - stop := make(chan struct{}) - - getSettings := func(useCustom bool) BreakerSettings { - var s BreakerSettings - synced(func() { - if useCustom { - s = customSettings[hosts[rand.Intn(customSettingsCount)]] - return - } - - s = settingsMap[hosts[rand.Intn(hostCount)]] - }) - - return s - } - - requestDuration := func() time.Duration { - mean := float64(requestDurationMean) - deviation := float64(requestDurationDeviation) - return time.Duration(rand.NormFloat64()*deviation + mean) - } - - makeRequest := func(useCustom bool) { - s := getSettings(useCustom) - b := r.Get(s) - if b.settings != s { - t.Error("invalid breaker received") - t.Log(b.settings, s) - close(stop) - } - - time.Sleep(requestDuration()) - } - - runAgent := func() { - for { - select { - case <-stop: - return - default: - } - - // 1% percent chance for getting a host replaced: - if rand.Intn(100) == 0 { - replaceHost() - } - - // 3% percent of the requests is custom: - makeRequest(rand.Intn(100) < 3) - } - } - - time.AfterFunc(duration, func() { - close(stop) - }) - - for i := 0; i < concurrentRequests; i++ { - go runAgent() - } - - <-stop -} diff --git a/circuit/registryrace_test.go b/circuit/registryrace_test.go new file mode 100644 index 0000000000..835b06530d --- /dev/null +++ b/circuit/registryrace_test.go @@ -0,0 +1,157 @@ +package circuit + +import ( + "math/rand" + "testing" + "time" +) + +func TestRegistryFuzzy(t *testing.T) { + if testing.Short() { + t.Skip() + } + + const ( + hostCount = 1200 + customSettingsCount = 120 + concurrentRequests = 2048 + requestDurationMean = 120 * time.Microsecond + requestDurationDeviation = 60 * time.Microsecond + idleTTL = time.Second + duration = 3 * time.Second + ) + + genHost := func() string { + const ( + minHostLength = 12 + maxHostLength = 36 + ) + + h := make([]byte, minHostLength+rand.Intn(maxHostLength-minHostLength)) + for i := range h { + h[i] = 'a' + byte(rand.Intn(int('z'+1-'a'))) + } + + return string(h) + } + + hosts := make([]string, hostCount) + for i := range hosts { + hosts[i] = genHost() + } + + settings := []BreakerSettings{{IdleTTL: idleTTL}} + + settingsMap := make(map[string]BreakerSettings) + for _, h := range hosts { + s := BreakerSettings{ + Host: h, + Type: ConsecutiveFailures, + Failures: 5, + IdleTTL: idleTTL, + } + settings = append(settings, s) + settingsMap[h] = s + } + + r := NewRegistry(settings...) + + // the first customSettingsCount hosts can have corresponding custom settings + customSettings := make(map[string]BreakerSettings) + for _, h := range hosts[:customSettingsCount] { + s := settingsMap[h] + s.Failures = 15 + s.IdleTTL = idleTTL + customSettings[h] = s + } + + var syncToken struct{} + sync := make(chan struct{}, 1) + sync <- syncToken + synced := func(f func()) { + t := <-sync + f() + sync <- t + } + + replaceHostSettings := func(settings map[string]BreakerSettings, old, nu string) { + if s, ok := settings[old]; ok { + delete(settings, old) + s.Host = nu + settings[nu] = s + } + } + + replaceHost := func() { + synced(func() { + i := rand.Intn(len(hosts)) + old := hosts[i] + nu := genHost() + hosts[i] = nu + replaceHostSettings(settingsMap, old, nu) + replaceHostSettings(customSettings, old, nu) + }) + } + + stop := make(chan struct{}) + + getSettings := func(useCustom bool) BreakerSettings { + var s BreakerSettings + synced(func() { + if useCustom { + s = customSettings[hosts[rand.Intn(customSettingsCount)]] + return + } + + s = settingsMap[hosts[rand.Intn(hostCount)]] + }) + + return s + } + + requestDuration := func() time.Duration { + mean := float64(requestDurationMean) + deviation := float64(requestDurationDeviation) + return time.Duration(rand.NormFloat64()*deviation + mean) + } + + makeRequest := func(useCustom bool) { + s := getSettings(useCustom) + b := r.Get(s) + if b.settings != s { + t.Error("invalid breaker received") + t.Log(b.settings, s) + close(stop) + } + + time.Sleep(requestDuration()) + } + + runAgent := func() { + for { + select { + case <-stop: + return + default: + } + + // 1% percent chance for getting a host replaced: + if rand.Intn(100) == 0 { + replaceHost() + } + + // 3% percent of the requests is custom: + makeRequest(rand.Intn(100) < 3) + } + } + + time.AfterFunc(duration, func() { + close(stop) + }) + + for i := 0; i < concurrentRequests; i++ { + go runAgent() + } + + <-stop +} diff --git a/dataclients/routestring/example_test.go b/dataclients/routestring/example_test.go index 8d3edd48aa..f0acfff4f3 100644 --- a/dataclients/routestring/example_test.go +++ b/dataclients/routestring/example_test.go @@ -1,3 +1,5 @@ +// +build !race + package routestring_test import ( diff --git a/eskip/eskip.go b/eskip/eskip.go index b47a7f36cf..9cde3e0333 100644 --- a/eskip/eskip.go +++ b/eskip/eskip.go @@ -156,6 +156,73 @@ type RouteInfo struct { ParseError error } +// Copy copies a filter to a new filter instance. The argument values are copied in a shallow way. +func (f *Filter) Copy() *Filter { + c := *f + c.Args = make([]interface{}, len(f.Args)) + copy(c.Args, f.Args) + return &c +} + +// Copy copies a predicate to a new filter instance. The argument values are copied in a shallow way. +func (p *Predicate) Copy() *Predicate { + c := *p + c.Args = make([]interface{}, len(p.Args)) + copy(c.Args, p.Args) + return &c +} + +// Copy copies a route to a new route instance with all the slice and map fields copied deep. +func (r *Route) Copy() *Route { + c := *r + + if len(r.HostRegexps) > 0 { + c.HostRegexps = make([]string, len(r.HostRegexps)) + copy(c.HostRegexps, r.HostRegexps) + } + + if len(r.PathRegexps) > 0 { + c.PathRegexps = make([]string, len(r.PathRegexps)) + copy(c.PathRegexps, r.PathRegexps) + } + + if len(r.Headers) > 0 { + c.Headers = make(map[string]string) + for k, v := range r.Headers { + c.Headers[k] = v + } + } + + if len(r.HeaderRegexps) > 0 { + c.HeaderRegexps = make(map[string][]string) + for k, vs := range r.HeaderRegexps { + c.HeaderRegexps[k] = make([]string, len(vs)) + copy(c.HeaderRegexps[k], vs) + } + } + + if len(r.Predicates) > 0 { + c.Predicates = make([]*Predicate, len(r.Predicates)) + for i, p := range r.Predicates { + c.Predicates[i] = p.Copy() + } + } + + if len(r.Filters) > 0 { + c.Filters = make([]*Filter, len(r.Filters)) + for i, p := range r.Filters { + c.Filters[i] = p.Copy() + } + } + + if len(r.LBEndpoints) > 0 { + c.LBEndpoints = make([]string, len(r.LBEndpoints)) + copy(c.LBEndpoints, r.LBEndpoints) + } + + return &c +} + func (t BackendType) String() string { switch t { case NetworkBackend: diff --git a/eskip/eskip_test.go b/eskip/eskip_test.go index 3cc3b20415..bbc5703a3c 100644 --- a/eskip/eskip_test.go +++ b/eskip/eskip_test.go @@ -442,3 +442,27 @@ func TestPredicateParsing(t *testing.T) { }) } } + +func TestClone(t *testing.T) { + r := &Route{ + Id: "foo", + Path: "/bar", + HostRegexps: []string{"[.]example[.]org$", "^www[.]"}, + PathRegexps: []string{"^/", "bar$"}, + Method: "GET", + Headers: map[string]string{"X-Foo": "bar"}, + HeaderRegexps: map[string][]string{"X-Bar": {"baz", "qux"}}, + Predicates: []*Predicate{{Name: "Foo", Args: []interface{}{"bar", "baz"}}}, + Filters: []*Filter{{Name: "foo", Args: []interface{}{42, 84}}}, + Backend: "https://www2.example.org", + } + + c := r.Copy() + if c == r { + t.Error("routes are of the same instance") + } + + if !reflect.DeepEqual(c, r) { + t.Error("failed to clone all the fields") + } +} diff --git a/eskipfile/watch.go b/eskipfile/watch.go index 6ac19d9e76..e1246087a8 100644 --- a/eskipfile/watch.go +++ b/eskipfile/watch.go @@ -79,6 +79,19 @@ func (c *WatchClient) deleteAllListIDs() []string { return ids } +func cloneRoutes(r []*eskip.Route) []*eskip.Route { + if len(r) == 0 { + return nil + } + + c := make([]*eskip.Route, len(r)) + for i, ri := range r { + c[i] = ri.Copy() + } + + return c +} + func (c *WatchClient) loadAll() watchResponse { content, err := ioutil.ReadFile(c.fileName) if err != nil { @@ -91,7 +104,7 @@ func (c *WatchClient) loadAll() watchResponse { } c.storeRoutes(r) - return watchResponse{routes: r} + return watchResponse{routes: cloneRoutes(r)} } func (c *WatchClient) loadUpdates() watchResponse { @@ -111,7 +124,7 @@ func (c *WatchClient) loadUpdates() watchResponse { } upsert, del := c.diffStoreRoutes(r) - return watchResponse{routes: upsert, deletedIDs: del} + return watchResponse{routes: cloneRoutes(upsert), deletedIDs: del} } func (c *WatchClient) watch() { diff --git a/eskipfile/watch_test.go b/eskipfile/watch_test.go index 75e736c34e..e756fd3b40 100644 --- a/eskipfile/watch_test.go +++ b/eskipfile/watch_test.go @@ -73,7 +73,7 @@ func initWatchTest(t *testing.T) *watchTest { Log: l, FilterRegistry: builtin.MakeRegistry(), DataClients: []routing.DataClient{f}, - PollTimeout: 6 * time.Millisecond, + PollTimeout: 15 * time.Millisecond, }), } } diff --git a/etcd/etcd_test.go b/etcd/etcd_test.go index 5e9277186a..73a4346971 100644 --- a/etcd/etcd_test.go +++ b/etcd/etcd_test.go @@ -1,30 +1,14 @@ -// Copyright 2015 Zalando SE -// -// Licensed 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. - package etcd import ( "encoding/base64" "errors" "log" - "net" "net/http" "net/http/httptest" "os" "strings" "testing" - "time" "github.com/zalando/skipper/eskip" "github.com/zalando/skipper/etcd/etcdtest" @@ -204,44 +188,6 @@ func TestFailedEndpointsRotation(t *testing.T) { } } -func TestTimedoutEndpointsRotation(t *testing.T) { - s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - })) - - s.Close() - - s2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(time.Duration(5 * time.Millisecond)) - })) - defer s2.Close() - - c, err := New(Options{Endpoints: []string{s.URL, s2.URL, "neverreached"}, Prefix: "/skippertest-invalid"}) - c.client.Timeout = time.Duration(1 * time.Millisecond) - - if err != nil { - t.Error(err) - return - } - - _, err = c.LoadAll() - - if err == nil { - t.Error("failed to fail") - } - - nerr, ok := err.(net.Error) - - if !ok || !nerr.Timeout() { - t.Error("timeout error expected") - } - - expectedEndpoints := []string{s2.URL, "neverreached", s.URL} - - if strings.Join(c.endpoints, ";") != strings.Join(expectedEndpoints, ";") { - t.Error("wrong endpoints rotation") - } -} - func TestValidatesDocument(t *testing.T) { s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte(`{"value": "different json"}`)) diff --git a/etcd/etcdtime_test.go b/etcd/etcdtime_test.go new file mode 100644 index 0000000000..322b57038a --- /dev/null +++ b/etcd/etcdtime_test.go @@ -0,0 +1,50 @@ +// +build !race + +package etcd + +import ( + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestTimedoutEndpointsRotation(t *testing.T) { + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + })) + + s.Close() + + s2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(time.Duration(5 * time.Millisecond)) + })) + defer s2.Close() + + c, err := New(Options{Endpoints: []string{s.URL, s2.URL, "neverreached"}, Prefix: "/skippertest-invalid"}) + c.client.Timeout = time.Duration(1 * time.Millisecond) + + if err != nil { + t.Error(err) + return + } + + _, err = c.LoadAll() + + if err == nil { + t.Error("failed to fail") + } + + nerr, ok := err.(net.Error) + + if !ok || !nerr.Timeout() { + t.Error("timeout error expected") + } + + expectedEndpoints := []string{s2.URL, "neverreached", s.URL} + + if strings.Join(c.endpoints, ";") != strings.Join(expectedEndpoints, ";") { + t.Error("wrong endpoints rotation") + } +} diff --git a/filters/auth/encrypter.go b/filters/auth/encrypter.go index 45bafd062e..b6c09d8f18 100644 --- a/filters/auth/encrypter.go +++ b/filters/auth/encrypter.go @@ -52,6 +52,7 @@ type encrypter struct { mux sync.RWMutex sSource secretSource closer chan struct{} + closedHook chan struct{} } func newEncrypter(secretsFile string) (*encrypter, error) { @@ -67,6 +68,8 @@ func newEncrypter(secretsFile string) (*encrypter, error) { } func (c *encrypter) createNonce() ([]byte, error) { + c.mux.RLock() + defer c.mux.RUnlock() if len(c.cipherSuites) > 0 { nonce := make([]byte, c.cipherSuites[0].NonceSize()) if _, err := io.ReadFull(crand.Reader, nonce); err != nil { @@ -148,6 +151,10 @@ func (c *encrypter) runCipherRefresher(refreshInterval time.Duration) error { for { select { case <-c.closer: + if c.closedHook != nil { + close(c.closedHook) + } + return case <-ticker.C: log.Debug("started refresh of ciphers") diff --git a/filters/auth/encrypter_test.go b/filters/auth/encrypter_test.go index 3caf553ff0..f825756d0a 100644 --- a/filters/auth/encrypter_test.go +++ b/filters/auth/encrypter_test.go @@ -39,11 +39,13 @@ func TestEncryptDecrypt(t *testing.T) { func TestCipherRefreshing(t *testing.T) { sSource := &testingSecretSource{secretKey: "abc"} enc := &encrypter{ - sSource: sSource, - closer: make(chan struct{}), + sSource: sSource, + closer: make(chan struct{}), + closedHook: make(chan struct{}), } enc.runCipherRefresher(1 * time.Second) time.Sleep(4 * time.Second) enc.close() + <-enc.closedHook assert.True(t, sSource.getCount >= 3, "secret fetched less than 3 time in 15 seconds") } diff --git a/filters/builtin/compress_test.go b/filters/builtin/compress_test.go index 8edffe9b9c..49e382e889 100644 --- a/filters/builtin/compress_test.go +++ b/filters/builtin/compress_test.go @@ -410,63 +410,65 @@ func TestCompress(t *testing.T) { "Content-Encoding": []string{"gzip"}, "Vary": []string{"Accept-Encoding"}}, }} { - s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - setHeaders(w.Header(), ti.responseHeader) - count := 0 - for count < ti.contentLength { - wl := writeLength - if count+wl > len(testContent) { - wl = len(testContent) - count + t.Run(ti.msg, func(t *testing.T) { + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + setHeaders(w.Header(), ti.responseHeader) + count := 0 + for count < ti.contentLength { + wl := writeLength + if count+wl > len(testContent) { + wl = len(testContent) - count + } + + w.Write(testContent[count : count+wl]) + count += wl + time.Sleep(writeDelay) } + })) + defer s.Close() - w.Write(testContent[count : count+wl]) - count += wl - time.Sleep(writeDelay) - } - })) - defer s.Close() - - p := proxytest.New(MakeRegistry(), &eskip.Route{ - Filters: []*eskip.Filter{{Name: CompressName, Args: ti.compressArgs}}, - Backend: s.URL}) - defer p.Close() + p := proxytest.New(MakeRegistry(), &eskip.Route{ + Filters: []*eskip.Filter{{Name: CompressName, Args: ti.compressArgs}}, + Backend: s.URL}) + defer p.Close() - req, err := http.NewRequest("GET", p.URL, nil) - if err != nil { - t.Error(ti.msg, err) - continue - } + req, err := http.NewRequest("GET", p.URL, nil) + if err != nil { + t.Error(err) + return + } - req.Header.Set("Accept-Encoding", ti.acceptEncoding) + req.Header.Set("Accept-Encoding", ti.acceptEncoding) - rsp, err := http.DefaultTransport.RoundTrip(req) - if err != nil { - t.Error(ti.msg, err) - continue - } + rsp, err := http.DefaultTransport.RoundTrip(req) + if err != nil { + t.Error(err) + return + } - defer rsp.Body.Close() + defer rsp.Body.Close() - rsp.Header.Del("Server") - rsp.Header.Del("X-Powered-By") - rsp.Header.Del("Date") - if rsp.Header.Get("Content-Type") == "application/octet-stream" { - rsp.Header.Del("Content-Type") - } + rsp.Header.Del("Server") + rsp.Header.Del("X-Powered-By") + rsp.Header.Del("Date") + if rsp.Header.Get("Content-Type") == "application/octet-stream" { + rsp.Header.Del("Content-Type") + } - if !compareHeaders(rsp.Header, ti.expectedHeader) { - printHeader(t, ti.expectedHeader, ti.msg, "invalid header", "expected") - printHeader(t, rsp.Header, ti.msg, "invalid header", "got") + if !compareHeaders(rsp.Header, ti.expectedHeader) { + printHeader(t, ti.expectedHeader, "invalid header", "expected") + printHeader(t, rsp.Header, "invalid header", "got") - t.Error(ti.msg, "invalid header") - continue - } + t.Error("invalid header") + return + } - if ok, err := compareBody(rsp, ti.contentLength); err != nil { - t.Error(ti.msg, err) - } else if !ok { - t.Error(ti.msg, "invalid content") - } + if ok, err := compareBody(rsp, ti.contentLength); err != nil { + t.Error(err) + } else if !ok { + t.Error("invalid content") + } + }) } } diff --git a/filters/serve/serve_test.go b/filters/serve/serve_test.go index d5afa85b31..b940d22c13 100644 --- a/filters/serve/serve_test.go +++ b/filters/serve/serve_test.go @@ -13,17 +13,19 @@ import ( const testDelay = 12 * time.Millisecond +type testItem struct { + msg string + sleep time.Duration + status int + body string + ret bool + blockForever bool + expectedTimeout time.Duration + timeout time.Duration +} + func TestBlock(t *testing.T) { - for _, ti := range []struct { - msg string - sleep time.Duration - status int - body string - ret bool - blockForever bool - expectedTimeout time.Duration - timeout time.Duration - }{{ + for _, ti := range []testItem{{ msg: "block forever", expectedTimeout: testDelay, }, { @@ -42,52 +44,54 @@ func TestBlock(t *testing.T) { ret: true, timeout: 9 * testDelay, }} { - done := make(chan struct{}) - quit := make(chan struct{}) - ctx := &filtertest.Context{} - go func() { - ServeHTTP(ctx, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - if ti.sleep > 0 { - time.Sleep(ti.sleep) - } - - if ti.status > 0 { - w.WriteHeader(ti.status) - } - - if ti.body != "" { - w.Write([]byte(ti.body)) - } + t.Run(ti.msg, func(t *testing.T) { + done := make(chan struct{}) + quit := make(chan struct{}) + ctx := &filtertest.Context{} + go func(ti testItem) { + ServeHTTP(ctx, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if ti.sleep > 0 { + time.Sleep(ti.sleep) + } + + if ti.status > 0 { + w.WriteHeader(ti.status) + } + + if ti.body != "" { + w.Write([]byte(ti.body)) + } + + if !ti.ret { + <-quit + } + })) + + close(done) + }(ti) + + var eto, to <-chan time.Time + if ti.expectedTimeout > 0 { + eto = time.After(ti.expectedTimeout) + } + if ti.timeout > 0 { + to = time.After(ti.timeout) + } - if !ti.ret { - <-quit + select { + case <-done: + if ti.blockForever { + t.Error("failed to block") + } else { + close(quit) } - })) - - close(done) - }() - - var eto, to <-chan time.Time - if ti.expectedTimeout > 0 { - eto = time.After(ti.expectedTimeout) - } - if ti.timeout > 0 { - to = time.After(ti.timeout) - } - - select { - case <-done: - if ti.blockForever { - t.Error(ti.msg, "failed to block") - } else { + case <-eto: + close(quit) + case <-to: + t.Error("timeout") close(quit) } - case <-eto: - close(quit) - case <-to: - t.Error(ti.msg, "timeout") - close(quit) - } + }) } } diff --git a/metrics/codahalenorace_test.go b/metrics/codahalenorace_test.go new file mode 100644 index 0000000000..a1cc2ea8f5 --- /dev/null +++ b/metrics/codahalenorace_test.go @@ -0,0 +1,33 @@ +/* +build !race */ + +package metrics + +import ( + "bytes" + "encoding/json" + "reflect" + "testing" + + "github.com/rcrowley/go-metrics" +) + +// not running this test with race detection because it writes to unsynchronized global variables in an imported +// package +func TestCodaHaleMetricSerialization(t *testing.T) { + metrics.UseNilMetrics = true + defer func() { metrics.UseNilMetrics = false }() + + for _, st := range serializationTests { + m := reflect.ValueOf(st.i).Call(nil)[0].Interface() + metrics := skipperMetrics{"test": m} + var buf bytes.Buffer + json.NewEncoder(&buf).Encode(metrics) + var got serializationResult + json.Unmarshal(buf.Bytes(), &got) + + if !reflect.DeepEqual(got, st.expected) { + t.Errorf("Got wrong serialization result. Expected '%v' but got '%v'", st.expected, got) + } + + } +} diff --git a/metrics/codehale_test.go b/metrics/codehale_test.go index de77114d3a..8f891efc54 100644 --- a/metrics/codehale_test.go +++ b/metrics/codehale_test.go @@ -1,11 +1,8 @@ package metrics import ( - "bytes" - "encoding/json" "fmt" "net/http" - "reflect" "testing" "time" @@ -229,25 +226,6 @@ var serializationTests = []serializationTest{ {func() int { return 42 }, serializationResult{"unknown": {"test": {"error": "unknown metrics type int"}}}}, } -func TestCodaHaleMetricSerialization(t *testing.T) { - metrics.UseNilMetrics = true - defer func() { metrics.UseNilMetrics = false }() - - for _, st := range serializationTests { - m := reflect.ValueOf(st.i).Call(nil)[0].Interface() - metrics := skipperMetrics{"test": m} - var buf bytes.Buffer - json.NewEncoder(&buf).Encode(metrics) - var got serializationResult - json.Unmarshal(buf.Bytes(), &got) - - if !reflect.DeepEqual(got, st.expected) { - t.Errorf("Got wrong serialization result. Expected '%v' but got '%v'", st.expected, got) - } - - } -} - type serveMetricsMeasure struct { route, host, method string status int diff --git a/plugins_test.go b/plugins_test.go new file mode 100644 index 0000000000..13c127995e --- /dev/null +++ b/plugins_test.go @@ -0,0 +1,32 @@ +// +build !race + +package skipper + +import "testing" + +func TestLoadPlugins(t *testing.T) { + if testing.Short() { + t.Skip() + } + + o := Options{ + PluginDirs: []string{"./_test_plugins"}, + FilterPlugins: [][]string{{"filter_noop"}}, + } + if err := o.findAndLoadPlugins(); err != nil { + t.Fatalf("Failed to load plugins: %s", err) + } +} + +func TestLoadPluginsFail(t *testing.T) { + if testing.Short() { + t.Skip() + } + + o := Options{ + PluginDirs: []string{"./_test_plugins_fail"}, + } + if err := o.findAndLoadPlugins(); err == nil { + t.Fatalf("did not fail to load plugins: %s", err) + } +} diff --git a/proxy/breaker_test.go b/proxy/breaker_test.go index 93c71ccffc..36cd0cfaa1 100644 --- a/proxy/breaker_test.go +++ b/proxy/breaker_test.go @@ -1,3 +1,5 @@ +// +build !race + package proxy_test import ( diff --git a/proxy/proxy.go b/proxy/proxy.go index 1678312839..e2e278b943 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -275,6 +275,7 @@ type Proxy struct { lb *loadbalancer.LB upgradeAuditLogOut io.Writer upgradeAuditLogErr io.Writer + auditLogHook chan struct{} } // proxyError is used to wrap errors during proxying and to indicate @@ -736,6 +737,7 @@ func (p *Proxy) makeUpgradeRequest(ctx *context, req *http.Request) error { useAuditLog: p.experimentalUpgradeAudit, auditLogOut: p.upgradeAuditLogOut, auditLogErr: p.upgradeAuditLogErr, + auditLogHook: p.auditLogHook, } upgradeProxy.serveHTTP(ctx.responseWriter, req) diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 59665f5255..266c95e0b1 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -62,7 +62,7 @@ type testProxy struct { type listener struct { inner net.Listener - lastConn net.Conn + lastConn chan net.Conn } func (cors *preserveOriginalSpec) Name() string { return "preserveOriginal" } @@ -227,7 +227,12 @@ func (l *listener) Accept() (c net.Conn, err error) { return } - l.lastConn = c + select { + case <-l.lastConn: + default: + } + + l.lastConn <- c return } @@ -1129,22 +1134,29 @@ func TestRoundtripperRetry(t *testing.T) { closeServer = false - if l.lastConn == nil { + var lastConn net.Conn + select { + case lastConn = <-l.lastConn: + default: + } + + if lastConn == nil { t.Error("failed to capture connection") return } - if err := l.lastConn.Close(); err != nil { + if err := lastConn.Close(); err != nil { t.Error(err) return } } - backend := httptest.NewServer(http.HandlerFunc(handler)) + backend := httptest.NewUnstartedServer(http.HandlerFunc(handler)) defer backend.Close() - l = &listener{inner: backend.Listener} + l = &listener{inner: backend.Listener, lastConn: make(chan net.Conn, 1)} backend.Listener = l + backend.Start() tp, err := newTestProxy(fmt.Sprintf(`* -> "%s"`, backend.URL), 0) if err != nil { @@ -1497,7 +1509,8 @@ func TestLogsAccess(t *testing.T) { tp.proxy.ServeHTTP(w, r) output := accessLog.String() - if !strings.Contains(output, fmt.Sprintf(`"%s - -" %d %d "-" "-" 0 - - -`, r.Method, http.StatusTeapot, len(response))) { + println(fmt.Sprintf(`"%s - -" %d %d "-" "-"`, r.Method, http.StatusTeapot, len(response))) + if !strings.Contains(output, fmt.Sprintf(`"%s - -" %d %d "-" "-"`, r.Method, http.StatusTeapot, len(response))) { t.Error("failed to log access", output) } } @@ -1663,7 +1676,7 @@ func TestEnableAccessLogWithFilter(t *testing.T) { tp.proxy.ServeHTTP(w, r) output := buf.String() - if ti.shouldLog != strings.Contains(output, fmt.Sprintf(`"%s - -" %d %d "-" "-" 0 - - -`, r.Method, ti.responseCode, len(response))) { + if ti.shouldLog != strings.Contains(output, fmt.Sprintf(`"%s - -" %d %d "-" "-"`, r.Method, ti.responseCode, len(response))) { t.Error("failed to log access", output) } }) @@ -1709,9 +1722,11 @@ func TestAccessLogOnFailedRequest(t *testing.T) { return } - expected := fmt.Sprintf(`"GET / HTTP/1.1" %d %d "-" "Go-http-client/1.1" 0 %s - -`, http.StatusBadGateway, rsp.ContentLength, proxyURL.Host) - if !strings.Contains(output, expected) { + expected := fmt.Sprintf(`"GET / HTTP/1.1" %d %d "-" "Go-http-client/1.1"`, http.StatusBadGateway, rsp.ContentLength) + if !strings.Contains(output, expected) || !strings.Contains(output, proxyURL.Host) { t.Error("failed to log access", output, expected) + t.Log(output) + t.Log(expected) } } diff --git a/proxy/upgrade.go b/proxy/upgrade.go index a5090130ea..ece5bc7ccd 100644 --- a/proxy/upgrade.go +++ b/proxy/upgrade.go @@ -46,6 +46,7 @@ type upgradeProxy struct { useAuditLog bool auditLogOut io.Writer auditLogErr io.Writer + auditLogHook chan struct{} } // TODO: add user here @@ -162,6 +163,13 @@ func (p *upgradeProxy) serveHTTP(w http.ResponseWriter, req *http.Request) { log.Debugf("Successfully upgraded to protocol %s by user request", getUpgradeRequest(req)) // Wait for goroutine to finish, such that the established connection does not break. wg.Wait() + + if p.useAuditLog { + select { + case p.auditLogHook <- struct{}{}: + default: + } + } } func (p *upgradeProxy) dialBackend(req *http.Request) (net.Conn, error) { diff --git a/proxy/upgrade_test.go b/proxy/upgrade_test.go index 3f75d4f0e2..fcd8cbd72d 100644 --- a/proxy/upgrade_test.go +++ b/proxy/upgrade_test.go @@ -177,11 +177,13 @@ func TestAuditLogging(t *testing.T) { t.Fatal(err) } + auditHook := make(chan struct{}, 1) p := WithParams(Params{ Routing: rt, ExperimentalUpgrade: true, ExperimentalUpgradeAudit: enabled, }) + p.auditLogHook = auditHook defer p.Close() ps := httptest.NewServer(p) @@ -201,8 +203,6 @@ func TestAuditLogging(t *testing.T) { t.Fatal(err) } - defer wsc.Close() - if _, err := wsc.Write([]byte(message)); err != nil { t.Fatal(err) } @@ -216,6 +216,11 @@ func TestAuditLogging(t *testing.T) { t.Fatal("send/receive failed") } + wsc.Close() + if enabled { + <-p.auditLogHook + } + check(t, sout, serr) } } diff --git a/ratelimit/cluster_test.go b/ratelimit/cluster_test.go index 61209fbe05..e50a5ec517 100644 --- a/ratelimit/cluster_test.go +++ b/ratelimit/cluster_test.go @@ -1,3 +1,5 @@ +// +build !race + package ratelimit import ( diff --git a/routing/datasource.go b/routing/datasource.go index b3eb901cfb..1b0cd52f7b 100644 --- a/routing/datasource.go +++ b/routing/datasource.go @@ -273,9 +273,11 @@ func getFreeStringArgs(count int, p *eskip.Predicate) ([]string, error) { return a, nil } -func mergeLegacyNonTreePredicates(r *eskip.Route) error { +func mergeLegacyNonTreePredicates(r *eskip.Route) (*eskip.Route, error) { var rest []*eskip.Predicate - for _, p := range r.Predicates { + c := r.Copy() + + for _, p := range c.Predicates { if isTreePredicate(p.Name) { rest = append(rest, p) continue @@ -285,53 +287,53 @@ func mergeLegacyNonTreePredicates(r *eskip.Route) error { case hostRegexpName: a, err := getFreeStringArgs(1, p) if err != nil { - return err + return nil, err } - r.HostRegexps = append(r.HostRegexps, a[0]) + c.HostRegexps = append(c.HostRegexps, a[0]) case pathRegexpName: a, err := getFreeStringArgs(1, p) if err != nil { - return err + return nil, err } - r.PathRegexps = append(r.PathRegexps, a[0]) + c.PathRegexps = append(c.PathRegexps, a[0]) case methodName: a, err := getFreeStringArgs(1, p) if err != nil { - return err + return nil, err } - r.Method = a[0] + c.Method = a[0] case headerName: a, err := getFreeStringArgs(2, p) if err != nil { - return err + return nil, err } - if r.Headers == nil { - r.Headers = make(map[string]string) + if c.Headers == nil { + c.Headers = make(map[string]string) } - r.Headers[a[0]] = a[1] + c.Headers[a[0]] = a[1] case headerRegexpName: a, err := getFreeStringArgs(2, p) if err != nil { - return err + return nil, err } - if r.HeaderRegexps == nil { - r.HeaderRegexps = make(map[string][]string) + if c.HeaderRegexps == nil { + c.HeaderRegexps = make(map[string][]string) } - r.HeaderRegexps[a[0]] = append(r.HeaderRegexps[a[0]], a[1]) + c.HeaderRegexps[a[0]] = append(c.HeaderRegexps[a[0]], a[1]) default: rest = append(rest, p) } } - r.Predicates = rest - return nil + c.Predicates = rest + return c, nil } // initialize predicate instances from their spec with the concrete arguments @@ -428,7 +430,8 @@ func processRouteDef(cpm map[string]PredicateSpec, fr filters.Registry, def *esk return nil, err } - if err := mergeLegacyNonTreePredicates(def); err != nil { + def, err = mergeLegacyNonTreePredicates(def) + if err != nil { return nil, err } diff --git a/routing/example_test.go b/routing/example_test.go index aa3ea88eb0..124a90edeb 100644 --- a/routing/example_test.go +++ b/routing/example_test.go @@ -16,12 +16,13 @@ package routing_test import ( "fmt" - "github.com/zalando/skipper/filters/builtin" - "github.com/zalando/skipper/routing" - "github.com/zalando/skipper/routing/testdataclient" "log" "net/http" "time" + + "github.com/zalando/skipper/filters/builtin" + "github.com/zalando/skipper/routing" + "github.com/zalando/skipper/routing/testdataclient" ) func Example() { diff --git a/routing/testdataclient/dataclient.go b/routing/testdataclient/dataclient.go index 177a72623b..d3fee74fdb 100644 --- a/routing/testdataclient/dataclient.go +++ b/routing/testdataclient/dataclient.go @@ -1,17 +1,3 @@ -// Copyright 2015 Zalando SE -// -// Licensed 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. - /* Package testdataclient provides a test implementation for the DataClient interface of the skipper/routing package. @@ -27,13 +13,18 @@ import ( "github.com/zalando/skipper/eskip" ) +type incomingUpdate struct { + upsert []*eskip.Route + deletedIDs []string +} + // DataClient implementation. type Client struct { routes map[string]*eskip.Route upsert []*eskip.Route - deletedIds []string + deletedIDs []string failNext int - signalUpdate chan int + signalUpdate chan incomingUpdate } // Creates a Client with an initial set of route definitions. @@ -45,7 +36,8 @@ func New(initial []*eskip.Route) *Client { return &Client{ routes: routes, - signalUpdate: make(chan int)} + signalUpdate: make(chan incomingUpdate), + } } // Creates a Client with an initial set of route definitions in eskip @@ -62,7 +54,7 @@ func NewDoc(doc string) (*Client, error) { // Returns the initial/current set of route definitions. func (c *Client) LoadAll() ([]*eskip.Route, error) { if c.failNext > 0 { - c.upsert, c.deletedIds = nil, nil + c.upsert, c.deletedIDs = nil, nil c.failNext-- return nil, errors.New("failed to get routes") } @@ -78,9 +70,10 @@ func (c *Client) LoadAll() ([]*eskip.Route, error) { // Returns the route definitions upserted/deleted since the last call to // LoadAll. func (c *Client) LoadUpdate() ([]*eskip.Route, []string, error) { - <-c.signalUpdate + update := <-c.signalUpdate + c.upsert, c.deletedIDs = update.upsert, update.deletedIDs - for _, id := range c.deletedIds { + for _, id := range c.deletedIDs { delete(c.routes, id) } @@ -89,7 +82,7 @@ func (c *Client) LoadUpdate() ([]*eskip.Route, []string, error) { } if c.failNext > 0 { - c.upsert, c.deletedIds = nil, nil + c.upsert, c.deletedIDs = nil, nil c.failNext-- return nil, nil, errors.New("failed to get routes") } @@ -99,27 +92,26 @@ func (c *Client) LoadUpdate() ([]*eskip.Route, []string, error) { d []string ) - u, d, c.upsert, c.deletedIds = c.upsert, c.deletedIds, nil, nil + u, d, c.upsert, c.deletedIDs = c.upsert, c.deletedIDs, nil, nil return u, d, nil } // Updates the current set of routes with new/modified and deleted // route definitions. -func (c *Client) Update(upsert []*eskip.Route, deletedIds []string) { - c.upsert, c.deletedIds = upsert, deletedIds - c.signalUpdate <- 42 +func (c *Client) Update(upsert []*eskip.Route, deletedIDs []string) { + c.signalUpdate <- incomingUpdate{upsert, deletedIDs} } // Updates the current set of routes with new/modified and deleted // route definitions in eskip format. In case the parsing of the // document fails, it returns an error. -func (c *Client) UpdateDoc(upsertDoc string, deletedIds []string) error { +func (c *Client) UpdateDoc(upsertDoc string, deletedIDs []string) error { routes, err := eskip.Parse(upsertDoc) if err != nil { return err } - c.Update(routes, deletedIds) + c.Update(routes, deletedIDs) return nil } diff --git a/skipper_test.go b/skipper_test.go index f2dfe190b6..c5dfe6ee97 100644 --- a/skipper_test.go +++ b/skipper_test.go @@ -208,22 +208,3 @@ func TestHTTPServer(t *testing.T) { t.Fatalf("Failed to stream response body: %v", err) } } - -func TestLoadPlugins(t *testing.T) { - o := Options{ - PluginDirs: []string{"./_test_plugins"}, - FilterPlugins: [][]string{{"filter_noop"}}, - } - if err := o.findAndLoadPlugins(); err != nil { - t.Fatalf("Failed to load plugins: %s", err) - } -} - -func TestLoadPluginsFail(t *testing.T) { - o := Options{ - PluginDirs: []string{"./_test_plugins_fail"}, - } - if err := o.findAndLoadPlugins(); err == nil { - t.Fatalf("did not fail to load plugins: %s", err) - } -}