From 984aee6ab079971f69da50f0fb27334fe61ec01f Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 9 Feb 2024 14:00:57 -0500 Subject: [PATCH 1/2] bug: Fakeclient: Do not consider an apply patch to be a strategic merge patch The fakeclient currently considers an apply patch to be a strategic merge patch. This is completely wrong, those are different things and apply patches are not supported in the fakeclientl, because in order to support them it would need the entire SSA logic which isn't implemented in upstream yet. --- pkg/client/fake/client.go | 6 ++++-- pkg/client/fake/client_test.go | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 790a1faab1..b90a6ebb8d 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -947,7 +947,7 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru if err := json.Unmarshal(modified, obj); err != nil { return nil, err } - case types.StrategicMergePatchType, types.ApplyPatchType: + case types.StrategicMergePatchType: mergedByte, err := strategicpatch.StrategicMergePatch(old, action.GetPatch(), obj) if err != nil { return nil, err @@ -955,8 +955,10 @@ func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (ru if err = json.Unmarshal(mergedByte, obj); err != nil { return nil, err } + case types.ApplyPatchType: + return nil, errors.New("apply patches are not supported in the fake client. Follow https://github.com/kubernetes/kubernetes/issues/115598 for the current status") default: - return nil, fmt.Errorf("PatchType is not supported") + return nil, fmt.Errorf("%s PatchType is not supported", action.GetPatchType()) } return obj, nil } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index d299175ba9..e5487de21a 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -359,6 +359,26 @@ var _ = Describe("Fake client", func() { Expect(list.Items).To(ConsistOf(*dep2)) }) + It("should reject apply patches, they are not supported in the fake client", func() { + By("Creating a new configmap") + cm := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "new-test-cm", + Namespace: "ns2", + }, + } + err := cl.Create(context.Background(), cm) + Expect(err).ToNot(HaveOccurred()) + + cm.Data = map[string]string{"foo": "bar"} + err = cl.Patch(context.Background(), cm, client.Apply, client.ForceOwnership) + Expect(err).To(MatchError(ContainSubstring("apply patches are not supported in the fake client"))) + }) + It("should be able to Create", func() { By("Creating a new configmap") newcm := &corev1.ConfigMap{ From 565aa5b9f00690ef8e405d8a6e7f4d6b4204ca5e Mon Sep 17 00:00:00 2001 From: Guillermo Gaston Date: Wed, 14 Feb 2024 18:40:16 +0000 Subject: [PATCH 2/2] 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. --- pkg/client/apiutil/restmapper.go | 28 +++- pkg/client/apiutil/restmapper_wb_test.go | 203 +++++++++++++++++++++++ 2 files changed, 227 insertions(+), 4 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 24af330c8b..927be22b4e 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 @@ -280,11 +280,15 @@ 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) + if m.isAPIGroupCached(groupVersion) { + delete(m.apiGroups, groupName) + } + if m.isGroupVersionCached(groupVersion) { + delete(m.knownGroups, groupName) + } continue } else if err != nil { failedGroups[groupVersion] = err @@ -313,3 +317,19 @@ 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 +} diff --git a/pkg/client/apiutil/restmapper_wb_test.go b/pkg/client/apiutil/restmapper_wb_test.go new file mode 100644 index 0000000000..96dbe79e77 --- /dev/null +++ b/pkg/client/apiutil/restmapper_wb_test.go @@ -0,0 +1,203 @@ +/* +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).NotTo(gmg.HaveOccurred()) + g.Expect(m.apiGroups).To(gmg.BeComparableTo(tt.expectedAPIGroups)) + g.Expect(m.knownGroups).To(gmg.BeComparableTo(tt.expectedKnownGroups)) + }) + } +}