From b0ff8d3f7412f5dd8428087a4d4fd9587f8e2b6b Mon Sep 17 00:00:00 2001 From: jennybuckley Date: Thu, 29 Aug 2019 22:40:01 -0700 Subject: [PATCH] Remove defaulting for x-k8s-list-type --- .../pkg/apis/apiextensions/v1/defaults.go | 44 --------------- .../apis/apiextensions/v1/defaults_test.go | 41 -------------- .../apiextensions/v1/zz_generated.defaults.go | 8 --- .../apis/apiextensions/v1beta1/defaults.go | 44 --------------- .../apiextensions/v1beta1/defaults_test.go | 28 ---------- .../v1beta1/zz_generated.defaults.go | 13 ----- .../apiextensions/validation/validation.go | 54 ++++++++++--------- .../validation/validation_test.go | 23 ++++---- test/e2e/apimachinery/crd_publish_openapi.go | 10 +--- 9 files changed, 40 insertions(+), 225 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults.go index 7bf26d5c7f0..5cebec927bd 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults.go @@ -59,47 +59,3 @@ func SetDefaults_ServiceReference(obj *ServiceReference) { obj.Port = utilpointer.Int32Ptr(443) } } - -// SetDefaults_JSONSchemaProps sets the defaults for JSONSchemaProps -func SetDefaults_JSONSchemaProps(obj *JSONSchemaProps) { - if obj == nil { - return - } - if obj.Type == "array" && obj.XListType == nil { - obj.XListType = utilpointer.StringPtr("atomic") - } - if obj.Items != nil { - SetDefaults_JSONSchemaProps(obj.Items.Schema) - defaultJSONSchemaPropsArray(obj.Items.JSONSchemas) - } - defaultJSONSchemaPropsArray(obj.AllOf) - defaultJSONSchemaPropsArray(obj.OneOf) - defaultJSONSchemaPropsArray(obj.AnyOf) - SetDefaults_JSONSchemaProps(obj.Not) - defaultJSONSchemaPropsMap(obj.Properties) - if obj.AdditionalProperties != nil { - SetDefaults_JSONSchemaProps(obj.AdditionalProperties.Schema) - } - defaultJSONSchemaPropsMap(obj.PatternProperties) - for i := range obj.Dependencies { - SetDefaults_JSONSchemaProps(obj.Dependencies[i].Schema) - } - if obj.AdditionalItems != nil { - SetDefaults_JSONSchemaProps(obj.AdditionalItems.Schema) - } - defaultJSONSchemaPropsMap(map[string]JSONSchemaProps(obj.Definitions)) -} - -func defaultJSONSchemaPropsArray(obj []JSONSchemaProps) { - for i := range obj { - SetDefaults_JSONSchemaProps(&obj[i]) - } -} - -func defaultJSONSchemaPropsMap(obj map[string]JSONSchemaProps) { - for i := range obj { - props := obj[i] - SetDefaults_JSONSchemaProps(&props) - obj[i] = props - } -} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults_test.go index 4f17503218f..7640c10c5c4 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/defaults_test.go @@ -124,47 +124,6 @@ func TestDefaults(t *testing.T) { }, }, }, - { - name: "list type extension defaults", - original: &CustomResourceDefinition{ - Spec: CustomResourceDefinitionSpec{ - Scope: NamespaceScoped, - Conversion: &CustomResourceConversion{Strategy: NoneConverter}, - Versions: []CustomResourceDefinitionVersion{ - { - Name: "v1", - Storage: true, - Schema: &CustomResourceValidation{ - OpenAPIV3Schema: &JSONSchemaProps{ - Type: "array", - }, - }, - }, - }, - }, - }, - expected: &CustomResourceDefinition{ - Spec: CustomResourceDefinitionSpec{ - Scope: NamespaceScoped, - Conversion: &CustomResourceConversion{Strategy: NoneConverter}, - Versions: []CustomResourceDefinitionVersion{ - { - Name: "v1", - Storage: true, - Schema: &CustomResourceValidation{ - OpenAPIV3Schema: &JSONSchemaProps{ - Type: "array", - XListType: utilpointer.StringPtr("atomic"), - }, - }, - }, - }, - }, - Status: CustomResourceDefinitionStatus{ - StoredVersions: []string{"v1"}, - }, - }, - }, } for _, test := range tests { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.defaults.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.defaults.go index 317522d1b86..ed03cdd886c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.defaults.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.defaults.go @@ -38,14 +38,6 @@ func RegisterDefaults(scheme *runtime.Scheme) error { func SetObjectDefaults_CustomResourceDefinition(in *CustomResourceDefinition) { SetDefaults_CustomResourceDefinition(in) SetDefaults_CustomResourceDefinitionSpec(&in.Spec) - for i := range in.Spec.Versions { - a := &in.Spec.Versions[i] - if a.Schema != nil { - if a.Schema.OpenAPIV3Schema != nil { - SetDefaults_JSONSchemaProps(a.Schema.OpenAPIV3Schema) - } - } - } if in.Spec.Conversion != nil { if in.Spec.Conversion.Webhook != nil { if in.Spec.Conversion.Webhook.ClientConfig != nil { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go index bffce311907..1a9c2238e33 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults.go @@ -80,47 +80,3 @@ func SetDefaults_ServiceReference(obj *ServiceReference) { obj.Port = utilpointer.Int32Ptr(443) } } - -// SetDefaults_JSONSchemaProps sets the defaults for JSONSchemaProps -func SetDefaults_JSONSchemaProps(obj *JSONSchemaProps) { - if obj == nil { - return - } - if obj.Type == "array" && obj.XListType == nil { - obj.XListType = utilpointer.StringPtr("atomic") - } - if obj.Items != nil { - SetDefaults_JSONSchemaProps(obj.Items.Schema) - defaultJSONSchemaPropsArray(obj.Items.JSONSchemas) - } - defaultJSONSchemaPropsArray(obj.AllOf) - defaultJSONSchemaPropsArray(obj.OneOf) - defaultJSONSchemaPropsArray(obj.AnyOf) - SetDefaults_JSONSchemaProps(obj.Not) - defaultJSONSchemaPropsMap(obj.Properties) - if obj.AdditionalProperties != nil { - SetDefaults_JSONSchemaProps(obj.AdditionalProperties.Schema) - } - defaultJSONSchemaPropsMap(obj.PatternProperties) - for i := range obj.Dependencies { - SetDefaults_JSONSchemaProps(obj.Dependencies[i].Schema) - } - if obj.AdditionalItems != nil { - SetDefaults_JSONSchemaProps(obj.AdditionalItems.Schema) - } - defaultJSONSchemaPropsMap(map[string]JSONSchemaProps(obj.Definitions)) -} - -func defaultJSONSchemaPropsArray(obj []JSONSchemaProps) { - for i := range obj { - SetDefaults_JSONSchemaProps(&obj[i]) - } -} - -func defaultJSONSchemaPropsMap(obj map[string]JSONSchemaProps) { - for i := range obj { - props := obj[i] - SetDefaults_JSONSchemaProps(&props) - obj[i] = props - } -} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults_test.go index dff89a77f1a..6aad4c5eee7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/defaults_test.go @@ -129,34 +129,6 @@ func TestDefaults(t *testing.T) { }, }, }, - { - name: "list type extension defaults", - original: &CustomResourceDefinition{ - Spec: CustomResourceDefinitionSpec{ - Scope: NamespaceScoped, - Conversion: &CustomResourceConversion{Strategy: NoneConverter}, - PreserveUnknownFields: utilpointer.BoolPtr(true), - Validation: &CustomResourceValidation{ - OpenAPIV3Schema: &JSONSchemaProps{ - Type: "array", - }, - }, - }, - }, - expected: &CustomResourceDefinition{ - Spec: CustomResourceDefinitionSpec{ - Scope: NamespaceScoped, - Conversion: &CustomResourceConversion{Strategy: NoneConverter}, - PreserveUnknownFields: utilpointer.BoolPtr(true), - Validation: &CustomResourceValidation{ - OpenAPIV3Schema: &JSONSchemaProps{ - Type: "array", - XListType: utilpointer.StringPtr("atomic"), - }, - }, - }, - }, - }, } for _, test := range tests { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/zz_generated.defaults.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/zz_generated.defaults.go index 7be1e1eeb3d..e1807243f01 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/zz_generated.defaults.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/zz_generated.defaults.go @@ -38,19 +38,6 @@ func RegisterDefaults(scheme *runtime.Scheme) error { func SetObjectDefaults_CustomResourceDefinition(in *CustomResourceDefinition) { SetDefaults_CustomResourceDefinition(in) SetDefaults_CustomResourceDefinitionSpec(&in.Spec) - if in.Spec.Validation != nil { - if in.Spec.Validation.OpenAPIV3Schema != nil { - SetDefaults_JSONSchemaProps(in.Spec.Validation.OpenAPIV3Schema) - } - } - for i := range in.Spec.Versions { - a := &in.Spec.Versions[i] - if a.Schema != nil { - if a.Schema.OpenAPIV3Schema != nil { - SetDefaults_JSONSchemaProps(a.Schema.OpenAPIV3Schema) - } - } - } if in.Spec.Conversion != nil { if in.Spec.Conversion.WebhookClientConfig != nil { if in.Spec.Conversion.WebhookClientConfig.Service != nil { 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 17c4e24a028..b92f5c2ceae 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 @@ -796,10 +796,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } } - if schema.XListType == nil && schema.Type == "array" { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-list-type"), "must be set if type is array")) - } - if schema.XListType != nil && *schema.XListType != "atomic" && *schema.XListType != "set" && *schema.XListType != "map" { allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, []string{"atomic", "set", "map"})) } @@ -812,32 +808,38 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch } } - if len(schema.XListMapKeys) == 0 && schema.XListType != nil && *schema.XListType == "map" { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-list-map-keys"), "must not be empty if x-kubernetes-list-type is map")) - } + if schema.XListType != nil && *schema.XListType == "map" { + if len(schema.XListMapKeys) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-list-map-keys"), "must not be empty if x-kubernetes-list-type is map")) + } - if schema.Items != nil && schema.Items.Schema == nil && schema.XListType != nil && *schema.XListType == "map" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items"), schema.Items, "must only have a single schema if x-kubernetes-list-type is map")) - } + if schema.Items == nil { + allErrs = append(allErrs, field.Required(fldPath.Child("items"), "must have a schema if x-kubernetes-list-type is map")) + } - if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type != "object" && schema.XListType != nil && *schema.XListType == "map" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("type"), schema.Items.Schema.Type, "must be object if parent array's x-kubernetes-list-type is map")) - } + if schema.Items != nil && schema.Items.Schema == nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("items"), schema.Items, "must only have a single schema if x-kubernetes-list-type is map")) + } - if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type == "object" && schema.XListType != nil && *schema.XListType == "map" { - keys := map[string]struct{}{} - for _, k := range schema.XListMapKeys { - if s, ok := schema.Items.Schema.Properties[k]; ok { - if s.Type == "array" || s.Type == "object" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("properties").Child(k).Child("type"), schema.Items.Schema.Type, "must be a scalar type if parent array's x-kubernetes-list-type is map")) + if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type != "object" { + allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("type"), schema.Items.Schema.Type, "must be object if parent array's x-kubernetes-list-type is map")) + } + + if schema.Items != nil && schema.Items.Schema != nil && schema.Items.Schema.Type == "object" { + keys := map[string]struct{}{} + for _, k := range schema.XListMapKeys { + if s, ok := schema.Items.Schema.Properties[k]; ok { + if s.Type == "array" || s.Type == "object" { + allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("properties").Child(k).Child("type"), schema.Items.Schema.Type, "must be a scalar type if parent array's x-kubernetes-list-type is map")) + } + } else { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "entries must all be names of item properties")) } - } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "entries must all be names of item properties")) + if _, ok := keys[k]; ok { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "must not contain duplicate entries")) + } + keys[k] = struct{}{} } - if _, ok := keys[k]; ok { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-list-map-keys"), schema.XListMapKeys, "must not contain duplicate entries")) - } - keys[k] = struct{}{} } } @@ -1142,7 +1144,7 @@ func schemaHasKubernetesExtensions(s *apiextensions.JSONSchemaProps) bool { return false } - if s.XEmbeddedResource || s.XPreserveUnknownFields != nil || s.XIntOrString { + if s.XEmbeddedResource || s.XPreserveUnknownFields != nil || s.XIntOrString || len(s.XListMapKeys) > 0 || s.XListType != nil { return true } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index e00bff9c917..0a0847dbad1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -6510,9 +6510,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { "array": { Type: "array", Nullable: true, - - // This value is defaulted - XListType: strPtr("atomic"), }, "number": { Type: "number", @@ -6594,15 +6591,6 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, wantError: true, }, - { - name: "unset list type extension with type array", - input: apiextensions.CustomResourceValidation{ - OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - Type: "array", - }, - }, - wantError: true, - }, { name: "invalid list type extension", input: apiextensions.CustomResourceValidation{ @@ -6643,6 +6631,17 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }, wantError: true, }, + { + name: "no items schema with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key"}, + }, + }, + wantError: true, + }, { name: "multiple schema items with list type extension map", input: apiextensions.CustomResourceValidation{ diff --git a/test/e2e/apimachinery/crd_publish_openapi.go b/test/e2e/apimachinery/crd_publish_openapi.go index 2179845677c..6dce7a38d59 100644 --- a/test/e2e/apimachinery/crd_publish_openapi.go +++ b/test/e2e/apimachinery/crd_publish_openapi.go @@ -35,7 +35,6 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - utildiff "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/wait" k8sclientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -508,8 +507,7 @@ func waitForDefinition(c k8sclientset.Interface, name string, schema []byte) err // drop properties and extension that we added dropDefaults(&d) if !apiequality.Semantic.DeepEqual(expect, d) { - diff := utildiff.ObjectGoPrintSideBySide(expect, d) - return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not match; expect: %v, actual: %v\n%v", name, expect, d, diff) + return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not match; expect: %v, actual: %v", name, expect, d) } } return true, "" @@ -629,7 +627,6 @@ properties: bars: description: List of Bars and their specs. type: array - x-kubernetes-list-type: atomic items: type: object required: @@ -646,7 +643,6 @@ properties: items: type: string type: array - x-kubernetes-list-type: atomic status: description: Status of Foo type: object @@ -654,7 +650,6 @@ properties: bars: description: List of Bars and their statuses. type: array - x-kubernetes-list-type: atomic items: type: object properties: @@ -686,7 +681,6 @@ properties: bars: description: List of Bars and their statuses. type: array - x-kubernetes-list-type: atomic items: type: object`) @@ -708,7 +702,6 @@ properties: bars: description: List of Bars and their statuses. type: array - x-kubernetes-list-type: atomic items: type: object`) @@ -730,6 +723,5 @@ properties: bars: description: List of Bars and their statuses. type: array - x-kubernetes-list-type: atomic items: type: object`)