Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 2, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions pkg/client/apiutil/restmapper.go
Original file line number Diff line number Diff line change
@@ -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 {
72 changes: 70 additions & 2 deletions pkg/client/apiutil/restmapper_test.go
Original file line number Diff line number Diff line change
@@ -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))
}