From 6bfa727bee264f5d7e4471066c1b48a28d0c1929 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 9 Jan 2026 16:54:10 +0100 Subject: [PATCH] client-go testing: fix List+Watch support 56448506075c3db1d added support for List+Watch to a fake client-go instance. However, that support was not quite working yet as seen when analyzing a test flake: - List returned early when there were no objects, without adding the ResourceVersion. The ResourceVersion should have been "0" instead. - When encountering "" as ResourceVersion, Watch didn't deliver any objects. That was meant to preserve compatibility with clients which don't expect objects from a Watch, but the right semantic of "" is "Start at most recent", which includes delivering existing objects. Tests which meddle with the List implementation via a reactor (like clustertrustbundlepublisher) have to be aware that Watch now may return objects when given an empty ResourceVersion. --- .../publisher_test.go | 4 ++ .../client-go/dynamic/fake/simple_test.go | 2 +- .../src/k8s.io/client-go/testing/fixture.go | 38 ++++++++++++------- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/pkg/controller/certificates/clustertrustbundlepublisher/publisher_test.go b/pkg/controller/certificates/clustertrustbundlepublisher/publisher_test.go index b68557e6272..b9bac8efaad 100644 --- a/pkg/controller/certificates/clustertrustbundlepublisher/publisher_test.go +++ b/pkg/controller/certificates/clustertrustbundlepublisher/publisher_test.go @@ -313,6 +313,10 @@ func fakeKubeClientSetWithCTBList(t *testing.T, signerName string, ctbs ...runti } } + // Ensure that Watch doesn't return any objects either by bumping up the resource version + // beyond what the fake client uses. + retList.ResourceVersion = "1000" + return true, retList, nil }) diff --git a/staging/src/k8s.io/client-go/dynamic/fake/simple_test.go b/staging/src/k8s.io/client-go/dynamic/fake/simple_test.go index 50c6bd87e3b..825c01da8d7 100644 --- a/staging/src/k8s.io/client-go/dynamic/fake/simple_test.go +++ b/staging/src/k8s.io/client-go/dynamic/fake/simple_test.go @@ -369,7 +369,7 @@ func TestListWithNoFixturesAndTypedScheme(t *testing.T) { expectedList := &unstructured.UnstructuredList{} expectedList.SetGroupVersionKind(listGVK) - expectedList.SetResourceVersion("") // by product of the fake setting resource version + expectedList.SetResourceVersion("0") // No objects created so far. expectedList.SetContinue("") if diff := cmp.Diff(expectedList, list); diff != "" { diff --git a/staging/src/k8s.io/client-go/testing/fixture.go b/staging/src/k8s.io/client-go/testing/fixture.go index 7b8b614d3b2..b031ce9c80d 100644 --- a/staging/src/k8s.io/client-go/testing/fixture.go +++ b/staging/src/k8s.io/client-go/testing/fixture.go @@ -322,6 +322,7 @@ type tracker struct { // but this is not how fake client-go has traditionally worked and starting to do // that now might break tests. type versionedObject struct { + // resourceVersion is always > 0 for a stored object. resourceVersion int64 runtime.Object } @@ -368,6 +369,10 @@ func (t *tracker) List(gvr schema.GroupVersionResource, gvk schema.GroupVersionK t.lock.RLock() defer t.lock.RUnlock() + if listMeta, err := meta.ListAccessor(list); err == nil { + listMeta.SetResourceVersion(fmt.Sprintf("%d", t.resourceVersions[gvr])) + } + objs, ok := t.objects[gvr] if !ok { return list, nil @@ -384,9 +389,6 @@ func (t *tracker) List(gvr schema.GroupVersionResource, gvk schema.GroupVersionK if err := meta.SetList(list, matchingObjs); err != nil { return nil, err } - if listMeta, err := meta.ListAccessor(list); err == nil { - listMeta.SetResourceVersion(fmt.Sprintf("%d", t.resourceVersions[gvr])) - } return list.DeepCopyObject(), nil } @@ -395,16 +397,26 @@ func (t *tracker) Watch(gvr schema.GroupVersionResource, ns string, opts ...meta if err != nil { return nil, err } + // By default, emulate the traditional behavior of the tracker and don't deliver - // *any* existing objects. This ensures compatibility which users - // which don't pass options and then also don't expect the additional objects. - resourceVersion := int64(-1) - if len(opts) > 0 && opts[0].ResourceVersion != "" { - rv, err := strconv.ParseInt(opts[0].ResourceVersion, 10, 64) - if err != nil { - return nil, fmt.Errorf("invalid ResourceVersion %q in ListOptions, must be int64: %w", opts[0].ResourceVersion, err) + // *any* existing objects unless list options are provided. + addExisting := false + addFromRV := int64(0) + if len(opts) > 0 { + // Providing options, as the generated client-go fake does, enables support + // for existing objects depending on the resource version. + // + // The default if ResourceVersion is empty is "start at most recent", + // which includes delivering all existing objects. addFromRV == 0 + // matches all objects below because all stored objects have addFromRV > 0. + addExisting = true + if opts[0].ResourceVersion != "" { + rv, err := strconv.ParseInt(opts[0].ResourceVersion, 10, 64) + if err != nil { + return nil, fmt.Errorf("invalid ResourceVersion %q in ListOptions, must be int64: %w", opts[0].ResourceVersion, err) + } + addFromRV = rv } - resourceVersion = rv } t.lock.Lock() @@ -419,14 +431,14 @@ func (t *tracker) Watch(gvr schema.GroupVersionResource, ns string, opts ...meta // Deliver all objects that match the list options, for example // between the initial List and the following Watch. - if resourceVersion != -1 { + if addExisting { objs := t.objects[gvr] matchingObjs, err := filterByNamespace(objs, ns) if err != nil { return nil, err } for _, obj := range matchingObjs { - if resourceVersion == 0 || resourceVersion < obj.resourceVersion { + if addFromRV < obj.resourceVersion { fakewatcher.Add(obj.Object) } }