bugfix: prevent API server cache list/watch from other namespaces #25342

mqliang · 2016-05-09T09:37:59Z

The previous code has a issue:

Assume we have two namespaces ns1 and ns11, when client list/watch key /registry/pods/ns1, API server cache will also return pods in ns11, since ns1 is a prefix of ns11

This PR fix this issue. @wojtek-t

wojtek-t · 2016-05-10T08:16:23Z

pkg/storage/cacher.go

 		}
-		return filter(obj)
+
+		return match && filter(obj)


Great catch in general. Can you please add a unit test for it?

xiang90 · 2016-05-11T01:37:36Z

/cc @hongchaodeng

hongchaodeng · 2016-05-11T03:01:16Z

pkg/storage/cacher.go

+		match := false
+		for _, segment := range segments {
+			currPath = path.Join(currPath, segment)
+			if currPath == key {


What if key ends with "/"?

One more problem: the code is incorrect for exact match case.
For exact match, i.e. filter("/abc", "/abc"), it will be ignored in WatchList(). See etcd_helper and etcd3/watcher.

Hmm.. - but it's only for main directory. So e.g. filter(/ns1/pod1, /ns1/pod1) will return true,

I think that we should base on the fact that here objKey is computed from runtime.Object and we should probably assume what it returns. That said, we should allow for exact match.

It's fine here because the results are already filtered out by underlying storage impl.?

mqliang · 2016-05-12T11:07:23Z

@wojtek-t IIUC, when a client LIST a nonexistent key, the cache layer will return a empty list, and if a client WATCH a nonexistent key, the cache layer will never return any events, while it should return 404 in such a case?

wojtek-t · 2016-05-16T08:26:29Z

@wojtek-t IIUC, when a client LIST a nonexistent key, the cache layer will return a empty list, and if a client WATCH a nonexistent key, the cache layer will never return any events, while it should return 404 in such a case?

I don't agree. If client WATCH a nonexistent key it should open the watch and wait until such object appears. It may appear at some point. If not, then we will never return anything and this is fine.

We definitely want to watch for objects that don't exist now, and this is a very common practice in fact.

mqliang · 2016-05-16T08:51:41Z

@wojtek-t

We definitely want to watch for objects that don't exist now, and this is a very common practice in fact.

Ok, the behavior when watch sounds good to me. But it seems that we should fix the behavior when list(cache should return KeyNotFound error instead of an empty list)? Since I think the cache layer should not break any assumption the old clients may hold before having such a cache. Thoughts?

wojtek-t · 2016-05-16T08:54:46Z

If apiserver without cacher returns something different than apiserver with cacher, then this is definitely a bug and we should make them return the same (change cacher).

wojtek-t · 2016-05-16T09:03:45Z

So to clarify - yes, they should be compatible.

wojtek-t · 2016-07-04T12:39:35Z

@mqliang - sorry for being super unresponsive last time - I was on vacation and before I was super busy with 1.3 release.
I think this is now your turn on this PR, right? If you expect to me to do something on this one, please ping me.

mqliang · 2016-07-04T12:56:37Z

@wojtek-t Never mind, it's not a urgent PR. Will update this tomorrow.

As for the "return KeyNotFound error instead of an empty list when list from a nonexistent namespace" issue, I will fix it after your #27277 are merged. I'd like add a namespace index for namespace scoped resource. It's very helpful for the above issue and for the "kubectl get XXX --namespcace=YYY" use case(it's also very common that many applications build on k8s need list a resource in a namespace).

wojtek-t · 2016-07-04T12:58:27Z

@mqliang - thanks

mqliang · 2016-07-07T11:58:32Z

@wojtek-t Updated. PTAL.

wojtek-t · 2016-07-07T11:59:47Z

@mqliang - thanks!

LGTM

k8s-github-robot · 2016-07-07T15:06:57Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-07-07T15:43:23Z

GCE e2e build/test passed for commit 3f7bca9.

wojtek-t · 2016-07-07T16:12:53Z

@mqliang - it seems the test you added is flaky:

k8s.io/kubernetes/pkg/storage TestWatchList 1.50s
 go test -v k8s.io/kubernetes/pkg/storage -run TestWatchList$

cacher_test.go:182: (called from line 353)
cacher_test.go:183: Expected (ADDED): &api.Pod{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"foo", GenerateName:"", Namespace:"ns", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil)}, Spec:api.PodSpec{Volumes:[]api.Volume(nil), InitContainers:[]api.Container(nil), Containers:[]api.Container(nil), RestartPolicy:"Always", TerminationGracePeriodSeconds:(*int64)(0xc82031fbf8), ActiveDeadlineSeconds:(*int64)(nil), DNSPolicy:"ClusterFirst", NodeSelector:map[string]string(nil), ServiceAccountName:"", NodeName:"fakeNode", SecurityContext:(*api.PodSecurityContext)(0xc82081ab00), ImagePullSecrets:[]api.LocalObjectReference(nil), Hostname:"", Subdomain:""}, Status:api.PodStatus{Phase:"", Conditions:[]api.PodCondition(nil), Message:"", Reason:"", HostIP:"", PodIP:"", StartTime:(*unversioned.Time)(nil), InitContainerStatuses:[]api.ContainerStatus(nil), ContainerStatuses:[]api.ContainerStatus(nil)}}, got: &api.Pod{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"bar", GenerateName:"", Namespace:"ns", SelfLink:"", UID:"", ResourceVersion:"5", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil)}, Spec:api.PodSpec{Volumes:[]api.Volume(nil), InitContainers:[]api.Container(nil), Containers:[]api.Container(nil), RestartPolicy:"Always", TerminationGracePeriodSeconds:(*int64)(0xc8202e3690), ActiveDeadlineSeconds:(*int64)(nil), DNSPolicy:"ClusterFirst", NodeSelector:map[string]string(nil), ServiceAccountName:"", NodeName:"", SecurityContext:(*api.PodSecurityContext)(0xc8208e5d00), ImagePullSecrets:[]api.LocalObjectReference(nil), Hostname:"", Subdomain:""}, Status:api.PodStatus{Phase:"", Conditions:[]api.PodCondition(nil), Message:"", Reason:"", HostIP:"", PodIP:"", StartTime:(*unversioned.Time)(nil), InitContainerStatuses:[]api.ContainerStatus(nil), ContainerStatuses:[]api.ContainerStatus(nil)}}
cacher_test.go:182: (called from line 357)
cacher_test.go:183: Expected (ADDED): &api.Pod{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"bar", GenerateName:"", Namespace:"ns", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil)}, Spec:api.PodSpec{Volumes:[]api.Volume(nil), InitContainers:[]api.Container(nil), Containers:[]api.Container(nil), RestartPolicy:"Always", TerminationGracePeriodSeconds:(*int64)(0xc82031fbe0), ActiveDeadlineSeconds:(*int64)(nil), DNSPolicy:"ClusterFirst", NodeSelector:map[string]string(nil), ServiceAccountName:"", NodeName:"", SecurityContext:(*api.PodSecurityContext)(0xc82081aa40), ImagePullSecrets:[]api.LocalObjectReference(nil), Hostname:"", Subdomain:""}, Status:api.PodStatus{Phase:"", Conditions:[]api.PodCondition(nil), Message:"", Reason:"", HostIP:"", PodIP:"", StartTime:(*unversioned.Time)(nil), InitContainerStatuses:[]api.ContainerStatus(nil), ContainerStatuses:[]api.ContainerStatus(nil)}}, got: &api.Pod{TypeMeta:unversioned.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:api.ObjectMeta{Name:"foo", GenerateName:"", Namespace:"ns", SelfLink:"", UID:"", ResourceVersion:"8", Generation:0, CreationTimestamp:unversioned.Time{Time:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*unversioned.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]api.OwnerReference(nil), Finalizers:[]string(nil)}, Spec:api.PodSpec{Volumes:[]api.Volume(nil), InitContainers:[]api.Container(nil), Containers:[]api.Container(nil), RestartPolicy:"Always", TerminationGracePeriodSeconds:(*int64)(0xc8202e3698), ActiveDeadlineSeconds:(*int64)(nil), DNSPolicy:"ClusterFirst", NodeSelector:map[string]string(nil), ServiceAccountName:"", NodeName:"fakeNode", SecurityContext:(*api.PodSecurityContext)(0xc8208e5d40), ImagePullSecrets:[]api.LocalObjectReference(nil), Hostname:"", Subdomain:""}, Status:api.PodStatus{Phase:"", Conditions:[]api.PodCondition(nil), Message:"", Reason:"", HostIP:"", PodIP:"", StartTime:(*unversioned.Time)(nil), InitContainerStatuses:[]api.ContainerStatus(nil), ContainerStatuses:[]api.ContainerStatus(nil)}}

Can you please take a look? [I'm removing lgtm for now.]

wojtek-t · 2016-07-12T12:52:11Z

@mqliang - will you find some time to debug this test?

k8s-github-robot · 2016-07-14T18:39:38Z

@mqliang PR needs rebase

wojtek-t · 2016-07-18T08:08:44Z

Already fixed by #28966 - closing this one.

googlebot added the cla: yes label May 9, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 9, 2016

fabioy assigned wojtek-t May 9, 2016

wojtek-t reviewed May 10, 2016
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2016

hongchaodeng reviewed May 11, 2016
View reviewed changes

mqliang mentioned this pull request May 13, 2016

Build namespace index at API server cache for namespace scoped resource to improve LIST performance #25534

Closed

mqliang force-pushed the cache-list-bug branch from 3924009 to 873ba2c Compare July 7, 2016 09:04

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 7, 2016

bugfix: prevent API server cache list/watch from other namespace

3f7bca9

mqliang force-pushed the cache-list-bug branch from 873ba2c to 3f7bca9 Compare July 7, 2016 11:12

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2016

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 7, 2016

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2016

wojtek-t removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2016

wojtek-t closed this Jul 18, 2016

mqliang deleted the cache-list-bug branch November 28, 2016 06:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: prevent API server cache list/watch from other namespaces #25342

bugfix: prevent API server cache list/watch from other namespaces #25342

mqliang commented May 9, 2016

wojtek-t May 10, 2016

xiang90 commented May 11, 2016

hongchaodeng May 11, 2016

hongchaodeng May 11, 2016 •

edited

Loading

wojtek-t May 11, 2016

hongchaodeng May 11, 2016

mqliang commented May 12, 2016 •

edited

Loading

wojtek-t commented May 16, 2016

mqliang commented May 16, 2016

wojtek-t commented May 16, 2016 •

edited

Loading

wojtek-t commented May 16, 2016

wojtek-t commented Jul 4, 2016

mqliang commented Jul 4, 2016

wojtek-t commented Jul 4, 2016

mqliang commented Jul 7, 2016

wojtek-t commented Jul 7, 2016

k8s-github-robot commented Jul 7, 2016

k8s-bot commented Jul 7, 2016

wojtek-t commented Jul 7, 2016

wojtek-t commented Jul 12, 2016

k8s-github-robot commented Jul 14, 2016

wojtek-t commented Jul 18, 2016

bugfix: prevent API server cache list/watch from other namespaces #25342

bugfix: prevent API server cache list/watch from other namespaces #25342

Conversation

mqliang commented May 9, 2016

wojtek-t May 10, 2016

Choose a reason for hiding this comment

xiang90 commented May 11, 2016

hongchaodeng May 11, 2016

Choose a reason for hiding this comment

hongchaodeng May 11, 2016 • edited Loading

Choose a reason for hiding this comment

wojtek-t May 11, 2016

Choose a reason for hiding this comment

hongchaodeng May 11, 2016

Choose a reason for hiding this comment

mqliang commented May 12, 2016 • edited Loading

wojtek-t commented May 16, 2016

mqliang commented May 16, 2016

wojtek-t commented May 16, 2016 • edited Loading

wojtek-t commented May 16, 2016

wojtek-t commented Jul 4, 2016

mqliang commented Jul 4, 2016

wojtek-t commented Jul 4, 2016

mqliang commented Jul 7, 2016

wojtek-t commented Jul 7, 2016

k8s-github-robot commented Jul 7, 2016

k8s-bot commented Jul 7, 2016

wojtek-t commented Jul 7, 2016

wojtek-t commented Jul 12, 2016

k8s-github-robot commented Jul 14, 2016

wojtek-t commented Jul 18, 2016

hongchaodeng May 11, 2016 •

edited

Loading

mqliang commented May 12, 2016 •

edited

Loading

wojtek-t commented May 16, 2016 •

edited

Loading