From f63fe9c52e9156480721598b92f2b1f9b7792d06 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 9 Sep 2015 13:03:00 -0400 Subject: [PATCH] List/Watch on field=metadata.name= across namespaces is broken MatchSingle assumes that namespace is set. That is not correct. --- pkg/registry/generic/etcd/etcd.go | 42 +++++----- pkg/registry/generic/etcd/etcd_test.go | 102 ++++++++++++++++++------- 2 files changed, 97 insertions(+), 47 deletions(-) diff --git a/pkg/registry/generic/etcd/etcd.go b/pkg/registry/generic/etcd/etcd.go index 343b6d1464c..b088ba04a44 100644 --- a/pkg/registry/generic/etcd/etcd.go +++ b/pkg/registry/generic/etcd/etcd.go @@ -153,23 +153,23 @@ func (e *Etcd) ListPredicate(ctx api.Context, m generic.Matcher) (runtime.Object filterFunc := e.filterAndDecorateFunction(m) defer trace.LogIfLong(600 * time.Millisecond) if name, ok := m.MatchesSingle(); ok { - trace.Step("About to read single object") - key, err := e.KeyFunc(ctx, name) - if err != nil { - return nil, err - } - err = e.Storage.GetToList(key, filterFunc, list) - trace.Step("Object extracted") - if err != nil { - return nil, err - } - } else { - trace.Step("About to list directory") - err := e.Storage.List(e.KeyRootFunc(ctx), filterFunc, list) - trace.Step("List extracted") - if err != nil { - return nil, err + if key, err := e.KeyFunc(ctx, name); err == nil { + trace.Step("About to read single object") + err := e.Storage.GetToList(key, filterFunc, list) + trace.Step("Object extracted") + if err != nil { + return nil, err + } + return list, nil } + // if we cannot extract a key based on the current context, the optimization is skipped + } + + trace.Step("About to list directory") + err := e.Storage.List(e.KeyRootFunc(ctx), filterFunc, list) + trace.Step("List extracted") + if err != nil { + return nil, err } return list, nil } @@ -452,11 +452,13 @@ func (e *Etcd) WatchPredicate(ctx api.Context, m generic.Matcher, resourceVersio filterFunc := e.filterAndDecorateFunction(m) if name, ok := m.MatchesSingle(); ok { - key, err := e.KeyFunc(ctx, name) - if err != nil { - return nil, err + if key, err := e.KeyFunc(ctx, name); err == nil { + if err != nil { + return nil, err + } + return e.Storage.Watch(key, version, filterFunc) } - return e.Storage.Watch(key, version, filterFunc) + // if we cannot extract a key based on the current context, the optimization is skipped } return e.Storage.WatchList(e.KeyRootFunc(ctx), version, filterFunc) diff --git a/pkg/registry/generic/etcd/etcd_test.go b/pkg/registry/generic/etcd/etcd_test.go index 8569ccc84df..2181b213a07 100644 --- a/pkg/registry/generic/etcd/etcd_test.go +++ b/pkg/registry/generic/etcd/etcd_test.go @@ -84,6 +84,9 @@ func NewTestGenericEtcdRegistry(t *testing.T) (*tools.FakeEtcdClient, *Etcd) { return podPrefix }, KeyFunc: func(ctx api.Context, id string) (string, error) { + if _, ok := api.NamespaceFrom(ctx); !ok { + return "", fmt.Errorf("namespace is required") + } return path.Join(podPrefix, id), nil }, ObjectNameFunc: func(obj runtime.Object) (string, error) { return obj.(*api.Pod).Name, nil }, @@ -126,11 +129,11 @@ func (everythingMatcher) MatchesSingle() (string, bool) { func TestEtcdList(t *testing.T) { podA := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo"}, + ObjectMeta: api.ObjectMeta{Namespace: "test", Name: "foo"}, Spec: api.PodSpec{NodeName: "machine"}, } podB := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "bar"}, + ObjectMeta: api.ObjectMeta{Namespace: "test", Name: "bar"}, Spec: api.PodSpec{NodeName: "machine"}, } @@ -148,10 +151,14 @@ func TestEtcdList(t *testing.T) { }, } + testContext := api.WithNamespace(api.NewContext(), "test") + noNamespaceContext := api.NewContext() + table := map[string]struct { in tools.EtcdResponseWithError m generic.Matcher out runtime.Object + context api.Context succeed bool }{ "empty": { @@ -194,6 +201,16 @@ func TestEtcdList(t *testing.T) { out: &api.PodList{Items: []api.Pod{*podA}}, succeed: true, }, + "normalFilteredNoNamespace": { + in: tools.EtcdResponseWithError{ + R: normalListResp, + E: nil, + }, + m: setMatcher{sets.NewString("foo")}, + out: &api.PodList{Items: []api.Pod{*podA}}, + context: noNamespaceContext, + succeed: true, + }, "normalFilteredMatchMultiple": { in: tools.EtcdResponseWithError{ R: normalListResp, @@ -206,23 +223,28 @@ func TestEtcdList(t *testing.T) { } for name, item := range table { + ctx := testContext + if item.context != nil { + ctx = item.context + } fakeClient, registry := NewTestGenericEtcdRegistry(t) if name, ok := item.m.MatchesSingle(); ok { - key, err := registry.KeyFunc(api.NewContext(), name) - if err != nil { - t.Errorf("Couldn't create key for %v", name) - continue + if key, err := registry.KeyFunc(ctx, name); err == nil { + key = etcdtest.AddPrefix(key) + fakeClient.Data[key] = item.in + } else { + key := registry.KeyRootFunc(ctx) + key = etcdtest.AddPrefix(key) + fakeClient.Data[key] = item.in } - key = etcdtest.AddPrefix(key) - fakeClient.Data[key] = item.in } else { - key, _ := registry.KeyFunc(api.NewContext(), name) + key := registry.KeyRootFunc(ctx) key = etcdtest.AddPrefix(key) fakeClient.Data[key] = item.in } - list, err := registry.ListPredicate(api.NewContext(), item.m) + list, err := registry.ListPredicate(ctx, item.m) if e, a := item.succeed, err == nil; e != a { - t.Errorf("%v: expected %v, got %v", name, e, a) + t.Errorf("%v: expected %v, got %v: %v", name, e, a, err) continue } if e, a := item.out, list; !api.Semantic.DeepDerivative(e, a) { @@ -233,11 +255,11 @@ func TestEtcdList(t *testing.T) { func TestEtcdCreate(t *testing.T) { podA := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault}, + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test"}, Spec: api.PodSpec{NodeName: "machine"}, } podB := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault}, + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test"}, Spec: api.PodSpec{NodeName: "machine2"}, } @@ -257,6 +279,8 @@ func TestEtcdCreate(t *testing.T) { E: tools.EtcdErrorNotFound, } + testContext := api.WithNamespace(api.NewContext(), "test") + table := map[string]struct { existing tools.EtcdResponseWithError expect tools.EtcdResponseWithError @@ -282,7 +306,7 @@ func TestEtcdCreate(t *testing.T) { fakeClient, registry := NewTestGenericEtcdRegistry(t) path := etcdtest.AddPrefix("pods/foo") fakeClient.Data[path] = item.existing - obj, err := registry.Create(api.NewDefaultContext(), item.toCreate) + obj, err := registry.Create(testContext, item.toCreate) if !item.errOK(err) { t.Errorf("%v: unexpected error: %v", name, err) } @@ -310,15 +334,15 @@ func TestEtcdCreate(t *testing.T) { func TestEtcdUpdate(t *testing.T) { podA := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault}, + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test"}, Spec: api.PodSpec{NodeName: "machine"}, } podB := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault, ResourceVersion: "1"}, + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "1"}, Spec: api.PodSpec{NodeName: "machine2"}, } podAWithResourceVersion := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: api.NamespaceDefault, ResourceVersion: "3"}, + ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "test", ResourceVersion: "3"}, Spec: api.PodSpec{NodeName: "machine"}, } nodeWithPodA := tools.EtcdResponseWithError{ @@ -369,6 +393,8 @@ func TestEtcdUpdate(t *testing.T) { E: tools.EtcdErrorNotFound, } + testContext := api.WithNamespace(api.NewContext(), "test") + table := map[string]struct { existing tools.EtcdResponseWithError expect tools.EtcdResponseWithError @@ -418,7 +444,7 @@ func TestEtcdUpdate(t *testing.T) { registry.UpdateStrategy.(*testRESTStrategy).allowUnconditionalUpdate = item.allowUnconditionalUpdate path := etcdtest.AddPrefix("pods/foo") fakeClient.Data[path] = item.existing - obj, _, err := registry.Update(api.NewDefaultContext(), item.toUpdate) + obj, _, err := registry.Update(testContext, item.toUpdate) if !item.errOK(err) { t.Errorf("%v: unexpected error: %v", name, err) } @@ -446,7 +472,7 @@ func TestEtcdUpdate(t *testing.T) { func TestEtcdGet(t *testing.T) { podA := &api.Pod{ - ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"}, + ObjectMeta: api.ObjectMeta{Namespace: "test", Name: "foo", ResourceVersion: "1"}, Spec: api.PodSpec{NodeName: "machine"}, } @@ -485,11 +511,13 @@ func TestEtcdGet(t *testing.T) { }, } + testContext := api.WithNamespace(api.NewContext(), "test") + for name, item := range table { fakeClient, registry := NewTestGenericEtcdRegistry(t) path := etcdtest.AddPrefix("pods/foo") fakeClient.Data[path] = item.existing - got, err := registry.Get(api.NewContext(), key) + got, err := registry.Get(testContext, key) if !item.errOK(err) { t.Errorf("%v: unexpected error: %v", name, err) } @@ -522,6 +550,8 @@ func TestEtcdDelete(t *testing.T) { E: tools.EtcdErrorNotFound, } + testContext := api.WithNamespace(api.NewContext(), "test") + key := "foo" table := map[string]struct { @@ -545,7 +575,7 @@ func TestEtcdDelete(t *testing.T) { fakeClient, registry := NewTestGenericEtcdRegistry(t) path := etcdtest.AddPrefix("pods/foo") fakeClient.Data[path] = item.existing - obj, err := registry.Delete(api.NewContext(), key, nil) + obj, err := registry.Delete(testContext, key, nil) if !item.errOK(err) { t.Errorf("%v: unexpected error: %v (%#v)", name, err, obj) } @@ -560,23 +590,41 @@ func TestEtcdDelete(t *testing.T) { } func TestEtcdWatch(t *testing.T) { - table := map[string]generic.Matcher{ - "single": setMatcher{sets.NewString("foo")}, - "multi": setMatcher{sets.NewString("foo", "bar")}, + testContext := api.WithNamespace(api.NewContext(), "test") + noNamespaceContext := api.NewContext() + + table := map[string]struct { + generic.Matcher + context api.Context + }{ + "single": { + Matcher: setMatcher{sets.NewString("foo")}, + }, + "multi": { + Matcher: setMatcher{sets.NewString("foo", "bar")}, + }, + "singleNoNamespace": { + Matcher: setMatcher{sets.NewString("foo")}, + context: noNamespaceContext, + }, } for name, m := range table { + ctx := testContext + if m.context != nil { + ctx = m.context + } podA := &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", - Namespace: api.NamespaceDefault, + Namespace: "test", ResourceVersion: "1", }, Spec: api.PodSpec{NodeName: "machine"}, } respWithPodA := &etcd.Response{ Node: &etcd.Node{ - Key: "/registry/pods/default/foo", + Key: "/registry/pods/test/foo", Value: runtime.EncodeOrDie(testapi.Default.Codec(), podA), ModifiedIndex: 1, CreatedIndex: 1, @@ -585,7 +633,7 @@ func TestEtcdWatch(t *testing.T) { } fakeClient, registry := NewTestGenericEtcdRegistry(t) - wi, err := registry.WatchPredicate(api.NewContext(), m, "1") + wi, err := registry.WatchPredicate(ctx, m, "1") if err != nil { t.Errorf("%v: unexpected error: %v", name, err) continue