From b38e1a9e42777103fdd491a9f76d1aa73bb32454 Mon Sep 17 00:00:00 2001 From: Nikhita Raghunath Date: Fri, 22 Jun 2018 14:59:51 +0530 Subject: [PATCH] Allow more fields at root of CRD schema if status is enabled Currently, we allow only properties, required and description at the root of the CRD schema when the status subresource is enabled. We can also include some other fields, even though sometimes they might not make sense (but they don't harm). The main idea is that when validation schema for status is extracted as properties["status"], validation for status is not lost. --- .../apiextensions/validation/validation.go | 29 ++++++++- .../validation/validation_test.go | 61 +++++++++++++++++-- .../test/integration/subresources_test.go | 4 +- 3 files changed, 83 insertions(+), 11 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 1179226dd75..2177cbd7e45 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 @@ -291,7 +291,8 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext } if schema := customResourceValidation.OpenAPIV3Schema; schema != nil { - // if subresources are enabled, only "properties", "required" and "description" are allowed inside the root schema + // if the status subresource is enabled, only certain fields are allowed inside the root schema. + // these fields are chosen such that, if status is extracted as properties["status"], it's validation is not lost. if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubresources) && statusSubresourceEnabled { v := reflect.ValueOf(schema).Elem() for i := 0; i < v.NumField(); i++ { @@ -300,8 +301,19 @@ func ValidateCustomResourceDefinitionValidation(customResourceValidation *apiext continue } - if name := v.Type().Field(i).Name; name != "Properties" && name != "Required" && name != "Description" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), *schema, fmt.Sprintf(`must only have "properties", "required" or "description" at the root if the status subresource is enabled`))) + fieldName := v.Type().Field(i).Name + + // only "object" type is valid at root of the schema since validation schema for status is extracted as properties["status"] + if fieldName == "Type" { + if schema.Type != "object" { + allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema.type"), schema.Type, fmt.Sprintf(`only "object" is allowed as the type at the root of the schema if the status subresource is enabled`))) + break + } + continue + } + + if !allowedAtRootSchema(fieldName) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), *schema, fmt.Sprintf(`only %v fields are allowed at the root of the schema if the status subresource is enabled`, allowedFieldsAtRootSchema))) break } } @@ -521,3 +533,14 @@ func validateSimpleJSONPath(s string, fldPath *field.Path) field.ErrorList { return allErrs } + +var allowedFieldsAtRootSchema = []string{"Description", "Type", "Format", "Title", "Maximum", "ExclusiveMaximum", "Minimum", "ExclusiveMinimum", "MaxLength", "MinLength", "Pattern", "MaxItems", "MinItems", "UniqueItems", "MultipleOf", "Required", "Items", "Properties", "ExternalDocs", "Example"} + +func allowedAtRootSchema(field string) bool { + for _, v := range allowedFieldsAtRootSchema { + if field == v { + return true + } + } + return false +} 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 68dc8aaede5..794142b7072 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 @@ -832,32 +832,71 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { name: "root type without status", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - Type: "object", + Type: "string", }, }, statusEnabled: false, wantError: false, }, { - name: "root type with status", + name: "root type having invalid value, with status", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ - Type: "object", + Type: "string", }, }, statusEnabled: true, wantError: true, }, { - name: "properties, required and description with status", + name: "non-allowed root field with status", input: apiextensions.CustomResourceValidation{ OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + AnyOf: []apiextensions.JSONSchemaProps{ + { + Description: "First schema", + }, + { + Description: "Second schema", + }, + }, + }, + }, + statusEnabled: true, + wantError: true, + }, + { + name: "all allowed fields at the root of the schema with status", + input: apiextensions.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensions.JSONSchemaProps{ + Description: "This is a description", + Type: "object", + Format: "date-time", + Title: "This is a title", + Maximum: float64Ptr(10), + ExclusiveMaximum: true, + Minimum: float64Ptr(5), + ExclusiveMinimum: true, + MaxLength: int64Ptr(10), + MinLength: int64Ptr(5), + Pattern: "^[a-z]$", + MaxItems: int64Ptr(10), + MinItems: int64Ptr(5), + MultipleOf: float64Ptr(3), + Required: []string{"spec", "status"}, + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Description: "This is a schema nested under Items", + }, + }, Properties: map[string]apiextensions.JSONSchemaProps{ "spec": {}, "status": {}, }, - Required: []string{"spec", "status"}, - Description: "This is a description", + ExternalDocs: &apiextensions.ExternalDocumentation{ + Description: "This is an external documentation description", + }, + Example: &example, }, }, statusEnabled: true, @@ -875,3 +914,13 @@ func TestValidateCustomResourceDefinitionValidation(t *testing.T) { }) } } + +var example = apiextensions.JSON(`"This is an example"`) + +func float64Ptr(f float64) *float64 { + return &f +} + +func int64Ptr(f int64) *int64 { + return &f +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go index bce63a4a5ab..a8cf13b5879 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go @@ -321,7 +321,7 @@ func TestScaleSubresource(t *testing.T) { } } -func TestValidationSchema(t *testing.T) { +func TestValidationSchemaWithStatus(t *testing.T) { stopCh, config, err := testserver.StartDefaultServer() if err != nil { t.Fatal(err) @@ -344,7 +344,7 @@ func TestValidationSchema(t *testing.T) { } _, err = testserver.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient) if err == nil { - t.Fatalf(`unexpected non-error, expected: must only have "properties" or "required" at the root if the status subresource is enabled`) + t.Fatalf(`unexpected non-error, expected: must not have "additionalProperties" at the root of the schema if the status subresource is enabled`) } // make sure we are not restricting fields to properties even in subschemas