Skip to content

Commit ffb74e5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#2219 from sbueringer/pr-cherry-lazy-restmapper-fix
[release-0.14] 🐛 Allow lazy restmapper to work with CRDs created at runtime
2 parents 0b49f2e + 66fe1a0 commit ffb74e5

File tree

2 files changed

+143
-37
lines changed

2 files changed

+143
-37
lines changed

‎pkg/client/apiutil/lazyrestmapper.go

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ type lazyRESTMapper struct {
3333
mapper meta.RESTMapper
3434
client *discovery.DiscoveryClient
3535
knownGroups map[string]*restmapper.APIGroupResources
36-
apiGroups *metav1.APIGroupList
36+
apiGroups []metav1.APIGroup
3737

3838
// mutex to provide thread-safe mapper reloading.
3939
mu sync.Mutex
@@ -45,6 +45,7 @@ func newLazyRESTMapperWithClient(discoveryClient *discovery.DiscoveryClient) (me
4545
mapper: restmapper.NewDiscoveryRESTMapper([]*restmapper.APIGroupResources{}),
4646
client: discoveryClient,
4747
knownGroups: map[string]*restmapper.APIGroupResources{},
48+
apiGroups: []metav1.APIGroup{},
4849
}, nil
4950
}
5051

@@ -147,7 +148,7 @@ func (m *lazyRESTMapper) addKnownGroupAndReload(groupName string, versions ...st
147148
// This operation requires 2 requests: /api and /apis, but only once. For all subsequent calls
148149
// this data will be taken from cache.
149150
if len(versions) == 0 {
150-
apiGroup, err := m.findAPIGroupByName(groupName)
151+
apiGroup, err := m.findAPIGroupByNameLocked(groupName)
151152
if err != nil {
152153
return err
153154
}
@@ -176,11 +177,22 @@ func (m *lazyRESTMapper) addKnownGroupAndReload(groupName string, versions ...st
176177
}
177178

178179
// Update information for group resources about the API group by adding new versions.
180+
// Ignore the versions that are already registered.
179181
for _, version := range versions {
180-
groupResources.Group.Versions = append(groupResources.Group.Versions, metav1.GroupVersionForDiscovery{
181-
GroupVersion: metav1.GroupVersion{Group: groupName, Version: version}.String(),
182-
Version: version,
183-
})
182+
found := false
183+
for _, v := range groupResources.Group.Versions {
184+
if v.Version == version {
185+
found = true
186+
break
187+
}
188+
}
189+
190+
if !found {
191+
groupResources.Group.Versions = append(groupResources.Group.Versions, metav1.GroupVersionForDiscovery{
192+
GroupVersion: metav1.GroupVersion{Group: groupName, Version: version}.String(),
193+
Version: version,
194+
})
195+
}
184196
}
185197

186198
// Update data in the cache.
@@ -197,28 +209,34 @@ func (m *lazyRESTMapper) addKnownGroupAndReload(groupName string, versions ...st
197209
return nil
198210
}
199211

200-
// findAPIGroupByName returns API group by its name.
201-
func (m *lazyRESTMapper) findAPIGroupByName(groupName string) (metav1.APIGroup, error) {
202-
// Ensure that required info about existing API groups is received and stored in the mapper.
203-
// It will make 2 API calls to /api and /apis, but only once.
204-
if m.apiGroups == nil {
205-
apiGroups, err := m.client.ServerGroups()
206-
if err != nil {
207-
return metav1.APIGroup{}, fmt.Errorf("failed to get server groups: %w", err)
208-
}
209-
if len(apiGroups.Groups) == 0 {
210-
return metav1.APIGroup{}, fmt.Errorf("received an empty API groups list")
212+
// findAPIGroupByNameLocked returns API group by its name.
213+
func (m *lazyRESTMapper) findAPIGroupByNameLocked(groupName string) (metav1.APIGroup, error) {
214+
// Looking in the cache first.
215+
for _, apiGroup := range m.apiGroups {
216+
if groupName == apiGroup.Name {
217+
return apiGroup, nil
211218
}
219+
}
212220

213-
m.apiGroups = apiGroups
221+
// Update the cache if nothing was found.
222+
apiGroups, err := m.client.ServerGroups()
223+
if err != nil {
224+
return metav1.APIGroup{}, fmt.Errorf("failed to get server groups: %w", err)
214225
}
226+
if len(apiGroups.Groups) == 0 {
227+
return metav1.APIGroup{}, fmt.Errorf("received an empty API groups list")
228+
}
229+
230+
m.apiGroups = apiGroups.Groups
215231

216-
for i := range m.apiGroups.Groups {
217-
if groupName == (&m.apiGroups.Groups[i]).Name {
218-
return m.apiGroups.Groups[i], nil
232+
// Looking in the cache again.
233+
for _, apiGroup := range m.apiGroups {
234+
if groupName == apiGroup.Name {
235+
return apiGroup, nil
219236
}
220237
}
221238

239+
// If there is still nothing, return an error.
222240
return metav1.APIGroup{}, fmt.Errorf("failed to find API group %s", groupName)
223241
}
224242

‎pkg/client/apiutil/lazyrestmapper_test.go

Lines changed: 104 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,19 @@ limitations under the License.
1717
package apiutil_test
1818

1919
import (
20+
"context"
2021
"net/http"
2122
"testing"
2223

2324
_ "github.com/onsi/ginkgo/v2"
2425
gmg "github.com/onsi/gomega"
2526

27+
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2628
"k8s.io/apimachinery/pkg/runtime/schema"
29+
"k8s.io/apimachinery/pkg/types"
30+
"k8s.io/client-go/kubernetes/scheme"
2731
"k8s.io/client-go/rest"
32+
"sigs.k8s.io/controller-runtime/pkg/client"
2833
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
2934
"sigs.k8s.io/controller-runtime/pkg/envtest"
3035
)
@@ -102,38 +107,38 @@ func TestLazyRestMapperProvider(t *testing.T) {
102107
// There are no requests before any call
103108
g.Expect(crt.GetRequestCount()).To(gmg.Equal(0))
104109

105-
mapping, err := lazyRestMapper.RESTMapping(schema.GroupKind{Group: "apps", Kind: "deployment"})
110+
mapping, err := lazyRestMapper.RESTMapping(schema.GroupKind{Group: "apps", Kind: "deployment"}, "v1")
106111
g.Expect(err).NotTo(gmg.HaveOccurred())
107112
g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal("deployment"))
108-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(3))
113+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(1))
109114

110-
mappings, err := lazyRestMapper.RESTMappings(schema.GroupKind{Group: "", Kind: "pod"})
115+
mappings, err := lazyRestMapper.RESTMappings(schema.GroupKind{Group: "", Kind: "pod"}, "v1")
111116
g.Expect(err).NotTo(gmg.HaveOccurred())
112117
g.Expect(len(mappings)).To(gmg.Equal(1))
113118
g.Expect(mappings[0].GroupVersionKind.Kind).To(gmg.Equal("pod"))
114-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
119+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(2))
115120

116121
kind, err := lazyRestMapper.KindFor(schema.GroupVersionResource{Group: "networking.k8s.io", Version: "v1", Resource: "ingresses"})
117122
g.Expect(err).NotTo(gmg.HaveOccurred())
118123
g.Expect(kind.Kind).To(gmg.Equal("Ingress"))
119-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(5))
124+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(3))
120125

121126
kinds, err := lazyRestMapper.KindsFor(schema.GroupVersionResource{Group: "authentication.k8s.io", Version: "v1", Resource: "tokenreviews"})
122127
g.Expect(err).NotTo(gmg.HaveOccurred())
123128
g.Expect(len(kinds)).To(gmg.Equal(1))
124129
g.Expect(kinds[0].Kind).To(gmg.Equal("TokenReview"))
125-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
130+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
126131

127132
resource, err := lazyRestMapper.ResourceFor(schema.GroupVersionResource{Group: "scheduling.k8s.io", Version: "v1", Resource: "priorityclasses"})
128133
g.Expect(err).NotTo(gmg.HaveOccurred())
129134
g.Expect(resource.Resource).To(gmg.Equal("priorityclasses"))
130-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(7))
135+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(5))
131136

132137
resources, err := lazyRestMapper.ResourcesFor(schema.GroupVersionResource{Group: "policy", Version: "v1", Resource: "poddisruptionbudgets"})
133138
g.Expect(err).NotTo(gmg.HaveOccurred())
134139
g.Expect(len(resources)).To(gmg.Equal(1))
135140
g.Expect(resources[0].Resource).To(gmg.Equal("poddisruptionbudgets"))
136-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(8))
141+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
137142
})
138143

139144
t.Run("LazyRESTMapper should cache fetched data and doesn't perform any additional requests", func(t *testing.T) {
@@ -344,29 +349,29 @@ func TestLazyRestMapperProvider(t *testing.T) {
344349
lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, apiutil.WithExperimentalLazyMapper)
345350
g.Expect(err).NotTo(gmg.HaveOccurred())
346351

347-
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: "apps", Kind: "INVALID"})
352+
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: "apps", Kind: "INVALID"}, "v1")
348353
g.Expect(err).To(gmg.HaveOccurred())
349-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(3))
354+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(1))
350355

351-
_, err = lazyRestMapper.RESTMappings(schema.GroupKind{Group: "", Kind: "INVALID"})
356+
_, err = lazyRestMapper.RESTMappings(schema.GroupKind{Group: "", Kind: "INVALID"}, "v1")
352357
g.Expect(err).To(gmg.HaveOccurred())
353-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
358+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(2))
354359

355360
_, err = lazyRestMapper.KindFor(schema.GroupVersionResource{Group: "networking.k8s.io", Version: "v1", Resource: "INVALID"})
356361
g.Expect(err).To(gmg.HaveOccurred())
357-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(5))
362+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(3))
358363

359364
_, err = lazyRestMapper.KindsFor(schema.GroupVersionResource{Group: "authentication.k8s.io", Version: "v1", Resource: "INVALID"})
360365
g.Expect(err).To(gmg.HaveOccurred())
361-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
366+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
362367

363368
_, err = lazyRestMapper.ResourceFor(schema.GroupVersionResource{Group: "scheduling.k8s.io", Version: "v1", Resource: "INVALID"})
364369
g.Expect(err).To(gmg.HaveOccurred())
365-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(7))
370+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(5))
366371

367372
_, err = lazyRestMapper.ResourcesFor(schema.GroupVersionResource{Group: "policy", Version: "v1", Resource: "INVALID"})
368373
g.Expect(err).To(gmg.HaveOccurred())
369-
g.Expect(crt.GetRequestCount()).To(gmg.Equal(8))
374+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
370375
})
371376

372377
t.Run("LazyRESTMapper should return an error if the version doesn't exist", func(t *testing.T) {
@@ -407,4 +412,87 @@ func TestLazyRestMapperProvider(t *testing.T) {
407412
g.Expect(err).To(gmg.HaveOccurred())
408413
g.Expect(crt.GetRequestCount()).To(gmg.Equal(6))
409414
})
415+
416+
t.Run("LazyRESTMapper can fetch CRDs if they were created at runtime", func(t *testing.T) {
417+
g := gmg.NewWithT(t)
418+
419+
// To fetch all versions mapper does 2 requests:
420+
// GET https://host/api
421+
// GET https://host/apis
422+
// Then, for each version it performs just one request to the API server as usual:
423+
// GET https://host/apis/<group>/<version>
424+
425+
// Note: We have to use a separate restCfg for the Client, otherwise we
426+
// get a race condition on the counting round tripper between the Client
427+
// and the lazy restmapper.
428+
clientRestCfg := rest.CopyConfig(restCfg)
429+
430+
var crt *countingRoundTripper
431+
restCfg := rest.CopyConfig(restCfg)
432+
restCfg.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {
433+
crt = newCountingRoundTripper(rt)
434+
return crt
435+
}
436+
437+
lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, apiutil.WithExperimentalLazyMapper)
438+
g.Expect(err).NotTo(gmg.HaveOccurred())
439+
440+
// There are no requests before any call
441+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(0))
442+
443+
// Since we don't specify what version we expect, restmapper will fetch them all and search there.
444+
// To fetch a list of available versions
445+
// #1: GET https://host/api
446+
// #2: GET https://host/apis
447+
// Then, for each currently registered version:
448+
// #3: GET https://host/apis/crew.example.com/v1
449+
// #4: GET https://host/apis/crew.example.com/v2
450+
mapping, err := lazyRestMapper.RESTMapping(schema.GroupKind{Group: "crew.example.com", Kind: "driver"})
451+
g.Expect(err).NotTo(gmg.HaveOccurred())
452+
g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal("driver"))
453+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
454+
455+
s := scheme.Scheme
456+
err = apiextensionsv1.AddToScheme(s)
457+
g.Expect(err).NotTo(gmg.HaveOccurred())
458+
459+
c, err := client.New(clientRestCfg, client.Options{Scheme: s})
460+
g.Expect(err).NotTo(gmg.HaveOccurred())
461+
462+
// Register another CRD in runtime - "riders.crew.example.com".
463+
464+
crd := &apiextensionsv1.CustomResourceDefinition{}
465+
err = c.Get(context.TODO(), types.NamespacedName{Name: "drivers.crew.example.com"}, crd)
466+
g.Expect(err).NotTo(gmg.HaveOccurred())
467+
g.Expect(crd.Spec.Names.Kind).To(gmg.Equal("Driver"))
468+
469+
newCRD := &apiextensionsv1.CustomResourceDefinition{}
470+
crd.DeepCopyInto(newCRD)
471+
newCRD.Name = "riders.crew.example.com"
472+
newCRD.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
473+
Kind: "Rider",
474+
Plural: "riders",
475+
}
476+
newCRD.ResourceVersion = ""
477+
478+
// Create the new CRD.
479+
g.Expect(c.Create(context.TODO(), newCRD)).To(gmg.Succeed())
480+
481+
// Wait a bit until the CRD is registered.
482+
g.Eventually(func() error {
483+
_, err := lazyRestMapper.RESTMapping(schema.GroupKind{Group: "crew.example.com", Kind: "rider"})
484+
return err
485+
}).Should(gmg.Succeed())
486+
487+
// Since we don't specify what version we expect, restmapper will fetch them all and search there.
488+
// To fetch a list of available versions
489+
// #1: GET https://host/api
490+
// #2: GET https://host/apis
491+
// Then, for each currently registered version:
492+
// #3: GET https://host/apis/crew.example.com/v1
493+
// #4: GET https://host/apis/crew.example.com/v2
494+
mapping, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: "crew.example.com", Kind: "rider"})
495+
g.Expect(err).NotTo(gmg.HaveOccurred())
496+
g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal("rider"))
497+
})
410498
}

0 commit comments

Comments
 (0)