Skip to content

Commit

Permalink
Merge pull request kubernetes#77653 from sttts/sttts-structural-schem…
Browse files Browse the repository at this point in the history 
…a-metadata

apiextensions: disallow metadata specs other than name and generateName
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored May 14, 2019
2 parents cf8e8e4 + d74a9a9 commit fb41b7a
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 12 deletions.
46 changes: 36 additions & 10 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const (
// - ... zero or more
//
// * every specified field or array in s is also specified outside of value validation.
// * metadata at the root can only restrict the name and generateName, and not be specified at all in nested contexts.
// * additionalProperties at the root is not allowed.
func ValidateStructural(s *Structural, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down Expand Up @@ -106,7 +107,7 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
}
}

allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, fldPath)...)
allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, lvl, fldPath)...)

if s.XEmbeddedResource && s.Type != "object" {
if len(s.Type) == 0 {
Expand All @@ -129,6 +130,26 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path)
allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), s.Type, "must be object at the root"))
}

// restrict metadata schemas to name and generateName only
if metadata, found := s.Properties["metadata"]; found && lvl == rootLevel {
// metadata is a shallow copy. We can mutate it.
_, foundName := metadata.Properties["name"]
_, foundGenerateName := metadata.Properties["generateName"]
if foundName && foundGenerateName && len(metadata.Properties) == 2 {
metadata.Properties = nil
} else if (foundName || foundGenerateName) && len(metadata.Properties) == 1 {
metadata.Properties = nil
}
metadata.Type = ""
if metadata.ValueValidation == nil {
metadata.ValueValidation = &ValueValidation{}
}
if !reflect.DeepEqual(metadata, Structural{ValueValidation: &ValueValidation{}}) {
// TODO: this is actually a field.Invalid error, but we cannot do JSON serialization of metadata here to get a proper message
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not specify anything other than name and generateName, but metadata is implicitly specified"))
}
}

if s.XEmbeddedResource && !s.XPreserveUnknownFields && s.Properties == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("properties"), "must not be empty if x-kubernetes-embedded-resource is true without x-kubernetes-preserve-unknown-fields"))
}
Expand Down Expand Up @@ -171,7 +192,7 @@ func validateExtensions(x *Extensions, fldPath *field.Path) field.ErrorList {
}

// validateValueValidation checks the value validation in a structural schema.
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
if v == nil {
return nil
}
Expand All @@ -180,7 +201,7 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf

if !skipAnyOf {
for i := range v.AnyOf {
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, fldPath.Child("anyOf").Index(i))...)
allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, lvl, fldPath.Child("anyOf").Index(i))...)
}
}

Expand All @@ -189,31 +210,31 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf
if skipFirstAllOfAnyOf && i == 0 {
skipAnyOf = true
}
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, fldPath.Child("allOf").Index(i))...)
allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, lvl, fldPath.Child("allOf").Index(i))...)
}

for i := range v.OneOf {
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, fldPath.Child("oneOf").Index(i))...)
allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, lvl, fldPath.Child("oneOf").Index(i))...)
}

allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, fldPath.Child("not"))...)
allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, lvl, fldPath.Child("not"))...)

return allErrs
}

// validateNestedValueValidation checks the nested value validation under a logic junctor in a structural schema.
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, fldPath *field.Path) field.ErrorList {
func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList {
if v == nil {
return nil
}

allErrs := field.ErrorList{}

allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, fldPath)...)
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, fldPath.Child("items"))...)
allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, lvl, fldPath)...)
allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, lvl, fldPath.Child("items"))...)

for k, fld := range v.Properties {
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fldPath.Child("properties").Key(k))...)
allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fieldLevel, fldPath.Child("properties").Key(k))...)
}

if len(v.ForbiddenGenerics.Type) > 0 {
Expand Down Expand Up @@ -245,5 +266,10 @@ func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllO
allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-int-or-string"), "must be false to be structural"))
}

// forbid reasoning about metadata because it can lead to metadata restriction we don't want
if _, found := v.Properties["metadata"]; found {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not be specified in a nested context"))
}

return allErrs
}
4 changes: 2 additions & 2 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestValidateNestedValueValidationComplete(t *testing.T) {
i := rand.Intn(x.NumField())
fuzzer.Fuzz(x.Field(i).Addr().Interface())

errs := validateNestedValueValidation(vv, false, false, nil)
errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil)
if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenGenerics, Generic{}) {
t.Errorf("expected ForbiddenGenerics validation errors for: %#v", vv)
}
Expand All @@ -63,7 +63,7 @@ func TestValidateNestedValueValidationComplete(t *testing.T) {
i := rand.Intn(x.NumField())
fuzzer.Fuzz(x.Field(i).Addr().Interface())

errs := validateNestedValueValidation(vv, false, false, nil)
errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil)
if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenExtensions, Extensions{}) {
t.Errorf("expected ForbiddenExtensions validation errors for: %#v", vv)
}
Expand Down
108 changes: 108 additions & 0 deletions staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,114 @@ not:
"spec.version[v1].schema.openAPIV3Schema.properties[d]: Required value: because it is defined in spec.version[v1].schema.openAPIV3Schema.not.properties[d]",
},
},
{
desc: "metadata with non-properties",
globalSchema: `
type: object
properties:
metadata:
minimum: 42.0
`,
expectedViolations: []string{
"spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified",
"spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields",
},
},
{
desc: "metadata with other properties",
globalSchema: `
type: object
properties:
metadata:
properties:
name:
pattern: "^[a-z]+$"
labels:
type: object
maxLength: 4
`,
expectedViolations: []string{
"spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified",
"spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields",
"spec.validation.openAPIV3Schema.properties[metadata].properties[name].type: Required value: must not be empty for specified object fields",
},
},
{
desc: "metadata with name property",
globalSchema: `
type: object
properties:
metadata:
type: object
properties:
name:
type: string
pattern: "^[a-z]+$"
`,
expectedViolations: []string{},
},
{
desc: "metadata with generateName property",
globalSchema: `
type: object
properties:
metadata:
type: object
properties:
generateName:
type: string
pattern: "^[a-z]+$"
`,
expectedViolations: []string{},
},
{
desc: "metadata with name and generateName property",
globalSchema: `
type: object
properties:
metadata:
type: object
properties:
name:
type: string
pattern: "^[a-z]+$"
generateName:
type: string
pattern: "^[a-z]+$"
`,
expectedViolations: []string{},
},
{
desc: "metadata under junctors",
globalSchema: `
type: object
properties:
metadata:
type: object
properties:
name:
type: string
pattern: "^[a-z]+$"
allOf:
- properties:
metadata: {}
anyOf:
- properties:
metadata: {}
oneOf:
- properties:
metadata: {}
not:
properties:
metadata: {}
`,
expectedViolations: []string{
"spec.validation.openAPIV3Schema.anyOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
"spec.validation.openAPIV3Schema.allOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
"spec.validation.openAPIV3Schema.oneOf[0].properties[metadata]: Forbidden: must not be specified in a nested context",
"spec.validation.openAPIV3Schema.not.properties[metadata]: Forbidden: must not be specified in a nested context",
},
},
}

for i := range tests {
Expand Down

0 comments on commit fb41b7a

Please sign in to comment.