Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow more fields at root of CRD schema if status is enabled #65357

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+83 −11
Diff settings

Always

Just for now

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.
  • Loading branch information...
nikhita committed Jun 22, 2018
commit b38e1a9e42777103fdd491a9f76d1aa73bb32454
@@ -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"}

This comment has been minimized.

Copy link
@sttts

sttts Jun 22, 2018

Contributor

none of these are harmful, most are useless. But for uniformity I would tend to allow all these.

This comment has been minimized.

Copy link
@nikhita

nikhita Jun 22, 2018

Author Member

most are useless.

yeah, most of them don't make sense once we have restricted the type to object as well.


func allowedAtRootSchema(field string) bool {
for _, v := range allowedFieldsAtRootSchema {
if field == v {
return true
}
}
return false
}
@@ -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
}
@@ -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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.