Merge pull request #82653 from liggitt/skip-publish-nonstructural

Skip publishing OpenAPI for nonstructural schemas

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

@@ -1236,7 +1236,7 @@ func buildOpenAPIModelsForApply(staticOpenAPISpec *spec.Swagger, crd *apiextensi
 
 	specs := []*spec.Swagger{}
 	for _, v := range crd.Spec.Versions {
-		s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: false, StripDefaults: true, StripValueValidation: true})
+		s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: false, StripDefaults: true, StripValueValidation: true, AllowNonStructural: true})
 		if err != nil {
 			return nil, err
 		}

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD

@@ -51,6 +51,7 @@ go_test(
         "//vendor/github.com/go-openapi/spec:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
     ],
 )
 

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go

@@ -75,6 +75,9 @@ type Options struct {
 
 	// Strip value validation.
 	StripValueValidation bool
+
+	// AllowNonStructural indicates swagger should be built for a schema that fits into the structural type but does not meet all structural invariants
+	AllowNonStructural bool
 }
 
 // BuildSwagger builds swagger for the given crd in the given version
@@ -88,17 +91,19 @@ func BuildSwagger(crd *apiextensions.CustomResourceDefinition, version string, o
 	if s != nil && s.OpenAPIV3Schema != nil {
 		if !validation.SchemaHasInvalidTypes(s.OpenAPIV3Schema) {
 			if ss, err := structuralschema.NewStructural(s.OpenAPIV3Schema); err == nil {
-				// skip non-structural schemas
-				schema = ss
+				// skip non-structural schemas unless explicitly asked to produce swagger from them
+				if opts.AllowNonStructural || len(structuralschema.ValidateStructural(nil, ss)) == 0 {
+					schema = ss
 
-				if opts.StripDefaults {
-					schema = schema.StripDefaults()
-				}
-				if opts.StripValueValidation {
-					schema = schema.StripValueValidations()
-				}
+					if opts.StripDefaults {
+						schema = schema.StripDefaults()
+					}
+					if opts.StripValueValidation {
+						schema = schema.StripValueValidations()
+					}
 
-				schema = schema.Unfold()
+					schema = schema.Unfold()
+				}
 			}
 		}
 	}
@@ -334,12 +339,12 @@ func (b *builder) buildRoute(root, path, httpMethod, actionVerb, operationVerb s
 
 // buildKubeNative builds input schema with Kubernetes' native object meta, type meta and
 // extensions
-func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool) (ret *spec.Schema) {
+func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool, crdPreserveUnknownFields 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 || (v2 && schema.XPreserveUnknownFields) {
+	if schema == nil || (v2 && (schema.XPreserveUnknownFields || crdPreserveUnknownFields)) {
 		ret = &spec.Schema{
 			SchemaProps: spec.SchemaProps{Type: []string{"object"}},
 		}
@@ -506,7 +511,8 @@ func newBuilder(crd *apiextensions.CustomResourceDefinition, version string, sch
 	}
 
 	// Pre-build schema with Kubernetes native properties
-	b.schema = b.buildKubeNative(schema, v2)
+	preserveUnknownFields := crd.Spec.PreserveUnknownFields != nil && *crd.Spec.PreserveUnknownFields
+	b.schema = b.buildKubeNative(schema, v2, preserveUnknownFields)
 	b.listSchema = b.buildListSchema()
 
 	return b

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/apiserver/pkg/endpoints"
 	"k8s.io/apiserver/pkg/features"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func TestNewBuilder(t *testing.T) {
@@ -554,50 +555,72 @@ func schemaDiff(a, b *spec.Schema) string {
 
 func TestBuildSwagger(t *testing.T) {
 	tests := []struct {
-		name         string
-		schema       string
-		wantedSchema string
-		opts         Options
+		name                  string
+		schema                string
+		preserveUnknownFields *bool
+		wantedSchema          string
+		opts                  Options
 	}{
 		{
 			"nil",
 			"",
+			nil,
 			`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"with properties",
 			`{"type":"object","properties":{"spec":{"type":"object"},"status":{"type":"object"}}}`,
+			nil,
 			`{"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"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"with invalid-typed properties",
 			`{"type":"object","properties":{"spec":{"type":"bug"},"status":{"type":"object"}}}`,
+			nil,
+			`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
+			Options{V2: true, StripDefaults: true},
+		},
+		{
+			"with non-structural schema",
+			`{"type":"object","properties":{"foo":{"type":"array"}}}`,
+			nil,
+			`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
+			Options{V2: true, StripDefaults: true},
+		},
+		{
+			"with spec.preseveUnknownFields=true",
+			`{"type":"object","properties":{"foo":{"type":"string"}}}`,
+			utilpointer.BoolPtr(true),
 			`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"with stripped defaults",
 			`{"type":"object","properties":{"foo":{"type":"string","default":"bar"}}}`,
+			nil,
 			`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"with stripped defaults",
 			`{"type":"object","properties":{"foo":{"type":"string","default":"bar"}}}`,
+			nil,
 			`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"v2",
 			`{"type":"object","properties":{"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}}}`,
+			nil,
 			`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: true, StripDefaults: true},
 		},
 		{
 			"v3",
 			`{"type":"object","properties":{"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}}}`,
+			nil,
 			`{"type":"object","properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"},"foo":{"type":"string","oneOf":[{"pattern":"a"},{"pattern":"b"}]}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
 			Options{V2: false, StripDefaults: true},
 		},
@@ -628,8 +651,9 @@ func TestBuildSwagger(t *testing.T) {
 						Kind:     "Foo",
 						ListKind: "FooList",
 					},
-					Scope:      apiextensions.NamespaceScoped,
-					Validation: validation,
+					Scope:                 apiextensions.NamespaceScoped,
+					Validation:            validation,
+					PreserveUnknownFields: tt.preserveUnknownFields,
 				},
 			}, "v1", tt.opts)
 			if err != nil {

test/integration/master/BUILD

@@ -70,6 +70,7 @@ go_test(
         "//vendor/github.com/evanphx/json-patch:go_default_library",
         "//vendor/github.com/go-openapi/spec:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
         "//vendor/sigs.k8s.io/yaml:go_default_library",
     ] + select({
         "@io_bazel_rules_go//go/platform:android": [

test/integration/master/crd_test.go

@@ -37,6 +37,7 @@ import (
 	kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
 	"k8s.io/kubernetes/test/integration/etcd"
 	"k8s.io/kubernetes/test/integration/framework"
+	utilpointer "k8s.io/utils/pointer"
 )
 
 func TestCRDShadowGroup(t *testing.T) {
@@ -172,6 +173,7 @@ func TestCRDOpenAPI(t *testing.T) {
 				Plural: "foos",
 				Kind:   "Foo",
 			},
+			PreserveUnknownFields: utilpointer.BoolPtr(false),
 			Validation: &apiextensionsv1beta1.CustomResourceValidation{
 				OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{
 					Type: "object",