From bc86aeba05604d1f446915151bf5f7b42513bb43 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 7 May 2019 14:03:31 +0200 Subject: [PATCH] apiextensions: switch OpenAPI pubilshing to structural schema --- .../pkg/controller/openapi/builder.go | 33 +- .../pkg/controller/openapi/builder_test.go | 317 +++++++++++++-- .../pkg/controller/openapi/conversion.go | 128 ++---- .../pkg/controller/openapi/conversion_test.go | 378 +++++++++--------- test/integration/master/crd_test.go | 1 + 5 files changed, 537 insertions(+), 320 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go index 2bbffda1742..ee31526cda5 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go @@ -26,6 +26,7 @@ import ( "github.com/go-openapi/spec" v1 "k8s.io/api/autoscaling/v1" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" "k8s.io/apimachinery/pkg/runtime" @@ -58,22 +59,24 @@ var namer *openapi.DefinitionNamer // BuildSwagger builds swagger for the given crd in the given version func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) (*spec.Swagger, error) { - var schema *spec.Schema + var schema *structuralschema.Structural s, err := apiextensions.GetSchemaForVersion(crd, version) if err != nil { return nil, err } if s != nil && s.OpenAPIV3Schema != nil { - schema, err = ConvertJSONSchemaPropsToOpenAPIv2Schema(s.OpenAPIV3Schema) - if err != nil { - return nil, err + ss, err := structuralschema.NewStructural(s.OpenAPIV3Schema) + if err == nil && len(structuralschema.ValidateStructural(ss, nil)) == 0 { + // skip non-structural schemas + schema = ss.Unfold() } } + // TODO(roycaihw): remove the WebService templating below. The following logic // comes from function registerResourceHandlers() in k8s.io/apiserver. // Alternatives are either (ideally) refactoring registerResourceHandlers() to // reuse the code, or faking an APIInstaller for CR to feed to registerResourceHandlers(). - b := newBuilder(crd, version, schema) + b := newBuilder(crd, version, schema, true) // Sample response types for building web service sample := &CRDCanonicalTypeNamer{ @@ -288,23 +291,27 @@ func (b *builder) buildRoute(root, path, action, verb string, sample interface{} // buildKubeNative builds input schema with Kubernetes' native object meta, type meta and // extensions -func (b *builder) buildKubeNative(schema *spec.Schema) *spec.Schema { +func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool) (ret *spec.Schema) { // only add properties if we have a schema. Otherwise, kubectl would (wrongly) assume additionalProperties=false // and forbid anything outside of apiVersion, kind and metadata. We have to fix kubectl to stop doing this, e.g. by // adding additionalProperties=true support to explicitly allow additional fields. // TODO: fix kubectl to understand additionalProperties=true if schema == nil { - schema = &spec.Schema{ + ret = &spec.Schema{ SchemaProps: spec.SchemaProps{Type: []string{"object"}}, } // no, we cannot add more properties here, not even TypeMeta/ObjectMeta because kubectl will complain about // unknown fields for anything else. } else { - schema.SetProperty("metadata", *spec.RefSchema(objectMetaSchemaRef). + if v2 { + schema = ToStructuralOpenAPIV2(schema) + } + ret = schema.ToGoOpenAPI() + ret.SetProperty("metadata", *spec.RefSchema(objectMetaSchemaRef). WithDescription(swaggerPartialObjectMetadataDescriptions["metadata"])) - addTypeMetaProperties(schema) + addTypeMetaProperties(ret) } - schema.AddExtension(endpoints.ROUTE_META_GVK, []interface{}{ + ret.AddExtension(endpoints.ROUTE_META_GVK, []interface{}{ map[string]interface{}{ "group": b.group, "version": b.version, @@ -312,7 +319,7 @@ func (b *builder) buildKubeNative(schema *spec.Schema) *spec.Schema { }, }) - return schema + return ret } // getDefinition gets definition for given Kubernetes type. This function is extracted from @@ -391,7 +398,7 @@ func (b *builder) getOpenAPIConfig() *common.Config { } } -func newBuilder(crd *apiextensions.CustomResourceDefinition, version string, schema *spec.Schema) *builder { +func newBuilder(crd *apiextensions.CustomResourceDefinition, version string, schema *structuralschema.Structural, v2 bool) *builder { b := &builder{ schema: &spec.Schema{ SchemaProps: spec.SchemaProps{Type: []string{"object"}}, @@ -410,7 +417,7 @@ func newBuilder(crd *apiextensions.CustomResourceDefinition, version string, sch } // Pre-build schema with Kubernetes native properties - b.schema = b.buildKubeNative(schema) + b.schema = b.buildKubeNative(schema, v2) b.listSchema = b.buildListSchema() return b diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go index 2738137aa5b..3b175cf282e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go @@ -22,14 +22,14 @@ import ( "github.com/go-openapi/spec" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" ) func TestNewBuilder(t *testing.T) { - type args struct { - } tests := []struct { name string @@ -37,41 +37,302 @@ func TestNewBuilder(t *testing.T) { wantedSchema string wantedItemsSchema string + + v2 bool // produce OpenAPIv2 }{ { "nil", "", `{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, + true, }, - {"empty", - "{}", - `{"properties":{"apiVersion":{},"kind":{},"metadata":{}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, + {"with properties", + `{"type":"object","properties":{"spec":{"type":"object"},"status":{"type":"object"}}}`, + `{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"spec":{"type":"object"},"status":{"type":"object"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, + true, }, - {"empty properties", - `{"properties":{"spec":{},"status":{}}}`, - `{"properties":{"apiVersion":{},"kind":{},"metadata":{},"spec":{},"status":{}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, - `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, - }, - {"filled properties", - `{"properties":{"spec":{"type":"object"},"status":{"type":"object"}}}`, - `{"properties":{"apiVersion":{},"kind":{},"metadata":{},"spec":{"type":"object"},"status":{"type":"object"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, - `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, - }, - {"type", + {"type only", `{"type":"object"}`, - `{"properties":{"apiVersion":{},"kind":{},"metadata":{}},"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, + `{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, + true, + }, + {"with extensions", + ` +{ + "type":"object", + "properties": { + "int-or-string-1": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, + "int-or-string-2": { + "x-kubernetes-int-or-string": true, + "allOf": [{ + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, { + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-3": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ], + "allOf": [{ + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-4": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"minimum": 42.0} + ] + }, + "int-or-string-5": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"minimum": 42.0} + ], + "allOf": [ + {"minimum": 42.0} + ] + }, + "int-or-string-6": { + "x-kubernetes-int-or-string": true + }, + "preserve-unknown-fields": { + "x-kubernetes-preserve-unknown-fields": true + }, + "embedded-object": { + "x-kubernetes-embedded-resource": true, + "x-kubernetes-preserve-unknown-fields": true, + "type": "object" + } + } +}`, + ` +{ + "type":"object", + "properties": { + "apiVersion": {"type":"string"}, + "kind": {"type":"string"}, + "metadata": {"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}, + "int-or-string-1": { + "x-kubernetes-int-or-string": true + }, + "int-or-string-2": { + "x-kubernetes-int-or-string": true + }, + "int-or-string-3": { + "x-kubernetes-int-or-string": true + }, + "int-or-string-4": { + "x-kubernetes-int-or-string": true + }, + "int-or-string-5": { + "x-kubernetes-int-or-string": true + }, + "int-or-string-6": { + "x-kubernetes-int-or-string": true + }, + "preserve-unknown-fields": { + "x-kubernetes-preserve-unknown-fields": true + }, + "embedded-object": { + "x-kubernetes-embedded-resource": true, + "x-kubernetes-preserve-unknown-fields": true, + "type": "object" + } + }, + "x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}] +}`, + `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, + true, + }, + {"with extensions as v3 schema", + ` +{ + "type":"object", + "properties": { + "int-or-string-1": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, + "int-or-string-2": { + "x-kubernetes-int-or-string": true, + "allOf": [{ + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, { + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-3": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ], + "allOf": [{ + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-4": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"minimum": 42.0} + ] + }, + "int-or-string-5": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"minimum": 42.0} + ], + "allOf": [ + {"minimum": 42.0} + ] + }, + "int-or-string-6": { + "x-kubernetes-int-or-string": true + }, + "preserve-unknown-fields": { + "x-kubernetes-preserve-unknown-fields": true + }, + "embedded-object": { + "x-kubernetes-embedded-resource": true, + "x-kubernetes-preserve-unknown-fields": true, + "type": "object" + } + } +}`, + ` +{ + "type":"object", + "properties": { + "apiVersion": {"type":"string"}, + "kind": {"type":"string"}, + "metadata": {"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}, + "int-or-string-1": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, + "int-or-string-2": { + "x-kubernetes-int-or-string": true, + "allOf": [{ + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, { + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-3": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ], + "allOf": [{ + "anyOf": [ + {"minimum": 42.0} + ] + }] + }, + "int-or-string-4": { + "x-kubernetes-int-or-string": true, + "allOf": [{ + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }], + "anyOf": [ + {"minimum": 42.0} + ] + }, + "int-or-string-5": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"minimum": 42.0} + ], + "allOf": [{ + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, { + "minimum": 42.0 + }] + }, + "int-or-string-6": { + "x-kubernetes-int-or-string": true, + "anyOf": [ + {"type":"integer"}, + {"type":"string"} + ] + }, + "preserve-unknown-fields": { + "x-kubernetes-preserve-unknown-fields": true + }, + "embedded-object": { + "x-kubernetes-embedded-resource": true, + "x-kubernetes-preserve-unknown-fields": true, + "type": "object" + } + }, + "x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}] +}`, + `{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`, + false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - var schema *spec.Schema + var schema *structuralschema.Structural if len(tt.schema) > 0 { - schema = &spec.Schema{} - if err := json.Unmarshal([]byte(tt.schema), schema); err != nil { + v1beta1Schema := &v1beta1.JSONSchemaProps{} + if err := json.Unmarshal([]byte(tt.schema), &v1beta1Schema); err != nil { t.Fatal(err) } + internalSchema := &apiextensions.JSONSchemaProps{} + v1beta1.Convert_v1beta1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(v1beta1Schema, internalSchema, nil) + var err error + schema, err = structuralschema.NewStructural(internalSchema) + if err != nil { + t.Fatalf("structural schema error: %v", err) + } + if errs := structuralschema.ValidateStructural(schema, nil); len(errs) > 0 { + t.Fatalf("structural schema validation error: %v", errs.ToAggregate()) + } + schema = schema.Unfold() } got := newBuilder(&apiextensions.CustomResourceDefinition{ @@ -86,7 +347,7 @@ func TestNewBuilder(t *testing.T) { }, Scope: apiextensions.NamespaceScoped, }, - }, "v1", schema) + }, "v1", schema, tt.v2) var wantedSchema, wantedItemsSchema spec.Schema if err := json.Unmarshal([]byte(tt.wantedSchema), &wantedSchema); err != nil { @@ -103,14 +364,12 @@ func TestNewBuilder(t *testing.T) { } // wipe out TypeMeta/ObjectMeta content, with those many lines of descriptions. We trust that they match here. - if _, found := got.schema.Properties["kind"]; found { - got.schema.Properties["kind"] = spec.Schema{} - } - if _, found := got.schema.Properties["apiVersion"]; found { - got.schema.Properties["apiVersion"] = spec.Schema{} - } - if _, found := got.schema.Properties["metadata"]; found { - got.schema.Properties["metadata"] = spec.Schema{} + for _, metaField := range []string{"kind", "apiVersion", "metadata"} { + if _, found := got.schema.Properties["kind"]; found { + prop := got.schema.Properties[metaField] + prop.Description = "" + got.schema.Properties[metaField] = prop + } } if !reflect.DeepEqual(&wantedSchema, got.schema) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go index 743fa196ac4..55360ea5303 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go @@ -17,106 +17,60 @@ limitations under the License. package openapi import ( - "strings" - - "github.com/go-openapi/spec" - - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" - "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" ) -// ConvertJSONSchemaPropsToOpenAPIv2Schema converts our internal OpenAPI v3 schema -// (*apiextensions.JSONSchemaProps) to an OpenAPI v2 schema (*spec.Schema). -func ConvertJSONSchemaPropsToOpenAPIv2Schema(in *apiextensions.JSONSchemaProps) (*spec.Schema, error) { +// ToStructuralOpenAPIV2 converts our internal OpenAPI v3 structural schema to +// to a v2 compatible schema. +func ToStructuralOpenAPIV2(in *structuralschema.Structural) *structuralschema.Structural { if in == nil { - return nil, nil + return nil } - // dirty hack to temporarily set the type at the root. See continuation at the func bottom. - // TODO: remove for Kubernetes 1.15 - oldRootType := in.Type - if len(in.Type) == 0 { - in.Type = "object" - } + out := in.DeepCopy() // Remove unsupported fields in OpenAPI v2 recursively - out := new(spec.Schema) - validation.ConvertJSONSchemaPropsWithPostProcess(in, out, func(p *spec.Schema) error { - p.OneOf = nil - // TODO(roycaihw): preserve cases where we only have one subtree in AnyOf, same for OneOf - p.AnyOf = nil - p.Not = nil - - // TODO: drop everything below in 1.15 when we have passed one version skew towards kube-openapi in <1.14, which rejects valid openapi schemata - - if p.Ref.String() != "" { - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R95 - p.Properties = nil - - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R99 - p.Type = nil - - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R104 - if !strings.HasPrefix(p.Ref.String(), "#/definitions/") { - p.Ref = spec.Ref{} - } - } - - switch { - case len(p.Type) == 2 && (p.Type[0] == "null" || p.Type[1] == "null"): - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-ce77fea74b9dd098045004410023e0c3R219 - p.Type = nil - case len(p.Type) == 1: - switch p.Type[0] { - case "null": - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-ce77fea74b9dd098045004410023e0c3R219 - p.Type = nil - case "array": - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R183 - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R184 - if p.Items == nil || (p.Items.Schema == nil && len(p.Items.Schemas) != 1) { - p.Type = nil - p.Items = nil + mapper := structuralschema.Visitor{ + Structural: func(s *structuralschema.Structural) bool { + changed := false + if s.ValueValidation != nil { + if s.ValueValidation.AllOf != nil { + s.ValueValidation.AllOf = nil + changed = true + } + if s.ValueValidation.OneOf != nil { + s.ValueValidation.OneOf = nil + changed = true + } + if s.ValueValidation.AnyOf != nil { + s.ValueValidation.AnyOf = nil + changed = true + } + if s.ValueValidation.Not != nil { + s.ValueValidation.Not = nil + changed = true } } - case len(p.Type) > 1: - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R272 - // We also set Properties to null to enforce parseArbitrary at https://github.com/kubernetes/kube-openapi/blob/814a8073653e40e0e324205d093770d4e7bb811f/pkg/util/proto/document.go#L247 - p.Type = nil - p.Properties = nil - default: - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R248 - p.Properties = nil - } - // normalize items - if p.Items != nil && len(p.Items.Schemas) == 1 { - p.Items = &spec.SchemaOrArray{Schema: &p.Items.Schemas[0]} - } + // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-ce77fea74b9dd098045004410023e0c3R219 + if s.Nullable { + s.Type = "" + s.Nullable = false - // general fixups not supported by gnostic - p.ID = "" - p.Schema = "" - p.Definitions = nil - p.AdditionalItems = nil - p.Dependencies = nil - p.PatternProperties = nil - if p.ExternalDocs != nil && len(p.ExternalDocs.URL) == 0 { - p.ExternalDocs = nil - } - if p.Items != nil && p.Items.Schemas != nil { - p.Items = nil - } + // untyped values break if items or properties are set in kubectl + // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R183 + s.Items = nil + s.Properties = nil - return nil - }) + changed = true + } - // restore root level type in input, and remove it in output if we had added it - // TODO: remove with Kubernetes 1.15 - in.Type = oldRootType - if len(oldRootType) == 0 { - out.Type = nil + return changed + }, + // we drop all junctors above, and hence, never reach nested value validations + NestedValueValidation: nil, } + mapper.Visit(out) - return out, nil + return out } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion_test.go index a6c6fd94540..f88e070a010 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion_test.go @@ -26,13 +26,15 @@ import ( "time" "github.com/go-openapi/spec" - "github.com/google/gofuzz" - "github.com/googleapis/gnostic/OpenAPIv2" + "github.com/google/go-cmp/cmp" + fuzz "github.com/google/gofuzz" + openapi_v2 "github.com/googleapis/gnostic/OpenAPIv2" "github.com/googleapis/gnostic/compiler" "gopkg.in/yaml.v2" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - "k8s.io/apimachinery/pkg/util/diff" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/kube-openapi/pkg/util/proto" ) @@ -94,11 +96,14 @@ properties: t.Fatal(err) } - schema, err := ConvertJSONSchemaPropsToOpenAPIv2Schema(&specInternal) + ss, err := structuralschema.NewStructural(&specInternal) if err != nil { t.Fatal(err) } + ssV2 := ToStructuralOpenAPIV2(ss) + schema := ssV2.ToGoOpenAPI() + if _, found := schema.Properties["spec"]; !found { t.Errorf("spec not found") } @@ -107,7 +112,7 @@ properties: } } -func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { +func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaByType(t *testing.T) { testStr := "test" testStr2 := "test2" testFloat64 := float64(6.4) @@ -115,41 +120,32 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { testApiextensionsJSON := apiextensions.JSON(testStr) tests := []struct { - name string - in *apiextensions.JSONSchemaProps - expected *spec.Schema + name string + in *apiextensions.JSONSchemaProps + expected *spec.Schema + expectError bool + expectDiff bool }{ { name: "id", in: &apiextensions.JSONSchemaProps{ ID: testStr, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: new(spec.Schema). - // WithID(testStr), + expectError: true, // rejected by kube validation and NewStructural }, { name: "$schema", in: &apiextensions.JSONSchemaProps{ Schema: "test", }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // Schema: "test", - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "$ref", in: &apiextensions.JSONSchemaProps{ Ref: &testStr, }, - expected: new(spec.Schema), - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R104 - // expected: spec.RefSchema(testStr), + expectError: true, // rejected by kube validation and NewStructural }, { name: "description", @@ -168,6 +164,14 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { expected: new(spec.Schema). Typed(testStr, testStr2), }, + { + name: "nullable", + in: &apiextensions.JSONSchemaProps{ + Type: "object", + Nullable: true, + }, + expected: new(spec.Schema), + }, { name: "title", in: &apiextensions.JSONSchemaProps{ @@ -317,18 +321,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, }, }, - expected: new(spec.Schema), - // https://github.com/kubernetes/kube-openapi/pull/143/files#diff-62afddb578e9db18fb32ffb6b7802d92R272 - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // Items: &spec.SchemaOrArray{ - // Schemas: []spec.Schema{ - // *spec.BooleanProperty(), - // *spec.StringProperty(), - // }, - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "allOf", @@ -338,8 +331,10 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { {Type: "string"}, }, }, - expected: new(spec.Schema). - WithAllOf(*spec.BooleanProperty(), *spec.StringProperty()), + expected: new(spec.Schema), + // intentionally not exported in v2 + // expected: new(spec.Schema). + // WithAllOf(*spec.BooleanProperty(), *spec.StringProperty()), }, { name: "oneOf", @@ -471,8 +466,10 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, }, }, - expected: new(spec.Schema). - WithAllOf(spec.Schema{}, spec.Schema{}, spec.Schema{}, *spec.StringProperty()), + expected: new(spec.Schema), + // not supported by OpenAPI v2 + allOf intentionally not exported + // expected: new(spec.Schema). + // WithAllOf(spec.Schema{}, spec.Schema{}, spec.Schema{}, *spec.StringProperty()), }, { name: "properties", @@ -485,22 +482,39 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { SetProperty(testStr, *spec.BooleanProperty()), }, { - name: "additionalProperties", + name: "additionalProperties schema", in: &apiextensions.JSONSchemaProps{ AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ - Allows: true, + Allows: false, Schema: &apiextensions.JSONSchemaProps{Type: "boolean"}, }, }, expected: &spec.Schema{ SchemaProps: spec.SchemaProps{ AdditionalProperties: &spec.SchemaOrBool{ - Allows: true, + Allows: false, Schema: spec.BooleanProperty(), }, }, }, }, + { + name: "additionalProperties bool", + in: &apiextensions.JSONSchemaProps{ + AdditionalProperties: &apiextensions.JSONSchemaPropsOrBool{ + Allows: true, + Schema: nil, + }, + }, + expected: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + AdditionalProperties: &spec.SchemaOrBool{ + Allows: true, + Schema: nil, + }, + }, + }, + }, { name: "patternProperties", in: &apiextensions.JSONSchemaProps{ @@ -508,15 +522,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { testStr: {Type: "boolean"}, }, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // PatternProperties: map[string]spec.Schema{ - // testStr: *spec.BooleanProperty(), - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "dependencies schema", @@ -527,17 +533,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, }, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // Dependencies: spec.Dependencies{ - // testStr: spec.SchemaOrStringArray{ - // Schema: spec.BooleanProperty(), - // }, - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "dependencies string array", @@ -548,17 +544,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, }, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // Dependencies: spec.Dependencies{ - // testStr: spec.SchemaOrStringArray{ - // Property: []string{testStr2}, - // }, - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "additionalItems", @@ -568,16 +554,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { Schema: &apiextensions.JSONSchemaProps{Type: "boolean"}, }, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // AdditionalItems: &spec.SchemaOrBool{ - // Allows: true, - // Schema: spec.BooleanProperty(), - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "definitions", @@ -586,15 +563,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { testStr: apiextensions.JSONSchemaProps{Type: "boolean"}, }, }, - expected: new(spec.Schema), - // not supported by gnostic - // expected: &spec.Schema{ - // SchemaProps: spec.SchemaProps{ - // Definitions: spec.Definitions{ - // testStr: *spec.BooleanProperty(), - // }, - // }, - // }, + expectError: true, // rejected by kube validation and NewStructural }, { name: "externalDocs", @@ -606,6 +575,7 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, expected: new(spec.Schema). WithExternalDocs(testStr, testStr2), + expectDiff: true, }, { name: "example", @@ -614,115 +584,127 @@ func Test_ConvertJSONSchemaPropsToOpenAPIv2SchemaFuzzing(t *testing.T) { }, expected: new(spec.Schema). WithExample(testStr), + expectDiff: true, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - out, err := ConvertJSONSchemaPropsToOpenAPIv2Schema(test.in) - if err != nil { - t.Fatalf("unexpected error in converting openapi schema: %v", err) + ss, err := structuralschema.NewStructural(test.in) + if err != nil && !test.expectError { + t.Fatalf("structural schema error: %v", err) + } else if err == nil && test.expectError { + t.Fatalf("expected NewStructural error, but didn't get any") } - if !reflect.DeepEqual(*out, *test.expected) { - t.Errorf("unexpected result:\n want=%v\n got=%v\n\n%s", *test.expected, *out, diff.ObjectDiff(*test.expected, *out)) + + if !test.expectError { + out := ToStructuralOpenAPIV2(ss).ToGoOpenAPI() + if equal := reflect.DeepEqual(*out, *test.expected); !equal && !test.expectDiff { + t.Errorf("unexpected result:\n want=%v\n got=%v\n\n%s", *test.expected, *out, cmp.Diff(*test.expected, *out, cmp.Comparer(refEqual))) + } else if equal && test.expectDiff { + t.Errorf("expected diff, but didn't get any") + } } }) } } +func refEqual(x spec.Ref, y spec.Ref) bool { + return x.String() == y.String() +} + // TestKubeOpenapiRejectionFiltering tests that the CRD openapi schema filtering leads to a spec that the // kube-openapi/pkg/util/proto model code support in version used in Kubernetes 1.13. func TestKubeOpenapiRejectionFiltering(t *testing.T) { for i := 0; i < 10000; i++ { - t.Run(fmt.Sprintf("iteration %d", i), func(t *testing.T) { - f := fuzz.New() - seed := time.Now().UnixNano() - randSource := rand.New(rand.NewSource(seed)) - f.RandSource(randSource) - t.Logf("seed = %d", seed) + f := fuzz.New() + seed := time.Now().UnixNano() + randSource := rand.New(rand.NewSource(seed)) + f.RandSource(randSource) + t.Logf("iteration %d with seed %d", i, seed) - fuzzFuncs(f, func(ref *spec.Ref, c fuzz.Continue, visible bool) { - var url string - if c.RandBool() { - url = fmt.Sprintf("http://%d", c.Intn(100000)) - } else { - url = "#/definitions/test" - } - r, err := spec.NewRef(url) - if err != nil { - t.Fatalf("failed to fuzz ref: %v", err) - } - *ref = r - }) - - // create go-openapi object and fuzz it (we start here because we have the powerful fuzzer already - s := &spec.Schema{} - f.Fuzz(s) - - // convert to apiextensions v1beta1 - bs, err := json.Marshal(s) + fuzzFuncs(f, func(ref *spec.Ref, c fuzz.Continue, visible bool) { + var url string + if c.RandBool() { + url = fmt.Sprintf("http://%d", c.Intn(100000)) + } else { + url = "#/definitions/test" + } + r, err := spec.NewRef(url) if err != nil { - t.Fatal(err) - } - t.Log(string(bs)) - - var schema *apiextensionsv1beta1.JSONSchemaProps - if err := json.Unmarshal(bs, &schema); err != nil { - t.Fatalf("failed to unmarshal JSON into apiextensions/v1beta1: %v", err) - } - - // convert to internal - internalSchema := &apiextensions.JSONSchemaProps{} - if err := apiextensionsv1beta1.Convert_v1beta1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(schema, internalSchema, nil); err != nil { - t.Fatalf("failed to convert from apiextensions/v1beta1 to internal: %v", err) - } - - // apply the filter - filtered, err := ConvertJSONSchemaPropsToOpenAPIv2Schema(internalSchema) - if err != nil { - t.Fatalf("failed to filter: %v", err) - } - - // create a doc out of it - filteredSwagger := &spec.Swagger{ - SwaggerProps: spec.SwaggerProps{ - Definitions: spec.Definitions{ - "test": *filtered, - }, - Info: &spec.Info{ - InfoProps: spec.InfoProps{ - Description: "test", - Version: "test", - Title: "test", - }, - }, - Swagger: "2.0", - }, - } - - // convert to JSON - bs, err = json.Marshal(filteredSwagger) - if err != nil { - t.Fatalf("failed to encode filtered to JSON: %v", err) - } - - // unmarshal as yaml - var yml yaml.MapSlice - if err := yaml.Unmarshal(bs, &yml); err != nil { - t.Fatalf("failed to decode filtered JSON by into memory: %v", err) - } - - // create gnostic doc - doc, err := openapi_v2.NewDocument(yml, compiler.NewContext("$root", nil)) - if err != nil { - t.Fatalf("failed to create gnostic doc: %v", err) - } - - // load with kube-openapi/pkg/util/proto - if _, err := proto.NewOpenAPIData(doc); err != nil { - t.Fatalf("failed to convert to kube-openapi/pkg/util/proto model: %v", err) + t.Fatalf("failed to fuzz ref: %v", err) } + *ref = r }) + + // create go-openapi object and fuzz it (we start here because we have the powerful fuzzer already + s := &spec.Schema{} + f.Fuzz(s) + + // convert to apiextensions v1beta1 + bs, err := json.Marshal(s) + if err != nil { + t.Fatal(err) + } + t.Log(string(bs)) + + var schema *apiextensionsv1beta1.JSONSchemaProps + if err := json.Unmarshal(bs, &schema); err != nil { + t.Fatalf("failed to unmarshal JSON into apiextensions/v1beta1: %v", err) + } + + // convert to internal + internalSchema := &apiextensions.JSONSchemaProps{} + if err := apiextensionsv1beta1.Convert_v1beta1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(schema, internalSchema, nil); err != nil { + t.Fatalf("failed to convert from apiextensions/v1beta1 to internal: %v", err) + } + + // apply the filter + ss, err := structuralschema.NewStructural(internalSchema) + if err != nil { + t.Fatal(err) + } + filtered := ToStructuralOpenAPIV2(ss).ToGoOpenAPI() + + // create a doc out of it + filteredSwagger := &spec.Swagger{ + SwaggerProps: spec.SwaggerProps{ + Definitions: spec.Definitions{ + "test": *filtered, + }, + Info: &spec.Info{ + InfoProps: spec.InfoProps{ + Description: "test", + Version: "test", + Title: "test", + }, + }, + Swagger: "2.0", + }, + } + + // convert to JSON + bs, err = json.Marshal(filteredSwagger) + if err != nil { + t.Fatalf("failed to encode filtered to JSON: %v", err) + } + + // unmarshal as yaml + var yml yaml.MapSlice + if err := yaml.Unmarshal(bs, &yml); err != nil { + t.Fatalf("failed to decode filtered JSON by into memory: %v", err) + } + + // create gnostic doc + doc, err := openapi_v2.NewDocument(yml, compiler.NewContext("$root", nil)) + if err != nil { + t.Fatalf("failed to create gnostic doc: %v", err) + } + + // load with kube-openapi/pkg/util/proto + if _, err := proto.NewOpenAPIData(doc); err != nil { + t.Fatalf("failed to convert to kube-openapi/pkg/util/proto model: %v", err) + } } } @@ -794,9 +776,11 @@ func fuzzFuncs(f *fuzz.Fuzzer, refFunc func(ref *spec.Ref, c fuzz.Continue, visi if p.Default != nil { p.Default = "42" } - if p.Example != nil { - p.Example = "42" - } + p.Example = nil + }, + func(s *spec.SwaggerSchemaProps, c fuzz.Continue) { + // nothing allowed + *s = spec.SwaggerSchemaProps{} }, func(s *spec.SchemaProps, c fuzz.Continue) { // gofuzz is broken and calls this even for *SchemaProps fields, ignoring NilChance, leading to infinite recursion @@ -809,14 +793,26 @@ func fuzzFuncs(f *fuzz.Fuzzer, refFunc func(ref *spec.Ref, c fuzz.Continue, visi c.FuzzNoCustom(s) - // we don't support multi-type schema props yet in apiextensions/v1beta1 - if len(s.Type) > 1 { - s.Type = s.Type[:1] + if c.RandBool() { + types := []string{"object", "array", "boolean", "string", "integer", "number"} + s.Type = []string{types[c.Intn(len(types))]} + } else { + s.Type = nil + } - s := apiextensionsv1beta1.JSONSchemaProps{} - if reflect.TypeOf(s.Type).String() != "string" { - panic(fmt.Errorf("this simplifaction is outdated: apiextensions/v1beta1 types not a single string anymore, but %T", s.Type)) - } + s.ID = "" + s.Ref = spec.Ref{} + s.AdditionalItems = nil + s.Dependencies = nil + s.Schema = "" + s.PatternProperties = nil + s.Definitions = nil + + if len(s.Type) == 1 && s.Type[0] == "array" { + s.Items = &spec.SchemaOrArray{Schema: &spec.Schema{}} + c.Fuzz(s.Items.Schema) + } else { + s.Items = nil } // reset JSON fields to some correct JSON diff --git a/test/integration/master/crd_test.go b/test/integration/master/crd_test.go index 2ed3784d125..778173da15d 100644 --- a/test/integration/master/crd_test.go +++ b/test/integration/master/crd_test.go @@ -160,6 +160,7 @@ func TestCRDOpenAPI(t *testing.T) { }, Validation: &apiextensionsv1beta1.CustomResourceValidation{ OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ "foo": {Type: "string"}, },