bug: Restmapper: Respect preferred version

When requesting a resource without version through methods
such as `RESTMapping`, the mapper would previously return a random
version rather than the preferred one, this change fixes that.

Author: alvaroaleman

pkg/client/apiutil/restmapper.go

@@ -246,10 +246,18 @@ func (m *mapper) addGroupVersionResourcesToCacheAndReloadLocked(gvr map[schema.G
 		}
 
 		if !found {
-			groupResources.Group.Versions = append(groupResources.Group.Versions, metav1.GroupVersionForDiscovery{
+			gv := metav1.GroupVersionForDiscovery{
 				GroupVersion: metav1.GroupVersion{Group: groupVersion.Group, Version: version}.String(),
 				Version:      version,
-			})
+			}
+
+			// Prepend if preferred version, else append. The upstream DiscoveryRestMappper assumes
+			// the first version is the preferred one: https://github.com/kubernetes/kubernetes/blob/ef54ac803b712137871c1a1f8d635d50e69ffa6c/staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go#L458-L461
+			if group, ok := m.apiGroups[groupVersion.Group]; ok && group.PreferredVersion.Version == version {
+				groupResources.Group.Versions = append([]metav1.GroupVersionForDiscovery{gv}, groupResources.Group.Versions...)
+			} else {
+				groupResources.Group.Versions = append(groupResources.Group.Versions, gv)
+			}
 		}
 
 		// Update data in the cache.
@@ -284,14 +292,14 @@ func (m *mapper) findAPIGroupByNameAndMaybeAggregatedDiscoveryLocked(groupName s
 	}
 
 	m.initialDiscoveryDone = true
-	if len(maybeResources) > 0 {
-		didAggregatedDiscovery = true
-		m.addGroupVersionResourcesToCacheAndReloadLocked(maybeResources)
-	}
 	for i := range apiGroups.Groups {
 		group := &apiGroups.Groups[i]
 		m.apiGroups[group.Name] = group
 	}
+	if len(maybeResources) > 0 {
+		didAggregatedDiscovery = true
+		m.addGroupVersionResourcesToCacheAndReloadLocked(maybeResources)
+	}
 
 	// Looking in the cache again.
 	// Don't return an error here if the API group is not present.

pkg/client/apiutil/restmapper_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"sync"
 	"testing"
 
 	_ "github.com/onsi/ginkgo/v2"
@@ -735,6 +736,33 @@ func TestLazyRestMapperProvider(t *testing.T) {
 				g.Expect(err).NotTo(gmg.HaveOccurred())
 				g.Expect(mapping.Resource.Version).To(gmg.Equal("v1"))
 			})
+
+			t.Run("Restmapper should consistently return the preferred version", func(t *testing.T) {
+				g := gmg.NewWithT(t)
+
+				wg := sync.WaitGroup{}
+				wg.Add(50)
+				for i := 0; i < 50; i++ {
+					go func() {
+						defer wg.Done()
+						httpClient, err := rest.HTTPClientFor(restCfg)
+						g.Expect(err).NotTo(gmg.HaveOccurred())
+
+						mapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient)
+						g.Expect(err).NotTo(gmg.HaveOccurred())
+
+						mapping, err := mapper.RESTMapping(schema.GroupKind{
+							Group: "crew.example.com",
+							Kind:  "Driver",
+						})
+						g.Expect(err).NotTo(gmg.HaveOccurred())
+						// APIServer seems to have a heuristic to prefer the higher
+						// version number.
+						g.Expect(mapping.GroupVersionKind.Version).To(gmg.Equal("v2"))
+					}()
+				}
+				wg.Wait()
+			})
 		})
 	}
 }