From 32a64cf14cebbc706efdb301fe3b290335927a80 Mon Sep 17 00:00:00 2001 From: Jefftree Date: Wed, 9 Dec 2020 14:33:50 -0800 Subject: [PATCH] tests for buildOpenAPIModelsForApply --- .../pkg/apiserver/BUILD | 4 + .../pkg/apiserver/customresource_handler.go | 3 +- .../apiserver/customresource_handler_test.go | 110 ++++++++---- .../pkg/controller/openapi/builder/builder.go | 5 +- .../apiserver/apply/apply_crd_test.go | 156 ------------------ 5 files changed, 86 insertions(+), 192 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD index 47bc09f8316..40a82914936 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD @@ -100,6 +100,9 @@ go_library( go_test( name = "go_default_test", srcs = ["customresource_handler_test.go"], + data = [ + "//api/openapi-spec", + ], embed = [":go_default_library"], deps = [ "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1:go_default_library", @@ -126,6 +129,7 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/storage/etcd3/testing:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/webhook:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", + "//vendor/github.com/go-openapi/spec:go_default_library", "//vendor/sigs.k8s.io/yaml:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index 0b897f8fa42..00ddbe71461 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -1320,7 +1320,8 @@ func buildOpenAPIModelsForApply(staticOpenAPISpec *goopenapispec.Swagger, crd *a specs := []*goopenapispec.Swagger{} for _, v := range crd.Spec.Versions { - s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: true, StripValueValidation: true, StripNullable: true, AllowNonStructural: false}) + // Defaults are not pruned here, but before being served. + s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: false, StripValueValidation: true, StripNullable: true, AllowNonStructural: false}) if err != nil { return nil, err } 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 0e35ff5b227..1f4a94ee476 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 @@ -25,12 +25,14 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" "strconv" + "strings" "testing" "time" "github.com/go-openapi/spec" - "sigs.k8s.io/yaml" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -761,52 +763,94 @@ func Test_defaultDeprecationWarning(t *testing.T) { func TestBuildOpenAPIModelsForApply(t *testing.T) { // This is a list of validation that we expect to work. tests := []apiextensionsv1.CustomResourceValidation{ - apiextensionsv1.CustomResourceValidation{ + { OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ Type: "object", Properties: map[string]apiextensionsv1.JSONSchemaProps{"num": {Type: "integer", Description: "v1beta1 num field"}}, }, }, - apiextensionsv1.CustomResourceValidation{ + { OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ Type: "", XIntOrString: true, }, }, + { + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "oneOf": { + OneOf: []apiextensionsv1.JSONSchemaProps{ + {Type: "boolean"}, + {Type: "string"}, + }, + }, + }, + }, + }, + { + OpenAPIV3Schema: &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "nullable": { + Type: "integer", + Nullable: true, + }, + }, + }, + }, + } + + staticSpec, err := getOpenAPISpecFromFile() + if err != nil { + t.Fatalf("Failed to load openapi spec: %v", err) + } + + crd := apiextensionsv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "example.stable.example.com", UID: types.UID("12345")}, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "stable.example.com", + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Plural: "examples", Singular: "example", Kind: "Example", ShortNames: []string{"ex"}, ListKind: "ExampleList", Categories: []string{"all"}, + }, + Conversion: &apiextensionsv1.CustomResourceConversion{Strategy: apiextensionsv1.NoneConverter}, + Scope: apiextensionsv1.ClusterScoped, + PreserveUnknownFields: false, + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1beta1", Served: true, Storage: true, + Subresources: &apiextensionsv1.CustomResourceSubresources{Status: &apiextensionsv1.CustomResourceSubresourceStatus{}}, + }, + }, + }, } for _, test := range tests { - crd := apiextensionsv1.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{Name: "example.stable.example.com", UID: types.UID("12345")}, - Spec: apiextensionsv1.CustomResourceDefinitionSpec{ - Group: "stable.example.com", - Names: apiextensionsv1.CustomResourceDefinitionNames{ - Plural: "examples", Singular: "example", Kind: "Example", ShortNames: []string{"ex"}, ListKind: "ExampleList", Categories: []string{"all"}, - }, - Conversion: &apiextensionsv1.CustomResourceConversion{Strategy: apiextensionsv1.NoneConverter}, - Scope: apiextensionsv1.ClusterScoped, - PreserveUnknownFields: false, - Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ - { - Name: "v1beta1", Served: true, Storage: true, - Subresources: &apiextensionsv1.CustomResourceSubresources{Status: &apiextensionsv1.CustomResourceSubresourceStatus{}}, - Schema: &test, - }, - }, - }, - } - if _, err := buildOpenAPIModelsForApply(&spec.Swagger{ - SwaggerProps: spec.SwaggerProps{ - Info: &spec.Info{ - InfoProps: spec.InfoProps{ - Title: "title", - Version: "v1", - }, - }, - Swagger: "2.0", - }, - }, &crd); err != nil { + crd.Spec.Versions[0].Schema = &test + if _, err := buildOpenAPIModelsForApply(staticSpec, &crd); err != nil { t.Fatalf("failed to convert to apply model: %v", err) } } } + +func getOpenAPISpecFromFile() (*spec.Swagger, error) { + path := filepath.Join( + strings.Repeat(".."+string(filepath.Separator), 6), + "api", "openapi-spec", "swagger.json") + _, err := os.Stat(path) + if err != nil { + return nil, err + } + byteSpec, err := ioutil.ReadFile(path) + if err != nil { + return nil, err + } + staticSpec := &spec.Swagger{} + + err = yaml.Unmarshal(byteSpec, staticSpec) + if err != nil { + return nil, err + } + + return staticSpec, nil +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go index 62b0949735e..234d785a5b3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go @@ -103,14 +103,15 @@ func BuildSwagger(crd *apiextensionsv1.CustomResourceDefinition, version string, if opts.AllowNonStructural || len(structuralschema.ValidateStructural(nil, ss)) == 0 { schema = ss + // This adds ValueValidation fields (anyOf, allOf) which may be stripped below if opts.StripValueValidation is true + schema = schema.Unfold() + if opts.StripValueValidation { schema = schema.StripValueValidations() } if opts.StripNullable { schema = schema.StripNullable() } - - schema = schema.Unfold() } } } diff --git a/test/integration/apiserver/apply/apply_crd_test.go b/test/integration/apiserver/apply/apply_crd_test.go index a6e910050e3..342a638cd8a 100644 --- a/test/integration/apiserver/apply/apply_crd_test.go +++ b/test/integration/apiserver/apply/apply_crd_test.go @@ -469,162 +469,6 @@ spec: } } -// TestApplyCRDNonStructuralSchema tests that when a CRD has a non-structural schema in its validation field, -// it will be used to construct the CR schema used by apply, but any non-structural parts of the schema will be treated as -// nested maps (same as a CRD without a schema) -func TestApplyCRDNonStructuralSchema(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.ServerSideApply, true)() - - server, err := apiservertesting.StartTestServer(t, apiservertesting.NewDefaultTestServerOptions(), nil, framework.SharedEtcd()) - if err != nil { - t.Fatal(err) - } - defer server.TearDownFn() - config := server.ClientConfig - - apiExtensionClient, err := clientset.NewForConfig(config) - if err != nil { - t.Fatal(err) - } - dynamicClient, err := dynamic.NewForConfig(config) - if err != nil { - t.Fatal(err) - } - - noxuDefinition := fixtures.NewNoxuCustomResourceDefinition(apiextensionsv1beta1.ClusterScoped) - - var c apiextensionsv1beta1.CustomResourceValidation - err = json.Unmarshal([]byte(`{ - "openAPIV3Schema": { - "type": "object", - "properties": { - "spec": { - "anyOf": [ - { - "type": "object", - "properties": { - "cronSpec": { - "type": "string", - "pattern": "^(\\d+|\\*)(/\\d+)?(\\s+(\\d+|\\*)(/\\d+)?){4}$" - } - } - }, { - "type": "string" - } - ] - } - } - } - }`), &c) - if err != nil { - t.Fatal(err) - } - noxuDefinition.Spec.Validation = &c - - noxuDefinition, err = fixtures.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient) - if err != nil { - t.Fatal(err) - } - - kind := noxuDefinition.Spec.Names.Kind - apiVersion := noxuDefinition.Spec.Group + "/" + noxuDefinition.Spec.Version - name := "mytest" - - rest := apiExtensionClient.Discovery().RESTClient() - yamlBody := []byte(fmt.Sprintf(` -apiVersion: %s -kind: %s -metadata: - name: %s - finalizers: - - test-finalizer -spec: - cronSpec: "* * * * */5" - replicas: 1`, apiVersion, kind, name)) - result, err := rest.Patch(types.ApplyPatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Param("fieldManager", "apply_test"). - Body(yamlBody). - DoRaw(context.TODO()) - if err != nil { - t.Fatalf("failed to create custom resource with apply: %v:\n%v", err, string(result)) - } - verifyNumFinalizers(t, result, 1) - verifyFinalizersIncludes(t, result, "test-finalizer") - verifyReplicas(t, result, 1.0) - - // Patch object to add another finalizer to the finalizers list - result, err = rest.Patch(types.MergePatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Body([]byte(`{"metadata":{"finalizers":["test-finalizer","another-one"]}}`)). - DoRaw(context.TODO()) - if err != nil { - t.Fatalf("failed to add finalizer with merge patch: %v:\n%v", err, string(result)) - } - verifyNumFinalizers(t, result, 2) - verifyFinalizersIncludes(t, result, "test-finalizer") - verifyFinalizersIncludes(t, result, "another-one") - - // Re-apply the same config, should work fine, since finalizers should have the list-type extension 'set'. - result, err = rest.Patch(types.ApplyPatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Param("fieldManager", "apply_test"). - SetHeader("Accept", "application/json"). - Body(yamlBody). - DoRaw(context.TODO()) - if err != nil { - t.Fatalf("failed to apply same config after adding a finalizer: %v:\n%v", err, string(result)) - } - verifyNumFinalizers(t, result, 2) - verifyFinalizersIncludes(t, result, "test-finalizer") - verifyFinalizersIncludes(t, result, "another-one") - - // Patch object to change the number of replicas - result, err = rest.Patch(types.MergePatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Body([]byte(`{"spec":{"replicas": 5}}`)). - DoRaw(context.TODO()) - if err != nil { - t.Fatalf("failed to update number of replicas with merge patch: %v:\n%v", err, string(result)) - } - verifyReplicas(t, result, 5.0) - - // Re-apply, we should get conflicts now, since the number of replicas was changed. - result, err = rest.Patch(types.ApplyPatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Param("fieldManager", "apply_test"). - Body(yamlBody). - DoRaw(context.TODO()) - if err == nil { - t.Fatalf("Expecting to get conflicts when applying object after updating replicas, got no error: %s", result) - } - status, ok := err.(*apierrors.StatusError) - if !ok { - t.Fatalf("Expecting to get conflicts as API error") - } - if len(status.Status().Details.Causes) != 1 { - t.Fatalf("Expecting to get one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) - } - - // Re-apply with force, should work fine. - result, err = rest.Patch(types.ApplyPatchType). - AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). - Name(name). - Param("force", "true"). - Param("fieldManager", "apply_test"). - Body(yamlBody). - DoRaw(context.TODO()) - if err != nil { - t.Fatalf("failed to apply object with force after updating replicas: %v:\n%v", err, string(result)) - } - verifyReplicas(t, result, 1.0) -} - // verifyNumFinalizers checks that len(.metadata.finalizers) == n func verifyNumFinalizers(t *testing.T, b []byte, n int) { obj := unstructured.Unstructured{}