From 5bac4c16b68c68646b0923ba27e9a46bb62b1198 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 19 Apr 2024 12:58:52 -0700 Subject: [PATCH] preserve errors for additionalProperties and xvalidations so the API surface stays the same Once this has had some soak time we can allow CRDs to use these abilities --- .../pkg/apiserver/schema/complete.go | 38 +++++--- .../pkg/apiserver/schema/validation.go | 75 ++++++++++++---- .../pkg/apiserver/schema/validation_test.go | 86 ++++++++++++++++++- 3 files changed, 165 insertions(+), 34 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/complete.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/complete.go index 08e222f0d0e..a6a81c12c73 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/complete.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/complete.go @@ -24,15 +24,15 @@ import ( // validateStructuralCompleteness checks that every specified field or array in s is also specified // outside of value validation. -func validateStructuralCompleteness(s *Structural, fldPath *field.Path) field.ErrorList { +func validateStructuralCompleteness(s *Structural, fldPath *field.Path, opts ValidationOptions) field.ErrorList { if s == nil { return nil } - return validateValueValidationCompleteness(s.ValueValidation, s, fldPath, fldPath) + return validateValueValidationCompleteness(s.ValueValidation, s, fldPath, fldPath, opts) } -func validateValueValidationCompleteness(v *ValueValidation, s *Structural, sPath, vPath *field.Path) field.ErrorList { +func validateValueValidationCompleteness(v *ValueValidation, s *Structural, sPath, vPath *field.Path, opts ValidationOptions) field.ErrorList { if v == nil { return nil } @@ -42,21 +42,21 @@ func validateValueValidationCompleteness(v *ValueValidation, s *Structural, sPat allErrs := field.ErrorList{} - allErrs = append(allErrs, validateNestedValueValidationCompleteness(v.Not, s, sPath, vPath.Child("not"))...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(v.Not, s, sPath, vPath.Child("not"), opts)...) for i := range v.AllOf { - allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.AllOf[i], s, sPath, vPath.Child("allOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.AllOf[i], s, sPath, vPath.Child("allOf").Index(i), opts)...) } for i := range v.AnyOf { - allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.AnyOf[i], s, sPath, vPath.Child("anyOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.AnyOf[i], s, sPath, vPath.Child("anyOf").Index(i), opts)...) } for i := range v.OneOf { - allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.OneOf[i], s, sPath, vPath.Child("oneOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(&v.OneOf[i], s, sPath, vPath.Child("oneOf").Index(i), opts)...) } return allErrs } -func validateNestedValueValidationCompleteness(v *NestedValueValidation, s *Structural, sPath, vPath *field.Path) field.ErrorList { +func validateNestedValueValidationCompleteness(v *NestedValueValidation, s *Structural, sPath, vPath *field.Path, opts ValidationOptions) field.ErrorList { if v == nil { return nil } @@ -66,17 +66,29 @@ func validateNestedValueValidationCompleteness(v *NestedValueValidation, s *Stru allErrs := field.ErrorList{} - allErrs = append(allErrs, validateValueValidationCompleteness(&v.ValueValidation, s, sPath, vPath)...) - allErrs = append(allErrs, validateNestedValueValidationCompleteness(v.Items, s.Items, sPath.Child("items"), vPath.Child("items"))...) + allErrs = append(allErrs, validateValueValidationCompleteness(&v.ValueValidation, s, sPath, vPath, opts)...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(v.Items, s.Items, sPath.Child("items"), vPath.Child("items"), opts)...) + + var additionalPropertiesSchema *Structural + if s.AdditionalProperties != nil && s.AdditionalProperties.Structural != nil { + additionalPropertiesSchema = s.AdditionalProperties.Structural + } + for k, vFld := range v.Properties { if sFld, ok := s.Properties[k]; !ok { - allErrs = append(allErrs, field.Required(sPath.Child("properties").Key(k), fmt.Sprintf("because it is defined in %s", vPath.Child("properties").Key(k)))) + if additionalPropertiesSchema == nil || !opts.AllowValidationPropertiesWithAdditionalProperties { + allErrs = append(allErrs, field.Required(sPath.Child("properties").Key(k), fmt.Sprintf("because it is defined in %s", vPath.Child("properties").Key(k)))) + } else { + allErrs = append(allErrs, validateNestedValueValidationCompleteness(&vFld, additionalPropertiesSchema, sPath.Child("additionalProperties"), vPath.Child("properties").Key(k), opts)...) + } } else { - allErrs = append(allErrs, validateNestedValueValidationCompleteness(&vFld, &sFld, sPath.Child("properties").Key(k), vPath.Child("properties").Key(k))...) + allErrs = append(allErrs, validateNestedValueValidationCompleteness(&vFld, &sFld, sPath.Child("properties").Key(k), vPath.Child("properties").Key(k), opts)...) } } - // don't check additionalProperties as this is not allowed (and checked during validation) + if v.AdditionalProperties != nil && opts.AllowNestedAdditionalProperties { + allErrs = append(allErrs, validateNestedValueValidationCompleteness(v.AdditionalProperties, s.AdditionalProperties.Structural, sPath.Child("additionalProperties"), vPath.Child("additionalProperties"), opts)...) + } return allErrs } 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 9fbacf434e4..9f904efedb3 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 @@ -42,6 +42,22 @@ const ( fieldLevel ) +type ValidationOptions struct { + // AllowNestedAdditionalProperties allows additionalProperties to be specified in + // nested contexts (allOf, anyOf, oneOf, not). + AllowNestedAdditionalProperties bool + + // AllowNestedXValidations allows x-kubernetes-validations to be specified in + // nested contexts (allOf, anyOf, oneOf, not). + AllowNestedXValidations bool + + // AllowValidationPropertiesWithAdditionalProperties allows + // value validations on specific properties that are structually + // defined by additionalProperties. i.e. additionalProperties in main structural + // schema, but properties within an allOf. + AllowValidationPropertiesWithAdditionalProperties bool +} + // ValidateStructural checks that s is a structural schema with the invariants: // // * structurality: both `ForbiddenGenerics` and `ForbiddenExtensions` only have zero values, with the two exceptions for IntOrString. @@ -61,10 +77,21 @@ const ( // * 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(fldPath *field.Path, s *Structural) field.ErrorList { + return ValidateStructuralWithOptions(fldPath, s, ValidationOptions{ + // This widens the schema for CRDs, so first few releases will still + // not admit any. But it can still be used by libraries and + // declarative validation for native types + AllowNestedAdditionalProperties: false, + AllowNestedXValidations: false, + AllowValidationPropertiesWithAdditionalProperties: false, + }) +} + +func ValidateStructuralWithOptions(fldPath *field.Path, s *Structural, opts ValidationOptions) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateStructuralInvariants(s, rootLevel, fldPath)...) - allErrs = append(allErrs, validateStructuralCompleteness(s, fldPath)...) + allErrs = append(allErrs, validateStructuralInvariants(s, rootLevel, fldPath, opts)...) + allErrs = append(allErrs, validateStructuralCompleteness(s, fldPath, opts)...) // sort error messages. Otherwise, the errors slice will change every time due to // maps in the types and randomized iteration. @@ -76,7 +103,7 @@ func ValidateStructural(fldPath *field.Path, s *Structural) field.ErrorList { } // validateStructuralInvariants checks the invariants of a structural schema. -func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) field.ErrorList { +func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path, opts ValidationOptions) field.ErrorList { if s == nil { return nil } @@ -86,10 +113,10 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) if s.Type == "array" && s.Items == nil { allErrs = append(allErrs, field.Required(fldPath.Child("items"), "must be specified")) } - allErrs = append(allErrs, validateStructuralInvariants(s.Items, itemLevel, fldPath.Child("items"))...) + allErrs = append(allErrs, validateStructuralInvariants(s.Items, itemLevel, fldPath.Child("items"), opts)...) for k, v := range s.Properties { - allErrs = append(allErrs, validateStructuralInvariants(&v, fieldLevel, fldPath.Child("properties").Key(k))...) + allErrs = append(allErrs, validateStructuralInvariants(&v, fieldLevel, fldPath.Child("properties").Key(k), opts)...) } if s.AdditionalProperties != nil { @@ -97,7 +124,7 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "must not be used at the root")) } if s.AdditionalProperties.Structural != nil { - allErrs = append(allErrs, validateStructuralInvariants(s.AdditionalProperties.Structural, fieldLevel, fldPath.Child("additionalProperties"))...) + allErrs = append(allErrs, validateStructuralInvariants(s.AdditionalProperties.Structural, fieldLevel, fldPath.Child("additionalProperties"), opts)...) } } @@ -116,7 +143,7 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) skipAnyOf := isIntOrStringAnyOfPattern(s) skipFirstAllOfAnyOf := isIntOrStringAllOfPattern(s) - allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, lvl, fldPath)...) + allErrs = append(allErrs, validateValueValidation(s.ValueValidation, skipAnyOf, skipFirstAllOfAnyOf, lvl, fldPath, opts)...) checkMetadata := (lvl == rootLevel) || s.XEmbeddedResource @@ -235,16 +262,23 @@ 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, lvl level, fldPath *field.Path) field.ErrorList { +func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf bool, lvl level, fldPath *field.Path, opts ValidationOptions) field.ErrorList { if v == nil { return nil } allErrs := field.ErrorList{} + // We still unconditionally forbid XValidations in quantifiers, the only + // quantifier that is allowed to have right now is AllOf. This is due to + // implementation constraints - the SchemaValidator would need to become + // aware of CEL to properly implement the other quantifiers. + optsWithCELDisabled := opts + optsWithCELDisabled.AllowNestedXValidations = false + if !skipAnyOf { for i := range v.AnyOf { - allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, lvl, fldPath.Child("anyOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidation(&v.AnyOf[i], false, false, lvl, fldPath.Child("anyOf").Index(i), optsWithCELDisabled)...) } } @@ -253,14 +287,14 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf if skipFirstAllOfAnyOf && i == 0 { skipAnyOf = true } - allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, lvl, fldPath.Child("allOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidation(&v.AllOf[i], skipAnyOf, false, lvl, fldPath.Child("allOf").Index(i), opts)...) } for i := range v.OneOf { - allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, lvl, fldPath.Child("oneOf").Index(i))...) + allErrs = append(allErrs, validateNestedValueValidation(&v.OneOf[i], false, false, lvl, fldPath.Child("oneOf").Index(i), optsWithCELDisabled)...) } - allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, lvl, fldPath.Child("not"))...) + allErrs = append(allErrs, validateNestedValueValidation(v.Not, false, false, lvl, fldPath.Child("not"), optsWithCELDisabled)...) if len(v.Pattern) > 0 { if _, err := regexp.Compile(v.Pattern); err != nil { @@ -272,24 +306,28 @@ func validateValueValidation(v *ValueValidation, skipAnyOf, skipFirstAllOfAnyOf } // validateNestedValueValidation checks the nested value validation under a logic junctor in a structural schema. -func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, lvl level, fldPath *field.Path) field.ErrorList { +func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllOfAnyOf bool, lvl level, fldPath *field.Path, opts ValidationOptions) field.ErrorList { if v == nil { return nil } allErrs := field.ErrorList{} - allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, lvl, fldPath)...) - allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, lvl, fldPath.Child("items"))...) - allErrs = append(allErrs, validateNestedValueValidation(v.AdditionalProperties, false, false, lvl, fldPath.Child("additionalProperties"))...) + allErrs = append(allErrs, validateValueValidation(&v.ValueValidation, skipAnyOf, skipAllOfAnyOf, lvl, fldPath, opts)...) + allErrs = append(allErrs, validateNestedValueValidation(v.Items, false, false, lvl, fldPath.Child("items"), opts)...) for k, fld := range v.Properties { - allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fieldLevel, fldPath.Child("properties").Key(k))...) + allErrs = append(allErrs, validateNestedValueValidation(&fld, false, false, fieldLevel, fldPath.Child("properties").Key(k), opts)...) } if len(v.ForbiddenGenerics.Type) > 0 { allErrs = append(allErrs, field.Forbidden(fldPath.Child("type"), "must be empty to be structural")) } + if v.AdditionalProperties != nil && !opts.AllowNestedAdditionalProperties { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "must be undefined to be structural")) + } else { + allErrs = append(allErrs, validateNestedValueValidation(v.AdditionalProperties, false, false, lvl, fldPath.Child("additionalProperties"), opts)...) + } if v.ForbiddenGenerics.Default.Object != nil { allErrs = append(allErrs, field.Forbidden(fldPath.Child("default"), "must be undefined to be structural")) } @@ -321,6 +359,9 @@ func validateNestedValueValidation(v *NestedValueValidation, skipAnyOf, skipAllO if v.ForbiddenExtensions.XMapType != nil { allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-map-type"), "must be undefined to be structural")) } + if len(v.ValidationExtensions.XValidations) > 0 && !opts.AllowNestedXValidations { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations"), "must be empty to be structural")) + } // forbid reasoning about metadata because it can lead to metadata restriction we don't want if _, found := v.Properties["metadata"]; found { 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 7b3595f8cb9..3a787999d39 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 @@ -21,6 +21,7 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" fuzz "github.com/google/gofuzz" ) @@ -146,6 +147,52 @@ func TestValidateStructuralMetadataInvariants(t *testing.T) { } } +func TestValidateStructuralCompleteness(t *testing.T) { + sch := Structural{ + AdditionalProperties: &StructuralOrBool{ + Structural: &Structural{ + Generic: Generic{ + Type: "object", + }, + Properties: map[string]Structural{ + "bar": { + Generic: Generic{ + Type: "string", + }, + }, + }, + }, + }, + ValueValidation: &ValueValidation{ + AllOf: []NestedValueValidation{ + { + Properties: map[string]NestedValueValidation{ + "foo": { + ValueValidation: ValueValidation{ + MinLength: ptr.To[int64](2), + }, + }, + }, + }, + }, + }, + } + + for _, allowedPropertiesForAdditionalProperties := range []bool{false, true} { + errs := validateStructuralCompleteness(&sch, nil, ValidationOptions{ + AllowValidationPropertiesWithAdditionalProperties: allowedPropertiesForAdditionalProperties, + }) + if allowedPropertiesForAdditionalProperties { + if len(errs) != 0 { + t.Errorf("unexpected validation errors for: %#v", sch) + } + } else if len(errs) == 0 { + t.Errorf("expected validation errors for: %#v", sch) + } + } + +} + func TestValidateNestedValueValidationComplete(t *testing.T) { fuzzer := fuzz.New() fuzzer.Funcs( @@ -154,9 +201,9 @@ func TestValidateNestedValueValidationComplete(t *testing.T) { s.Object = float64(42.0) } }, - func(s **StructuralOrBool, c fuzz.Continue) { + func(s **NestedValueValidation, c fuzz.Continue) { if c.RandBool() { - *s = &StructuralOrBool{} + *s = &NestedValueValidation{} } }, ) @@ -169,7 +216,7 @@ func TestValidateNestedValueValidationComplete(t *testing.T) { x := reflect.ValueOf(&vv.ForbiddenGenerics).Elem() fuzzer.Fuzz(x.Field(i).Addr().Interface()) - errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil) + errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil, ValidationOptions{}) if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenGenerics, Generic{}) { t.Errorf("expected ForbiddenGenerics validation errors for: %#v", vv) } @@ -182,9 +229,40 @@ func TestValidateNestedValueValidationComplete(t *testing.T) { x := reflect.ValueOf(&vv.ForbiddenExtensions).Elem() fuzzer.Fuzz(x.Field(i).Addr().Interface()) - errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil) + errs := validateNestedValueValidation(vv, false, false, fieldLevel, nil, ValidationOptions{}) if len(errs) == 0 && !reflect.DeepEqual(vv.ForbiddenExtensions, Extensions{}) { t.Errorf("expected ForbiddenExtensions validation errors for: %#v", vv) } } + + for _, allowedNestedXValidations := range []bool{false, true} { + for _, allowedNestedAdditionalProperties := range []bool{false, true} { + opts := ValidationOptions{ + AllowNestedXValidations: allowedNestedXValidations, + AllowNestedAdditionalProperties: allowedNestedAdditionalProperties, + } + + vv := NestedValueValidation{} + fuzzer.Fuzz(&vv.ValidationExtensions.XValidations) + errs := validateNestedValueValidation(&vv, false, false, fieldLevel, nil, opts) + if allowedNestedXValidations { + if len(errs) != 0 { + t.Errorf("unexpected XValidations validation errors for: %#v", vv) + } + } else if len(errs) == 0 && len(vv.ValidationExtensions.XValidations) != 0 { + t.Errorf("expected XValidations validation errors for: %#v", vv) + } + + vv = NestedValueValidation{} + fuzzer.Fuzz(&vv.AdditionalProperties) + errs = validateNestedValueValidation(&vv, false, false, fieldLevel, nil, opts) + if allowedNestedAdditionalProperties { + if len(errs) != 0 { + t.Errorf("unexpected AdditionalProperties validation errors for: %#v", vv) + } + } else if len(errs) == 0 && vv.AdditionalProperties != nil { + t.Errorf("expected AdditionalProperties validation errors for: %#v", vv) + } + } + } }