Skip to content

Commit df3a90a

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. 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.
1 parent f8acd6e commit df3a90a

File tree

2 files changed

+234
-5
lines changed

2 files changed

+234
-5
lines changed

‎pkg/client/apiutil/restmapper.go

Lines changed: 30 additions & 5 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

@@ -278,15 +278,24 @@ func (m *mapper) fetchGroupVersionResourcesLocked(groupName string, versions ...
278278
groupVersion := schema.GroupVersion{Group: groupName, Version: version}
279279

280280
apiResourceList, err := m.client.ServerResourcesForGroupVersion(groupVersion.String())
281-
if apierrors.IsNotFound(err) && m.isGroupVersionCached(groupVersion) {
281+
if apierrors.IsNotFound(err) {
282282
// If the version is not found, we remove the group from the cache
283283
// so it gets refreshed on the next call.
284-
delete(m.apiGroups, groupName)
285-
delete(m.knownGroups, groupName)
284+
cacheInvalidated := false
285+
if m.isAPIGroupCached(groupVersion) {
286+
delete(m.apiGroups, groupName)
287+
cacheInvalidated = true
288+
}
289+
if m.isGroupVersionCached(groupVersion) {
290+
delete(m.knownGroups, groupName)
291+
cacheInvalidated = true
292+
}
286293
// It's important to refresh the mapper after invalidating the cache, since returning an error
287294
// aborts the call and leaves the underlying mapper unchanged. If not refreshed, the next call
288295
// will still return a match for the NotFound version.
289-
m.refreshMapper()
296+
if cacheInvalidated {
297+
m.refreshMapper()
298+
}
290299
}
291300

292301
if err != nil {
@@ -317,6 +326,22 @@ func (m *mapper) isGroupVersionCached(gv schema.GroupVersion) bool {
317326
return false
318327
}
319328

329+
// isAPIGroupCached checks if a version for a group is cached in the api groups cache.
330+
func (m *mapper) isAPIGroupCached(gv schema.GroupVersion) bool {
331+
cachedGroup, ok := m.apiGroups[gv.Group]
332+
if !ok {
333+
return false
334+
}
335+
336+
for _, version := range cachedGroup.Versions {
337+
if version.Version == gv.Version {
338+
return true
339+
}
340+
}
341+
342+
return false
343+
}
344+
320345
func (m *mapper) refreshMapper() {
321346
updatedGroupResources := make([]*restmapper.APIGroupResources, 0, len(m.knownGroups))
322347
for _, agr := range m.knownGroups {

‎pkg/client/apiutil/restmapper_wb_test.go

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
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).To(gmg.HaveOccurred())
199+
g.Expect(err).To(gmg.BeAssignableToTypeOf(&ErrResourceDiscoveryFailed{}))
200+
g.Expect(m.apiGroups).To(gmg.BeComparableTo(tt.expectedAPIGroups))
201+
g.Expect(m.knownGroups).To(gmg.BeComparableTo(tt.expectedKnownGroups))
202+
})
203+
}
204+
}

0 commit comments

Comments
 (0)