Repository: kubernetes-sigs/controller-runtime

Issue:
RESTMapper randomly returns API versions
After #2901 we are observing in [Flux](https://github.com/fluxcd/kustomize-controller/issues/1373) that the RESTMapper can not be used reliably to determine the preferred API version of a custom resource. 

```go
	// RESTMapping identifies a preferred resource mapping for the provided group kind.
	RESTMapping(gk schema.GroupKind, versions ...string) (*RESTMapping, error)
``` 

When calling `RESTMapping` for a `GroupKind` without specified versions (this is what `kubernetes-sigs/cli-utils/pkg/kstatus` does [here](https://github.com/kubernetes-sigs/cli-utils/blob/1a9b04556153279603429504ed7ef75c82be6da6/pkg/kstatus/watcher/object_status_reporter.go#L324)), the controller-runtime RESTMapper will randomly return deprecated versions as the preferred resource mapping.

Given that the `RESTMapping` implementation picks the first mapping from the version array, it suggests that the controller-runtime RESTMapper using the Aggregated Discovery reorders the array or somehow it only contains a single recored even if in etcd the CRD has multiple versions.

```go
func (m *DefaultRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*RESTMapping, error) {
	mappings, err := m.RESTMappings(gk, versions...)
	if err != nil {
		return nil, err
	}
	if len(mappings) == 0 {
		return nil, &NoKindMatchError{GroupKind: gk, SearchedVersions: versions}
	}
	// since we rely on RESTMappings method
	// take the first match and return to the caller
	// as this was the existing behavior.
	return mappings[0], nil
}
```

Logs from Flux kustomize-controller:

```
{"level":"info","ts":"2025-02-24T13:28:10.312Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:30:10.365Z","logger":"KubeAPIWarningLogger","msg":"v2beta2 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:32:10.423Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:34:10.497Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:36:10.555Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:38:10.631Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:40:10.699Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:42:10.760Z","logger":"KubeAPIWarningLogger","msg":"v2beta2 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:44:10.823Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:46:10.950Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:48:11.022Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:50:11.082Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:52:11.140Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:54:11.210Z","logger":"KubeAPIWarningLogger","msg":"v2beta2 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:56:11.268Z","logger":"KubeAPIWarningLogger","msg":"v2beta2 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T13:58:11.339Z","logger":"KubeAPIWarningLogger","msg":"v2beta2 HelmRelease is deprecated, upgrade to v2"}
{"level":"info","ts":"2025-02-24T14:00:11.591Z","logger":"KubeAPIWarningLogger","msg":"v2beta1 HelmRelease is deprecated, upgrade to v2"}
```

To fix this in Flux, we decided to vendor the RESTMapper from controller-runtime v0.19 in https://github.com/fluxcd/pkg/pull/873.

Discussion:
@erikgb: I can add that even after vendoring the RESTMapper from controller-runtime v0.19 in Flux, I see warnings about incorrect API versions being selected/preferred by Flux. Example from a test-bootstrap in an ephemeral cluster:

````
$ k logs -n flux-system kustomize-controller-6b478c49db-l682l | grep deprecated
{"level":"info","ts":"2025-02-25T07:58:21.495Z","logger":"KubeAPIWarningLogger","msg":"kyverno.io/v2beta1 ClusterCleanupPolicy is deprecated; use kyverno.io/v2 ClusterCleanupPolicy"}
{"level":"info","ts":"2025-02-25T07:58:21.499Z","logger":"KubeAPIWarningLogger","msg":"kyverno.io/v2beta1 ClusterCleanupPolicy is deprecated; use kyverno.io/v2 ClusterCleanupPolicy"}
````

And all our `ClusterCleanupPolicy` resources are declared as v2 in Git.@caozhuozi: To address the issue, do we need an option in the restmapper of controller-runtime to keep the preferred version info?@alvaroaleman: > To address the issue, do we need an option in the restmapper of controller-runtime to keep the preferred version info?

We don't want to duplicate the complexity of the upstream restmapper regarding preferred version selection. Controller-Runtime itself has no use-case where the api version isn't known. The best compromise I can think of is to construct an upstream restmapper when someone calls one of the methods that select a version on our restmapper@alvaroaleman: > We don't want to duplicate the complexity of the upstream restmapper regarding preferred version selection. Controller-Runtime itself has no use-case where the api version isn't known. The best compromise I can think of is to construct an upstream restmapper when someone calls one of the methods that select a version on our restmapper

It turns out this isn't needed, I misremembered the related code, the server tells us what the preferred version is. I posted a fix for this issue in #3151 

What confuses me a lot here though is that this issue definitely predates the introduction of aggregated discovery, backporting the testcase I added to 0.19 makes it fail the same way. @stefanprodan what makes you think this is related to aggregated discovery?

From what I can tell this issue exists since the introduction of a truly lazy resmapper in [v0.15](https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0)@stefanprodan: > what makes you think this is related to aggregated discovery?

I assumed it's the aggregated discovery because we didn't had this issue in older versions of Flux using controller-runtime 0.19 and older. It may well be that the latest RESTMapper made the version flipping happen more often. I will test the fix and see if we can switch back. Thanks a lot for looking into this.

Solution patch:
diff --git a/pkg/client/apiutil/restmapper.go b/pkg/client/apiutil/restmapper.go
index ad898617..7a7a0d11 100644
--- a/pkg/client/apiutil/restmapper.go
+++ b/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.



Test patch:
diff --git a/pkg/client/apiutil/restmapper_test.go b/pkg/client/apiutil/restmapper_test.go
index e4e701bb..1b080aee 100644
--- a/pkg/client/apiutil/restmapper_test.go
+++ b/pkg/client/apiutil/restmapper_test.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"sync"
 	"testing"
 
 	_ "github.com/onsi/ginkgo/v2"
@@ -740,6 +741,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()
+			})
 		})
 	}
 }