Merge pull request #28967 from liggitt/cache-filter-release-1.2

release-1.2: Fix watch cache filtering

pkg/storage/cacher.go

@@ -21,7 +21,6 @@ import (
 	"net/http"
 	"reflect"
 	"strconv"
-	"strings"
 	"sync"
 	"time"
 
@@ -413,7 +412,7 @@ func filterFunction(key string, keyFunc func(runtime.Object) (string, error), fi
 			glog.Errorf("invalid object for filter: %v", obj)
 			return false
 		}
-		if !strings.HasPrefix(objKey, key) {
+		if !hasPathPrefix(objKey, key) {
 			return false
 		}
 		return filter(obj)

pkg/storage/cacher_test.go

@@ -71,7 +71,7 @@ func makeTestPod(name string) *api.Pod {
 }
 
 func updatePod(t *testing.T, s storage.Interface, obj, old *api.Pod) *api.Pod {
-	key := etcdtest.AddPrefix("pods/ns/" + obj.Name)
+	key := etcdtest.AddPrefix("pods/" + obj.Namespace + "/" + obj.Name)
 	result := &api.Pod{}
 	if old == nil {
 		if err := s.Create(context.TODO(), key, obj, result, 0); err != nil {
@@ -108,6 +108,12 @@ func TestList(t *testing.T) {
 
 	_ = updatePod(t, etcdStorage, podFooPrime, fooCreated)
 
+	// Create a pod in a namespace that contains "ns" as a prefix
+	// Make sure it is not returned in a watch of "ns"
+	podFooNS2 := makeTestPod("foo")
+	podFooNS2.Namespace += "2"
+	updatePod(t, etcdStorage, podFooNS2, nil)
+
 	deleted := api.Pod{}
 	if err := etcdStorage.Delete(context.TODO(), etcdtest.AddPrefix("pods/ns/bar"), &deleted); err != nil {
 		t.Errorf("Unexpected error: %v", err)
@@ -145,6 +151,10 @@ func TestList(t *testing.T) {
 		item.ResourceVersion = ""
 		item.CreationTimestamp = unversioned.Time{}
 
+		if item.Namespace != "ns" {
+			t.Errorf("Unexpected namespace: %s", item.Namespace)
+		}
+
 		var expected *api.Pod
 		switch item.Name {
 		case "foo":
@@ -208,6 +218,9 @@ func TestWatch(t *testing.T) {
 	podFooBis := makeTestPod("foo")
 	podFooBis.Spec.NodeName = "anotherFakeNode"
 
+	podFooNS2 := makeTestPod("foo")
+	podFooNS2.Namespace += "2"
+
 	// initialVersion is used to initate the watcher at the beginning of the world,
 	// which is not defined precisely in etcd.
 	initialVersion, err := cacher.LastSyncResourceVersion()
@@ -223,6 +236,9 @@ func TestWatch(t *testing.T) {
 	}
 	defer watcher.Stop()
 
+	// Create in another namespace first to make sure events from other namespaces don't get delivered
+	updatePod(t, etcdStorage, podFooNS2, nil)
+
 	fooCreated := updatePod(t, etcdStorage, podFoo, nil)
 	_ = updatePod(t, etcdStorage, podBar, nil)
 	fooUpdated := updatePod(t, etcdStorage, podFooPrime, fooCreated)
@@ -318,6 +334,13 @@ func TestFiltering(t *testing.T) {
 	podFooPrime.Labels = map[string]string{"filter": "foo"}
 	podFooPrime.Spec.NodeName = "fakeNode"
 
+	podFooNS2 := makeTestPod("foo")
+	podFooNS2.Namespace += "2"
+	podFooNS2.Labels = map[string]string{"filter": "foo"}
+
+	// Create in another namespace first to make sure events from other namespaces don't get delivered
+	updatePod(t, etcdStorage, podFooNS2, nil)
+
 	fooCreated := updatePod(t, etcdStorage, podFoo, nil)
 	fooFiltered := updatePod(t, etcdStorage, podFooFiltered, fooCreated)
 	fooUnfiltered := updatePod(t, etcdStorage, podFoo, fooFiltered)

pkg/storage/util.go

@@ -19,6 +19,7 @@ package storage
 import (
 	"fmt"
 	"strconv"
+	"strings"
 
 	"k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/meta"
@@ -90,3 +91,28 @@ func NoNamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
 	}
 	return prefix + "/" + name, nil
 }
+
+// hasPathPrefix returns true if the string matches pathPrefix exactly, or if is prefixed with pathPrefix at a path segment boundary
+func hasPathPrefix(s, pathPrefix string) bool {
+	// Short circuit if s doesn't contain the prefix at all
+	if !strings.HasPrefix(s, pathPrefix) {
+		return false
+	}
+
+	pathPrefixLength := len(pathPrefix)
+
+	if len(s) == pathPrefixLength {
+		// Exact match
+		return true
+	}
+	if strings.HasSuffix(pathPrefix, "/") {
+		// pathPrefix already ensured a path segment boundary
+		return true
+	}
+	if s[pathPrefixLength:pathPrefixLength+1] == "/" {
+		// The next character in s is a path segment boundary
+		// Check this instead of normalizing pathPrefix to avoid allocating on every call
+		return true
+	}
+	return false
+}

pkg/storage/util_test.go

@@ -55,3 +55,51 @@ func TestEtcdParseWatchResourceVersion(t *testing.T) {
 		}
 	}
 }
+
+func TestHasPathPrefix(t *testing.T) {
+	validTestcases := []struct {
+		s      string
+		prefix string
+	}{
+		// Exact matches
+		{"", ""},
+		{"a", "a"},
+		{"a/", "a/"},
+		{"a/../", "a/../"},
+
+		// Path prefix matches
+		{"a/b", "a"},
+		{"a/b", "a/"},
+		{"中文/", "中文"},
+	}
+	for i, tc := range validTestcases {
+		if !hasPathPrefix(tc.s, tc.prefix) {
+			t.Errorf(`%d: Expected hasPathPrefix("%s","%s") to be true`, i, tc.s, tc.prefix)
+		}
+	}
+
+	invalidTestcases := []struct {
+		s      string
+		prefix string
+	}{
+		// Mismatch
+		{"a", "b"},
+
+		// Dir requirement
+		{"a", "a/"},
+
+		// Prefix mismatch
+		{"ns2", "ns"},
+		{"ns2", "ns/"},
+		{"中文文", "中文"},
+
+		// Ensure no normalization is applied
+		{"a/c/../b/", "a/b/"},
+		{"a/", "a/b/.."},
+	}
+	for i, tc := range invalidTestcases {
+		if hasPathPrefix(tc.s, tc.prefix) {
+			t.Errorf(`%d: Expected hasPathPrefix("%s","%s") to be false`, i, tc.s, tc.prefix)
+		}
+	}
+}