From ccb607f06b91496d02a3b94253261e03e3280630 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Fri, 7 Mar 2025 12:26:05 +0100 Subject: [PATCH] Unify ListOptions validation between cache and etcd --- .../apiserver/pkg/storage/etcd3/store.go | 53 +----- .../apiserver/pkg/storage/etcd3/store_test.go | 123 -------------- .../apiserver/pkg/storage/interfaces.go | 41 +++++ .../apiserver/pkg/storage/interfaces_test.go | 157 ++++++++++++++++++ 4 files changed, 200 insertions(+), 174 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go index 4adf797d8ca..ee5f3d67682 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/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 } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go index 8fcee6f4950..ee748252413 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store_test.go +++ b/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{} diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index 2c756c3cb37..0befb3fc6ea 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/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 +} diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces_test.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces_test.go index 5a55f20e8c4..8f4826694a5 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces_test.go @@ -20,6 +20,8 @@ import ( "errors" "strings" "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestPreconditionsCheckWithNilObject(t *testing.T) { @@ -37,3 +39,158 @@ func TestPreconditionsCheckWithNilObject(t *testing.T) { t.Errorf("expected error to contain %q", want) } } + +func TestValidateListOptions(t *testing.T) { + testCases := []struct { + name string + opts ListOptions + expectedError string + expectedRev int64 + expectedContinueKey string + }{ + { + name: "specifying resource version when using continue", + opts: ListOptions{ + Recursive: true, + ResourceVersion: "200", + Predicate: SelectionPredicate{ + Continue: encodeContinueOrDie("meta.k8s.io/v1", 100, "continue"), + }, + }, + expectedError: "specifying resource version is not allowed when using continue", + }, + { + name: "invalid resource version", + opts: ListOptions{ + ResourceVersion: "invalid", + }, + expectedError: "invalid resource version", + }, + { + name: "unknown ResourceVersionMatch value", + opts: ListOptions{ + ResourceVersion: "200", + ResourceVersionMatch: "unknown", + }, + expectedError: "unknown ResourceVersionMatch value", + }, + { + name: "use continueRV", + opts: ListOptions{ + ResourceVersion: "0", + Recursive: true, + Predicate: SelectionPredicate{ + Continue: encodeContinueOrDie("meta.k8s.io/v1", 100, "continue"), + }, + }, + expectedRev: 100, + expectedContinueKey: "continue", + }, + { + name: "use continueRV with empty rv", + opts: ListOptions{ + ResourceVersion: "", + Recursive: true, + Predicate: SelectionPredicate{ + Continue: encodeContinueOrDie("meta.k8s.io/v1", 100, "continue"), + }, + }, + expectedRev: 100, + expectedContinueKey: "continue", + }, + { + name: "continueRV = 0", + opts: ListOptions{ + ResourceVersion: "", + Recursive: true, + Predicate: SelectionPredicate{ + Continue: encodeContinueOrDie("meta.k8s.io/v1", 0, "continue"), + }, + }, + expectedError: "invalid continue token", + }, + { + name: "continueRV < 0", + opts: ListOptions{ + ResourceVersion: "", + Recursive: true, + Predicate: SelectionPredicate{ + Continue: encodeContinueOrDie("meta.k8s.io/v1", -1, "continue"), + }, + }, + expectedRev: 0, + expectedContinueKey: "continue", + }, + { + name: "default", + opts: ListOptions{}, + expectedRev: 0, + }, + { + name: "rev resolve to 0 if ResourceVersionMatchNotOlderThan", + opts: ListOptions{ + ResourceVersion: "200", + ResourceVersionMatch: metav1.ResourceVersionMatchNotOlderThan, + }, + expectedRev: 0, + }, + { + name: "specified rev if ResourceVersionMatchExact", + opts: ListOptions{ + ResourceVersion: "200", + ResourceVersionMatch: metav1.ResourceVersionMatchExact, + }, + expectedRev: 200, + }, + { + name: "rev resolve to 0 if not recursive", + opts: ListOptions{ + ResourceVersion: "200", + Predicate: SelectionPredicate{ + Limit: 1, + }, + }, + expectedRev: 0, + }, + { + name: "rev resolve to 0 if limit unspecified", + opts: ListOptions{ + ResourceVersion: "200", + Recursive: true, + }, + expectedRev: 0, + }, + { + name: "specified rev if recursive with limit", + opts: ListOptions{ + ResourceVersion: "200", + Recursive: true, + Predicate: SelectionPredicate{ + Limit: 1, + }, + }, + expectedRev: 200, + }, + } + for _, tt := range testCases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + withRev, continueKey, err := ValidateListOptions("", APIObjectVersioner{}, tt.opts) + 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 withRev != tt.expectedRev { + t.Errorf("%s: expecting rev = %d, but get %d", tt.name, tt.expectedRev, withRev) + } + if continueKey != tt.expectedContinueKey { + t.Errorf("%s: expecting continueKey = %q, but get %q", tt.name, tt.expectedContinueKey, continueKey) + } + }) + } +}