⚠️ RESTMapper: don't treat non-existing GroupVersions as errors (#2571)

* Explicitly test for `No*MatchErrors`

* RESTMapper: don't treat non-existing GroupVersions as errors

---------

Co-authored-by: Tim Ebert <[email protected]>

Author: ary1992

pkg/client/apiutil/restmapper.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"sync"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -166,8 +167,10 @@ func (m *mapper) addKnownGroupAndReload(groupName string, versions ...string) er
 		if err != nil {
 			return err
 		}
-		for _, version := range apiGroup.Versions {
-			versions = append(versions, version.Version)
+		if apiGroup != nil {
+			for _, version := range apiGroup.Versions {
+				versions = append(versions, version.Version)
+			}
 		}
 	}
 
@@ -254,17 +257,12 @@ func (m *mapper) findAPIGroupByName(groupName string) (*metav1.APIGroup, error)
 	m.mu.Unlock()
 
 	// Looking in the cache again.
-	{
-		m.mu.RLock()
-		group, ok := m.apiGroups[groupName]
-		m.mu.RUnlock()
-		if ok {
-			return group, nil
-		}
-	}
+	m.mu.RLock()
+	defer m.mu.RUnlock()
 
-	// If there is still nothing, return an error.
-	return nil, fmt.Errorf("failed to find API group %q", groupName)
+	// Don't return an error here if the API group is not present.
+	// The reloaded RESTMapper will take care of returning a NoMatchError.
+	return m.apiGroups[groupName], nil
 }
 
 // fetchGroupVersionResources fetches the resources for the specified group and its versions.
@@ -276,7 +274,7 @@ func (m *mapper) fetchGroupVersionResources(groupName string, versions ...string
 		groupVersion := schema.GroupVersion{Group: groupName, Version: version}
 
 		apiResourceList, err := m.client.ServerResourcesForGroupVersion(groupVersion.String())
-		if err != nil {
+		if err != nil && !apierrors.IsNotFound(err) {
 			failedGroups[groupVersion] = err
 		}
 		if apiResourceList != nil {

pkg/client/apiutil/restmapper_test.go

@@ -18,19 +18,22 @@ package apiutil_test
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"testing"
 
-	"k8s.io/apimachinery/pkg/api/meta"
-
 	_ "github.com/onsi/ginkgo/v2"
 	gmg "github.com/onsi/gomega"
+	"github.com/onsi/gomega/format"
+	gomegatypes "github.com/onsi/gomega/types"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
@@ -303,6 +306,10 @@ func TestLazyRestMapperProvider(t *testing.T) {
 		lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient)
 		g.Expect(err).NotTo(gmg.HaveOccurred())
 
+		// A version is specified but the group doesn't exist.
+		// For each group, we expect 1 call to the version-specific discovery endpoint:
+		// 	#1: GET https://host/apis/<group>/<version>
+
 		_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: "INVALID1"}, "v1")
 		g.Expect(err).To(gmg.HaveOccurred())
 		g.Expect(meta.IsNoMatchError(err)).To(gmg.BeTrue())
@@ -332,6 +339,35 @@ func TestLazyRestMapperProvider(t *testing.T) {
 		g.Expect(err).To(gmg.HaveOccurred())
 		g.Expect(meta.IsNoMatchError(err)).To(gmg.BeTrue())
 		g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
+
+		// No version is specified but the group doesn't exist.
+		// For each group, we expect 2 calls to discover all group versions:
+		// 	#1: GET https://host/api
+		// 	#2: GET https://host/apis
+
+		_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: "INVALID7"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(8))
+
+		_, err = lazyRestMapper.RESTMappings(schema.GroupKind{Group: "INVALID8"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(10))
+
+		_, err = lazyRestMapper.KindFor(schema.GroupVersionResource{Group: "INVALID9"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(12))
+
+		_, err = lazyRestMapper.KindsFor(schema.GroupVersionResource{Group: "INVALID10"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(14))
+
+		_, err = lazyRestMapper.ResourceFor(schema.GroupVersionResource{Group: "INVALID11"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(16))
+
+		_, err = lazyRestMapper.ResourcesFor(schema.GroupVersionResource{Group: "INVALID12"})
+		g.Expect(err).To(beNoMatchError())
+		g.Expect(crt.GetRequestCount()).To(gmg.Equal(18))
 	})
 
 	t.Run("LazyRESTMapper should return an error if a resource doesn't exist", func(t *testing.T) {
@@ -529,3 +565,35 @@ func TestLazyRestMapperProvider(t *testing.T) {
 		g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal("rider"))
 	})
 }
+
+func beNoMatchError() gomegatypes.GomegaMatcher {
+	return &errorMatcher{
+		checkFunc: meta.IsNoMatchError,
+		message:   "NoMatch",
+	}
+}
+
+type errorMatcher struct {
+	checkFunc func(error) bool
+	message   string
+}
+
+func (e *errorMatcher) Match(actual interface{}) (success bool, err error) {
+	if actual == nil {
+		return false, nil
+	}
+
+	actualErr, actualOk := actual.(error)
+	if !actualOk {
+		return false, fmt.Errorf("expected an error-type. got:\n%s", format.Object(actual, 1))
+	}
+
+	return e.checkFunc(actualErr), nil
+}
+
+func (e *errorMatcher) FailureMessage(actual interface{}) (message string) {
+	return format.Message(actual, fmt.Sprintf("to be %s error", e.message))
+}
+func (e *errorMatcher) NegatedFailureMessage(actual interface{}) (message string) {
+	return format.Message(actual, fmt.Sprintf("not to be %s error", e.message))
+}