Merge pull request #136143 from pohly/client-go-fake-list-and-watch-fix

client-go testing: fix List+Watch support

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
 	})
 

staging/src/k8s.io/client-go/dynamic/fake/simple_test.go

@@ -186,7 +186,7 @@ func Test_ListKind(t *testing.T) {
 			"kind":       "TheKindList",
 			"metadata": map[string]interface{}{
 				"continue":        "",
-				"resourceVersion": "3", // Three objects created so far.
+				"resourceVersion": "4", // Three objects created so far, starting value is 1.
 			},
 		},
 		Items: []unstructured.Unstructured{
@@ -340,7 +340,7 @@ func TestListWithUnstructuredObjectsAndTypedScheme(t *testing.T) {
 
 	expectedList := &unstructured.UnstructuredList{}
 	expectedList.SetGroupVersionKind(listGVK)
-	expectedList.SetResourceVersion("1") // One object created so far.
+	expectedList.SetResourceVersion("2") // One object created so far, initial value is 1.
 	expectedList.SetContinue("")
 	expectedList.Items = append(expectedList.Items, u)
 
@@ -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("1") // No objects created so far.
 	expectedList.SetContinue("")
 
 	if diff := cmp.Diff(expectedList, list); diff != "" {
@@ -402,7 +402,7 @@ func TestListWithNoScheme(t *testing.T) {
 
 	expectedList := &unstructured.UnstructuredList{}
 	expectedList.SetGroupVersionKind(listGVK)
-	expectedList.SetResourceVersion("1") // One object created so far.
+	expectedList.SetResourceVersion("2") // One object created so far, initial value is 1.
 	expectedList.SetContinue("")
 	expectedList.Items = append(expectedList.Items, u)
 
@@ -443,7 +443,7 @@ func TestListWithTypedFixtures(t *testing.T) {
 
 	expectedList := &unstructured.UnstructuredList{}
 	expectedList.SetGroupVersionKind(listGVK)
-	expectedList.SetResourceVersion("1") // One object created so far.
+	expectedList.SetResourceVersion("2") // One object created so far, initial value is 1.
 	expectedList.SetContinue("")
 	expectedList.Items = []unstructured.Unstructured{u}
 

staging/src/k8s.io/client-go/testing/fixture.go

@@ -297,8 +297,11 @@ type tracker struct {
 	// see apimachinery/pkg/watch.DefaultChanSize) will cause a panic.
 	watchers map[schema.GroupVersionResource]map[string][]*watch.RaceFreeFakeWatcher
 	// resourceVersions is the highest resource version of any tracked object with
-	// a certain gvr. The resource version for that set of objects gets bumped before
-	// storing a new or modified object, so all entries are larger than 0.
+	// a certain gvr. Conceptually it starts at 1 when no objects are stored (0 is
+	// special in queries) but the map contains no entries in that case.
+	// The resource version for that set of objects gets bumped before
+	// storing a new or modified object.
+	//
 	// Object content does not get changed to preserve the traditional behavior
 	// (hence also the versionedObject type instead of storing a runtime.Object
 	// with modified ResourceVersion).
@@ -322,6 +325,8 @@ 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 > 1 for a stored object because 1
+	// is the initial value for an empty set of objects.
 	resourceVersion int64
 	runtime.Object
 }
@@ -368,6 +373,14 @@ 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 {
+		resourceVersion, ok := t.resourceVersions[gvr]
+		if !ok {
+			resourceVersion = 1
+		}
+		listMeta.SetResourceVersion(fmt.Sprintf("%d", resourceVersion))
+	}
+
 	objs, ok := t.objects[gvr]
 	if !ok {
 		return list, nil
@@ -384,9 +397,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 +405,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 +439,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)
 			}
 		}
@@ -632,16 +652,23 @@ func (t *tracker) add(gvr schema.GroupVersionResource, obj runtime.Object, ns st
 		t.objects[gvr] = make(map[types.NamespacedName]versionedObject)
 	}
 
+	// Determine resource version for the new or updated object.
+	resourceVersion, ok := t.resourceVersions[gvr]
+	if !ok {
+		resourceVersion = 1
+	}
+	resourceVersion++
+
 	namespacedName := types.NamespacedName{Namespace: newMeta.GetNamespace(), Name: newMeta.GetName()}
 	if _, ok = t.objects[gvr][namespacedName]; ok {
 		if replaceExisting {
-			resourceVersion := t.resourceVersions[gvr] + 1
 			t.resourceVersions[gvr] = resourceVersion
+			t.objects[gvr][namespacedName] = versionedObject{resourceVersion, obj}
+
 			for _, w := range t.getWatches(gvr, ns) {
 				// To avoid the object from being accidentally modified by watcher
 				w.Modify(obj.DeepCopyObject())
 			}
-			t.objects[gvr][namespacedName] = versionedObject{resourceVersion, obj}
 			return nil
 		}
 		return apierrors.NewAlreadyExists(gr, newMeta.GetName())
@@ -652,7 +679,6 @@ func (t *tracker) add(gvr schema.GroupVersionResource, obj runtime.Object, ns st
 		return apierrors.NewNotFound(gr, newMeta.GetName())
 	}
 
-	resourceVersion := t.resourceVersions[gvr] + 1
 	t.resourceVersions[gvr] = resourceVersion
 	t.objects[gvr][namespacedName] = versionedObject{resourceVersion, obj}
 

staging/src/k8s.io/client-go/testing/internal/listandwatch_test.go

@@ -67,14 +67,14 @@ func testListAndWatch(t *testing.T) {
 				logger.Info("Listed", "configMaps", objs, "err", err)
 				if err != nil {
 					t.Errorf("Unexpected List error: %v", err)
-				} else if objs.ResourceVersion != "1" {
-					t.Errorf("Expected ListMeta ResourceVersion 1, got %q", objs.ResourceVersion)
+				} else if objs.ResourceVersion != "2" {
+					t.Errorf("Expected ListMeta ResourceVersion 2, got %q", objs.ResourceVersion)
 				}
 				return objs, err
 			},
 			WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
-				if options.ResourceVersion != "1" {
-					t.Errorf("Expected ListOptions ResourceVersion 1, got %q", options.ResourceVersion)
+				if options.ResourceVersion != "2" {
+					t.Errorf("Expected ListOptions ResourceVersion 2, got %q", options.ResourceVersion)
 				}
 				logger.Info("Delaying Watch...")
 				<-createDone
@@ -84,6 +84,17 @@ func testListAndWatch(t *testing.T) {
 		}, client), &v1.ConfigMap{}, defaultEventHandlerResyncPeriod, nil)
 	})
 
+	var adds, updates, deletes int
+	handle, err := configMapInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    func(_ any) { adds++ },
+		UpdateFunc: func(_, _ any) { updates++ },
+		DeleteFunc: func(_ any) { deletes++ },
+	})
+	if err != nil {
+		t.Fatalf("Unexpected error adding event handler: %v", err)
+	}
+	defer configMapInformer.RemoveEventHandler(handle)
+
 	configMapStore := configMapInformer.GetStore()
 	f.Start(stopCh)
 	f.WaitForCacheSync(stopCh)
@@ -100,7 +111,7 @@ func testListAndWatch(t *testing.T) {
 			Namespace: "default",
 		},
 	}
-	_, err := client.CoreV1().ConfigMaps(cm.Namespace).Create(ctx, cm, metav1.CreateOptions{})
+	_, err = client.CoreV1().ConfigMaps(cm.Namespace).Create(ctx, cm, metav1.CreateOptions{})
 	if err != nil {
 		t.Fatalf("Unexpected error creating ConfigMap: %v", err)
 	}
@@ -112,6 +123,13 @@ func testListAndWatch(t *testing.T) {
 
 	objs = configMapStore.List()
 	if len(objs) != 2 {
-		t.Fatalf("Unexpected item(s) in informer cache, want 2, got %d = %v", len(objs), objs)
+		t.Errorf("Unexpected item(s) in informer cache, want 2, got %d = %v", len(objs), objs)
+	}
+
+	if !handle.HasSynced() {
+		t.Error("Expected event handler to have synced, it didn't")
+	}
+	if adds != 2 || updates != 0 || deletes != 0 {
+		t.Errorf("Expected two new objects, got adds/updates/deletes %d/%d/%d", adds, updates, deletes)
 	}
 }

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/helpers_test.go

@@ -51,7 +51,7 @@ func TestGetPodList(t *testing.T) {
 			sortBy:  func(pods []*corev1.Pod) sort.Interface { return podutils.ByLogging(pods) },
 			expected: &corev1.PodList{
 				ListMeta: metav1.ListMeta{
-					ResourceVersion: "2", // Two objects created.
+					ResourceVersion: "3", // Two objects created, initial value is 1.
 				},
 				Items: []corev1.Pod{
 					{