Skip to content

Commit e818ce4

Browse files
g-gastong-gaston
committed
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.
1 parent 4282ca1 commit e818ce4

File tree

2 files changed

+227
-4
lines changed

2 files changed

+227
-4
lines changed

‎pkg/client/apiutil/restmapper.go‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func NewDynamicRESTMapper(cfg *rest.Config, httpClient *http.Client) (meta.RESTM
5353
// client for discovery information to do REST mappings.
5454
type mapper struct {
5555
mapper meta.RESTMapper
56-
client *discovery.DiscoveryClient
56+
client discovery.DiscoveryInterface
5757
knownGroups map[string]*restmapper.APIGroupResources
5858
apiGroups map[string]*metav1.APIGroup
5959

@@ -280,11 +280,15 @@ func (m *mapper) fetchGroupVersionResourcesLocked(groupName string, versions ...
280280
groupVersion := schema.GroupVersion{Group: groupName, Version: version}
281281

282282
apiResourceList, err := m.client.ServerResourcesForGroupVersion(groupVersion.String())
283-
if apierrors.IsNotFound(err) && m.isGroupVersionCached(groupVersion) {
283+
if apierrors.IsNotFound(err) {
284284
// If the version is not found, we remove the group from the cache
285285
// so it gets refreshed on the next call.
286-
delete(m.apiGroups, groupName)
287-
delete(m.knownGroups, groupName)
286+
if m.isAPIGroupCached(groupVersion) {
287+
delete(m.apiGroups, groupName)
288+
}
289+
if m.isGroupVersionCached(groupVersion) {
290+
delete(m.knownGroups, groupName)
291+
}
288292
continue
289293
} else if err != nil {
290294
failedGroups[groupVersion] = err
@@ -313,3 +317,19 @@ func (m *mapper) isGroupVersionCached(gv schema.GroupVersion) bool {
313317

314318
return false
315319
}
320+
321+
// isAPIGroupCached checks if a version for a group is cached in the api groups cache.
322+
func (m *mapper) isAPIGroupCached(gv schema.GroupVersion) bool {
323+
cachedGroup, ok := m.apiGroups[gv.Group]
324+
if !ok {
325+
return false
326+
}
327+
328+
for _, version := range cachedGroup.Versions {
329+
if version.Version == gv.Version {
330+
return true
331+
}
332+
}
333+
334+
return false
335+
}

‎pkg/client/apiutil/restmapper_wb_test.go‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
/*
2+
Copyright 2024 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package apiutil
18+
19+
import (
20+
"testing"
21+
22+
gmg "github.com/onsi/gomega"
23+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
"k8s.io/client-go/kubernetes/fake"
25+
"k8s.io/client-go/restmapper"
26+
)
27+
28+
func TestLazyRestMapper_fetchGroupVersionResourcesLocked_CacheInvalidation(t *testing.T) {
29+
tests := []struct {
30+
name string
31+
groupName string
32+
versions []string
33+
cachedAPIGroups, expectedAPIGroups map[string]*metav1.APIGroup
34+
cachedKnownGroups, expectedKnownGroups map[string]*restmapper.APIGroupResources
35+
}{
36+
{
37+
name: "Not found version for cached groupVersion in apiGroups and knownGroups",
38+
groupName: "group1",
39+
versions: []string{"v1", "v2"},
40+
cachedAPIGroups: map[string]*metav1.APIGroup{
41+
"group1": {
42+
Name: "group1",
43+
Versions: []metav1.GroupVersionForDiscovery{
44+
{
45+
Version: "v1",
46+
},
47+
},
48+
},
49+
},
50+
cachedKnownGroups: map[string]*restmapper.APIGroupResources{
51+
"group1": {
52+
VersionedResources: map[string][]metav1.APIResource{
53+
"v1": {
54+
{
55+
Name: "resource1",
56+
},
57+
},
58+
},
59+
},
60+
},
61+
expectedAPIGroups: map[string]*metav1.APIGroup{},
62+
expectedKnownGroups: map[string]*restmapper.APIGroupResources{},
63+
},
64+
{
65+
name: "Not found version for cached groupVersion only in apiGroups",
66+
groupName: "group1",
67+
versions: []string{"v1", "v2"},
68+
cachedAPIGroups: map[string]*metav1.APIGroup{
69+
"group1": {
70+
Name: "group1",
71+
Versions: []metav1.GroupVersionForDiscovery{
72+
{
73+
Version: "v1",
74+
},
75+
},
76+
},
77+
},
78+
cachedKnownGroups: map[string]*restmapper.APIGroupResources{
79+
"group1": {
80+
VersionedResources: map[string][]metav1.APIResource{
81+
"v3": {
82+
{
83+
Name: "resource1",
84+
},
85+
},
86+
},
87+
},
88+
},
89+
expectedAPIGroups: map[string]*metav1.APIGroup{},
90+
expectedKnownGroups: map[string]*restmapper.APIGroupResources{
91+
"group1": {
92+
VersionedResources: map[string][]metav1.APIResource{
93+
"v3": {
94+
{
95+
Name: "resource1",
96+
},
97+
},
98+
},
99+
},
100+
},
101+
},
102+
{
103+
name: "Not found version for cached groupVersion only in knownGroups",
104+
groupName: "group1",
105+
versions: []string{"v1", "v2"},
106+
cachedAPIGroups: map[string]*metav1.APIGroup{
107+
"group1": {
108+
Name: "group1",
109+
Versions: []metav1.GroupVersionForDiscovery{
110+
{
111+
Version: "v3",
112+
},
113+
},
114+
},
115+
},
116+
cachedKnownGroups: map[string]*restmapper.APIGroupResources{
117+
"group1": {
118+
VersionedResources: map[string][]metav1.APIResource{
119+
"v2": {
120+
{
121+
Name: "resource1",
122+
},
123+
},
124+
},
125+
},
126+
},
127+
expectedAPIGroups: map[string]*metav1.APIGroup{
128+
"group1": {
129+
Name: "group1",
130+
Versions: []metav1.GroupVersionForDiscovery{
131+
{
132+
Version: "v3",
133+
},
134+
},
135+
},
136+
},
137+
expectedKnownGroups: map[string]*restmapper.APIGroupResources{},
138+
},
139+
{
140+
name: "Not found version for non cached groupVersion",
141+
groupName: "group1",
142+
versions: []string{"v1", "v2"},
143+
cachedAPIGroups: map[string]*metav1.APIGroup{
144+
"group1": {
145+
Name: "group1",
146+
Versions: []metav1.GroupVersionForDiscovery{
147+
{
148+
Version: "v3",
149+
},
150+
},
151+
},
152+
},
153+
cachedKnownGroups: map[string]*restmapper.APIGroupResources{
154+
"group1": {
155+
VersionedResources: map[string][]metav1.APIResource{
156+
"v3": {
157+
{
158+
Name: "resource1",
159+
},
160+
},
161+
},
162+
},
163+
},
164+
expectedAPIGroups: map[string]*metav1.APIGroup{
165+
"group1": {
166+
Name: "group1",
167+
Versions: []metav1.GroupVersionForDiscovery{
168+
{
169+
Version: "v3",
170+
},
171+
},
172+
},
173+
},
174+
expectedKnownGroups: map[string]*restmapper.APIGroupResources{
175+
"group1": {
176+
VersionedResources: map[string][]metav1.APIResource{
177+
"v3": {
178+
{
179+
Name: "resource1",
180+
},
181+
},
182+
},
183+
},
184+
},
185+
},
186+
}
187+
188+
for _, tt := range tests {
189+
t.Run(tt.name, func(t *testing.T) {
190+
g := gmg.NewWithT(t)
191+
m := &mapper{
192+
mapper: restmapper.NewDiscoveryRESTMapper([]*restmapper.APIGroupResources{}),
193+
client: fake.NewSimpleClientset().Discovery(),
194+
apiGroups: tt.cachedAPIGroups,
195+
knownGroups: tt.cachedKnownGroups,
196+
}
197+
_, err := m.fetchGroupVersionResourcesLocked(tt.groupName, tt.versions...)
198+
g.Expect(err).NotTo(gmg.HaveOccurred())
199+
g.Expect(m.apiGroups).To(gmg.BeComparableTo(tt.expectedAPIGroups))
200+
g.Expect(m.knownGroups).To(gmg.BeComparableTo(tt.expectedKnownGroups))
201+
})
202+
}
203+
}

0 commit comments

Comments
 (0)