From aebb47e7f410e5a7ff1c6b22a54b33a9971532f3 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 30 Apr 2020 14:45:06 -0700 Subject: [PATCH 1/3] Only create SSA models for CRDs with structural schema And also, the current code only understands V2, so we should use that exclusively, or we're guaranteed to have errors in the future. --- .../pkg/apiserver/customresource_handler.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 d3eddb8c6f1..0b897f8fa42 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,8 +1320,7 @@ func buildOpenAPIModelsForApply(staticOpenAPISpec *goopenapispec.Swagger, crd *a specs := []*goopenapispec.Swagger{} for _, v := range crd.Spec.Versions { - // 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: true}) + s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: true, StripValueValidation: true, StripNullable: true, AllowNonStructural: false}) if err != nil { return nil, err } From 216d820f35e4229a204525eb3fd05b4e46206efb Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Mon, 4 May 2020 21:21:46 -0700 Subject: [PATCH 2/3] Start writing a unit-test for buildOpenAPIForApplyModels --- .../apiserver/customresource_handler_test.go | 55 +++++++++++++++++++ 1 file changed, 55 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 fee93bc4295..0e35ff5b227 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 @@ -29,6 +29,8 @@ import ( "testing" "time" + "github.com/go-openapi/spec" + "sigs.k8s.io/yaml" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -755,3 +757,56 @@ 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, + }, + }, + } + + 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 { + t.Fatalf("failed to convert to apply model: %v", err) + } + } +} From 32a64cf14cebbc706efdb301fe3b290335927a80 Mon Sep 17 00:00:00 2001 From: Jefftree Date: Wed, 9 Dec 2020 14:33:50 -0800 Subject: [PATCH 3/3] 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{}