From 6d5aece4d81cc2b23308dd034d0c9276feb5cc55 Mon Sep 17 00:00:00 2001 From: Marek Siarkowicz Date: Thu, 11 Sep 2025 13:42:19 +0200 Subject: [PATCH] Ensure keys used in storage and cacher start with resourcePrefix --- .../apiserver/pkg/storage/cacher/cacher.go | 19 ++++++++--- .../cacher/cacher_testing_utils_test.go | 2 +- .../apiserver/pkg/storage/etcd3/store.go | 8 ++++- .../apiserver/pkg/storage/etcd3/store_test.go | 30 ++++++++++++----- .../apiserver/pkg/storage/interfaces.go | 5 ++- .../pkg/storage/testing/store_tests.go | 33 ++++++++++++++++++- 6 files changed, 81 insertions(+), 16 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go index 267e5b37358..a5f7ab6d692 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go @@ -21,6 +21,7 @@ import ( "fmt" "net/http" "reflect" + "strings" "sync" "time" @@ -373,8 +374,18 @@ func NewCacherFromConfig(config Config) (*Cacher, error) { config.Clock = clock.RealClock{} } objType := reflect.TypeOf(obj) + resourcePrefix := config.ResourcePrefix + if resourcePrefix == "" { + return nil, fmt.Errorf("resourcePrefix cannot be empty") + } + if resourcePrefix == "/" { + return nil, fmt.Errorf("resourcePrefix cannot be /") + } + if !strings.HasPrefix(resourcePrefix, "/") { + return nil, fmt.Errorf("resourcePrefix needs to start from /") + } cacher := &Cacher{ - resourcePrefix: config.ResourcePrefix, + resourcePrefix: resourcePrefix, ready: newReady(config.Clock), storage: config.Storage, objectType: objType, @@ -425,8 +436,8 @@ func NewCacherFromConfig(config Config) (*Cacher, error) { watchCache := newWatchCache( config.KeyFunc, cacher.processEvent, config.GetAttrsFunc, config.Versioner, config.Indexers, config.Clock, eventFreshDuration, config.GroupResource, progressRequester, config.Storage.GetCurrentResourceVersion) - listerWatcher := NewListerWatcher(config.Storage, config.ResourcePrefix, config.NewListFunc, contextMetadata) - reflectorName := "storage/cacher.go:" + config.ResourcePrefix + listerWatcher := NewListerWatcher(config.Storage, resourcePrefix, config.NewListFunc, contextMetadata) + reflectorName := "storage/cacher.go:" + resourcePrefix reflector := cache.NewNamedReflector(reflectorName, listerWatcher, obj, watchCache, 0) // Configure reflector's pager to for an appropriate pagination chunk size for fetching data from @@ -1169,7 +1180,7 @@ func (c *Cacher) Stop() { } func (c *Cacher) prepareKey(key string, recursive bool) (string, error) { - return storage.PrepareKey(key, recursive) + return storage.PrepareKey(c.resourcePrefix, key, recursive) } func forgetWatcher(c *Cacher, w *cacheWatcher, index int, scope namespacedName, triggerValue string, triggerSupported bool) func(bool) { diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_testing_utils_test.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_testing_utils_test.go index 7809c6909c0..245e9a0b99f 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_testing_utils_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_testing_utils_test.go @@ -70,7 +70,7 @@ func newEtcdTestStorage(t testing.TB, prefix string) (*etcd3testing.EtcdTestServ newPod, newPodList, prefix, - "/pods", + "/pods/", schema.GroupResource{Resource: "pods"}, identity.NewEncryptCheckTransformer(), etcd3.NewDefaultLeaseManagerConfig(), 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 61b0c6536ec..5e5b72d771f 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go @@ -157,6 +157,12 @@ func New(c *kubernetes.Client, compactor Compactor, codec runtime.Codec, newFunc if resourcePrefix == "" { return nil, fmt.Errorf("resourcePrefix cannot be empty") } + if resourcePrefix == "/" { + return nil, fmt.Errorf("resourcePrefix cannot be /") + } + if !strings.HasPrefix(resourcePrefix, "/") { + return nil, fmt.Errorf("resourcePrefix needs to start from /") + } listErrAggrFactory := defaultListErrorAggregatorFactory if utilfeature.DefaultFeatureGate.Enabled(features.AllowUnsafeMalformedObjectDeletion) { @@ -1105,7 +1111,7 @@ func (s *store) validateMinimumResourceVersion(minimumResourceVersion string, ac } func (s *store) prepareKey(key string, recursive bool) (string, error) { - key, err := storage.PrepareKey(key, recursive) + key, err := storage.PrepareKey(s.resourcePrefix, key, recursive) 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 9d7b41be8c1..04e9f64df4b 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 @@ -662,16 +662,30 @@ func testSetup(t testing.TB, opts ...setupOption) (context.Context, *store, *kub func TestPrepareKey(t *testing.T) { validKeys := []string{ - "/foo/bar/baz/a.b.c/", - "/foo", - "foo/bar/baz", - "/foo/bar..baz/", - "/foo/bar..", - "foo", - "foo/bar", - "/foo/bar/", + "/pods/foo/bar/baz/a.b.c/", + "/pods/foo", + "/pods/foo/bar/baz", + "/pods/foo/bar..baz/", + "/pods/foo/bar..", + "/pods/foo", + "/pods/foo/bar", + "/pods/foo/bar/", + "/pods/", } invalidKeys := []string{ + "/pods", + "/pods/foo/bar/../a.b.c/", + "/pods/..", + "/pods/../", + "/pods/foo/bar/..", + "/pods/../foo/bar", + "/pods/../foo", + "/pods/foo/bar/../", + "/pods/.", + "/pods/./", + "/pods/foo/.", + "/pods/./bar", + "/pods/foo/./bar/", "/foo/bar/../a.b.c/", "..", "/..", diff --git a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go index d8ea90bf2db..be9980cfc7c 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/interfaces.go @@ -392,7 +392,7 @@ type Stats struct { EstimatedAverageObjectSizeBytes int64 } -func PrepareKey(key string, recursive bool) (string, error) { +func PrepareKey(resourcePrefix, key string, recursive bool) (string, error) { if key == ".." || strings.HasPrefix(key, "../") || strings.HasSuffix(key, "/..") || @@ -415,5 +415,8 @@ func PrepareKey(key string, recursive bool) (string, error) { if recursive && !strings.HasSuffix(key, "/") { key += "/" } + if !strings.HasPrefix(key, resourcePrefix) { + return "", fmt.Errorf("invalid key: %q lacks resource prefix: %q", key, resourcePrefix) + } return key, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go b/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go index f0a76dada70..5c6e457559f 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/testing/store_tests.go @@ -3785,6 +3785,10 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface require.ErrorContains(t, store.Create(ctx, "/", createObj, createOut, 0), "empty key") require.ErrorContains(t, store.Create(ctx, ".", createObj, createOut, 0), "invalid key") require.ErrorContains(t, store.Create(ctx, "..", createObj, createOut, 0), "invalid key") + require.ErrorContains(t, store.Create(ctx, "pods", createObj, createOut, 0), "lacks resource prefix") + require.ErrorContains(t, store.Create(ctx, "/pods", createObj, createOut, 0), "lacks resource prefix") + require.ErrorContains(t, store.Create(ctx, "/pods.apps", createObj, createOut, 0), "lacks resource prefix") + require.ErrorContains(t, store.Create(ctx, "/foo/", createObj, createOut, 0), "lacks resource prefix") require.NoError(t, store.Create(ctx, "/pods/", createObj, createOut, 0)) require.NoError(t, store.Create(ctx, "/pods/name", createObj, createOut, 0)) require.NoError(t, store.Create(ctx, "/pods/namespace", createObj, createOut, 0)) @@ -3792,10 +3796,15 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface listOut := &example.PodList{} recursiveListOpts := storage.ListOptions{Predicate: storage.Everything, Recursive: true} + nonRecursiveListOpts := storage.ListOptions{Predicate: storage.Everything, Recursive: false} require.ErrorContains(t, store.GetList(ctx, "", recursiveListOpts, listOut), "empty key") require.ErrorContains(t, store.GetList(ctx, "/", recursiveListOpts, listOut), "empty key") require.ErrorContains(t, store.GetList(ctx, ".", recursiveListOpts, listOut), "invalid key") require.ErrorContains(t, store.GetList(ctx, "..", recursiveListOpts, listOut), "invalid key") + require.ErrorContains(t, store.GetList(ctx, "pods", recursiveListOpts, listOut), "lacks resource prefix") + require.ErrorContains(t, store.GetList(ctx, "/pods.apps", recursiveListOpts, listOut), "lacks resource prefix") + require.ErrorContains(t, store.GetList(ctx, "/foo/", recursiveListOpts, listOut), "lacks resource prefix") + require.ErrorContains(t, store.GetList(ctx, "/pods", nonRecursiveListOpts, listOut), "lacks resource prefix") require.NoError(t, store.GetList(ctx, "/pods", recursiveListOpts, listOut)) require.NoError(t, store.GetList(ctx, "/pods/", recursiveListOpts, listOut)) require.NoError(t, store.GetList(ctx, "/pods/namespace", recursiveListOpts, listOut)) @@ -3807,6 +3816,10 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface require.ErrorContains(t, store.Get(ctx, "/", getOpts, getOut), "empty key") require.ErrorContains(t, store.Get(ctx, ".", getOpts, getOut), "invalid key") require.ErrorContains(t, store.Get(ctx, "..", getOpts, getOut), "invalid key") + require.ErrorContains(t, store.Get(ctx, "pods", getOpts, getOut), "lacks resource prefix") + require.ErrorContains(t, store.Get(ctx, "/pods", getOpts, getOut), "lacks resource prefix") + require.ErrorContains(t, store.Get(ctx, "/pods.apps", getOpts, getOut), "lacks resource prefix") + require.ErrorContains(t, store.Get(ctx, "/foo/", getOpts, getOut), "lacks resource prefix") require.NoError(t, store.Get(ctx, "/pods/", getOpts, getOut)) require.NoError(t, store.Get(ctx, "/pods/namespace", getOpts, getOut)) require.NoError(t, store.Get(ctx, "/pods/namespace/name", getOpts, getOut)) @@ -3819,7 +3832,17 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface require.ErrorContains(t, err, "invalid key") _, err = store.Watch(ctx, "..", recursiveListOpts) require.ErrorContains(t, err, "invalid key") - w, err := store.Watch(ctx, "/pods/", recursiveListOpts) + _, err = store.Watch(ctx, "pods", recursiveListOpts) + require.ErrorContains(t, err, "lacks resource prefix") + _, err = store.Watch(ctx, "/pods.apps", recursiveListOpts) + require.ErrorContains(t, err, "lacks resource prefix") + _, err = store.Watch(ctx, "/foo/", recursiveListOpts) + require.ErrorContains(t, err, "lacks resource prefix") + _, err = store.Watch(ctx, "/pods", nonRecursiveListOpts) + require.ErrorContains(t, err, "lacks resource prefix") + w, err := store.Watch(ctx, "/pods", recursiveListOpts) + require.NoError(t, err) + w.Stop() require.NoError(t, err) w.Stop() w, err = store.Watch(ctx, "/pods/namespace", recursiveListOpts) @@ -3838,6 +3861,10 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface require.ErrorContains(t, store.GuaranteedUpdate(ctx, "/", updateOut, false, nil, updateFunc, nil), "empty key") require.ErrorContains(t, store.GuaranteedUpdate(ctx, ".", updateOut, false, nil, updateFunc, nil), "invalid key") require.ErrorContains(t, store.GuaranteedUpdate(ctx, "..", updateOut, false, nil, updateFunc, nil), "invalid key") + require.ErrorContains(t, store.GuaranteedUpdate(ctx, "pods", updateOut, false, nil, updateFunc, nil), "lacks resource prefix") + require.ErrorContains(t, store.GuaranteedUpdate(ctx, "/pods", updateOut, false, nil, updateFunc, nil), "lacks resource prefix") + require.ErrorContains(t, store.GuaranteedUpdate(ctx, "/pods.apps", updateOut, false, nil, updateFunc, nil), "lacks resource prefix") + require.ErrorContains(t, store.GuaranteedUpdate(ctx, "/foo/", updateOut, false, nil, updateFunc, nil), "lacks resource prefix") require.NoError(t, store.GuaranteedUpdate(ctx, "/pods/", updateOut, false, nil, updateFunc, nil)) require.NoError(t, store.GuaranteedUpdate(ctx, "/pods/namespace", updateOut, false, nil, updateFunc, nil)) require.NoError(t, store.GuaranteedUpdate(ctx, "/pods/namespace/name", updateOut, false, nil, updateFunc, nil)) @@ -3851,6 +3878,10 @@ func RunTestKeySchema(ctx context.Context, t *testing.T, store storage.Interface require.ErrorContains(t, store.Delete(ctx, "/", deleteOut, nil, deleteFunc, nil, deleteOpts), "empty key") require.ErrorContains(t, store.Delete(ctx, ".", deleteOut, nil, deleteFunc, nil, deleteOpts), "invalid key") require.ErrorContains(t, store.Delete(ctx, "..", deleteOut, nil, deleteFunc, nil, deleteOpts), "invalid key") + require.ErrorContains(t, store.Delete(ctx, "pods", deleteOut, nil, deleteFunc, nil, deleteOpts), "lacks resource prefix") + require.ErrorContains(t, store.Delete(ctx, "/pods", deleteOut, nil, deleteFunc, nil, deleteOpts), "lacks resource prefix") + require.ErrorContains(t, store.Delete(ctx, "/pods.apps", deleteOut, nil, deleteFunc, nil, deleteOpts), "lacks resource prefix") + require.ErrorContains(t, store.Delete(ctx, "/foo", deleteOut, nil, deleteFunc, nil, deleteOpts), "lacks resource prefix") require.NoError(t, store.Delete(ctx, "/pods/", deleteOut, nil, deleteFunc, nil, deleteOpts)) require.NoError(t, store.Delete(ctx, "/pods/namespace", deleteOut, nil, deleteFunc, nil, deleteOpts)) require.NoError(t, store.Delete(ctx, "/pods/namespace/name", deleteOut, nil, deleteFunc, nil, deleteOpts))