From df3a90a60090c247e9e607685bc9375ed071d695 Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Thu, 15 Feb 2024 13:50:30 +0000 Subject: [PATCH] Fix lazy rest mapper cache invalidation When a group version is not found, if the group version is cached in apiGroups but not cached in knownGroups, the cache is not invalidated. Moreover and even worse, in that scenario an error is returned. This is only an issue under the very rare case where a version has been removed during a call just after caching in apiGroups but before it's cached in knownGroups. After the change now an error is never returned for not found versions no matter if the version was cached or not. And now the two different caches are checked and invalidated independently. --- pkg/client/apiutil/restmapper.go | 35 +++- pkg/client/apiutil/restmapper_wb_test.go | 204 +++++++++++++++++++++++ 2 files changed, 234 insertions(+), 5 deletions(-) create mode 100644 pkg/client/apiutil/restmapper_wb_test.go diff --git a/pkg/client/apiutil/restmapper.go b/pkg/client/apiutil/restmapper.go index 2320cd26fc..496b4c7863 100644 --- a/pkg/client/apiutil/restmapper.go +++ b/pkg/client/apiutil/restmapper.go @@ -53,7 +53,7 @@ func NewDynamicRESTMapper(cfg *rest.Config, httpClient *http.Client) (meta.RESTM // client for discovery information to do REST mappings. type mapper struct { mapper meta.RESTMapper - client *discovery.DiscoveryClient + client discovery.DiscoveryInterface knownGroups map[string]*restmapper.APIGroupResources apiGroups map[string]*metav1.APIGroup @@ -278,15 +278,24 @@ func (m *mapper) fetchGroupVersionResourcesLocked(groupName string, versions ... groupVersion := schema.GroupVersion{Group: groupName, Version: version} apiResourceList, err := m.client.ServerResourcesForGroupVersion(groupVersion.String()) - if apierrors.IsNotFound(err) && m.isGroupVersionCached(groupVersion) { + if apierrors.IsNotFound(err) { // If the version is not found, we remove the group from the cache // so it gets refreshed on the next call. - delete(m.apiGroups, groupName) - delete(m.knownGroups, groupName) + cacheInvalidated := false + if m.isAPIGroupCached(groupVersion) { + delete(m.apiGroups, groupName) + cacheInvalidated = true + } + if m.isGroupVersionCached(groupVersion) { + delete(m.knownGroups, groupName) + cacheInvalidated = true + } // It's important to refresh the mapper after invalidating the cache, since returning an error // aborts the call and leaves the underlying mapper unchanged. If not refreshed, the next call // will still return a match for the NotFound version. - m.refreshMapper() + if cacheInvalidated { + m.refreshMapper() + } } if err != nil { @@ -317,6 +326,22 @@ func (m *mapper) isGroupVersionCached(gv schema.GroupVersion) bool { return false } +// isAPIGroupCached checks if a version for a group is cached in the api groups cache. +func (m *mapper) isAPIGroupCached(gv schema.GroupVersion) bool { + cachedGroup, ok := m.apiGroups[gv.Group] + if !ok { + return false + } + + for _, version := range cachedGroup.Versions { + if version.Version == gv.Version { + return true + } + } + + return false +} + func (m *mapper) refreshMapper() { updatedGroupResources := make([]*restmapper.APIGroupResources, 0, len(m.knownGroups)) for _, agr := range m.knownGroups { diff --git a/pkg/client/apiutil/restmapper_wb_test.go b/pkg/client/apiutil/restmapper_wb_test.go new file mode 100644 index 0000000000..9d0bba9643 --- /dev/null +++ b/pkg/client/apiutil/restmapper_wb_test.go @@ -0,0 +1,204 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package apiutil + +import ( + "testing" + + gmg "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/restmapper" +) + +func TestLazyRestMapper_fetchGroupVersionResourcesLocked_CacheInvalidation(t *testing.T) { + tests := []struct { + name string + groupName string + versions []string + cachedAPIGroups, expectedAPIGroups map[string]*metav1.APIGroup + cachedKnownGroups, expectedKnownGroups map[string]*restmapper.APIGroupResources + }{ + { + name: "Not found version for cached groupVersion in apiGroups and knownGroups", + groupName: "group1", + versions: []string{"v1", "v2"}, + cachedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v1", + }, + }, + }, + }, + cachedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v1": { + { + Name: "resource1", + }, + }, + }, + }, + }, + expectedAPIGroups: map[string]*metav1.APIGroup{}, + expectedKnownGroups: map[string]*restmapper.APIGroupResources{}, + }, + { + name: "Not found version for cached groupVersion only in apiGroups", + groupName: "group1", + versions: []string{"v1", "v2"}, + cachedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v1", + }, + }, + }, + }, + cachedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v3": { + { + Name: "resource1", + }, + }, + }, + }, + }, + expectedAPIGroups: map[string]*metav1.APIGroup{}, + expectedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v3": { + { + Name: "resource1", + }, + }, + }, + }, + }, + }, + { + name: "Not found version for cached groupVersion only in knownGroups", + groupName: "group1", + versions: []string{"v1", "v2"}, + cachedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v3", + }, + }, + }, + }, + cachedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v2": { + { + Name: "resource1", + }, + }, + }, + }, + }, + expectedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v3", + }, + }, + }, + }, + expectedKnownGroups: map[string]*restmapper.APIGroupResources{}, + }, + { + name: "Not found version for non cached groupVersion", + groupName: "group1", + versions: []string{"v1", "v2"}, + cachedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v3", + }, + }, + }, + }, + cachedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v3": { + { + Name: "resource1", + }, + }, + }, + }, + }, + expectedAPIGroups: map[string]*metav1.APIGroup{ + "group1": { + Name: "group1", + Versions: []metav1.GroupVersionForDiscovery{ + { + Version: "v3", + }, + }, + }, + }, + expectedKnownGroups: map[string]*restmapper.APIGroupResources{ + "group1": { + VersionedResources: map[string][]metav1.APIResource{ + "v3": { + { + Name: "resource1", + }, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gmg.NewWithT(t) + m := &mapper{ + mapper: restmapper.NewDiscoveryRESTMapper([]*restmapper.APIGroupResources{}), + client: fake.NewSimpleClientset().Discovery(), + apiGroups: tt.cachedAPIGroups, + knownGroups: tt.cachedKnownGroups, + } + _, err := m.fetchGroupVersionResourcesLocked(tt.groupName, tt.versions...) + g.Expect(err).To(gmg.HaveOccurred()) + g.Expect(err).To(gmg.BeAssignableToTypeOf(&ErrResourceDiscoveryFailed{})) + g.Expect(m.apiGroups).To(gmg.BeComparableTo(tt.expectedAPIGroups)) + g.Expect(m.knownGroups).To(gmg.BeComparableTo(tt.expectedKnownGroups)) + }) + } +}