Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kv: Ignore backend servers with no url #1196

Merged
merged 4 commits into from
Apr 6, 2017
Merged

kv: Ignore backend servers with no url #1196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
020a8e3
kv: Ignore backend servers with no url
klausenbusk Feb 26, 2017
c864d80
kv: Add test for server without url key
klausenbusk Mar 7, 2017
f621a46
kv: Log error when checking existence of server url key
klausenbusk Mar 7, 2017
b4dfb72
kv: Extend test with support for specifying custom error for Get/List
klausenbusk Mar 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions provider/kv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,13 @@ func (provider *Kv) list(keys ...string) []string {
func (provider *Kv) listServers(backend string) []string {
serverNames := provider.list(backend, "/servers/")
return fun.Filter(func(serverName string) bool {
key := fmt.Sprint(serverName, "/url")
if _, err := provider.kvclient.Get(key); err != nil {
if err != store.ErrKeyNotFound {
log.Errorf("Failed to retrieve value for key %s: %s", key, err)
}
return false
}
return provider.checkConstraints(serverName, "/tags")
}, serverNames).([]string)
}
Expand Down
34 changes: 26 additions & 8 deletions provider/kv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ func TestKvList(t *testing.T) {
// Error case
provider := &Kv{
kvclient: &Mock{
Error: true,
Error: KvError{
List: store.ErrKeyNotFound,
},
},
}
actual := provider.list("anything")
Expand Down Expand Up @@ -187,7 +189,9 @@ func TestKvGet(t *testing.T) {
// Error case
provider := &Kv{
kvclient: &Mock{
Error: true,
Error: KvError{
Get: store.ErrKeyNotFound,
},
},
}
actual := provider.get("", "anything")
Expand Down Expand Up @@ -284,9 +288,15 @@ func TestKvWatchTree(t *testing.T) {
}
}

// Override Get/List to return a error
type KvError struct {
Get error
List error
}

// Extremely limited mock store so we can test initialization
type Mock struct {
Error bool
Error KvError
KVPairs []*store.KVPair
WatchTreeMethod func() <-chan []*store.KVPair
}
Expand All @@ -296,15 +306,15 @@ func (s *Mock) Put(key string, value []byte, opts *store.WriteOptions) error {
}

func (s *Mock) Get(key string) (*store.KVPair, error) {
if s.Error {
return nil, errors.New("Error")
if err := s.Error.Get; err != nil {
return nil, err
}
for _, kvPair := range s.KVPairs {
if kvPair.Key == key {
return kvPair, nil
}
}
return nil, nil
return nil, store.ErrKeyNotFound
}

func (s *Mock) Delete(key string) error {
Expand Down Expand Up @@ -333,8 +343,8 @@ func (s *Mock) NewLock(key string, options *store.LockOptions) (store.Locker, er

// List mock
func (s *Mock) List(prefix string) ([]*store.KVPair, error) {
if s.Error {
return nil, errors.New("Error")
if err := s.Error.List; err != nil {
return nil, err
}
kv := []*store.KVPair{}
for _, kvPair := range s.KVPairs {
Expand Down Expand Up @@ -410,6 +420,14 @@ func TestKVLoadConfig(t *testing.T) {
Key: "traefik/backends/backend.with.dot.too/servers/server.with.dot/weight",
Value: []byte("0"),
},
{
Key: "traefik/backends/backend.with.dot.too/servers/server.with.dot.without.url",
Value: []byte(""),
},
{
Key: "traefik/backends/backend.with.dot.too/servers/server.with.dot.without.url/weight",
Value: []byte("0"),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also have added a test where we encounter a "true" error and make sure we filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate a bit? I'm not sure what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new production code, we have added two paths: One where we encounter a "not found" error and another one where we encounter a critical, "true" error. Both paths should do the same, which is to return false.

You have changed the mock to return the "not found" error if Get cannot find a given key among the configured ones. That's great because there's now also a test case to execute that path. However, for the "true" error path to happen, we'd need to configure the mock to return an error by another test case. However, I don't see any new test case with the error switch being set.

So I'm thinking that we might be missing a test case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure how it can/should be implemented. We can't rely on the Mock Error bool, as that cause List to error out.

I think we need to crate our own KVPair type with a error "field", but that seems kind of messy. Do you have a simpler idea?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the Mock struct to separate the currently single error into one for Get and one for List calls. We'll then have to adjust all tests that test error cases, but I think it's worth it: We have to improve our test coverage in the future anyway, and more fine-grained error distinctions are most likely going to be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the Mock struct to separate the currently single error into one for Get and one for List calls.

Done, see last commit. I will look on a listServers test when I get some more time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

},
},
}
Expand Down