From 787b0a3ac7700c6ab5740c41edfeb4961d5d9c85 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Doumenjou Date: Fri, 26 Apr 2019 09:36:03 +0200 Subject: [PATCH] Enhance KV client error management Co-authored-by: Ludovic Fernandez --- provider/kv/kv.go | 12 +++- provider/kv/kv_config.go | 47 +++++++++--- provider/kv/kv_config_test.go | 131 ++++++++++++++++++++++++++++++---- 3 files changed, 166 insertions(+), 24 deletions(-) diff --git a/provider/kv/kv.go b/provider/kv/kv.go index 23c70e382..4b1a8d647 100644 --- a/provider/kv/kv.go +++ b/provider/kv/kv.go @@ -75,7 +75,11 @@ func (p *Provider) watchKv(configurationChan chan<- types.ConfigMessage, prefix if !ok { return errors.New("watchtree channel closed") } - configuration := p.buildConfiguration() + configuration, errC := p.buildConfiguration() + if errC != nil { + return errC + } + if configuration != nil { configurationChan <- types.ConfigMessage{ ProviderName: string(p.storeType), @@ -110,7 +114,11 @@ func (p *Provider) Provide(configurationChan chan<- types.ConfigMessage, pool *s } }) } - configuration := p.buildConfiguration() + configuration, err := p.buildConfiguration() + if err != nil { + return err + } + configurationChan <- types.ConfigMessage{ ProviderName: string(p.storeType), Configuration: configuration, diff --git a/provider/kv/kv_config.go b/provider/kv/kv_config.go index cbf8d6349..2c2e73b2a 100644 --- a/provider/kv/kv_config.go +++ b/provider/kv/kv_config.go @@ -18,7 +18,7 @@ import ( "github.com/containous/traefik/types" ) -func (p *Provider) buildConfiguration() *types.Configuration { +func (p *Provider) buildConfiguration() (*types.Configuration, error) { templateObjects := struct { Prefix string }{ @@ -26,7 +26,7 @@ func (p *Provider) buildConfiguration() *types.Configuration { Prefix: strings.TrimSuffix(p.get(p.Prefix, p.Prefix+pathAlias), pathSeparator), } - var KvFuncMap = template.FuncMap{ + kvFuncMap := template.FuncMap{ "List": p.list, "ListServers": p.listServers, "Get": p.get, @@ -69,9 +69,9 @@ func (p *Provider) buildConfiguration() *types.Configuration { "getStickinessCookieName": p.getStickinessCookieName, // Deprecated [breaking] } - configuration, err := p.GetConfiguration("templates/kv.tmpl", KvFuncMap, templateObjects) + configuration, err := p.safeGetConfiguration("templates/kv.tmpl", kvFuncMap, templateObjects) if err != nil { - log.Error(err) + return nil, err } for key, frontend := range configuration.Frontends { @@ -80,7 +80,19 @@ func (p *Provider) buildConfiguration() *types.Configuration { } } - return configuration + return configuration, nil +} + +func (p *Provider) safeGetConfiguration(defaultTemplate string, funcMap template.FuncMap, templateObjects interface{}) (configuration *types.Configuration, err error) { + defer func() { + e := recover() + if e != nil { + err = fmt.Errorf("%v", e) + } + }() + + configuration, err = p.GetConfiguration("templates/kv.tmpl", funcMap, templateObjects) + return } // Deprecated @@ -564,9 +576,9 @@ func (p *Provider) listServers(backend string) []string { func (p *Provider) serverFilter(serverName string) bool { key := fmt.Sprint(serverName, pathBackendServerURL) if _, err := p.kvClient.Get(key, nil); err != nil { - if err != store.ErrKeyNotFound { - log.Errorf("Failed to retrieve value for key %s: %s", key, err) - } + checkError(err) + + log.Errorf("failed to retrieve value for key %s: %s", key, err) return false } return p.checkConstraints(serverName, pathTags) @@ -575,6 +587,9 @@ func (p *Provider) serverFilter(serverName string) bool { func (p *Provider) checkConstraints(keys ...string) bool { joinedKeys := strings.Join(keys, "") keyPair, err := p.kvClient.Get(joinedKeys, nil) + if err != nil { + checkError(err) + } value := "" if err == nil && keyPair != nil && keyPair.Value != nil { @@ -625,9 +640,13 @@ func (p *Provider) get(defaultValue string, keyParts ...string) string { keyPair, err := p.kvClient.Get(key, nil) if err != nil { + checkError(err) + log.Debugf("Cannot get key %s %s, setting default %s", key, err, defaultValue) return defaultValue - } else if keyPair == nil { + } + + if keyPair == nil { log.Debugf("Cannot get key %s, setting default %s", key, defaultValue) return defaultValue } @@ -663,6 +682,8 @@ func (p *Provider) hasPrefix(keyParts ...string) bool { listKeys, err := p.kvClient.List(baseKey, nil) if err != nil { + checkError(err) + log.Debugf("Cannot list keys under %q: %v", baseKey, err) return false } @@ -705,6 +726,8 @@ func (p *Provider) list(keyParts ...string) []string { keysPairs, err := p.kvClient.List(rootKey, nil) if err != nil { + checkError(err) + log.Debugf("Cannot list keys under %q: %v", rootKey, err) return nil } @@ -776,3 +799,9 @@ func (p *Provider) getMap(keyParts ...string) map[string]string { return mapData } + +func checkError(err error) { + if err != store.ErrKeyNotFound { + panic(err) + } +} diff --git a/provider/kv/kv_config_test.go b/provider/kv/kv_config_test.go index 75e6c3637..8e61e339f 100644 --- a/provider/kv/kv_config_test.go +++ b/provider/kv/kv_config_test.go @@ -12,6 +12,7 @@ import ( "github.com/containous/traefik/tls" "github.com/containous/traefik/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func aKVPair(key string, value string) *store.KVPair { @@ -22,6 +23,7 @@ func TestProviderBuildConfiguration(t *testing.T) { testCases := []struct { desc string kvPairs []*store.KVPair + kvError KvError expected *types.Configuration }{ { @@ -553,6 +555,22 @@ func TestProviderBuildConfiguration(t *testing.T) { }, }, }, + { + desc: "Should recover on panic", + kvPairs: filler("traefik", + frontend("frontend", + withPair(pathFrontendBackend, "backend"), + withPair(pathFrontendAuthHeaderField, "X-WebAuth-User"), + withPair(pathFrontendAuthBasicRemoveHeader, "true"), + withList(pathFrontendAuthBasicUsers, "test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/", "test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0"), + ), + backend("backend"), + ), + kvError: KvError{ + List: store.ErrNotReachable, + }, + expected: nil, + }, } for _, test := range testCases { @@ -564,19 +582,66 @@ func TestProviderBuildConfiguration(t *testing.T) { Prefix: "traefik", kvClient: &Mock{ KVPairs: test.kvPairs, + Error: test.kvError, }, } - actual := p.buildConfiguration() - assert.NotNil(t, actual) + actual, err := p.buildConfiguration() + if test.kvError.Get != nil || test.kvError.List != nil { + require.Error(t, err) + require.Nil(t, actual) + } else { + require.NoError(t, err) + require.NotNil(t, actual) + assert.EqualValues(t, test.expected.Backends, actual.Backends) + assert.EqualValues(t, test.expected.Frontends, actual.Frontends) + } - assert.EqualValues(t, test.expected.Backends, actual.Backends) - assert.EqualValues(t, test.expected.Frontends, actual.Frontends) assert.EqualValues(t, test.expected, actual) }) } } +func TestProviderListShouldPanic(t *testing.T) { + testCases := []struct { + desc string + panic bool + kvError error + }{ + { + desc: "Should panic on an unexpected error", + kvError: store.ErrBackendNotSupported, + panic: true, + }, + { + desc: "Should not panic on an ErrKeyNotFound error", + kvError: store.ErrKeyNotFound, + }, + } + + for _, test := range testCases { + test := test + + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + kvPairs := []*store.KVPair{ + aKVPair("foo", "bar"), + } + p := &Provider{ + kvClient: newKvClientMock(kvPairs, test.kvError), + } + + keyParts := []string{"foo"} + if test.panic { + assert.Panics(t, func() { p.list(keyParts...) }) + } else { + assert.NotPanics(t, func() { p.list(keyParts...) }) + } + }) + } +} + func TestProviderList(t *testing.T) { testCases := []struct { desc string @@ -622,8 +687,8 @@ func TestProviderList(t *testing.T) { expected: []string{"foo/baz/1", "foo/baz/2"}, }, { - desc: "when KV error", - kvError: store.ErrNotReachable, + desc: "when KV error key not found", + kvError: store.ErrKeyNotFound, kvPairs: []*store.KVPair{ aKVPair("foo/baz/1", "bar1"), aKVPair("foo/baz/2", "bar2"), @@ -651,6 +716,46 @@ func TestProviderList(t *testing.T) { } } +func TestProviderGetShouldPanic(t *testing.T) { + testCases := []struct { + desc string + panic bool + kvError error + }{ + { + desc: "Should panic on an unexpected error", + kvError: store.ErrBackendNotSupported, + panic: true, + }, + { + desc: "Should not panic on an ErrKeyNotFound error", + kvError: store.ErrKeyNotFound, + }, + } + + for _, test := range testCases { + test := test + + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + kvPairs := []*store.KVPair{ + aKVPair("foo", "bar"), + } + p := &Provider{ + kvClient: newKvClientMock(kvPairs, test.kvError), + } + + keyParts := []string{"foo"} + if test.panic { + assert.Panics(t, func() { p.get("", keyParts...) }) + } else { + assert.NotPanics(t, func() { p.get("", keyParts...) }) + } + }) + } +} + func TestProviderGet(t *testing.T) { testCases := []struct { desc string @@ -718,7 +823,7 @@ func TestProviderGet(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: []*store.KVPair{ aKVPair("foo/baz/1", "bar1"), aKVPair("foo/baz/2", "bar2"), @@ -824,7 +929,7 @@ func TestProviderSplitGet(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", ""), @@ -900,7 +1005,7 @@ func TestProviderGetList(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", ""), @@ -973,7 +1078,7 @@ func TestProviderGetSlice(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", ""), @@ -1046,7 +1151,7 @@ func TestProviderGetBool(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", "true"), @@ -1111,7 +1216,7 @@ func TestProviderGetInt(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", "true"), @@ -1176,7 +1281,7 @@ func TestProviderGetInt64(t *testing.T) { }, { desc: "when KV error", - kvError: store.ErrNotReachable, + kvError: store.ErrKeyNotFound, kvPairs: filler("traefik", frontend("foo", withPair("bar", "true"),