From 09649e58b5a1368929e194991a763afc8011795e Mon Sep 17 00:00:00 2001 From: Andrea Nodari Date: Thu, 11 Mar 2021 16:51:46 +0100 Subject: [PATCH] Check request info when updating managed fields during scale - Test all versions to make sure each resource version is in the mappings - Fail when request info contains an unrecognized version. We have tests that guarantee that all known versions are in the mappings. If we get a version in request info that is not there we should fail fast to prevent inconsistent behaviour (e.g. for some reason the mappings is not up to date). Ensure all known versions are in mappings --- .../apps/deployment/storage/storage.go | 23 ++++- .../apps/replicaset/storage/storage.go | 22 ++++- .../apps/statefulset/storage/storage.go | 23 ++++- .../replicationcontroller/storage/storage.go | 19 +++- .../handlers/fieldmanager/scalehandler.go | 18 ++-- .../integration/apiserver/apply/scale_test.go | 88 +++++++++++++++++++ 6 files changed, 177 insertions(+), 16 deletions(-) diff --git a/pkg/registry/apps/deployment/storage/storage.go b/pkg/registry/apps/deployment/storage/storage.go index 409d946929a..773ec94b184 100644 --- a/pkg/registry/apps/deployment/storage/storage.go +++ b/pkg/registry/apps/deployment/storage/storage.go @@ -26,12 +26,14 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/apiserver/pkg/storage" storeerr "k8s.io/apiserver/pkg/storage/errors" "k8s.io/apiserver/pkg/util/dryrun" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/apis/apps" appsv1beta1 "k8s.io/kubernetes/pkg/apis/apps/v1beta1" appsv1beta2 "k8s.io/kubernetes/pkg/apis/apps/v1beta2" @@ -55,9 +57,16 @@ type DeploymentStorage struct { Rollback *RollbackREST } +// ReplicasPathMappings returns the mappings between each group version and a replicas path +func ReplicasPathMappings() fieldmanager.ResourcePathMappings { + return replicasPathInDeployment +} + // maps a group version to the replicas path in a deployment object var replicasPathInDeployment = fieldmanager.ResourcePathMappings{ - schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1beta1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1beta2"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), } // NewStorage returns new instance of DeploymentStorage. @@ -383,9 +392,19 @@ func (i *scaleUpdatedObjectInfo) UpdatedObject(ctx context.Context, oldObj runti return nil, errors.NewNotFound(apps.Resource("deployments/scale"), i.name) } + groupVersion := schema.GroupVersion{Group: "apps", Version: "v1"} + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + requestGroupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + if _, ok := replicasPathInDeployment[requestGroupVersion.String()]; ok { + groupVersion = requestGroupVersion + } else { + klog.Fatal("Unrecognized group/version in request info %q", requestGroupVersion.String()) + } + } + managedFieldsHandler := fieldmanager.NewScaleHandler( deployment.ManagedFields, - schema.GroupVersion{Group: "apps", Version: "v1"}, + groupVersion, replicasPathInDeployment, ) diff --git a/pkg/registry/apps/replicaset/storage/storage.go b/pkg/registry/apps/replicaset/storage/storage.go index e16a4664608..11e51a300b4 100644 --- a/pkg/registry/apps/replicaset/storage/storage.go +++ b/pkg/registry/apps/replicaset/storage/storage.go @@ -27,9 +27,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/apis/apps" appsv1beta1 "k8s.io/kubernetes/pkg/apis/apps/v1beta1" appsv1beta2 "k8s.io/kubernetes/pkg/apis/apps/v1beta2" @@ -51,9 +53,15 @@ type ReplicaSetStorage struct { Scale *ScaleREST } +// ReplicasPathMappings returns the mappings between each group version and a replicas path +func ReplicasPathMappings() fieldmanager.ResourcePathMappings { + return replicasPathInReplicaSet +} + // maps a group version to the replicas path in a replicaset object var replicasPathInReplicaSet = fieldmanager.ResourcePathMappings{ - schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1beta2"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), } // NewStorage returns new instance of ReplicaSetStorage. @@ -285,9 +293,19 @@ func (i *scaleUpdatedObjectInfo) UpdatedObject(ctx context.Context, oldObj runti return nil, errors.NewNotFound(apps.Resource("replicasets/scale"), i.name) } + groupVersion := schema.GroupVersion{Group: "apps", Version: "v1"} + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + requestGroupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + if _, ok := replicasPathInReplicaSet[requestGroupVersion.String()]; ok { + groupVersion = requestGroupVersion + } else { + klog.Fatal("Unrecognized group/version in request info %q", requestGroupVersion.String()) + } + } + managedFieldsHandler := fieldmanager.NewScaleHandler( replicaset.ManagedFields, - schema.GroupVersion{Group: "apps", Version: "v1"}, + groupVersion, replicasPathInReplicaSet, ) diff --git a/pkg/registry/apps/statefulset/storage/storage.go b/pkg/registry/apps/statefulset/storage/storage.go index 50b9954784c..6035350ac07 100644 --- a/pkg/registry/apps/statefulset/storage/storage.go +++ b/pkg/registry/apps/statefulset/storage/storage.go @@ -25,9 +25,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/apis/apps" appsv1beta1 "k8s.io/kubernetes/pkg/apis/apps/v1beta1" appsv1beta2 "k8s.io/kubernetes/pkg/apis/apps/v1beta2" @@ -48,9 +50,16 @@ type StatefulSetStorage struct { Scale *ScaleREST } +// ReplicasPathMappings returns the mappings between each group version and a replicas path +func ReplicasPathMappings() fieldmanager.ResourcePathMappings { + return replicasPathInStatefulSet +} + // maps a group version to the replicas path in a statefulset object var replicasPathInStatefulSet = fieldmanager.ResourcePathMappings{ - schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1beta1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1beta2"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), + schema.GroupVersion{Group: "apps", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), } // NewStorage returns new instance of StatefulSetStorage. @@ -271,9 +280,19 @@ func (i *scaleUpdatedObjectInfo) UpdatedObject(ctx context.Context, oldObj runti return nil, errors.NewNotFound(apps.Resource("statefulsets/scale"), i.name) } + groupVersion := schema.GroupVersion{Group: "apps", Version: "v1"} + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + requestGroupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + if _, ok := replicasPathInStatefulSet[requestGroupVersion.String()]; ok { + groupVersion = requestGroupVersion + } else { + klog.Fatal("Unrecognized group/version in request info %q", requestGroupVersion.String()) + } + } + managedFieldsHandler := fieldmanager.NewScaleHandler( statefulset.ManagedFields, - schema.GroupVersion{Group: "apps", Version: "v1"}, + groupVersion, replicasPathInStatefulSet, ) diff --git a/pkg/registry/core/replicationcontroller/storage/storage.go b/pkg/registry/core/replicationcontroller/storage/storage.go index d8457e54a6c..925a308eec2 100644 --- a/pkg/registry/core/replicationcontroller/storage/storage.go +++ b/pkg/registry/core/replicationcontroller/storage/storage.go @@ -28,9 +28,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/apis/autoscaling" autoscalingv1 "k8s.io/kubernetes/pkg/apis/autoscaling/v1" "k8s.io/kubernetes/pkg/apis/autoscaling/validation" @@ -50,6 +52,11 @@ type ControllerStorage struct { Scale *ScaleREST } +// ReplicasPathMappings returns the mappings between each group version and a replicas path +func ReplicasPathMappings() fieldmanager.ResourcePathMappings { + return replicasPathInReplicationController +} + // maps a group version to the replicas path in a deployment object var replicasPathInReplicationController = fieldmanager.ResourcePathMappings{ schema.GroupVersion{Group: "", Version: "v1"}.String(): fieldpath.MakePathOrDie("spec", "replicas"), @@ -245,9 +252,19 @@ func (i *scaleUpdatedObjectInfo) UpdatedObject(ctx context.Context, oldObj runti return nil, errors.NewNotFound(api.Resource("replicationcontrollers/scale"), i.name) } + groupVersion := schema.GroupVersion{Group: "", Version: "v1"} + if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found { + requestGroupVersion := schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion} + if _, ok := replicasPathInReplicationController[requestGroupVersion.String()]; ok { + groupVersion = requestGroupVersion + } else { + klog.Fatal("Unrecognized group/version in request info %q", requestGroupVersion.String()) + } + } + managedFieldsHandler := fieldmanager.NewScaleHandler( replicationcontroller.ManagedFields, - schema.GroupVersion{Group: "", Version: "v1"}, + groupVersion, replicasPathInReplicationController, ) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go index d551a43cb59..114cdb1845d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/scalehandler.go @@ -39,17 +39,17 @@ type ResourcePathMappings map[string]fieldpath.Path // ScaleHandler manages the conversion of managed fields between a main // resource and the scale subresource type ScaleHandler struct { - parentEntries []metav1.ManagedFieldsEntry - defaultGroupVersion schema.GroupVersion - mappings ResourcePathMappings + parentEntries []metav1.ManagedFieldsEntry + groupVersion schema.GroupVersion + mappings ResourcePathMappings } // NewScaleHandler creates a new ScaleHandler -func NewScaleHandler(parentEntries []metav1.ManagedFieldsEntry, defaultGroupVersion schema.GroupVersion, mappings ResourcePathMappings) *ScaleHandler { +func NewScaleHandler(parentEntries []metav1.ManagedFieldsEntry, groupVersion schema.GroupVersion, mappings ResourcePathMappings) *ScaleHandler { return &ScaleHandler{ - parentEntries: parentEntries, - defaultGroupVersion: defaultGroupVersion, - mappings: mappings, + parentEntries: parentEntries, + groupVersion: groupVersion, + mappings: mappings, } } @@ -136,8 +136,8 @@ func (h *ScaleHandler) ToParent(scaleEntries []metav1.ManagedFieldsEntry) ([]met for manager, versionedSet := range scaleFields { newVersionedSet := fieldpath.NewVersionedSet( - fieldpath.NewSet(h.mappings[h.defaultGroupVersion.String()]), - fieldpath.APIVersion(h.defaultGroupVersion.String()), + fieldpath.NewSet(h.mappings[h.groupVersion.String()]), + fieldpath.APIVersion(h.groupVersion.String()), versionedSet.Applied(), ) f[manager] = newVersionedSet diff --git a/test/integration/apiserver/apply/scale_test.go b/test/integration/apiserver/apply/scale_test.go index de8005e6893..ad0db889f7b 100644 --- a/test/integration/apiserver/apply/scale_test.go +++ b/test/integration/apiserver/apply/scale_test.go @@ -27,11 +27,18 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" genericfeatures "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" clientset "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/scheme" featuregatetesting "k8s.io/component-base/featuregate/testing" + deploymentstorage "k8s.io/kubernetes/pkg/registry/apps/deployment/storage" + replicasetstorage "k8s.io/kubernetes/pkg/registry/apps/replicaset/storage" + statefulsetstorage "k8s.io/kubernetes/pkg/registry/apps/statefulset/storage" + replicationcontrollerstorage "k8s.io/kubernetes/pkg/registry/core/replicationcontroller/storage" ) type scaleTest struct { @@ -231,6 +238,87 @@ func TestScaleAllResources(t *testing.T) { } } +func TestScaleUpdateOnlyStatus(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + + _, client, closeFn := setup(t) + defer closeFn() + + resource := "deployments" + path := "/apis/apps/v1" + validObject := []byte(validAppsV1("Deployment")) + + // Create the object + _, err := client.CoreV1().RESTClient().Patch(types.ApplyPatchType). + AbsPath(path). + Namespace("default"). + Resource(resource). + Name("test"). + Param("fieldManager", "apply_test"). + Body(validObject). + Do(context.TODO()).Get() + if err != nil { + t.Fatalf("Failed to create object using apply: %v", err) + } + obj := retrieveObject(t, client, path, resource) + assertReplicasValue(t, obj, 1) + assertReplicasOwnership(t, obj, "apply_test") + + // Call scale subresource to update replicas + _, err = client.CoreV1().RESTClient(). + Patch(types.MergePatchType). + AbsPath(path). + Namespace("default"). + Resource(resource). + Name("test"). + SubResource("scale"). + Param("fieldManager", "scale_test"). + Body([]byte(`{"status":{"replicas": 42}}`)). + Do(context.TODO()).Get() + if err != nil { + t.Fatalf("Failed to scale object: %v", err) + } + obj = retrieveObject(t, client, path, resource) + assertReplicasValue(t, obj, 1) + assertReplicasOwnership(t, obj, "apply_test") +} + +func TestAllKnownVersionsAreInMappings(t *testing.T) { + cases := []struct { + groupKind schema.GroupKind + mappings fieldmanager.ResourcePathMappings + }{ + { + groupKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"}, + mappings: replicasetstorage.ReplicasPathMappings(), + }, + { + groupKind: schema.GroupKind{Group: "apps", Kind: "StatefulSet"}, + mappings: statefulsetstorage.ReplicasPathMappings(), + }, + { + groupKind: schema.GroupKind{Group: "apps", Kind: "Deployment"}, + mappings: deploymentstorage.ReplicasPathMappings(), + }, + { + groupKind: schema.GroupKind{Group: "", Kind: "ReplicationController"}, + mappings: replicationcontrollerstorage.ReplicasPathMappings(), + }, + } + for _, c := range cases { + knownVersions := scheme.Scheme.VersionsForGroupKind(c.groupKind) + for _, version := range knownVersions { + if _, ok := c.mappings[version.String()]; !ok { + t.Errorf("missing version %v for %v mappings", version, c.groupKind) + } + } + + if len(knownVersions) != len(c.mappings) { + t.Errorf("%v mappings has extra items: %v vs %v", c.groupKind, c.mappings, knownVersions) + } + } +} + func validAppsV1(kind string) string { return fmt.Sprintf(`{ "apiVersion": "apps/v1",