From ed5fd905f2b42e9919d99c40a1cb25014f0a7f89 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Wed, 13 Apr 2022 10:57:02 -0700 Subject: [PATCH] pkg/storage/etcd3: validate revision invariant We must ensure that we notice if the etcd behavior on linearized reads changes. Signed-off-by: Steve Kuznetsov --- .../pkg/storage/etcd3/linearized_read_test.go | 64 +++++++++++++++++++ .../pkg/storage/etcd3/watcher_test.go | 18 +++++- 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/storage/etcd3/linearized_read_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/linearized_read_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/linearized_read_test.go new file mode 100644 index 00000000000..bb1b9df7818 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/linearized_read_test.go @@ -0,0 +1,64 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package etcd3 + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/apis/example" + "k8s.io/apiserver/pkg/storage" +) + +func TestLinearizedReadRevisionInvariant(t *testing.T) { + // The etcd documentation [1] states that "linearized requests must go through the Raft consensus process." + // A full round of Raft consensus adds a new item to the Raft log, some of which is surfaced by etcd as a + // higher store revision in the response header. Kubernetes exposes this header revision in e.g. List calls, + // so it is ultimately client-facing. By default, all the requests that our *etcd3.store{} issues are + // linearized. However, this also includes *read* requests, and we would not expect non-mutating requests + // against etcd to, by "go[ing] through the Raft consensus process," result in a higher resource version on + // List calls. Today, the mechanism etcd uses to increment the store revision ensures that linearized reads + // do *not* bump the key-value store revision. This test exists to ensure that we notice if this implementation + // detail ever changes. + // [1] https://etcd.io/docs/v3.5/learning/api_guarantees/#isolation-level-and-consistency-of-replicas + ctx, store, etcdClient := testSetup(t) + + key := "/testkey" + out := &example.Pod{} + obj := &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo", SelfLink: "testlink"}} + + if err := store.Create(ctx, key, obj, out, 0); err != nil { + t.Fatalf("Set failed: %v", err) + } + originalRevision := out.ResourceVersion + + for i := 0; i < 5; i++ { + if _, err := etcdClient.KV.Get(ctx, key); err != nil { // this is by default linearizable, the only option the client library exposes is WithSerializable() to make it *not* a linearized read + t.Fatalf("failed to get key: %v", err) + } + } + + list := &example.PodList{} + if err := store.GetList(ctx, "/", storage.ListOptions{Predicate: storage.Everything, Recursive: true}, list); err != nil { + t.Errorf("Unexpected List error: %v", err) + } + finalRevision := list.ResourceVersion + + if originalRevision != finalRevision { + t.Fatalf("original revision (%s) did not match final revision after linearized reads (%s)", originalRevision, finalRevision) + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go index 27f281808e7..4b5a0ff119b 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher_test.go @@ -343,6 +343,7 @@ func TestProgressNotify(t *testing.T) { if err := store.Create(ctx, key, input, out, 0); err != nil { t.Fatalf("Create failed: %v", err) } + validateResourceVersion := resourceVersionNotOlderThan(out.ResourceVersion) opts := storage.ListOptions{ ResourceVersion: out.ResourceVersion, @@ -353,12 +354,27 @@ func TestProgressNotify(t *testing.T) { if err != nil { t.Fatalf("Watch failed: %v", err) } + + // when we send a bookmark event, the client expects the event to contain an + // object of the correct type, but with no fields set other than the resourceVersion testCheckResultFunc(t, watch.Bookmark, w, func(object runtime.Object) error { + // first, check that we have the correct resource version + obj, ok := object.(metav1.Object) + if !ok { + return fmt.Errorf("got %T, not metav1.Object", object) + } + if err := validateResourceVersion(obj.GetResourceVersion()); err != nil { + return err + } + + // then, check that we have the right type and content pod, ok := object.(*example.Pod) if !ok { return fmt.Errorf("got %T, not *example.Pod", object) } - return resourceVersionNotOlderThan(out.ResourceVersion)(pod.ResourceVersion) + pod.ResourceVersion = "" + expectNoDiff(t, "bookmark event should contain an object with no fields set other than resourceVersion", newPod(), pod) + return nil }) }