Merge pull request kubernetes#130637 from serathius/watchcache-unify-validation

Unify ListOptions validation between cache and etcd

Author: k8s-ci-robot

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

@@ -33,7 +33,6 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/fields"
@@ -645,47 +644,6 @@ func (s *store) ReadinessCheck() error {
 	return nil
 }
 
-// resolveGetListRev is used by GetList to resolve the rev to use in the client.KV.Get request.
-func (s *store) resolveGetListRev(continueKey string, continueRV int64, opts storage.ListOptions) (int64, error) {
-	var withRev int64
-	// Uses continueRV if this is a continuation request.
-	if len(continueKey) > 0 {
-		if len(opts.ResourceVersion) > 0 && opts.ResourceVersion != "0" {
-			return withRev, apierrors.NewBadRequest("specifying resource version is not allowed when using continue")
-		}
-		// If continueRV > 0, the LIST request needs a specific resource version.
-		// continueRV==0 is invalid.
-		// If continueRV < 0, the request is for the latest resource version.
-		if continueRV > 0 {
-			withRev = continueRV
-		}
-		return withRev, nil
-	}
-	// Returns 0 if ResourceVersion is not specified.
-	if len(opts.ResourceVersion) == 0 {
-		return withRev, nil
-	}
-	parsedRV, err := s.versioner.ParseResourceVersion(opts.ResourceVersion)
-	if err != nil {
-		return withRev, apierrors.NewBadRequest(fmt.Sprintf("invalid resource version: %v", err))
-	}
-
-	switch opts.ResourceVersionMatch {
-	case metav1.ResourceVersionMatchNotOlderThan:
-		// The not older than constraint is checked after we get a response from etcd,
-		// and returnedRV is then set to the revision we get from the etcd response.
-	case metav1.ResourceVersionMatchExact:
-		withRev = int64(parsedRV)
-	case "": // legacy case
-		if opts.Recursive && opts.Predicate.Limit > 0 && parsedRV > 0 {
-			withRev = int64(parsedRV)
-		}
-	default:
-		return withRev, fmt.Errorf("unknown ResourceVersionMatch value: %v", opts.ResourceVersionMatch)
-	}
-	return withRev, nil
-}
-
 func (s *store) GetCurrentResourceVersion(ctx context.Context) (uint64, error) {
 	emptyList := s.newListFunc()
 	pred := storage.SelectionPredicate{
@@ -753,15 +711,8 @@ func (s *store) GetList(ctx context.Context, key string, opts storage.ListOption
 	paging := opts.Predicate.Limit > 0
 	newItemFunc := getNewItemFunc(listObj, v)
 
-	var continueRV, withRev int64
-	var continueKey string
-	if opts.Recursive && len(opts.Predicate.Continue) > 0 {
-		continueKey, continueRV, err = storage.DecodeContinue(opts.Predicate.Continue, keyPrefix)
-		if err != nil {
-			return apierrors.NewBadRequest(fmt.Sprintf("invalid continue token: %v", err))
-		}
-	}
-	if withRev, err = s.resolveGetListRev(continueKey, continueRV, opts); err != nil {
+	withRev, continueKey, err := storage.ValidateListOptions(keyPrefix, s.versioner, opts)
+	if err != nil {
 		return err
 	}
 

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go

@@ -696,129 +696,6 @@ func TestInvalidKeys(t *testing.T) {
 	expectInvalidKey("Count", countErr)
 }
 
-func TestResolveGetListRev(t *testing.T) {
-	_, store, _ := testSetup(t)
-	testCases := []struct {
-		name          string
-		continueKey   string
-		continueRV    int64
-		rv            string
-		rvMatch       metav1.ResourceVersionMatch
-		recursive     bool
-		expectedError string
-		limit         int64
-		expectedRev   int64
-	}{
-		{
-			name:          "specifying resource versionwhen using continue",
-			continueKey:   "continue",
-			continueRV:    100,
-			rv:            "200",
-			expectedError: "specifying resource version is not allowed when using continue",
-		},
-		{
-			name:          "invalid resource version",
-			rv:            "invalid",
-			expectedError: "invalid resource version",
-		},
-		{
-			name:          "unknown ResourceVersionMatch value",
-			rv:            "200",
-			rvMatch:       "unknown",
-			expectedError: "unknown ResourceVersionMatch value",
-		},
-		{
-			name:        "use continueRV",
-			continueKey: "continue",
-			continueRV:  100,
-			rv:          "0",
-			expectedRev: 100,
-		},
-		{
-			name:        "use continueRV with empty rv",
-			continueKey: "continue",
-			continueRV:  100,
-			rv:          "",
-			expectedRev: 100,
-		},
-		{
-			name:        "continueRV = 0",
-			continueKey: "continue",
-			continueRV:  0,
-			rv:          "",
-			expectedRev: 0,
-		},
-		{
-			name:        "continueRV < 0",
-			continueKey: "continue",
-			continueRV:  -1,
-			rv:          "",
-			expectedRev: 0,
-		},
-		{
-			name:        "default",
-			expectedRev: 0,
-		},
-		{
-			name:        "rev resolve to 0 if ResourceVersionMatchNotOlderThan",
-			rv:          "200",
-			rvMatch:     metav1.ResourceVersionMatchNotOlderThan,
-			expectedRev: 0,
-		},
-		{
-			name:        "specified rev if ResourceVersionMatchExact",
-			rv:          "200",
-			rvMatch:     metav1.ResourceVersionMatchExact,
-			expectedRev: 200,
-		},
-		{
-			name:        "rev resolve to 0 if not recursive",
-			rv:          "200",
-			limit:       1,
-			expectedRev: 0,
-		},
-		{
-			name:        "rev resolve to 0 if limit unspecified",
-			rv:          "200",
-			recursive:   true,
-			expectedRev: 0,
-		},
-		{
-			name:        "specified rev if recursive with limit",
-			rv:          "200",
-			recursive:   true,
-			limit:       1,
-			expectedRev: 200,
-		},
-	}
-	for _, tt := range testCases {
-		tt := tt
-		t.Run(tt.name, func(t *testing.T) {
-			storageOpts := storage.ListOptions{
-				ResourceVersion:      tt.rv,
-				ResourceVersionMatch: tt.rvMatch,
-				Predicate: storage.SelectionPredicate{
-					Limit: tt.limit,
-				},
-				Recursive: tt.recursive,
-			}
-			rev, err := store.resolveGetListRev(tt.continueKey, tt.continueRV, storageOpts)
-			if len(tt.expectedError) > 0 {
-				if err == nil || !strings.Contains(err.Error(), tt.expectedError) {
-					t.Fatalf("expected error: %s, but got: %v", tt.expectedError, err)
-				}
-				return
-			}
-			if err != nil {
-				t.Fatalf("resolveRevForGetList failed: %v", err)
-			}
-			if rev != tt.expectedRev {
-				t.Errorf("%s: expecting rev = %d, but get %d", tt.name, tt.expectedRev, rev)
-			}
-		})
-	}
-}
-
 func BenchmarkStore_GetList(b *testing.B) {
 	generateBigPod := func(index int, total int, expect int) runtime.Object {
 		l := map[string]string{}

staging/src/k8s.io/apiserver/pkg/storage/interfaces.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
@@ -329,3 +330,43 @@ type DeleteOptions struct {
 	// object which otherwise can not be deleted using the normal flow
 	IgnoreStoreReadError bool
 }
+
+func ValidateListOptions(keyPrefix string, versioner Versioner, opts ListOptions) (withRev int64, continueKey string, err error) {
+	if opts.Recursive && len(opts.Predicate.Continue) > 0 {
+		continueKey, continueRV, err := DecodeContinue(opts.Predicate.Continue, keyPrefix)
+		if err != nil {
+			return 0, "", apierrors.NewBadRequest(fmt.Sprintf("invalid continue token: %v", err))
+		}
+		if len(opts.ResourceVersion) > 0 && opts.ResourceVersion != "0" {
+			return 0, "", apierrors.NewBadRequest("specifying resource version is not allowed when using continue")
+		}
+		// If continueRV > 0, the LIST request needs a specific resource version.
+		// continueRV==0 is invalid.
+		// If continueRV < 0, the request is for the latest resource version.
+		if continueRV > 0 {
+			withRev = continueRV
+		}
+		return withRev, continueKey, nil
+	}
+	if len(opts.ResourceVersion) == 0 {
+		return withRev, "", nil
+	}
+	parsedRV, err := versioner.ParseResourceVersion(opts.ResourceVersion)
+	if err != nil {
+		return withRev, "", apierrors.NewBadRequest(fmt.Sprintf("invalid resource version: %v", err))
+	}
+	switch opts.ResourceVersionMatch {
+	case metav1.ResourceVersionMatchNotOlderThan:
+		// The not older than constraint is checked after we get a response from etcd,
+		// and returnedRV is then set to the revision we get from the etcd response.
+	case metav1.ResourceVersionMatchExact:
+		withRev = int64(parsedRV)
+	case "": // legacy case
+		if opts.Recursive && opts.Predicate.Limit > 0 && parsedRV > 0 {
+			withRev = int64(parsedRV)
+		}
+	default:
+		return withRev, "", fmt.Errorf("unknown ResourceVersionMatch value: %v", opts.ResourceVersionMatch)
+	}
+	return withRev, "", nil
+}