From 94cb60e0e86f1dae68a9988a2e5e3f8e80247972 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Wed, 18 Mar 2020 12:51:56 -0700 Subject: [PATCH 1/2] Use discovery to test apply all status --- test/integration/apiserver/apply/BUILD | 2 + .../apiserver/apply/status_test.go | 219 ++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 test/integration/apiserver/apply/status_test.go diff --git a/test/integration/apiserver/apply/BUILD b/test/integration/apiserver/apply/BUILD index c9f8dc267fe..a4011615c9d 100644 --- a/test/integration/apiserver/apply/BUILD +++ b/test/integration/apiserver/apply/BUILD @@ -7,6 +7,7 @@ go_test( "apply_crd_test.go", "apply_test.go", "main_test.go", + "status_test.go", ], tags = [ "etcd", @@ -32,6 +33,7 @@ go_test( "//staging/src/k8s.io/client-go/kubernetes:go_default_library", "//staging/src/k8s.io/client-go/rest:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//test/integration/etcd:go_default_library", "//test/integration/framework:go_default_library", "//vendor/sigs.k8s.io/yaml:go_default_library", ], diff --git a/test/integration/apiserver/apply/status_test.go b/test/integration/apiserver/apply/status_test.go new file mode 100644 index 00000000000..2214c72a499 --- /dev/null +++ b/test/integration/apiserver/apply/status_test.go @@ -0,0 +1,219 @@ +/* +Copyright 2020 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 apiserver + +import ( + "context" + "encoding/json" + "strings" + "testing" + + v1 "k8s.io/api/core/v1" + apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + genericfeatures "k8s.io/apiserver/pkg/features" + utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" + featuregatetesting "k8s.io/component-base/featuregate/testing" + apiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/test/integration/etcd" + "k8s.io/kubernetes/test/integration/framework" + "sigs.k8s.io/yaml" +) + +// namespace used for all tests, do not change this +const testNamespace = "statusnamespace" + +var statusData = map[schema.GroupVersionResource]string{ + gvr("", "v1", "persistentvolumes"): `{"status": {"message": "hello"}}`, + gvr("", "v1", "resourcequotas"): `{"status": {"used": {"cpu": "5M"}}}`, + gvr("", "v1", "services"): `{"status": {"loadBalancer": {"ingress": [{"ip": "127.0.0.1"}]}}}`, + gvr("extensions", "v1beta1", "ingresses"): `{"status": {"loadBalancer": {"ingress": [{"ip": "127.0.0.1"}]}}}`, + gvr("networking.k8s.io", "v1beta1", "ingresses"): `{"status": {"loadBalancer": {"ingress": [{"ip": "127.0.0.1"}]}}}`, + gvr("autoscaling", "v1", "horizontalpodautoscalers"): `{"status": {"currentReplicas": 5}}`, + gvr("batch", "v1beta1", "cronjobs"): `{"status": {"lastScheduleTime": null}}`, + gvr("batch", "v2alpha1", "cronjobs"): `{"status": {"lastScheduleTime": null}}`, + gvr("storage.k8s.io", "v1", "volumeattachments"): `{"status": {"attached": true}}`, + gvr("policy", "v1beta1", "poddisruptionbudgets"): `{"status": {"currentHealthy": 5}}`, + gvr("certificates.k8s.io", "v1beta1", "certificatesigningrequests"): `{"status": {"conditions": [{"type": "MyStatus"}]}}`, +} + +const statusDefault = `{"status": {"conditions": [{"type": "MyStatus", "status":"true"}]}}` + +// DO NOT ADD TO THIS LIST. +// This list is used to ignore known bugs. We shouldn't introduce new bugs. +var ignoreList = map[schema.GroupVersionResource]struct{}{ + // TODO(#89264): apiservices doesn't work because the openapi is not routed properly. + gvr("apiregistration.k8s.io", "v1beta1", "apiservices"): {}, + gvr("apiregistration.k8s.io", "v1", "apiservices"): {}, +} + +func gvr(g, v, r string) schema.GroupVersionResource { + return schema.GroupVersionResource{Group: g, Version: v, Resource: r} +} + +func createMapping(groupVersion string, resource metav1.APIResource) (*meta.RESTMapping, error) { + gv, err := schema.ParseGroupVersion(groupVersion) + if err != nil { + return nil, err + } + if len(resource.Group) > 0 || len(resource.Version) > 0 { + gv = schema.GroupVersion{ + Group: resource.Group, + Version: resource.Version, + } + } + gvk := gv.WithKind(resource.Kind) + gvr := gv.WithResource(strings.TrimSuffix(resource.Name, "/status")) + scope := meta.RESTScopeRoot + if resource.Namespaced { + scope = meta.RESTScopeNamespace + } + return &meta.RESTMapping{ + Resource: gvr, + GroupVersionKind: gvk, + Scope: scope, + }, nil +} + +// TestApplyStatus makes sure that applying the status works for all known types. +func TestApplyStatus(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() + server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), []string{"--disable-admission-plugins", "ServiceAccount,TaintNodesByCondition"}, framework.SharedEtcd()) + if err != nil { + t.Fatal(err) + } + defer server.TearDownFn() + + client, err := kubernetes.NewForConfig(server.ClientConfig) + if err != nil { + t.Fatal(err) + } + dynamicClient, err := dynamic.NewForConfig(server.ClientConfig) + if err != nil { + t.Fatal(err) + } + + // create CRDs so we can make sure that custom resources do not get lost + etcd.CreateTestCRDs(t, apiextensionsclientset.NewForConfigOrDie(server.ClientConfig), false, etcd.GetCustomResourceDefinitionData()...) + if _, err := client.CoreV1().Namespaces().Create(context.TODO(), &v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testNamespace}}, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + createData := etcd.GetEtcdStorageData() + + // gather resources to test + _, resourceLists, err := client.Discovery().ServerGroupsAndResources() + if err != nil { + t.Fatalf("Failed to get ServerGroupsAndResources with error: %+v", err) + } + + for _, resourceList := range resourceLists { + for _, resource := range resourceList.APIResources { + if !strings.HasSuffix(resource.Name, "/status") { + continue + } + mapping, err := createMapping(resourceList.GroupVersion, resource) + if err != nil { + t.Fatal(err) + } + t.Run(mapping.Resource.String(), func(t *testing.T) { + if _, ok := ignoreList[mapping.Resource]; ok { + t.Skip() + } + status, ok := statusData[mapping.Resource] + if !ok { + status = statusDefault + } + newResource, ok := createData[mapping.Resource] + if !ok { + t.Fatalf("no test data for %s. Please add a test for your new type to etcd.GetEtcdStorageData().", mapping.Resource) + } + newObj := unstructured.Unstructured{} + if err := json.Unmarshal([]byte(newResource.Stub), &newObj.Object); err != nil { + t.Fatal(err) + } + + namespace := testNamespace + if mapping.Scope == meta.RESTScopeRoot { + namespace = "" + } + name := newObj.GetName() + rsc := dynamicClient.Resource(mapping.Resource).Namespace(namespace) + _, err := rsc.Create(context.TODO(), &newObj, metav1.CreateOptions{FieldManager: "create_test"}) + if err != nil { + t.Fatal(err) + } + + statusObj := unstructured.Unstructured{} + if err := json.Unmarshal([]byte(status), &statusObj.Object); err != nil { + t.Fatal(err) + } + statusObj.SetAPIVersion(mapping.GroupVersionKind.GroupVersion().String()) + statusObj.SetKind(mapping.GroupVersionKind.Kind) + statusObj.SetName(name) + statusYAML, err := yaml.Marshal(statusObj.Object) + if err != nil { + t.Fatal(err) + } + + True := true + obj, err := dynamicClient. + Resource(mapping.Resource). + Namespace(namespace). + Patch(context.TODO(), name, types.ApplyPatchType, statusYAML, metav1.PatchOptions{FieldManager: "apply_status_test", Force: &True}, "status") + if err != nil { + t.Fatalf("Failed to apply: %v", err) + } + + accessor, err := meta.Accessor(obj) + if err != nil { + t.Fatalf("Failed to get meta accessor: %v:\n%v", err, obj) + } + + managedFields := accessor.GetManagedFields() + if managedFields == nil { + t.Fatal("Empty managed fields") + } + if !findManager(managedFields, "apply_status_test") { + t.Fatalf("Couldn't find apply_status_test: %v", managedFields) + } + if !findManager(managedFields, "create_test") { + t.Fatalf("Couldn't find create_test: %v", managedFields) + } + + if err := rsc.Delete(context.TODO(), name, *metav1.NewDeleteOptions(0)); err != nil { + t.Fatalf("deleting final object failed: %v", err) + } + }) + } + } +} + +func findManager(managedFields []metav1.ManagedFieldsEntry, manager string) bool { + for _, entry := range managedFields { + if entry.Manager == manager { + return true + } + } + return false +} From dfe1703ffab9ebd824f560f843a60147cc4de5c5 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Wed, 18 Mar 2020 14:17:32 -0700 Subject: [PATCH 2/2] Do not reset managedFields in status update strategy --- pkg/registry/flowcontrol/flowschema/strategy.go | 5 +++++ .../flowcontrol/prioritylevelconfiguration/strategy.go | 5 +++++ .../pkg/registry/customresource/status_strategy.go | 5 +++++ staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go | 4 +++- .../src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go | 1 + 5 files changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/registry/flowcontrol/flowschema/strategy.go b/pkg/registry/flowcontrol/flowschema/strategy.go index fee6b97a4d6..6286e1a1cc0 100644 --- a/pkg/registry/flowcontrol/flowschema/strategy.go +++ b/pkg/registry/flowcontrol/flowschema/strategy.go @@ -94,7 +94,12 @@ var StatusStrategy = flowSchemaStatusStrategy{Strategy} func (flowSchemaStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newFlowSchema := obj.(*flowcontrol.FlowSchema) oldFlowSchema := old.(*flowcontrol.FlowSchema) + + // managedFields must be preserved since it's been modified to + // track changed fields in the status update. + managedFields := newFlowSchema.ManagedFields newFlowSchema.ObjectMeta = oldFlowSchema.ObjectMeta + newFlowSchema.ManagedFields = managedFields newFlowSchema.Spec = oldFlowSchema.Spec } diff --git a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go index 9340407ed76..9cc7877a4d7 100644 --- a/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go +++ b/pkg/registry/flowcontrol/prioritylevelconfiguration/strategy.go @@ -94,7 +94,12 @@ var StatusStrategy = priorityLevelConfigurationStatusStrategy{Strategy} func (priorityLevelConfigurationStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newPriorityLevelConfiguration := obj.(*flowcontrol.PriorityLevelConfiguration) oldPriorityLevelConfiguration := old.(*flowcontrol.PriorityLevelConfiguration) + + // managedFields must be preserved since it's been modified to + // track changed fields in the status update. + managedFields := newPriorityLevelConfiguration.ManagedFields newPriorityLevelConfiguration.ObjectMeta = oldPriorityLevelConfiguration.ObjectMeta + newPriorityLevelConfiguration.ManagedFields = managedFields newPriorityLevelConfiguration.Spec = oldPriorityLevelConfiguration.Spec } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go index 1710eb2e15a..b31b627039f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go @@ -38,6 +38,10 @@ func (a statusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.O newCustomResource := newCustomResourceObject.UnstructuredContent() status, ok := newCustomResource["status"] + // managedFields must be preserved since it's been modified to + // track changed fields in the status update. + managedFields := newCustomResourceObject.GetManagedFields() + // copy old object into new object oldCustomResourceObject := old.(*unstructured.Unstructured) // overridding the resourceVersion in metadata is safe here, we have already checked that @@ -45,6 +49,7 @@ func (a statusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.O *newCustomResourceObject = *oldCustomResourceObject.DeepCopy() // set status + newCustomResourceObject.SetManagedFields(managedFields) newCustomResource = newCustomResourceObject.UnstructuredContent() if ok { newCustomResource["status"] = status diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go index ec016fd3c8d..ad989ad75ca 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go @@ -252,7 +252,9 @@ func ResetObjectMetaForStatus(meta, existingMeta Object) { meta.SetAnnotations(existingMeta.GetAnnotations()) meta.SetFinalizers(existingMeta.GetFinalizers()) meta.SetOwnerReferences(existingMeta.GetOwnerReferences()) - meta.SetManagedFields(existingMeta.GetManagedFields()) + // managedFields must be preserved since it's been modified to + // track changed fields in the status update. + //meta.SetManagedFields(existingMeta.GetManagedFields()) } // MarshalJSON implements json.Marshaler diff --git a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go index 54963c4ca21..62023c07967 100644 --- a/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go @@ -189,6 +189,7 @@ func TestResetObjectMetaForStatus(t *testing.T) { existingMeta.SetCreationTimestamp(Time{}) existingMeta.SetDeletionTimestamp(nil) existingMeta.SetDeletionGracePeriodSeconds(nil) + existingMeta.SetManagedFields(nil) if !reflect.DeepEqual(meta, existingMeta) { t.Error(diff.ObjectDiff(meta, existingMeta))