From d74a9a9da658e1cb9ae23ae0622089fe09d65564 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Thu, 9 May 2019 13:23:52 +0200 Subject: [PATCH] apiextensions: disallow metadata specs other than name and generateName --- .../pkg/apiserver/schema/validation.go | 46 ++++++-- .../pkg/apiserver/schema/validation_test.go | 4 +- .../test/integration/validation_test.go | 108 ++++++++++++++++++ 3 files changed, 146 insertions(+), 12 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go index 1e59ee183030e..7f476241d1db2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go @@ -55,6 +55,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{} @@ -99,7 +100,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 { @@ -122,6 +123,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")) } @@ -164,7 +185,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 } @@ -173,7 +194,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))...) } } @@ -182,31 +203,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 { @@ -238,5 +259,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 } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go index 619040771a10b..3067f672f9d64 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go @@ -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) } @@ -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) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 59b0f22870f15..54a30fc50f607 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -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 {