Test recursive in TestGetListCacheBypass and separate overrides

Author: serathius

staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go

@@ -218,53 +218,87 @@ func (d *dummyCacher) Ready() bool {
 
 func TestGetListCacheBypass(t *testing.T) {
 	type opts struct {
+		Recursive            bool
 		ResourceVersion      string
 		ResourceVersionMatch metav1.ResourceVersionMatch
 		Limit                int64
 		Continue             string
 	}
+	toInternalOpts := func(opt opts) *internalversion.ListOptions {
+		return &internalversion.ListOptions{
+			ResourceVersion:      opt.ResourceVersion,
+			ResourceVersionMatch: opt.ResourceVersionMatch,
+			Limit:                opt.Limit,
+			Continue:             opt.Continue,
+		}
+	}
+
+	toStorageOpts := func(opt opts) storage.ListOptions {
+		return storage.ListOptions{
+			Recursive:            opt.Recursive,
+			ResourceVersion:      opt.ResourceVersion,
+			ResourceVersionMatch: opt.ResourceVersionMatch,
+			Predicate: storage.SelectionPredicate{
+				Continue: opt.Continue,
+				Limit:    opt.Limit,
+			},
+		}
+	}
+
+	keyPrefix := "/pods/"
+	continueOnRev1, err := storage.EncodeContinue(keyPrefix+"foo", keyPrefix, 1)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	testCases := map[opts]bool{}
-	testCases[opts{}] = false
-	testCases[opts{Limit: 100}] = false
-	testCases[opts{Continue: "continue"}] = true
-	testCases[opts{Limit: 100, Continue: "continue"}] = true
+	testCases[opts{}] = true
+	testCases[opts{Limit: 100}] = true
+	testCases[opts{Continue: continueOnRev1}] = true
+	testCases[opts{Limit: 100, Continue: continueOnRev1}] = true
 	testCases[opts{ResourceVersion: "0"}] = false
 	testCases[opts{ResourceVersion: "0", Limit: 100}] = false
-	testCases[opts{ResourceVersion: "0", Continue: "continue"}] = true
-	testCases[opts{ResourceVersion: "0", Limit: 100, Continue: "continue"}] = true
+	testCases[opts{ResourceVersion: "0", Continue: continueOnRev1}] = true
+	testCases[opts{ResourceVersion: "0", Limit: 100, Continue: continueOnRev1}] = true
 	testCases[opts{ResourceVersion: "0", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan}] = false
 	testCases[opts{ResourceVersion: "0", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, Limit: 100}] = false
 	testCases[opts{ResourceVersion: "1"}] = false
 	testCases[opts{ResourceVersion: "1", Limit: 100}] = true
-	testCases[opts{ResourceVersion: "1", Continue: "continue"}] = true
-	testCases[opts{ResourceVersion: "1", Limit: 100, Continue: "continue"}] = true
 	testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchExact}] = true
 	testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchExact, Limit: 100}] = true
 	testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan}] = false
 	testCases[opts{ResourceVersion: "1", ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, Limit: 100}] = false
 
+	// Bypass for most requests doesn't depend on Recursive
+	for opts, expectBypass := range testCases {
+		opts.Recursive = true
+		testCases[opts] = expectBypass
+	}
+	// Continue is ignored on non recursive LIST
+	testCases[opts{ResourceVersion: "1", Continue: continueOnRev1}] = true
+	testCases[opts{ResourceVersion: "1", Continue: continueOnRev1, Limit: 100}] = true
+
 	for _, rv := range []string{"", "0", "1"} {
 		for _, match := range []metav1.ResourceVersionMatch{"", metav1.ResourceVersionMatchExact, metav1.ResourceVersionMatchNotOlderThan} {
-			for _, c := range []string{"", "continue"} {
+			for _, continueKey := range []string{"", continueOnRev1} {
 				for _, limit := range []int64{0, 100} {
-					errs := validation.ValidateListOptions(&internalversion.ListOptions{
-						ResourceVersion:      rv,
-						ResourceVersionMatch: match,
-						Limit:                limit,
-						Continue:             c,
-					}, false)
-					if len(errs) != 0 {
-						continue
-					}
-					opt := opts{
-						ResourceVersion:      rv,
-						ResourceVersionMatch: match,
-						Limit:                limit,
-						Continue:             c,
-					}
-					_, found := testCases[opt]
-					if !found {
-						t.Errorf("Test case not covered, but passes validation: %+v", opt)
+					for _, recursive := range []bool{true, false} {
+						opt := opts{
+							Recursive:            recursive,
+							ResourceVersion:      rv,
+							ResourceVersionMatch: match,
+							Limit:                limit,
+							Continue:             continueKey,
+						}
+						if errs := validation.ValidateListOptions(toInternalOpts(opt), false); len(errs) != 0 {
+							continue
+						}
+						if _, _, err = storage.ValidateListOptions(keyPrefix, storage.APIObjectVersioner{}, toStorageOpts(opt)); err != nil {
+							continue
+						}
+						_, found := testCases[opt]
+						if !found {
+							t.Errorf("Test case not covered, but passes validation: %+v", opt)
+						}
 					}
 
 				}
@@ -273,35 +307,37 @@ func TestGetListCacheBypass(t *testing.T) {
 	}
 
 	for opt := range testCases {
-		errs := validation.ValidateListOptions(&internalversion.ListOptions{
-			ResourceVersion:      opt.ResourceVersion,
-			ResourceVersionMatch: opt.ResourceVersionMatch,
-			Limit:                opt.Limit,
-			Continue:             opt.Continue,
-		}, false)
-		if len(errs) != 0 {
+		if errs := validation.ValidateListOptions(toInternalOpts(opt), false); len(errs) != 0 {
+			t.Errorf("Invalid LIST request that should not be tested %+v", opt)
+			continue
+		}
+		if _, _, err = storage.ValidateListOptions(keyPrefix, storage.APIObjectVersioner{}, toStorageOpts(opt)); err != nil {
 			t.Errorf("Invalid LIST request that should not be tested %+v", opt)
 			continue
 		}
 	}
 
-	t.Run("ConsistentListFromStorage", func(t *testing.T) {
-		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, false)
-		testCases[opts{}] = true
-		testCases[opts{Limit: 100}] = true
+	runTestCases := func(t *testing.T, testcases map[opts]bool, overrides ...map[opts]bool) {
 		for opt, expectBypass := range testCases {
-			testGetListCacheBypass(t, storage.ListOptions{
-				ResourceVersion:      opt.ResourceVersion,
-				ResourceVersionMatch: opt.ResourceVersionMatch,
-				Predicate: storage.SelectionPredicate{
-					Continue: opt.Continue,
-					Limit:    opt.Limit,
-				},
-			}, expectBypass)
+			for _, override := range overrides {
+				if bypass, ok := override[opt]; ok {
+					expectBypass = bypass
+				}
+			}
+			testGetListCacheBypass(t, toStorageOpts(opt), expectBypass)
 		}
+	}
+	consistentListFromCacheOverrides := map[opts]bool{}
+	for _, recursive := range []bool{true, false} {
+		consistentListFromCacheOverrides[opts{Recursive: recursive}] = false
+		consistentListFromCacheOverrides[opts{Limit: 100, Recursive: recursive}] = false
+	}
 
+	t.Run("ConsistentListFromCache=false", func(t *testing.T) {
+		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, false)
+		runTestCases(t, testCases)
 	})
-	t.Run("ConsistentListFromCache", func(t *testing.T) {
+	t.Run("ConsistentListFromCache=true", func(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ConsistentListFromCache, true)
 
 		// TODO(p0lyn0mial): the following tests assume that etcdfeature.DefaultFeatureSupportChecker.Supports(storage.RequestWatchProgress)
@@ -311,19 +347,7 @@ func TestGetListCacheBypass(t *testing.T) {
 		// However in CI all test are run and there must be a test(s) that properly
 		// initialize the storage layer so that the mentioned method evaluates to true
 		forceRequestWatchProgressSupport(t)
-
-		testCases[opts{}] = false
-		testCases[opts{Limit: 100}] = false
-		for opt, expectBypass := range testCases {
-			testGetListCacheBypass(t, storage.ListOptions{
-				ResourceVersion:      opt.ResourceVersion,
-				ResourceVersionMatch: opt.ResourceVersionMatch,
-				Predicate: storage.SelectionPredicate{
-					Continue: opt.Continue,
-					Limit:    opt.Limit,
-				},
-			}, expectBypass)
-		}
+		runTestCases(t, testCases, consistentListFromCacheOverrides)
 	})
 }