From a72f24d33c1631fe786795634a824ac3a94ff393 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Sat, 12 Feb 2022 07:35:44 +0000 Subject: [PATCH 1/3] start CRD integration benchmarks --- .../apiserver/field_validation_test.go | 539 +++++++++++++++++- 1 file changed, 537 insertions(+), 2 deletions(-) diff --git a/test/integration/apiserver/field_validation_test.go b/test/integration/apiserver/field_validation_test.go index 909a22b0605..d5ae480bfbb 100644 --- a/test/integration/apiserver/field_validation_test.go +++ b/test/integration/apiserver/field_validation_test.go @@ -261,7 +261,8 @@ spec: "apiVersion": "%s", "kind": "%s", "metadata": { - "name": "%s" + "name": "%s", + "resourceVersion": "%s" }, "spec": { "knownField1": "val1", @@ -295,6 +296,20 @@ spec: hostPort: 8082 unknownNested: val` + crdValidBodyYAML = ` +apiVersion: "%s" +kind: "%s" +metadata: + name: "%s" + resourceVersion: "%s" +spec: + knownField1: val1 + ports: + - name: portName + containerPort: 8080 + protocol: TCP + hostPort: 8081` + crdApplyInvalidBody = ` { "apiVersion": "%s", @@ -333,6 +348,24 @@ spec: } }` + crdApplyValidBody2 = ` +{ + "apiVersion": "%s", + "kind": "%s", + "metadata": { + "name": "%s" + }, + "spec": { + "knownField1": "val2", + "ports": [{ + "name": "portName", + "containerPort": 8080, + "protocol": "TCP", + "hostPort": 8083 + }] + } +}` + patchYAMLBody = ` apiVersion: %s kind: %s @@ -2914,7 +2947,7 @@ func testFieldValidationApplyUpdateCRDSchemaless(t *testing.T, rest rest.Interfa } } -func setupCRD(t *testing.T, config *rest.Config, apiGroup string, schemaless bool) *apiextensionsv1.CustomResourceDefinition { +func setupCRD(t testing.TB, config *rest.Config, apiGroup string, schemaless bool) *apiextensionsv1.CustomResourceDefinition { apiExtensionClient, err := apiextensionsclient.NewForConfig(config) if err != nil { t.Fatal(err) @@ -2970,6 +3003,32 @@ func BenchmarkFieldValidation(b *testing.B) { client := clientset.NewForConfigOrDie(config) + schemaCRD := setupCRD(b, config, "schema.example.com", false) + schemaGVR := schema.GroupVersionResource{ + Group: schemaCRD.Spec.Group, + Version: schemaCRD.Spec.Versions[0].Name, + Resource: schemaCRD.Spec.Names.Plural, + } + schemaGVK := schema.GroupVersionKind{ + Group: schemaCRD.Spec.Group, + Version: schemaCRD.Spec.Versions[0].Name, + Kind: schemaCRD.Spec.Names.Kind, + } + + schemalessCRD := setupCRD(b, config, "schemaless.example.com", true) + schemalessGVR := schema.GroupVersionResource{ + Group: schemalessCRD.Spec.Group, + Version: schemalessCRD.Spec.Versions[0].Name, + Resource: schemalessCRD.Spec.Names.Plural, + } + schemalessGVK := schema.GroupVersionKind{ + Group: schemalessCRD.Spec.Group, + Version: schemalessCRD.Spec.Versions[0].Name, + Kind: schemalessCRD.Spec.Names.Kind, + } + + rest := client.Discovery().RESTClient() + b.Run("Post", func(b *testing.B) { benchFieldValidationPost(b, client) }) b.Run("Put", func(b *testing.B) { benchFieldValidationPut(b, client) }) b.Run("PatchTyped", func(b *testing.B) { benchFieldValidationPatchTyped(b, client) }) @@ -2977,6 +3036,18 @@ func BenchmarkFieldValidation(b *testing.B) { b.Run("ApplyCreate", func(b *testing.B) { benchFieldValidationApplyCreate(b, client) }) b.Run("ApplyUpdate", func(b *testing.B) { benchFieldValidationApplyUpdate(b, client) }) + b.Run("PostCRD", func(b *testing.B) { benchFieldValidationPostCRD(b, rest, schemaGVK, schemaGVR) }) + b.Run("PutCRD", func(b *testing.B) { benchFieldValidationPutCRD(b, rest, schemaGVK, schemaGVR) }) + b.Run("PatchCRD", func(b *testing.B) { benchFieldValidationPatchCRD(b, rest, schemaGVK, schemaGVR) }) + b.Run("ApplyCreateCRD", func(b *testing.B) { benchFieldValidationApplyCreateCRD(b, rest, schemaGVK, schemaGVR) }) + b.Run("ApplyUpdateCRD", func(b *testing.B) { benchFieldValidationApplyUpdateCRD(b, rest, schemaGVK, schemaGVR) }) + + b.Run("PostCRDSchemaless", func(b *testing.B) { benchFieldValidationPostCRD(b, rest, schemalessGVK, schemalessGVR) }) + b.Run("PutCRDSchemaless", func(b *testing.B) { benchFieldValidationPutCRD(b, rest, schemalessGVK, schemalessGVR) }) + b.Run("PatchCRDSchemaless", func(b *testing.B) { benchFieldValidationPatchCRD(b, rest, schemalessGVK, schemalessGVR) }) + b.Run("ApplyCreateCRDSchemaless", func(b *testing.B) { benchFieldValidationApplyCreateCRD(b, rest, schemalessGVK, schemalessGVR) }) + b.Run("ApplyUpdateCRDSchemaless", func(b *testing.B) { benchFieldValidationApplyUpdateCRD(b, rest, schemalessGVK, schemalessGVR) }) + } func benchFieldValidationPost(b *testing.B, client clientset.Interface) { @@ -3477,3 +3548,467 @@ func benchFieldValidationApplyUpdate(b *testing.B, client clientset.Interface) { }) } } + +func benchFieldValidationPostCRD(b *testing.B, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + var testcases = []struct { + name string + opts metav1.PatchOptions + body string + contentType string + }{ + { + name: "crd-post-strict-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + body: crdValidBody, + }, + { + name: "crd-post-warn-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + body: crdValidBody, + }, + { + name: "crd-post-ignore-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + body: crdValidBody, + }, + { + name: "crd-post-no-validation", + body: crdValidBody, + }, + { + name: "crd-post-strict-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + body: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-post-warn-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + body: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-post-ignore-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + body: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-post-no-validation-yaml", + body: crdValidBodyYAML, + contentType: "application/yaml", + }, + } + for _, tc := range testcases { + b.Run(tc.name, func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + for n := 0; n < b.N; n++ { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + + // create the CR as specified by the test case + jsonBody := []byte(fmt.Sprintf(tc.body, apiVersion, kind, fmt.Sprintf("test-dep-%s-%d-%d-%d", tc.name, n, b.N, time.Now().UnixNano()))) + req := rest.Post(). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + SetHeader("Content-Type", tc.contentType). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := req.Body([]byte(jsonBody)).Do(context.TODO()) + + if result.Error() != nil { + b.Fatalf("unexpected post err: %v", result.Error()) + } + } + }) + } +} + +func benchFieldValidationPutCRD(b *testing.B, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + var testcases = []struct { + name string + opts metav1.PatchOptions + putBody string + contentType string + }{ + { + name: "crd-put-strict-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + putBody: crdValidBody, + }, + { + name: "crd-put-warn-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + putBody: crdValidBody, + }, + { + name: "crd-put-ignore-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + putBody: crdValidBody, + }, + { + name: "crd-put-no-validation", + putBody: crdValidBody, + }, + { + name: "crd-put-strict-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + putBody: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-put-warn-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + putBody: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-put-ignore-validation-yaml", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + putBody: crdValidBodyYAML, + contentType: "application/yaml", + }, + { + name: "crd-put-no-validation-yaml", + putBody: crdValidBodyYAML, + contentType: "application/yaml", + }, + } + for _, tc := range testcases { + b.Run(tc.name, func(b *testing.B) { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + names := make([]string, b.N) + resourceVersions := make([]string, b.N) + for n := 0; n < b.N; n++ { + deployName := fmt.Sprintf("test-dep-%s-%d-%d-%d", tc.name, n, b.N, time.Now().UnixNano()) + names[n] = deployName + + // create the CR as specified by the test case + jsonPostBody := []byte(fmt.Sprintf(crdValidBody, apiVersion, kind, deployName)) + postReq := rest.Post(). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + VersionedParams(&tc.opts, metav1.ParameterCodec) + postResult, err := postReq.Body([]byte(jsonPostBody)).Do(context.TODO()).Raw() + if err != nil { + b.Fatalf("unexpeted error on CR creation: %v", err) + } + postUnstructured := &unstructured.Unstructured{} + if err := postUnstructured.UnmarshalJSON(postResult); err != nil { + b.Fatalf("unexpeted error unmarshalling created CR: %v", err) + } + resourceVersions[n] = postUnstructured.GetResourceVersion() + } + b.ResetTimer() + b.ReportAllocs() + for n := 0; n < b.N; n++ { + // update the CR as specified by the test case + putBody := []byte(fmt.Sprintf(tc.putBody, apiVersion, kind, names[n], resourceVersions[n])) + putReq := rest.Put(). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(names[n]). + SetHeader("Content-Type", tc.contentType). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := putReq.Body([]byte(putBody)).Do(context.TODO()) + if result.Error() != nil { + b.Fatalf("unexpected put err: %v", result.Error()) + } + } + }) + } +} + +func benchFieldValidationPatchCRD(b *testing.B, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + patchYAMLBody := ` +apiVersion: %s +kind: %s +metadata: + name: %s + finalizers: + - test-finalizer +spec: + cronSpec: "* * * * */5" + ports: + - name: x + containerPort: 80 + protocol: TCP` + + mergePatchBody := ` +{ + "spec": { + "knownField1": "val1", + "ports": [{ + "name": "portName", + "containerPort": 8080, + "protocol": "TCP", + "hostPort": 8081 + }] + } +} + ` + jsonPatchBody := ` + [ + {"op": "add", "path": "/spec/knownField1", "value": "val1"}, + {"op": "add", "path": "/spec/ports/0/name", "value": "portName"}, + {"op": "add", "path": "/spec/ports/0/containerPort", "value": 8080}, + {"op": "add", "path": "/spec/ports/0/protocol", "value": "TCP"}, + {"op": "add", "path": "/spec/ports/0/hostPort", "value": 8081} + ] + ` + var testcases = []struct { + name string + patchType types.PatchType + opts metav1.PatchOptions + body string + }{ + { + name: "crd-merge-patch-strict-validation", + patchType: types.MergePatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + body: mergePatchBody, + }, + { + name: "crd-merge-patch-warn-validation", + patchType: types.MergePatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + body: mergePatchBody, + }, + { + name: "crd-merge-patch-ignore-validation", + patchType: types.MergePatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + body: mergePatchBody, + }, + { + name: "crd-merge-patch-no-validation", + patchType: types.MergePatchType, + body: mergePatchBody, + }, + { + name: "crd-json-patch-strict-validation", + patchType: types.JSONPatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + }, + body: jsonPatchBody, + }, + { + name: "crd-json-patch-warn-validation", + patchType: types.JSONPatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + }, + body: jsonPatchBody, + }, + { + name: "crd-json-patch-ignore-validation", + patchType: types.JSONPatchType, + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + }, + body: jsonPatchBody, + }, + { + name: "crd-json-patch-no-validation", + patchType: types.JSONPatchType, + body: jsonPatchBody, + }, + } + for _, tc := range testcases { + b.Run(tc.name, func(b *testing.B) { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + names := make([]string, b.N) + for n := 0; n < b.N; n++ { + deployName := fmt.Sprintf("test-dep-%s-%d-%d-%d", tc.name, n, b.N, time.Now().UnixNano()) + names[n] = deployName + + // create a CR + yamlBody := []byte(fmt.Sprintf(string(patchYAMLBody), apiVersion, kind, deployName)) + createResult, err := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(deployName). + Param("fieldManager", "apply_test"). + Body(yamlBody). + DoRaw(context.TODO()) + if err != nil { + b.Fatalf("failed to create custom resource with apply: %v:\n%v", err, string(createResult)) + } + } + b.ResetTimer() + b.ReportAllocs() + for n := 0; n < b.N; n++ { + // patch the CR as specified by the test case + req := rest.Patch(tc.patchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(names[n]). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := req.Body([]byte(tc.body)).Do(context.TODO()) + if result.Error() != nil { + b.Fatalf("unexpected patch err: %v", result.Error()) + } + } + + }) + } +} + +func benchFieldValidationApplyCreateCRD(b *testing.B, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + var testcases = []struct { + name string + opts metav1.PatchOptions + }{ + { + name: "strict-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + FieldManager: "mgr", + }, + }, + { + name: "warn-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + FieldManager: "mgr", + }, + }, + { + name: "ignore-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + FieldManager: "mgr", + }, + }, + { + name: "no-validation", + opts: metav1.PatchOptions{ + FieldManager: "mgr", + }, + }, + } + + for _, tc := range testcases { + b.Run(tc.name, func(b *testing.B) { + b.ResetTimer() + b.ReportAllocs() + for n := 0; n < b.N; n++ { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + name := fmt.Sprintf("test-dep-%s-%d-%d-%d", tc.name, n, b.N, time.Now().UnixNano()) + + // create the CR as specified by the test case + applyCreateBody := []byte(fmt.Sprintf(crdApplyValidBody, apiVersion, kind, name)) + + req := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(name). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := req.Body(applyCreateBody).Do(context.TODO()) + if result.Error() != nil { + b.Fatalf("unexpected apply err: %v", result.Error()) + } + + } + }) + } +} + +func benchFieldValidationApplyUpdateCRD(b *testing.B, rest rest.Interface, gvk schema.GroupVersionKind, gvr schema.GroupVersionResource) { + var testcases = []struct { + name string + opts metav1.PatchOptions + }{ + { + name: "strict-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Strict", + FieldManager: "mgr", + }, + }, + { + name: "warn-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Warn", + FieldManager: "mgr", + }, + }, + { + name: "ignore-validation", + opts: metav1.PatchOptions{ + FieldValidation: "Ignore", + FieldManager: "mgr", + }, + }, + { + name: "no-validation", + opts: metav1.PatchOptions{ + FieldManager: "mgr", + }, + }, + } + + for _, tc := range testcases { + b.Run(tc.name, func(b *testing.B) { + kind := gvk.Kind + apiVersion := gvk.Group + "/" + gvk.Version + names := make([]string, b.N) + + for n := 0; n < b.N; n++ { + names[n] = fmt.Sprintf("apply-update-crd-%s-%d-%d-%d", tc.name, n, b.N, time.Now().UnixNano()) + applyCreateBody := []byte(fmt.Sprintf(crdApplyValidBody, apiVersion, kind, names[n])) + createReq := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(names[n]). + VersionedParams(&tc.opts, metav1.ParameterCodec) + createResult := createReq.Body(applyCreateBody).Do(context.TODO()) + if createResult.Error() != nil { + b.Fatalf("unexpected apply create err: %v", createResult.Error()) + } + } + b.ResetTimer() + b.ReportAllocs() + for n := 0; n < b.N; n++ { + applyUpdateBody := []byte(fmt.Sprintf(crdApplyValidBody2, apiVersion, kind, names[n])) + updateReq := rest.Patch(types.ApplyPatchType). + AbsPath("/apis", gvr.Group, gvr.Version, gvr.Resource). + Name(names[n]). + VersionedParams(&tc.opts, metav1.ParameterCodec) + result := updateReq.Body(applyUpdateBody).Do(context.TODO()) + + if result.Error() != nil { + b.Fatalf("unexpected apply err: %v", result.Error()) + } + } + + }) + } +} From 68ac44c2b1831a34e0fd66e1bd826c0aaad60b4f Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Wed, 16 Feb 2022 18:31:05 +0000 Subject: [PATCH 2/3] WIP: start debuging crd handler testing --- .../apiserver/customresource_handler_test.go | 221 ++++++++++++++++++ 1 file changed, 221 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go index b981c3a66a9..6bab1d221d3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go @@ -17,8 +17,11 @@ limitations under the License. package apiserver import ( + "bytes" "context" "encoding/json" + "errors" + "fmt" "io" "io/ioutil" "net" @@ -33,8 +36,13 @@ import ( "sigs.k8s.io/yaml" + apiextensionshelpers "k8s.io/apiextensions-apiserver/pkg/apihelpers" + apiextensionsinternal "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/conversion" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + structuraldefaulting "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" informers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions" listers "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1" @@ -44,8 +52,10 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + serializerjson "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/runtime/serializer/protobuf" "k8s.io/apimachinery/pkg/types" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/discovery" @@ -638,6 +648,217 @@ func testHandlerConversion(t *testing.T, enableWatchCache bool) { } } +func TestDecoder(t *testing.T) { + multiVersion := ` + { + "apiVersion": "stable.example.com/v1beta1", + "kind": "MultiVersion", + "metadata": { + "name": "my-mv" + }, + "num": 1, + "num": 2, + "unknown": "foo" + } + ` + multiVersionYAML := ` +apiVersion: stable.example.com/v1beta1 +kind: MultiVersion +metadata: + name: my-mv +num: 1 +num: 2 +unknown: foo` + + decoded := &unstructured.Unstructured{Object: map[string]interface{}{}} + decoded.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "stable.example.com", + Version: "v1beta1", + Kind: "MultiVersion", + }) + decoded.SetName("my-mv") + decoded.SetGeneration(1) + unstructured.SetNestedField(decoded.Object, int64(2), "num") + unstructured.SetNestedField(decoded.Object, nil, "metadata", "creationTimestamp") + + decodedPreserveUnknown := decoded.DeepCopy() + unstructured.SetNestedField(decodedPreserveUnknown.Object, "foo", "unknown") + + testcases := []struct { + name string + body string + yaml bool + strictDecoding bool + preserveUnknownFields bool + crd *v1.CustomResourceDefinition + expectedObj *unstructured.Unstructured + expectedErr error + }{ + { + name: "strict-decoding", + body: multiVersion, + strictDecoding: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decoded, + expectedErr: errors.New(`strict decoding error: duplicate field "num", unknown field "unknown"`), + }, + { + name: "non-strict-decoding", + body: multiVersion, + strictDecoding: false, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decoded, + expectedErr: nil, + }, + { + name: "strict-decoding-preserve-unknown", + body: multiVersion, + strictDecoding: true, + preserveUnknownFields: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decodedPreserveUnknown, + expectedErr: errors.New(`strict decoding error: duplicate field "num"`), + }, + { + name: "non-strict-decoding-preserve-unknown", + body: multiVersion, + strictDecoding: false, + preserveUnknownFields: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decodedPreserveUnknown, + expectedErr: nil, + }, + { + name: "strict-decoding-yaml", + body: multiVersionYAML, + yaml: true, + strictDecoding: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decoded, + expectedErr: errors.New(`strict decoding error: yaml: unmarshal errors: + line 7: key "num" already set in map, unknown field "unknown"`), + }, + { + name: "non-strict-decoding-yaml", + body: multiVersionYAML, + yaml: true, + strictDecoding: false, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decoded, + expectedErr: nil, + }, + { + name: "strict-decoding-preserve-unknown-yaml", + body: multiVersionYAML, + yaml: true, + strictDecoding: true, + preserveUnknownFields: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decodedPreserveUnknown, + expectedErr: errors.New(`strict decoding error: yaml: unmarshal errors: + line 7: key "num" already set in map`), + }, + { + name: "non-strict-decoding-preserve-unknown-yaml", + body: multiVersionYAML, + yaml: true, + strictDecoding: false, + preserveUnknownFields: true, + crd: multiVersionFixture.DeepCopy(), + expectedObj: decodedPreserveUnknown, + expectedErr: nil, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + v := "v1beta1" + + var err error + structuralSchemas := map[string]*structuralschema.Structural{} + structuralSchemas[v], err = generateStructuralSchema(tc.crd, v) + if err != nil { + t.Fatal(err) + } + delegate := serializerjson.NewSerializerWithOptions(serializerjson.DefaultMetaFactory, unstructuredCreator{}, nil, serializerjson.SerializerOptions{tc.yaml, false, tc.strictDecoding}) + decoder := &schemaCoercingDecoder{ + delegate: delegate, + validator: unstructuredSchemaCoercer{ + dropInvalidMetadata: true, + repairGeneration: true, + structuralSchemas: structuralSchemas, + structuralSchemaGK: schema.GroupKind{ + Group: "stable.example.com", + Kind: "MultiVersion", + }, + returnUnknownFieldPaths: tc.strictDecoding, + preserveUnknownFields: tc.preserveUnknownFields, + }, + } + + obj, _, err := decoder.Decode([]byte(tc.body), nil, nil) + if obj != nil { + unstructured, ok := obj.(*unstructured.Unstructured) + if !ok { + t.Fatalf("obj is not an unstructured: %v", obj) + } + objBytes, err := unstructured.MarshalJSON() + if err != nil { + t.Fatalf("err marshaling json: %v", err) + } + expectedBytes, err := tc.expectedObj.MarshalJSON() + if err != nil { + t.Fatalf("err marshaling json: %v", err) + } + if bytes.Compare(objBytes, expectedBytes) != 0 { + t.Fatalf("expected obj: \n%v\n got obj: \n%v\n", tc.expectedObj, obj) + } + } + if err == nil || tc.expectedErr == nil { + if err != nil || tc.expectedErr != nil { + t.Fatalf("expected err: %v, got err: %v", tc.expectedErr, err) + } + } else if err.Error() != tc.expectedErr.Error() { + t.Fatalf("expected err: \n%v\n got err: \n%v\n", tc.expectedErr, err) + } + }) + } + +} + +func generateStructuralSchema(crd *v1.CustomResourceDefinition, version string) (*structuralschema.Structural, error) { + val, err := apiextensionshelpers.GetSchemaForVersion(crd, version) + if err != nil { + utilruntime.HandleError(err) + return nil, fmt.Errorf("the server could not properly serve the CR schema") + } + if val == nil { + return nil, nil + } + internalValidation := &apiextensionsinternal.CustomResourceValidation{} + if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(val, internalValidation, nil); err != nil { + return nil, fmt.Errorf("failed converting CRD validation to internal version: %v", err) + } + s, err := structuralschema.NewStructural(internalValidation.OpenAPIV3Schema) + if crd.Spec.PreserveUnknownFields == false && err != nil { + // This should never happen. If it does, it is a programming error. + utilruntime.HandleError(fmt.Errorf("failed to convert schema to structural: %v", err)) + return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this + } + + if crd.Spec.PreserveUnknownFields == false { + // we don't own s completely, e.g. defaults are not deep-copied. So better make a copy here. + s = s.DeepCopy() + + if err := structuraldefaulting.PruneDefaults(s); err != nil { + // This should never happen. If it does, it is a programming error. + utilruntime.HandleError(fmt.Errorf("failed to prune defaults: %v", err)) + return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this + } + } + return s, nil + +} + type dummyAdmissionImpl struct{} func (dummyAdmissionImpl) Handles(operation admission.Operation) bool { return false } From bbf1208480919a38a84bef7a33778933858d83c7 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Fri, 25 Feb 2022 23:07:41 +0000 Subject: [PATCH 3/3] PR feedback --- .../apiserver/customresource_handler_test.go | 129 +++++++----------- 1 file changed, 52 insertions(+), 77 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go index 6bab1d221d3..a97f9baed31 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go @@ -21,7 +21,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "io" "io/ioutil" "net" @@ -36,13 +35,10 @@ import ( "sigs.k8s.io/yaml" - apiextensionshelpers "k8s.io/apiextensions-apiserver/pkg/apihelpers" - apiextensionsinternal "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/conversion" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" - structuraldefaulting "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake" informers "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions" listers "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1" @@ -55,7 +51,6 @@ import ( serializerjson "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/runtime/serializer/protobuf" "k8s.io/apimachinery/pkg/types" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/discovery" @@ -649,7 +644,7 @@ func testHandlerConversion(t *testing.T, enableWatchCache bool) { } func TestDecoder(t *testing.T) { - multiVersion := ` + multiVersionJSON := ` { "apiVersion": "stable.example.com/v1beta1", "kind": "MultiVersion", @@ -670,19 +665,40 @@ num: 1 num: 2 unknown: foo` - decoded := &unstructured.Unstructured{Object: map[string]interface{}{}} - decoded.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "stable.example.com", - Version: "v1beta1", - Kind: "MultiVersion", - }) - decoded.SetName("my-mv") - decoded.SetGeneration(1) - unstructured.SetNestedField(decoded.Object, int64(2), "num") - unstructured.SetNestedField(decoded.Object, nil, "metadata", "creationTimestamp") + expectedObjUnknownNotPreserved := &unstructured.Unstructured{} + err := expectedObjUnknownNotPreserved.UnmarshalJSON([]byte(` + { + "apiVersion": "stable.example.com/v1beta1", + "kind": "MultiVersion", + "metadata": { + "creationTimestamp": null, + "generation": 1, + "name": "my-mv" + }, + "num": 2 + } + `)) + if err != nil { + t.Fatal(err) + } - decodedPreserveUnknown := decoded.DeepCopy() - unstructured.SetNestedField(decodedPreserveUnknown.Object, "foo", "unknown") + expectedObjUnknownPreserved := &unstructured.Unstructured{} + err = expectedObjUnknownPreserved.UnmarshalJSON([]byte(` + { + "apiVersion": "stable.example.com/v1beta1", + "kind": "MultiVersion", + "metadata": { + "creationTimestamp": null, + "generation": 1, + "name": "my-mv" + }, + "num": 2, + "unknown": "foo" + } + `)) + if err != nil { + t.Fatal(err) + } testcases := []struct { name string @@ -690,42 +706,37 @@ unknown: foo` yaml bool strictDecoding bool preserveUnknownFields bool - crd *v1.CustomResourceDefinition expectedObj *unstructured.Unstructured expectedErr error }{ { name: "strict-decoding", - body: multiVersion, + body: multiVersionJSON, strictDecoding: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decoded, + expectedObj: expectedObjUnknownNotPreserved, expectedErr: errors.New(`strict decoding error: duplicate field "num", unknown field "unknown"`), }, { name: "non-strict-decoding", - body: multiVersion, + body: multiVersionJSON, strictDecoding: false, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decoded, + expectedObj: expectedObjUnknownNotPreserved, expectedErr: nil, }, { name: "strict-decoding-preserve-unknown", - body: multiVersion, + body: multiVersionJSON, strictDecoding: true, preserveUnknownFields: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decodedPreserveUnknown, + expectedObj: expectedObjUnknownPreserved, expectedErr: errors.New(`strict decoding error: duplicate field "num"`), }, { name: "non-strict-decoding-preserve-unknown", - body: multiVersion, + body: multiVersionJSON, strictDecoding: false, preserveUnknownFields: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decodedPreserveUnknown, + expectedObj: expectedObjUnknownPreserved, expectedErr: nil, }, { @@ -733,8 +744,7 @@ unknown: foo` body: multiVersionYAML, yaml: true, strictDecoding: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decoded, + expectedObj: expectedObjUnknownNotPreserved, expectedErr: errors.New(`strict decoding error: yaml: unmarshal errors: line 7: key "num" already set in map, unknown field "unknown"`), }, @@ -743,8 +753,7 @@ unknown: foo` body: multiVersionYAML, yaml: true, strictDecoding: false, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decoded, + expectedObj: expectedObjUnknownNotPreserved, expectedErr: nil, }, { @@ -753,8 +762,7 @@ unknown: foo` yaml: true, strictDecoding: true, preserveUnknownFields: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decodedPreserveUnknown, + expectedObj: expectedObjUnknownPreserved, expectedErr: errors.New(`strict decoding error: yaml: unmarshal errors: line 7: key "num" already set in map`), }, @@ -764,21 +772,22 @@ unknown: foo` yaml: true, strictDecoding: false, preserveUnknownFields: true, - crd: multiVersionFixture.DeepCopy(), - expectedObj: decodedPreserveUnknown, + expectedObj: expectedObjUnknownPreserved, expectedErr: nil, }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { v := "v1beta1" - - var err error structuralSchemas := map[string]*structuralschema.Structural{} - structuralSchemas[v], err = generateStructuralSchema(tc.crd, v) + structuralSchema, err := structuralschema.NewStructural(&apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{"num": {Type: "integer", Description: "v1beta1 num field"}}, + }) if err != nil { t.Fatal(err) } + structuralSchemas[v] = structuralSchema delegate := serializerjson.NewSerializerWithOptions(serializerjson.DefaultMetaFactory, unstructuredCreator{}, nil, serializerjson.SerializerOptions{tc.yaml, false, tc.strictDecoding}) decoder := &schemaCoercingDecoder{ delegate: delegate, @@ -825,40 +834,6 @@ unknown: foo` } -func generateStructuralSchema(crd *v1.CustomResourceDefinition, version string) (*structuralschema.Structural, error) { - val, err := apiextensionshelpers.GetSchemaForVersion(crd, version) - if err != nil { - utilruntime.HandleError(err) - return nil, fmt.Errorf("the server could not properly serve the CR schema") - } - if val == nil { - return nil, nil - } - internalValidation := &apiextensionsinternal.CustomResourceValidation{} - if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(val, internalValidation, nil); err != nil { - return nil, fmt.Errorf("failed converting CRD validation to internal version: %v", err) - } - s, err := structuralschema.NewStructural(internalValidation.OpenAPIV3Schema) - if crd.Spec.PreserveUnknownFields == false && err != nil { - // This should never happen. If it does, it is a programming error. - utilruntime.HandleError(fmt.Errorf("failed to convert schema to structural: %v", err)) - return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this - } - - if crd.Spec.PreserveUnknownFields == false { - // we don't own s completely, e.g. defaults are not deep-copied. So better make a copy here. - s = s.DeepCopy() - - if err := structuraldefaulting.PruneDefaults(s); err != nil { - // This should never happen. If it does, it is a programming error. - utilruntime.HandleError(fmt.Errorf("failed to prune defaults: %v", err)) - return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this - } - } - return s, nil - -} - type dummyAdmissionImpl struct{} func (dummyAdmissionImpl) Handles(operation admission.Operation) bool { return false }