From 1ec8746c3a78cd3e877a586734e64693e7dc91ed Mon Sep 17 00:00:00 2001 From: Haowei Cai Date: Wed, 3 Jul 2019 11:29:27 -0700 Subject: [PATCH] do not publish openapi for a schema containing bad types minor rewords & refactor run script: update misc remove null from supported types --- .../apiextensions/validation/validation.go | 73 ++++++++++++++++ .../pkg/controller/openapi/BUILD | 1 + .../pkg/controller/openapi/builder.go | 12 +-- .../pkg/controller/openapi/builder_test.go | 84 +++++++++++++++++++ 4 files changed, 165 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 648aa205dca..fb57ccf1eeb 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -47,6 +47,7 @@ import ( var ( printerColumnDatatypes = sets.NewString("integer", "number", "string", "boolean", "date") customResourceColumnDefinitionFormats = sets.NewString("int32", "int64", "float", "double", "byte", "date", "date-time", "password") + openapiV3Types = sets.NewString("string", "number", "integer", "boolean", "array", "object") ) // ValidateCustomResourceDefinition statically validates @@ -1157,3 +1158,75 @@ func validateAPIApproval(newCRD, oldCRD *apiextensions.CustomResourceDefinition, return field.ErrorList{field.Invalid(field.NewPath("metadata", "annotations").Key(v1beta1.KubeAPIApprovedAnnotation), newCRD.Annotations[v1beta1.KubeAPIApprovedAnnotation], reason)} } } + +// SchemaHasInvalidTypes returns true if it contains invalid offending openapi-v3 specification. +func SchemaHasInvalidTypes(s *apiextensions.JSONSchemaProps) bool { + if s == nil { + return false + } + + if len(s.Type) > 0 && !openapiV3Types.Has(s.Type) { + return true + } + + if s.Items != nil { + if s.Items != nil && SchemaHasInvalidTypes(s.Items.Schema) { + return true + } + for _, s := range s.Items.JSONSchemas { + if SchemaHasInvalidTypes(&s) { + return true + } + } + } + for _, s := range s.AllOf { + if SchemaHasInvalidTypes(&s) { + return true + } + } + for _, s := range s.AnyOf { + if SchemaHasInvalidTypes(&s) { + return true + } + } + for _, s := range s.OneOf { + if SchemaHasInvalidTypes(&s) { + return true + } + } + if SchemaHasInvalidTypes(s.Not) { + return true + } + for _, s := range s.Properties { + if SchemaHasInvalidTypes(&s) { + return true + } + } + if s.AdditionalProperties != nil { + if SchemaHasInvalidTypes(s.AdditionalProperties.Schema) { + return true + } + } + for _, s := range s.PatternProperties { + if SchemaHasInvalidTypes(&s) { + return true + } + } + if s.AdditionalItems != nil { + if SchemaHasInvalidTypes(s.AdditionalItems.Schema) { + return true + } + } + for _, s := range s.Definitions { + if SchemaHasInvalidTypes(&s) { + return true + } + } + for _, d := range s.Dependencies { + if SchemaHasInvalidTypes(d.Schema) { + return true + } + } + + return false +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD index efdc22ef250..73a6d322b50 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/BUILD @@ -14,6 +14,7 @@ go_library( deps = [ "//staging/src/k8s.io/api/autoscaling/v1:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/informers/internalversion/apiextensions/internalversion:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/internalversion:go_default_library", 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 56ab468c009..776eddf5751 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 @@ -22,10 +22,11 @@ import ( "strings" "sync" - restful "github.com/emicklei/go-restful" + "github.com/emicklei/go-restful" "github.com/go-openapi/spec" v1 "k8s.io/api/autoscaling/v1" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" @@ -68,11 +69,12 @@ func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string) ( if err != nil { return nil, err } + if s != nil && s.OpenAPIV3Schema != nil { - ss, err := structuralschema.NewStructural(s.OpenAPIV3Schema) - if err == nil && len(structuralschema.ValidateStructural(ss, nil)) == 0 { - // skip non-structural schemas - schema = ss.Unfold() + if !validation.SchemaHasInvalidTypes(s.OpenAPIV3Schema) { + if ss, err := structuralschema.NewStructural(s.OpenAPIV3Schema); err == nil { + schema = ss.Unfold() + } } } 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 dc22e6e9299..181f85fb798 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 @@ -540,3 +540,87 @@ func schemaDiff(a, b *spec.Schema) string { } return diff.StringDiff(string(as), string(bs)) } + +func TestBuildSwagger(t *testing.T) { + tests := []struct { + name string + schema string + wantedSchema string + }{ + { + "nil", + "", + `{"type":"object","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"}]}`, + }, + { + "with invalid-typed properties", + `{"type":"object","properties":{"spec":{"type":"bug"},"status":{"type":"object"}}}`, + `{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var validation *apiextensions.CustomResourceValidation + if len(tt.schema) > 0 { + 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) + validation = &apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: internalSchema, + } + } + + // TODO: mostly copied from the test above. reuse code to cleanup + got, err := BuildSwagger(&apiextensions.CustomResourceDefinition{ + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "bar.k8s.io", + Version: "v1", + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "foos", + Singular: "foo", + Kind: "Foo", + ListKind: "FooList", + }, + Scope: apiextensions.NamespaceScoped, + Validation: validation, + }, + }, "v1") + if err != nil { + t.Fatal(err) + } + + var wantedSchema spec.Schema + if err := json.Unmarshal([]byte(tt.wantedSchema), &wantedSchema); err != nil { + t.Fatal(err) + } + + gotSchema := got.Definitions["io.k8s.bar.v1.Foo"] + gotProperties := properties(gotSchema.Properties) + wantedProperties := properties(wantedSchema.Properties) + if !gotProperties.Equal(wantedProperties) { + t.Fatalf("unexpected properties, got: %s, expected: %s", gotProperties.List(), wantedProperties.List()) + } + + // wipe out TypeMeta/ObjectMeta content, with those many lines of descriptions. We trust that they match here. + for _, metaField := range []string{"kind", "apiVersion", "metadata"} { + if _, found := gotSchema.Properties["kind"]; found { + prop := gotSchema.Properties[metaField] + prop.Description = "" + gotSchema.Properties[metaField] = prop + } + } + + if !reflect.DeepEqual(&wantedSchema, &gotSchema) { + t.Errorf("unexpected schema: %s\nwant = %#v\ngot = %#v", schemaDiff(&wantedSchema, &gotSchema), &wantedSchema, &gotSchema) + } + }) + } +}