diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go index 06380a4e8e0..77abe9e36d0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go @@ -87,7 +87,7 @@ type JSONSchemaProps struct { // - ... zero or more XIntOrString bool - // x-kubernetes-list-map-keys annotates lists with the x-kubernetes-list-type `map` by specifying the keys used + // x-kubernetes-list-map-keys annotates an array with the x-kubernetes-list-type `map` by specifying the keys used // as the index of the map. // // This tag MUST only be used on lists that have the "x-kubernetes-list-type" @@ -95,19 +95,19 @@ type JSONSchemaProps struct { // be a scalar typed field of the child structure (no nesting is supported). XListMapKeys []string - // x-kubernetes-list-type annotates a list to further describe its topology. + // x-kubernetes-list-type annotates an array to further describe its topology. // This extension must only be used on lists and may have 3 possible values: // // 1) `atomic`: the list is treated as a single entity, like a scalar. // Atomic lists will be entirely replaced when updated. This extension // may be used on any type of list (struct, scalar, ...). // 2) `set`: - // Sets are lists that must not have multiple times the same value. Each + // Sets are lists that must not have multiple items with the same value. Each // value must be a scalar (or another atomic type). // 3) `map`: // These lists are like maps in that their elements have a non-index key // used to identify them. Order is preserved upon merge. The map tag - // must only be used on a list with struct elements. + // must only be used on a list with elements of type object. XListType *string } 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 5cebec927bd..7bf26d5c7f0 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,3 +59,47 @@ 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 7640c10c5c4..4f17503218f 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,6 +124,47 @@ 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/types_jsonschema.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go index 1d3727fed87..37d0f75b0f2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go @@ -91,27 +91,31 @@ type JSONSchemaProps struct { // - ... zero or more XIntOrString bool `json:"x-kubernetes-int-or-string,omitempty" protobuf:"bytes,40,opt,name=xKubernetesIntOrString"` - // x-kubernetes-list-map-keys annotates lists with the x-kubernetes-list-type `map` by specifying the keys used + // x-kubernetes-list-map-keys annotates an array with the x-kubernetes-list-type `map` by specifying the keys used // as the index of the map. // // This tag MUST only be used on lists that have the "x-kubernetes-list-type" // extension set to "map". Also, the values specified for this attribute must // be a scalar typed field of the child structure (no nesting is supported). + // + // +optional XListMapKeys []string `json:"x-kubernetes-list-map-keys,omitempty" protobuf:"bytes,41,rep,name=xKubernetesListMapKeys"` - // x-kubernetes-list-type annotates a list to further describe its topology. + // x-kubernetes-list-type annotates an array to further describe its topology. // This extension must only be used on lists and may have 3 possible values: // // 1) `atomic`: the list is treated as a single entity, like a scalar. // Atomic lists will be entirely replaced when updated. This extension // may be used on any type of list (struct, scalar, ...). // 2) `set`: - // Sets are lists that must not have multiple times the same value. Each + // Sets are lists that must not have multiple items with the same value. Each // value must be a scalar (or another atomic type). // 3) `map`: // These lists are like maps in that their elements have a non-index key // used to identify them. Order is preserved upon merge. The map tag - // must only be used on a list with struct elements. + // must only be used on a list with elements of type object. + // Defaults to atomic for arrays. + // +optional XListType *string `json:"x-kubernetes-list-type,omitempty" protobuf:"bytes,42,opt,name=xKubernetesListType"` } 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 1a9c2238e33..bffce311907 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,3 +80,47 @@ 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 6aad4c5eee7..dff89a77f1a 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,6 +129,34 @@ 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/types_jsonschema.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types_jsonschema.go index 9ef486747d0..90097e4e5b2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types_jsonschema.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types_jsonschema.go @@ -91,27 +91,31 @@ type JSONSchemaProps struct { // - ... zero or more XIntOrString bool `json:"x-kubernetes-int-or-string,omitempty" protobuf:"bytes,40,opt,name=xKubernetesIntOrString"` - // x-kubernetes-list-map-keys annotates lists with the x-kubernetes-list-type `map` by specifying the keys used + // x-kubernetes-list-map-keys annotates an array with the x-kubernetes-list-type `map` by specifying the keys used // as the index of the map. // // This tag MUST only be used on lists that have the "x-kubernetes-list-type" // extension set to "map". Also, the values specified for this attribute must // be a scalar typed field of the child structure (no nesting is supported). + // + // +optional XListMapKeys []string `json:"x-kubernetes-list-map-keys,omitempty" protobuf:"bytes,41,rep,name=xKubernetesListMapKeys"` - // x-kubernetes-list-type annotates a list to further describe its topology. + // x-kubernetes-list-type annotates an array to further describe its topology. // This extension must only be used on lists and may have 3 possible values: // // 1) `atomic`: the list is treated as a single entity, like a scalar. // Atomic lists will be entirely replaced when updated. This extension // may be used on any type of list (struct, scalar, ...). // 2) `set`: - // Sets are lists that must not have multiple times the same value. Each + // Sets are lists that must not have multiple items with the same value. Each // value must be a scalar (or another atomic type). // 3) `map`: // These lists are like maps in that their elements have a non-index key // used to identify them. Order is preserved upon merge. The map tag - // must only be used on a list with struct elements. + // must only be used on a list with elements of type object. + // Defaults to atomic for arrays. + // +optional XListType *string `json:"x-kubernetes-list-type,omitempty" protobuf:"bytes,42,opt,name=xKubernetesListType"` } 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 defc42e1914..17c4e24a028 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,8 +796,12 @@ 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.Invalid(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, "must be one of 'atomic', 'set', 'map', or unset")) + allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-list-type"), *schema.XListType, []string{"atomic", "set", "map"})) } if len(schema.XListMapKeys) > 0 && (schema.XListType == nil || *schema.XListType != "map") { @@ -808,6 +812,35 @@ 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.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 && 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 && 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")) + } + } 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{}{} + } + } + return allErrs } 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 11a94aea1be..e00bff9c917 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,6 +6510,9 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { "array": { Type: "array", Nullable: true, + + // This value is defaulted + XListType: strPtr("atomic"), }, "number": { Type: "number", @@ -6577,7 +6580,7 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "object", - XListType: strPtr("map"), + XListType: strPtr("set"), }, }, wantError: true, @@ -6586,13 +6589,32 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { name: "unset type with list type extension set", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - XListType: strPtr("map"), + XListType: strPtr("set"), }, }, wantError: true, }, { - name: "invalid list type extension with list map keys extension set", + 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{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("invalid"), + }, + }, + wantError: true, + }, + { + name: "invalid list type extension with list map keys extension non-empty", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "array", @@ -6603,25 +6625,142 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { wantError: true, }, { - name: "unset list type extension with list map keys extension set", + name: "unset list type extension with list map keys extension non-empty", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - Type: "array", XListMapKeys: []string{"key"}, }, }, wantError: true, }, { - name: "invalid list type", + name: "empty list map keys extension with list type extension map", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ Type: "array", - XListType: strPtr("invalid"), + XListType: strPtr("map"), }, }, wantError: true, }, + { + name: "multiple schema items with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + JSONSchemas: []apiextensions.JSONSchemaProps{ + { + Type: "string", + }, { + Type: "integer", + }, + }, + }, + }, + }, + wantError: true, + }, + { + name: "non object item with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + wantError: true, + }, + { + name: "items with key missing from properties with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + }, + }, + }, + }, + wantError: true, + }, + { + name: "items with non scalar key property type with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "key": { + Type: "object", + }, + }, + }, + }, + }, + }, + wantError: true, + }, + { + name: "duplicate map keys with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"key", "key"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "key": { + Type: "string", + }, + }, + }, + }, + }, + }, + wantError: true, + }, + { + name: "allowed schema with list type extension map", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Type: "array", + XListType: strPtr("map"), + XListMapKeys: []string{"keyA", "keyB"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensions.JSONSchemaProps{ + "keyA": { + Type: "string", + }, + "keyB": { + Type: "integer", + }, + }, + }, + }, + }, + }, + wantError: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index bf0d809fec8..23d4b236e32 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -655,7 +655,11 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd } specs = append(specs, s) } - mergedOpenAPI := builder.MergeSpecs(r.staticOpenAPISpec, specs...) + mergedOpenAPI, err := builder.MergeSpecs(r.staticOpenAPISpec, specs...) + if err != nil { + utilruntime.HandleError(err) + return nil, fmt.Errorf("the server could not properly merge the CR schema") + } openAPIModels, err = utilopenapi.ToProtoModels(mergedOpenAPI) if err != nil { utilruntime.HandleError(err) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi.go index d9bc667c66c..0db56b85637 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi.go @@ -82,7 +82,7 @@ func (x *Extensions) toGoOpenAPI(ret *spec.Schema) { ret.VendorExtensible.AddExtension("x-kubernetes-list-map-keys", x.XListMapKeys) } if x.XListType != nil { - ret.VendorExtensible.AddExtension("x-kubernetes-list-type", x.XListType) + ret.VendorExtensible.AddExtension("x-kubernetes-list-type", *x.XListType) } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go index 0d57200d086..23d09eb9ef7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/structural.go @@ -108,12 +108,12 @@ type Extensions struct { // Atomic lists will be entirely replaced when updated. This extension // may be used on any type of list (struct, scalar, ...). // 2) `set`: - // Sets are lists that must not have multiple times the same value. Each + // Sets are lists that must not have multiple items with the same value. Each // value must be a scalar (or another atomic type). // 3) `map`: // These lists are like maps in that their elements have a non-index key // used to identify them. Order is preserved upon merge. The map tag - // must only be used on a list with struct elements. + // must only be used on a list with elements of type object. XListType *string } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go index 58f72fe2a11..c5e9892e2c8 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go @@ -250,7 +250,7 @@ func ConvertJSONSchemaPropsWithPostProcess(in *apiextensions.JSONSchemaProps, ou out.VendorExtensible.AddExtension("x-kubernetes-list-map-keys", in.XListMapKeys) } if in.XListType != nil { - out.VendorExtensible.AddExtension("x-kubernetes-list-type", in.XListType) + out.VendorExtensible.AddExtension("x-kubernetes-list-type", *in.XListType) } return nil diff --git a/test/e2e/apimachinery/crd_publish_openapi.go b/test/e2e/apimachinery/crd_publish_openapi.go index 6dce7a38d59..2179845677c 100644 --- a/test/e2e/apimachinery/crd_publish_openapi.go +++ b/test/e2e/apimachinery/crd_publish_openapi.go @@ -35,6 +35,7 @@ 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" @@ -507,7 +508,8 @@ 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) { - return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not match; expect: %v, actual: %v", name, 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 true, "" @@ -627,6 +629,7 @@ properties: bars: description: List of Bars and their specs. type: array + x-kubernetes-list-type: atomic items: type: object required: @@ -643,6 +646,7 @@ properties: items: type: string type: array + x-kubernetes-list-type: atomic status: description: Status of Foo type: object @@ -650,6 +654,7 @@ properties: bars: description: List of Bars and their statuses. type: array + x-kubernetes-list-type: atomic items: type: object properties: @@ -681,6 +686,7 @@ properties: bars: description: List of Bars and their statuses. type: array + x-kubernetes-list-type: atomic items: type: object`) @@ -702,6 +708,7 @@ properties: bars: description: List of Bars and their statuses. type: array + x-kubernetes-list-type: atomic items: type: object`) @@ -723,5 +730,6 @@ properties: bars: description: List of Bars and their statuses. type: array + x-kubernetes-list-type: atomic items: type: object`) diff --git a/test/integration/apiserver/apply/apply_crd_test.go b/test/integration/apiserver/apply/apply_crd_test.go index ccb56cbaeae..83796feaa51 100644 --- a/test/integration/apiserver/apply/apply_crd_test.go +++ b/test/integration/apiserver/apply/apply_crd_test.go @@ -19,12 +19,14 @@ package apiserver import ( "encoding/json" "fmt" + "reflect" "testing" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/apiextensions-apiserver/test/integration/fixtures" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" genericfeatures "k8s.io/apiserver/pkg/features" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -83,6 +85,7 @@ spec: if err != nil { t.Fatalf("failed to create custom resource with apply: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 1) // Patch object to change the number of replicas result, err = rest.Patch(types.MergePatchType). @@ -93,6 +96,7 @@ spec: if err != nil { t.Fatalf("failed to update number of replicas with merge patch: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 5) // Re-apply, we should get conflicts now, since the number of replicas was changed. result, err = rest.Patch(types.ApplyPatchType). @@ -108,8 +112,8 @@ spec: if !ok { t.Fatalf("Expecting to get conflicts as API error") } - if len(status.Status().Details.Causes) < 1 { - t.Fatalf("Expecting to get at least one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) + if len(status.Status().Details.Causes) != 1 { + t.Fatalf("Expecting to get one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) } // Re-apply with force, should work fine. @@ -123,6 +127,7 @@ spec: if err != nil { t.Fatalf("failed to apply object with force after updating replicas: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 1) } // TestApplyCRDStructuralSchema tests that when a CRD has a structural schema in its validation field, @@ -239,27 +244,38 @@ spec: if err != nil { t.Fatalf("failed to create custom resource with apply: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 1) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyReplicas(t, result, 1) + verifyNumPorts(t, result, 1) // Patch object to add another finalizer to the finalizers list result, err = rest.Patch(types.MergePatchType). AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). Name(name). - Body([]byte(`{"metadata":{"finalizers":["another-one"]}}`)). + Body([]byte(`{"metadata":{"finalizers":["test-finalizer","another-one"]}}`)). DoRaw() if err != nil { t.Fatalf("failed to add finalizer with merge patch: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 2) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyFinalizersIncludes(t, result, "another-one") // Re-apply the same config, should work fine, since finalizers should have the list-type extension 'set'. result, err = rest.Patch(types.ApplyPatchType). AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). Name(name). Param("fieldManager", "apply_test"). + SetHeader("Accept", "application/json"). Body(yamlBody). DoRaw() if err != nil { t.Fatalf("failed to apply same config after adding a finalizer: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 2) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyFinalizersIncludes(t, result, "another-one") // Patch object to change the number of replicas result, err = rest.Patch(types.MergePatchType). @@ -270,6 +286,7 @@ spec: if err != nil { t.Fatalf("failed to update number of replicas with merge patch: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 5) // Re-apply, we should get conflicts now, since the number of replicas was changed. result, err = rest.Patch(types.ApplyPatchType). @@ -285,8 +302,8 @@ spec: if !ok { t.Fatalf("Expecting to get conflicts as API error") } - if len(status.Status().Details.Causes) < 1 { - t.Fatalf("Expecting to get at least one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) + if len(status.Status().Details.Causes) != 1 { + t.Fatalf("Expecting to get one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) } // Re-apply with force, should work fine. @@ -300,6 +317,7 @@ spec: if err != nil { t.Fatalf("failed to apply object with force after updating replicas: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 1) // New applier tries to edit an existing list item, we should get conflicts. result, err = rest.Patch(types.ApplyPatchType). @@ -324,8 +342,8 @@ spec: if !ok { t.Fatalf("Expecting to get conflicts as API error") } - if len(status.Status().Details.Causes) < 1 { - t.Fatalf("Expecting to get at least one conflict when a different applier updates existing list item, got: %v", status.Status().Details.Causes) + if len(status.Status().Details.Causes) != 1 { + t.Fatalf("Expecting to get one conflict when a different applier updates existing list item, got: %v", status.Status().Details.Causes) } // New applier tries to add a new list item, should work fine. @@ -343,10 +361,12 @@ spec: - name: "y" containerPort: 8080 protocol: TCP`, apiVersion, kind, name))). + SetHeader("Accept", "application/json"). DoRaw() if err != nil { t.Fatalf("failed to add a new list item to the object as a different applier: %v:\n%v", err, string(result)) } + verifyNumPorts(t, result, 2) } // TestApplyCRDNonStructuralSchema tests that when a CRD has a non-structural schema in its validation field, @@ -430,27 +450,37 @@ spec: if err != nil { t.Fatalf("failed to create custom resource with apply: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 1) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyReplicas(t, result, 1.0) // Patch object to add another finalizer to the finalizers list result, err = rest.Patch(types.MergePatchType). AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). Name(name). - Body([]byte(`{"metadata":{"finalizers":["another-one"]}}`)). + Body([]byte(`{"metadata":{"finalizers":["test-finalizer","another-one"]}}`)). DoRaw() if err != nil { t.Fatalf("failed to add finalizer with merge patch: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 2) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyFinalizersIncludes(t, result, "another-one") // Re-apply the same config, should work fine, since finalizers should have the list-type extension 'set'. result, err = rest.Patch(types.ApplyPatchType). AbsPath("/apis", noxuDefinition.Spec.Group, noxuDefinition.Spec.Version, noxuDefinition.Spec.Names.Plural). Name(name). Param("fieldManager", "apply_test"). + SetHeader("Accept", "application/json"). Body(yamlBody). DoRaw() if err != nil { t.Fatalf("failed to apply same config after adding a finalizer: %v:\n%v", err, string(result)) } + verifyNumFinalizers(t, result, 2) + verifyFinalizersIncludes(t, result, "test-finalizer") + verifyFinalizersIncludes(t, result, "another-one") // Patch object to change the number of replicas result, err = rest.Patch(types.MergePatchType). @@ -461,6 +491,7 @@ spec: if err != nil { t.Fatalf("failed to update number of replicas with merge patch: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 5.0) // Re-apply, we should get conflicts now, since the number of replicas was changed. result, err = rest.Patch(types.ApplyPatchType). @@ -476,8 +507,8 @@ spec: if !ok { t.Fatalf("Expecting to get conflicts as API error") } - if len(status.Status().Details.Causes) < 1 { - t.Fatalf("Expecting to get at least one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) + if len(status.Status().Details.Causes) != 1 { + t.Fatalf("Expecting to get one conflict when applying object after updating replicas, got: %v", status.Status().Details.Causes) } // Re-apply with force, should work fine. @@ -491,4 +522,88 @@ spec: if err != nil { t.Fatalf("failed to apply object with force after updating replicas: %v:\n%v", err, string(result)) } + verifyReplicas(t, result, 1.0) +} + +// verifyNumFinalizers checks that len(.metadata.finalizers) == n +func verifyNumFinalizers(t *testing.T, b []byte, n int) { + obj := unstructured.Unstructured{} + err := obj.UnmarshalJSON(b) + if err != nil { + t.Fatalf("failed to unmarshal response: %v", err) + } + if actual, expected := len(obj.GetFinalizers()), n; actual != expected { + t.Fatalf("expected %v finalizers but got %v:\n%v", expected, actual, string(b)) + } +} + +// verifyFinalizersIncludes checks that .metadata.finalizers includes e +func verifyFinalizersIncludes(t *testing.T, b []byte, e string) { + obj := unstructured.Unstructured{} + err := obj.UnmarshalJSON(b) + if err != nil { + t.Fatalf("failed to unmarshal response: %v", err) + } + for _, a := range obj.GetFinalizers() { + if a == e { + return + } + } + t.Fatalf("expected finalizers to include %q but got: %v", e, obj.GetFinalizers()) +} + +// verifyReplicas checks that .spec.replicas == r +func verifyReplicas(t *testing.T, b []byte, r int) { + obj := unstructured.Unstructured{} + err := obj.UnmarshalJSON(b) + if err != nil { + t.Fatalf("failed to find replicas number in response: %v:\n%v", err, string(b)) + } + spec, ok := obj.Object["spec"] + if !ok { + t.Fatalf("failed to find replicas number in response:\n%v", string(b)) + } + specMap, ok := spec.(map[string]interface{}) + if !ok { + t.Fatalf("failed to find replicas number in response:\n%v", string(b)) + } + replicas, ok := specMap["replicas"] + if !ok { + t.Fatalf("failed to find replicas number in response:\n%v", string(b)) + } + replicasNumber, ok := replicas.(int64) + if !ok { + t.Fatalf("failed to find replicas number in response: expected int64 but got: %v", reflect.TypeOf(replicas)) + } + if actual, expected := replicasNumber, int64(r); actual != expected { + t.Fatalf("expected %v ports but got %v:\n%v", expected, actual, string(b)) + } +} + +// verifyNumPorts checks that len(.spec.ports) == n +func verifyNumPorts(t *testing.T, b []byte, n int) { + obj := unstructured.Unstructured{} + err := obj.UnmarshalJSON(b) + if err != nil { + t.Fatalf("failed to find ports list in response: %v:\n%v", err, string(b)) + } + spec, ok := obj.Object["spec"] + if !ok { + t.Fatalf("failed to find ports list in response:\n%v", string(b)) + } + specMap, ok := spec.(map[string]interface{}) + if !ok { + t.Fatalf("failed to find ports list in response:\n%v", string(b)) + } + ports, ok := specMap["ports"] + if !ok { + t.Fatalf("failed to find ports list in response:\n%v", string(b)) + } + portsList, ok := ports.([]interface{}) + if !ok { + t.Fatalf("failed to find ports list in response: expected array but got: %v", reflect.TypeOf(ports)) + } + if actual, expected := len(portsList), n; actual != expected { + t.Fatalf("expected %v ports but got %v:\n%v", expected, actual, string(b)) + } }