Skip to content

Commit 5923139

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2676 from k8s-infra-cherrypick-robot/cherry-pick-2663-to-release-0.17
[release-0.17] 🐛 Clean restmapper cache if a version is notFound
2 parents 11e5a5e + 0811bad commit 5923139

File tree

2 files changed

+184
-27
lines changed

2 files changed

+184
-27
lines changed

Diff for: ‎pkg/client/apiutil/restmapper.go

+33-10
Original file line numberDiff line numberDiff line change
@@ -182,23 +182,28 @@ func (m *mapper) addKnownGroupAndReload(groupName string, versions ...string) er
182182
Group: metav1.APIGroup{Name: groupName},
183183
VersionedResources: make(map[string][]metav1.APIResource),
184184
}
185-
if _, ok := m.knownGroups[groupName]; ok {
186-
groupResources = m.knownGroups[groupName]
187-
}
188185

189186
// Update information for group resources about versioned resources.
190187
// The number of API calls is equal to the number of versions: /apis/<group>/<version>.
191-
groupVersionResources, err := m.fetchGroupVersionResources(groupName, versions...)
188+
// If we encounter a missing API version (NotFound error), we will remove the group from
189+
// the m.apiGroups and m.knownGroups caches.
190+
// If this happens, in the next call the group will be added back to apiGroups
191+
// and only the existing versions will be loaded in knownGroups.
192+
groupVersionResources, err := m.fetchGroupVersionResourcesLocked(groupName, versions...)
192193
if err != nil {
193194
return fmt.Errorf("failed to get API group resources: %w", err)
194195
}
195-
for version, resources := range groupVersionResources {
196-
groupResources.VersionedResources[version.Version] = resources.APIResources
196+
197+
if _, ok := m.knownGroups[groupName]; ok {
198+
groupResources = m.knownGroups[groupName]
197199
}
198200

199201
// Update information for group resources about the API group by adding new versions.
200202
// Ignore the versions that are already registered.
201-
for _, version := range versions {
203+
for groupVersion, resources := range groupVersionResources {
204+
version := groupVersion.Version
205+
206+
groupResources.VersionedResources[version] = resources.APIResources
202207
found := false
203208
for _, v := range groupResources.Group.Versions {
204209
if v.Version == version {
@@ -265,18 +270,26 @@ func (m *mapper) findAPIGroupByName(groupName string) (*metav1.APIGroup, error)
265270
return m.apiGroups[groupName], nil
266271
}
267272

268-
// fetchGroupVersionResources fetches the resources for the specified group and its versions.
269-
func (m *mapper) fetchGroupVersionResources(groupName string, versions ...string) (map[schema.GroupVersion]*metav1.APIResourceList, error) {
273+
// fetchGroupVersionResourcesLocked fetches the resources for the specified group and its versions.
274+
// This method might modify the cache so it needs to be called under the lock.
275+
func (m *mapper) fetchGroupVersionResourcesLocked(groupName string, versions ...string) (map[schema.GroupVersion]*metav1.APIResourceList, error) {
270276
groupVersionResources := make(map[schema.GroupVersion]*metav1.APIResourceList)
271277
failedGroups := make(map[schema.GroupVersion]error)
272278

273279
for _, version := range versions {
274280
groupVersion := schema.GroupVersion{Group: groupName, Version: version}
275281

276282
apiResourceList, err := m.client.ServerResourcesForGroupVersion(groupVersion.String())
277-
if err != nil && !apierrors.IsNotFound(err) {
283+
if apierrors.IsNotFound(err) && m.isGroupVersionCached(groupVersion) {
284+
// If the version is not found, we remove the group from the cache
285+
// so it gets refreshed on the next call.
286+
delete(m.apiGroups, groupName)
287+
delete(m.knownGroups, groupName)
288+
continue
289+
} else if err != nil {
278290
failedGroups[groupVersion] = err
279291
}
292+
280293
if apiResourceList != nil {
281294
// even in case of error, some fallback might have been returned.
282295
groupVersionResources[groupVersion] = apiResourceList
@@ -290,3 +303,13 @@ func (m *mapper) fetchGroupVersionResources(groupName string, versions ...string
290303

291304
return groupVersionResources, nil
292305
}
306+
307+
// isGroupVersionCached checks if a version for a group is cached in the known groups cache.
308+
func (m *mapper) isGroupVersionCached(gv schema.GroupVersion) bool {
309+
if cachedGroup, ok := m.knownGroups[gv.Group]; ok {
310+
_, cached := cachedGroup.VersionedResources[gv.Version]
311+
return cached
312+
}
313+
314+
return false
315+
}

Diff for: ‎pkg/client/apiutil/restmapper_test.go

+151-17
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,11 @@ import (
2828
gomegatypes "github.com/onsi/gomega/types"
2929

3030
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
31+
apierrors "k8s.io/apimachinery/pkg/api/errors"
3132
"k8s.io/apimachinery/pkg/api/meta"
3233
"k8s.io/apimachinery/pkg/runtime/schema"
3334
"k8s.io/apimachinery/pkg/types"
35+
"k8s.io/client-go/discovery"
3436
"k8s.io/client-go/kubernetes/scheme"
3537
"k8s.io/client-go/rest"
3638

@@ -529,23 +531,7 @@ func TestLazyRestMapperProvider(t *testing.T) {
529531
g.Expect(err).NotTo(gmg.HaveOccurred())
530532

531533
// Register another CRD in runtime - "riders.crew.example.com".
532-
533-
crd := &apiextensionsv1.CustomResourceDefinition{}
534-
err = c.Get(context.TODO(), types.NamespacedName{Name: "drivers.crew.example.com"}, crd)
535-
g.Expect(err).NotTo(gmg.HaveOccurred())
536-
g.Expect(crd.Spec.Names.Kind).To(gmg.Equal("Driver"))
537-
538-
newCRD := &apiextensionsv1.CustomResourceDefinition{}
539-
crd.DeepCopyInto(newCRD)
540-
newCRD.Name = "riders.crew.example.com"
541-
newCRD.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
542-
Kind: "Rider",
543-
Plural: "riders",
544-
}
545-
newCRD.ResourceVersion = ""
546-
547-
// Create the new CRD.
548-
g.Expect(c.Create(context.TODO(), newCRD)).To(gmg.Succeed())
534+
createNewCRD(context.TODO(), g, c, "crew.example.com", "Rider", "riders")
549535

550536
// Wait a bit until the CRD is registered.
551537
g.Eventually(func() error {
@@ -564,6 +550,153 @@ func TestLazyRestMapperProvider(t *testing.T) {
564550
g.Expect(err).NotTo(gmg.HaveOccurred())
565551
g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal("rider"))
566552
})
553+
554+
t.Run("LazyRESTMapper should invalidate the group cache if a version is not found", func(t *testing.T) {
555+
g := gmg.NewWithT(t)
556+
ctx := context.Background()
557+
558+
httpClient, err := rest.HTTPClientFor(restCfg)
559+
g.Expect(err).NotTo(gmg.HaveOccurred())
560+
561+
crt := newCountingRoundTripper(httpClient.Transport)
562+
httpClient.Transport = crt
563+
564+
lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient)
565+
g.Expect(err).NotTo(gmg.HaveOccurred())
566+
567+
s := scheme.Scheme
568+
err = apiextensionsv1.AddToScheme(s)
569+
g.Expect(err).NotTo(gmg.HaveOccurred())
570+
571+
c, err := client.New(restCfg, client.Options{Scheme: s})
572+
g.Expect(err).NotTo(gmg.HaveOccurred())
573+
574+
// Register a new CRD ina new group to avoid collisions when deleting versions - "taxis.inventory.example.com".
575+
group := "inventory.example.com"
576+
kind := "Taxi"
577+
plural := "taxis"
578+
crdName := plural + "." + group
579+
// Create a CRD with two versions: v1alpha1 and v1 where both are served and
580+
// v1 is the storage version so we can easily remove v1alpha1 later.
581+
crd := newCRD(ctx, g, c, group, kind, plural)
582+
v1alpha1 := crd.Spec.Versions[0]
583+
v1alpha1.Name = "v1alpha1"
584+
v1alpha1.Storage = false
585+
v1alpha1.Served = true
586+
v1 := crd.Spec.Versions[0]
587+
v1.Name = "v1"
588+
v1.Storage = true
589+
v1.Served = true
590+
crd.Spec.Versions = []apiextensionsv1.CustomResourceDefinitionVersion{v1alpha1, v1}
591+
g.Expect(c.Create(ctx, crd)).To(gmg.Succeed())
592+
t.Cleanup(func() {
593+
g.Expect(c.Delete(ctx, crd)).To(gmg.Succeed())
594+
})
595+
596+
// Wait until the CRD is registered.
597+
discHTTP, err := rest.HTTPClientFor(restCfg)
598+
g.Expect(err).NotTo(gmg.HaveOccurred())
599+
discClient, err := discovery.NewDiscoveryClientForConfigAndClient(restCfg, discHTTP)
600+
g.Expect(err).NotTo(gmg.HaveOccurred())
601+
g.Eventually(func(g gmg.Gomega) {
602+
_, err = discClient.ServerResourcesForGroupVersion(group + "/v1")
603+
g.Expect(err).NotTo(gmg.HaveOccurred())
604+
}).Should(gmg.Succeed(), "v1 should be available")
605+
606+
// There are no requests before any call
607+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(0))
608+
609+
// Since we don't specify what version we expect, restmapper will fetch them all and search there.
610+
// To fetch a list of available versions
611+
// #1: GET https://host/api
612+
// #2: GET https://host/apis
613+
// Then, for all available versions:
614+
// #3: GET https://host/apis/inventory.example.com/v1alpha1
615+
// #4: GET https://host/apis/inventory.example.com/v1
616+
// This should fill the cache for apiGroups and versions.
617+
mapping, err := lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: kind})
618+
g.Expect(err).NotTo(gmg.HaveOccurred())
619+
g.Expect(mapping.GroupVersionKind.Kind).To(gmg.Equal(kind))
620+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(4))
621+
crt.Reset() // We reset the counter to check how many additional requests are made later.
622+
623+
// At this point v1alpha1 should be cached
624+
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: kind}, "v1alpha1")
625+
g.Expect(err).NotTo(gmg.HaveOccurred())
626+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(0))
627+
628+
// We update the CRD to only have v1 version.
629+
g.Expect(c.Get(ctx, types.NamespacedName{Name: crdName}, crd)).To(gmg.Succeed())
630+
for _, version := range crd.Spec.Versions {
631+
if version.Name == "v1" {
632+
v1 = version
633+
break
634+
}
635+
}
636+
crd.Spec.Versions = []apiextensionsv1.CustomResourceDefinitionVersion{v1}
637+
g.Expect(c.Update(ctx, crd)).To(gmg.Succeed())
638+
639+
// We wait until v1alpha1 is not available anymore.
640+
g.Eventually(func(g gmg.Gomega) {
641+
_, err = discClient.ServerResourcesForGroupVersion(group + "/v1alpha1")
642+
g.Expect(apierrors.IsNotFound(err)).To(gmg.BeTrue(), "v1alpha1 should not be available anymore")
643+
}).Should(gmg.Succeed())
644+
645+
// Although v1alpha1 is not available anymore, the cache is not invalidated yet so it should return a mapping.
646+
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: kind}, "v1alpha1")
647+
g.Expect(err).NotTo(gmg.HaveOccurred())
648+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(0))
649+
650+
// We request Limo, which is not in the mapper because it doesn't exist.
651+
// This will trigger a reload of the lazy mapper cache.
652+
// Reloading the cache will read v2 again and since it's not available anymore, it should invalidate the cache.
653+
// #1: GET https://host/apis/inventory.example.com/v1alpha1
654+
// #2: GET https://host/apis/inventory.example.com/v1
655+
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: "Limo"})
656+
g.Expect(err).To(beNoMatchError())
657+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(2))
658+
crt.Reset()
659+
660+
// Now we request v1alpha1 again and it should return an error since the cache was invalidated.
661+
// #1: GET https://host/apis/inventory.example.com/v1alpha1
662+
_, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: kind}, "v1alpha1")
663+
g.Expect(err).To(beNoMatchError())
664+
g.Expect(crt.GetRequestCount()).To(gmg.Equal(1))
665+
666+
// Verify that when requesting the mapping without a version, it doesn't error
667+
// and it returns v1.
668+
mapping, err = lazyRestMapper.RESTMapping(schema.GroupKind{Group: group, Kind: kind})
669+
g.Expect(err).NotTo(gmg.HaveOccurred())
670+
g.Expect(mapping.Resource.Version).To(gmg.Equal("v1"))
671+
})
672+
}
673+
674+
// createNewCRD creates a new CRD with the given group, kind, and plural and returns it.
675+
func createNewCRD(ctx context.Context, g gmg.Gomega, c client.Client, group, kind, plural string) *apiextensionsv1.CustomResourceDefinition {
676+
newCRD := newCRD(ctx, g, c, group, kind, plural)
677+
g.Expect(c.Create(ctx, newCRD)).To(gmg.Succeed())
678+
679+
return newCRD
680+
}
681+
682+
// newCRD returns a new CRD with the given group, kind, and plural.
683+
func newCRD(ctx context.Context, g gmg.Gomega, c client.Client, group, kind, plural string) *apiextensionsv1.CustomResourceDefinition {
684+
crd := &apiextensionsv1.CustomResourceDefinition{}
685+
err := c.Get(ctx, types.NamespacedName{Name: "drivers.crew.example.com"}, crd)
686+
g.Expect(err).NotTo(gmg.HaveOccurred())
687+
g.Expect(crd.Spec.Names.Kind).To(gmg.Equal("Driver"))
688+
689+
newCRD := &apiextensionsv1.CustomResourceDefinition{}
690+
crd.DeepCopyInto(newCRD)
691+
newCRD.Spec.Group = group
692+
newCRD.Name = plural + "." + group
693+
newCRD.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
694+
Kind: kind,
695+
Plural: plural,
696+
}
697+
newCRD.ResourceVersion = ""
698+
699+
return newCRD
567700
}
568701

569702
func beNoMatchError() gomegatypes.GomegaMatcher {
@@ -594,6 +727,7 @@ func (e *errorMatcher) Match(actual interface{}) (success bool, err error) {
594727
func (e *errorMatcher) FailureMessage(actual interface{}) (message string) {
595728
return format.Message(actual, fmt.Sprintf("to be %s error", e.message))
596729
}
730+
597731
func (e *errorMatcher) NegatedFailureMessage(actual interface{}) (message string) {
598732
return format.Message(actual, fmt.Sprintf("not to be %s error", e.message))
599733
}

0 commit comments

Comments
 (0)