Merge pull request kubernetes#79587 from yue9944882/bugfix/tighten-primitive-json-schema-type-validation

DO NOT publish openapi specs containing bad types

Author: k8s-ci-robot

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
+}

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",

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()
+			}
 		}
 	}
 

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)
+			}
+		})
+	}
+}