From efc67b46b64e02cf686cd0382a6f53d94b799acd Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Fri, 3 Nov 2023 15:22:07 -0700 Subject: [PATCH] ratcheting-cel: use Optional[T] for oldSelf when optionalOldSelf is true Kubernetes-commit: eef15158152702c6315b1959fc3c28d08087dc26 --- pkg/apiserver/schema/cel/compilation.go | 42 +- pkg/apiserver/schema/cel/compilation_test.go | 91 +++++ pkg/apiserver/schema/cel/validation.go | 56 ++- pkg/apiserver/schema/cel/validation_test.go | 399 +++++++++++++++++++ test/integration/ratcheting_test.go | 44 ++ 5 files changed, 616 insertions(+), 16 deletions(-) diff --git a/pkg/apiserver/schema/cel/compilation.go b/pkg/apiserver/schema/cel/compilation.go index da6494e53..b5035eadb 100644 --- a/pkg/apiserver/schema/cel/compilation.go +++ b/pkg/apiserver/schema/cel/compilation.go @@ -23,6 +23,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/checker" + "github.com/google/cel-go/common/types" apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" @@ -126,7 +127,7 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit } celRules := s.Extensions.XValidations - envSet, err := prepareEnvSet(baseEnvSet, declType) + oldSelfEnvSet, optionalOldSelfEnvSet, err := prepareEnvSet(baseEnvSet, declType) if err != nil { return nil, err } @@ -135,15 +136,20 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit compResults := make([]CompilationResult, len(celRules)) maxCardinality := maxCardinality(declType.MinSerializedSize) for i, rule := range celRules { - compResults[i] = compileRule(s, rule, envSet, envLoader, estimator, maxCardinality, perCallLimit) + ruleEnvSet := oldSelfEnvSet + if rule.OptionalOldSelf != nil && *rule.OptionalOldSelf { + ruleEnvSet = optionalOldSelfEnvSet + } + compResults[i] = compileRule(s, rule, ruleEnvSet, envLoader, estimator, maxCardinality, perCallLimit) } return compResults, nil } -func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (*environment.EnvSet, error) { +func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (oldSelfEnvSet *environment.EnvSet, optionalOldSelfEnvSet *environment.EnvSet, err error) { scopedType := declType.MaybeAssignTypeName(generateUniqueSelfTypeName()) - return baseEnvSet.Extend( + + oldSelfEnvSet, err = baseEnvSet.Extend( environment.VersionedOptions{ // Feature epoch was actually 1.23, but we artificially set it to 1.0 because these // options should always be present. @@ -162,6 +168,34 @@ func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclTy }, }, ) + if err != nil { + return nil, nil, err + } + + optionalOldSelfEnvSet, err = baseEnvSet.Extend( + environment.VersionedOptions{ + // Feature epoch was actually 1.23, but we artificially set it to 1.0 because these + // options should always be present. + IntroducedVersion: version.MajorMinor(1, 0), + EnvOptions: []cel.EnvOption{ + cel.Variable(ScopedVarName, scopedType.CelType()), + }, + DeclTypes: []*apiservercel.DeclType{ + scopedType, + }, + }, + environment.VersionedOptions{ + IntroducedVersion: version.MajorMinor(1, 24), + EnvOptions: []cel.EnvOption{ + cel.Variable(OldScopedVarName, types.NewOptionalType(scopedType.CelType())), + }, + }, + ) + if err != nil { + return nil, nil, err + } + + return oldSelfEnvSet, optionalOldSelfEnvSet, nil } func compileRule(s *schema.Structural, rule apiextensions.ValidationRule, envSet *environment.EnvSet, envLoader EnvLoader, estimator *library.CostEstimator, maxCardinality uint64, perCallLimit uint64) (compilationResult CompilationResult) { diff --git a/pkg/apiserver/schema/cel/compilation_test.go b/pkg/apiserver/schema/cel/compilation_test.go index 18df5a424..f76d71a07 100644 --- a/pkg/apiserver/schema/cel/compilation_test.go +++ b/pkg/apiserver/schema/cel/compilation_test.go @@ -29,10 +29,14 @@ import ( apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" "k8s.io/apimachinery/pkg/util/version" celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/cel" "k8s.io/apiserver/pkg/cel/environment" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/ptr" ) const ( @@ -151,12 +155,99 @@ func (v transitionRuleMatcher) String() string { } func TestCelCompilation(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)() cases := []struct { name string input schema.Structural expectedResults []validationMatcher unmodified bool }{ + { + name: "optional primitive transition rule type checking", + input: schema.Structural{ + Generic: schema.Generic{ + Type: "integer", + }, + Extensions: schema.Extensions{ + XValidations: apiextensions.ValidationRules{ + {Rule: "self >= oldSelf.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self >= oldSelf.orValue(1)", OptionalOldSelf: ptr.To(true)}, + {Rule: "oldSelf.hasValue() ? self >= oldSelf.value() : true", OptionalOldSelf: ptr.To(true)}, + {Rule: "self >= oldSelf", OptionalOldSelf: ptr.To(true)}, + {Rule: "self >= oldSelf.orValue('')", OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + expectedResults: []validationMatcher{ + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(invalidError("optional")), + matchesAll(invalidError("orValue")), + }, + }, + { + name: "optional complex transition rule type checking", + input: schema.Structural{ + Generic: schema.Generic{ + Type: "object", + }, + Properties: map[string]schema.Structural{ + "i": {Generic: schema.Generic{Type: "integer"}}, + "b": {Generic: schema.Generic{Type: "boolean"}}, + "s": {Generic: schema.Generic{Type: "string"}}, + "a": { + Generic: schema.Generic{Type: "array"}, + Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}}, + }, + "o": { + Generic: schema.Generic{Type: "object"}, + Properties: map[string]schema.Structural{ + "i": {Generic: schema.Generic{Type: "integer"}}, + "b": {Generic: schema.Generic{Type: "boolean"}}, + "s": {Generic: schema.Generic{Type: "string"}}, + "a": { + Generic: schema.Generic{Type: "array"}, + Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}}, + }, + "o": { + Generic: schema.Generic{Type: "object"}, + }, + }, + }, + }, + Extensions: schema.Extensions{ + XValidations: apiextensions.ValidationRules{ + {Rule: "self.i >= oldSelf.i.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.s == oldSelf.s.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.b == oldSelf.b.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o == oldSelf.o.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.i >= oldSelf.o.i.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.s == oldSelf.o.s.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.b == oldSelf.o.b.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.o == oldSelf.o.o.value()", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.i >= oldSelf.o.i.orValue(1)", OptionalOldSelf: ptr.To(true)}, + {Rule: "oldSelf.hasValue() ? self.o.i >= oldSelf.o.i.value() : true", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.i >= oldSelf.o.i", OptionalOldSelf: ptr.To(true)}, + {Rule: "self.o.i >= oldSelf.o.s.orValue(0)", OptionalOldSelf: ptr.To(true)}, + }, + }, + }, + expectedResults: []validationMatcher{ + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(noError(), transitionRule(true)), + matchesAll(invalidError("optional")), + matchesAll(invalidError("orValue")), + }, + }, { name: "valid object", input: schema.Structural{ diff --git a/pkg/apiserver/schema/cel/validation.go b/pkg/apiserver/schema/cel/validation.go index 968538e37..0710bead7 100644 --- a/pkg/apiserver/schema/cel/validation.go +++ b/pkg/apiserver/schema/cel/validation.go @@ -32,15 +32,18 @@ import ( "github.com/google/cel-go/interpreter" "k8s.io/klog/v2" + "k8s.io/utils/ptr" apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model" + "k8s.io/apiextensions-apiserver/pkg/features" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/cel" "k8s.io/apiserver/pkg/cel/common" "k8s.io/apiserver/pkg/cel/environment" "k8s.io/apiserver/pkg/cel/metrics" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/apiserver/pkg/warning" celconfig "k8s.io/apiserver/pkg/apis/cel" @@ -65,9 +68,10 @@ type Validator struct { // custom resource being validated, or the root of an XEmbeddedResource object. isResourceRoot bool - // celActivationFactory produces an Activation, which resolves identifiers (e.g. self and - // oldSelf) to CEL values. - celActivationFactory func(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation + // celActivationFactory produces a Activations, which resolve identifiers + // (e.g. self and oldSelf) to CEL values. One activation must be produced + // for each of the cases when oldSelf is optional and non-optional. + celActivationFactory func(sts *schema.Structural, obj, oldObj interface{}) (activation interpreter.Activation, optionalOldSelfActivation interpreter.Activation) } // NewValidator returns compiles all the CEL programs defined in x-kubernetes-validations extensions @@ -122,7 +126,7 @@ func validator(s *schema.Structural, isResourceRoot bool, declType *cel.DeclType additionalPropertiesValidator = validator(s.AdditionalProperties.Structural, s.AdditionalProperties.Structural.XEmbeddedResource, declType.ElemType, perCallLimit) } if len(compiledRules) > 0 || err != nil || itemsValidator != nil || additionalPropertiesValidator != nil || len(propertiesValidators) > 0 { - var activationFactory func(*schema.Structural, interface{}, interface{}) interpreter.Activation = validationActivationWithoutOldSelf + activationFactory := validationActivationWithoutOldSelf for _, rule := range compiledRules { if rule.UsesOldSelf { activationFactory = validationActivationWithOldSelf @@ -289,7 +293,7 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path if s.isResourceRoot { sts = model.WithTypeAndObjectMeta(sts) } - activation := s.celActivationFactory(sts, obj, oldObj) + activation, optionalOldSelfActivation := s.celActivationFactory(sts, obj, oldObj) for i, compiled := range s.compiledRules { rule := sts.XValidations[i] if compiled.Error != nil { @@ -300,11 +304,29 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path // rule is empty continue } + + // If ratcheting is enabled, allow rule with oldSelf to evaluate + // when `optionalOldSelf` is set to true + optionalOldSelfRule := ptr.Deref(rule.OptionalOldSelf, false) if compiled.UsesOldSelf && oldObj == nil { // transition rules are evaluated only if there is a comparable existing value - continue + // But if the rule uses optional oldSelf and gate is enabled we allow + // the rule to be evaluated + if !utilfeature.DefaultFeatureGate.Enabled(features.CRDValidationRatcheting) { + continue + } + + if !optionalOldSelfRule { + continue + } + } + + ruleActivation := activation + if optionalOldSelfRule { + ruleActivation = optionalOldSelfActivation } - evalResult, evalDetails, err := compiled.Program.ContextEval(ctx, activation) + + evalResult, evalDetails, err := compiled.Program.ContextEval(ctx, ruleActivation) if evalDetails == nil { errs = append(errs, field.InternalError(fldPath, fmt.Errorf("runtime cost could not be calculated for validation rule: %v, no further validation rules will be run", ruleErrorString(rule)))) return errs, -1 @@ -622,21 +644,31 @@ type validationActivation struct { hasOldSelf bool } -func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation { +func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) (activation interpreter.Activation, optionalOldSelfActivation interpreter.Activation) { va := &validationActivation{ self: UnstructuredToVal(obj, sts), } + optionalVA := &validationActivation{ + self: va.self, + hasOldSelf: true, // this means the oldSelf variable is defined for CEL to reference, not that it has a value + oldSelf: types.OptionalNone, + } + if oldObj != nil { va.oldSelf = UnstructuredToVal(oldObj, sts) // +k8s:verify-mutation:reason=clone - va.hasOldSelf = true // +k8s:verify-mutation:reason=clone + va.hasOldSelf = true + + optionalVA.oldSelf = types.OptionalOf(va.oldSelf) // +k8s:verify-mutation:reason=clone } - return va + + return va, optionalVA } -func validationActivationWithoutOldSelf(sts *schema.Structural, obj, _ interface{}) interpreter.Activation { - return &validationActivation{ +func validationActivationWithoutOldSelf(sts *schema.Structural, obj, _ interface{}) (interpreter.Activation, interpreter.Activation) { + res := &validationActivation{ self: UnstructuredToVal(obj, sts), } + return res, res } func (a *validationActivation) ResolveName(name string) (interface{}, bool) { diff --git a/pkg/apiserver/schema/cel/validation_test.go b/pkg/apiserver/schema/cel/validation_test.go index a306c1c5d..63119ebaa 100644 --- a/pkg/apiserver/schema/cel/validation_test.go +++ b/pkg/apiserver/schema/cel/validation_test.go @@ -31,16 +31,20 @@ import ( "github.com/stretchr/testify/require" "k8s.io/klog/v2" "k8s.io/kube-openapi/pkg/validation/strfmt" + "k8s.io/utils/ptr" apiextensionsinternal "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/util/yaml" celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/cel/common" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/apiserver/pkg/warning" + featuregatetesting "k8s.io/component-base/featuregate/testing" ) // TestValidationExpressions tests CEL integration with custom resource values and OpenAPIv3. @@ -3618,6 +3622,401 @@ func TestRatcheting(t *testing.T) { } } +// Runs transition rule cases with OptionalOldSelf set to true on the schema +func TestOptionalOldSelf(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)() + + tests := []struct { + name string + schema *schema.Structural + obj interface{} + oldObj interface{} + errors []string // strings that error message must contain + }{ + { + name: "allow new value if old value is null", + obj: map[string]interface{}{ + "foo": "bar", + }, + schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ + "foo": stringType, + }), "self.foo == 'not bar' || !oldSelf.hasValue()"), + }, + { + name: "block new value if old value is not null", + obj: map[string]interface{}{ + "foo": "invalid", + }, + oldObj: map[string]interface{}{ + "foo": "bar", + }, + schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ + "foo": stringType, + }), "self.foo == 'valid' || !oldSelf.hasValue()"), + errors: []string{"failed rule"}, + }, + { + name: "allow invalid new value if old value is also invalid", + obj: map[string]interface{}{ + "foo": "invalid again", + }, + oldObj: map[string]interface{}{ + "foo": "invalid", + }, + schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ + "foo": stringType, + }), "self.foo == 'valid' || (oldSelf.hasValue() && oldSelf.value().foo != 'valid')"), + }, + { + name: "allow invalid new value if old value is also invalid with chained optionals", + obj: map[string]interface{}{ + "foo": "invalid again", + }, + oldObj: map[string]interface{}{ + "foo": "invalid", + }, + schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ + "foo": stringType, + }), "self.foo == 'valid' || oldSelf.foo.orValue('') != 'valid'"), + }, + { + name: "block invalid new value if old value is valid", + obj: map[string]interface{}{ + "foo": "invalid", + }, + oldObj: map[string]interface{}{ + "foo": "valid", + }, + schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ + "foo": stringType, + }), "self.foo == 'valid' || (oldSelf.hasValue() && oldSelf.value().foo != 'valid')"), + errors: []string{"failed rule"}, + }, + { + name: "create: new min or allow higher than oldValue", + obj: 10, + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + }, + { + name: "block create: new min or allow higher than oldValue", + obj: 9, + // Can't use != null because type is integer and no overload + // workaround by comparing type, but kinda hacky + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + errors: []string{"failed rule"}, + }, + { + name: "update: new min or allow higher than oldValue", + obj: 10, + oldObj: 5, + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + }, + { + name: "ratchet update: new min or allow higher than oldValue", + obj: 9, + oldObj: 5, + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + }, + { + name: "ratchet noop update: new min or allow higher than oldValue", + obj: 5, + oldObj: 5, + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + }, + { + name: "block update: new min or allow higher than oldValue", + obj: 4, + oldObj: 5, + schema: cloneWithRule(&integerType, "self >= 10 || (oldSelf.hasValue() && oldSelf.value() <= self)"), + errors: []string{"failed rule"}, + }, + } + + for _, tt := range tests { + tt := tt + tp := true + for i := range tt.schema.XValidations { + tt.schema.XValidations[i].OptionalOldSelf = &tp + } + + t.Run(tt.name, func(t *testing.T) { + // t.Parallel() + + ctx := context.TODO() + celValidator := validator(tt.schema, true, model.SchemaDeclType(tt.schema, false), celconfig.PerCallLimit) + if celValidator == nil { + t.Fatal("expected non nil validator") + } + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), tt.schema, tt.obj, tt.oldObj, math.MaxInt) + unmatched := map[string]struct{}{} + for _, e := range tt.errors { + unmatched[e] = struct{}{} + } + for _, err := range errs { + if err.Type != field.ErrorTypeInvalid { + t.Errorf("expected only ErrorTypeInvalid errors, but got: %v", err) + continue + } + matched := false + for expected := range unmatched { + if strings.Contains(err.Error(), expected) { + delete(unmatched, expected) + matched = true + break + } + } + if !matched { + t.Errorf("expected error to contain one of %v, but got: %v", unmatched, err) + } + } + if len(unmatched) > 0 { + t.Errorf("expected errors %v", unmatched) + } + }) + } +} + +// Shows that type(oldSelf) == null_type works for all supported OpenAPI types +// both when oldSelf is null and when it is not null +func TestOptionalOldSelfCheckForNull(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)() + + tests := []struct { + name string + schema schema.Structural + obj interface{} + oldObj interface{} + }{ + { + name: "object", + obj: map[string]interface{}{ + "foo": "bar", + }, + oldObj: map[string]interface{}{ + "foo": "baz", + }, + schema: withRule(objectType(map[string]schema.Structural{ + "foo": stringType, + }), `!oldSelf.hasValue() || self.foo == "bar"`), + }, + { + name: "object - conditional field", + obj: map[string]interface{}{ + "foo": "bar", + }, + oldObj: map[string]interface{}{ + "foo": "baz", + }, + schema: withRule(objectType(map[string]schema.Structural{ + "foo": stringType, + }), `self.foo != "bar" || oldSelf.?foo.orValue("baz") == "baz"`), + }, + { + name: "string", + obj: "bar", + oldObj: "baz", + schema: withRule(stringType, ` + !oldSelf.hasValue() || self == "bar" + `), + }, + { + name: "integer", + obj: 1, + oldObj: 2, + schema: withRule(integerType, ` + !oldSelf.hasValue() || self == 1 + `), + }, + { + name: "number", + obj: 1.1, + oldObj: 2.2, + schema: withRule(numberType, ` + !oldSelf.hasValue() || self == 1.1 + `), + }, + { + name: "boolean", + obj: true, + oldObj: false, + schema: withRule(booleanType, ` + !oldSelf.hasValue() || self == true + `), + }, + { + name: "array", + obj: []interface{}{"bar"}, + oldObj: []interface{}{"baz"}, + schema: withRule(arrayType("", nil, &stringSchema), ` + !oldSelf.hasValue() || self[0] == "bar" + `), + }, + { + name: "array - conditional index", + obj: []interface{}{}, + oldObj: []interface{}{ + "baz", + }, + schema: withRule(arrayType("", nil, &stringSchema), ` + self.size() > 0 || oldSelf[?0].orValue("baz") == "baz" + `), + }, + { + name: "set-array", + obj: []interface{}{"bar"}, + oldObj: []interface{}{"baz"}, + schema: withRule(arrayType("set", nil, &stringSchema), ` + !oldSelf.hasValue() || self[0] == "bar" + `), + }, + { + name: "map-array", + obj: []interface{}{map[string]interface{}{ + "key": "foo", + "value": "bar", + }}, + oldObj: []interface{}{map[string]interface{}{ + "key": "foo", + "value": "baz", + }}, + schema: withRule(arrayType("map", []string{"key"}, objectTypePtr(map[string]schema.Structural{ + "key": stringType, + "value": stringType, + })), ` + !oldSelf.hasValue() || self[0].value == "bar" + `), + }, + } + + for _, tt := range tests { + tt := tt + tp := true + for i := range tt.schema.XValidations { + tt.schema.XValidations[i].OptionalOldSelf = &tp + } + + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + celValidator := validator(&tt.schema, false, model.SchemaDeclType(&tt.schema, false), celconfig.PerCallLimit) + if celValidator == nil { + t.Fatal("expected non nil validator") + } + + t.Run("null old", func(t *testing.T) { + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &tt.schema, tt.obj, nil, math.MaxInt) + if len(errs) != 0 { + t.Errorf("expected no errors, but got: %v", errs) + } + }) + + t.Run("non-null old", func(t *testing.T) { + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &tt.schema, tt.obj, tt.oldObj, math.MaxInt) + if len(errs) != 0 { + t.Errorf("expected no errors, but got: %v", errs) + } + }) + }) + } +} + +// Show that we cant just use oldSelf as if it was unwrapped +func TestOptionalOldSelfIsOptionalType(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)() + + cases := []struct { + name string + schema schema.Structural + obj interface{} + errors []string + }{ + { + name: "forbid direct usage of optional integer", + schema: withRule(integerType, ` + oldSelf + self > 5 + `), + obj: 5, + errors: []string{"no matching overload for '_+_' applied to '(optional(int), int)"}, + }, + { + name: "forbid direct usage of optional string", + schema: withRule(stringType, ` + oldSelf == "foo" + `), + obj: "bar", + errors: []string{"no matching overload for '_==_' applied to '(optional(string), string)"}, + }, + { + name: "forbid direct usage of optional array", + schema: withRule(arrayType("", nil, &stringSchema), ` + oldSelf.all(x, x == x) + `), + obj: []interface{}{"bar"}, + errors: []string{"expression of type 'optional(list(string))' cannot be range of a comprehension"}, + }, + { + name: "forbid direct usage of optional array element", + schema: withRule(arrayType("", nil, &stringSchema), ` + oldSelf[0] == "foo" + `), + obj: []interface{}{"bar"}, + errors: []string{"found no matching overload for '_==_' applied to '(optional(string), string)"}, + }, + { + name: "forbid direct usage of optional struct", + schema: withRule(arrayType("map", []string{"key"}, objectTypePtr(map[string]schema.Structural{ + "key": stringType, + "value": stringType, + })), `oldSelf.key == "foo"`), + obj: []interface{}{map[string]interface{}{ + "key": "bar", + "value": "baz", + }}, + errors: []string{"does not support field selection"}, + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + ctx := context.TODO() + + for i := range tt.schema.XValidations { + tt.schema.XValidations[i].OptionalOldSelf = ptr.To(true) + } + + celValidator := validator(&tt.schema, false, model.SchemaDeclType(&tt.schema, false), celconfig.PerCallLimit) + if celValidator == nil { + t.Fatal("expected non nil validator") + } + errs, _ := celValidator.Validate(ctx, field.NewPath("root"), &tt.schema, tt.obj, tt.obj, math.MaxInt) + unmatched := map[string]struct{}{} + for _, e := range tt.errors { + unmatched[e] = struct{}{} + } + for _, err := range errs { + if err.Type != field.ErrorTypeInvalid { + t.Errorf("expected only ErrorTypeInvalid errors, but got: %v", err) + continue + } + matched := false + for expected := range unmatched { + if strings.Contains(err.Error(), expected) { + delete(unmatched, expected) + matched = true + break + } + } + if !matched { + t.Errorf("expected error to contain one of %v, but got: %v", unmatched, err) + } + } + + if len(unmatched) > 0 { + t.Errorf("expected errors %v", unmatched) + } + }) + } +} + func genString(n int, c rune) string { b := strings.Builder{} for i := 0; i < n; i++ { diff --git a/test/integration/ratcheting_test.go b/test/integration/ratcheting_test.go index a08277a18..c2932e26d 100644 --- a/test/integration/ratcheting_test.go +++ b/test/integration/ratcheting_test.go @@ -1340,6 +1340,50 @@ func TestRatchetingFunctionality(t *testing.T) { }}}, }, }, + { + Name: "CEL Optional OldSelf", + Operations: []ratchetingTestOperation{ + updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "field": { + Type: "string", + XValidations: []apiextensionsv1.ValidationRule{ + { + Rule: "!oldSelf.hasValue()", + Message: "oldSelf must be null", + OptionalOldSelf: ptr(true), + }, + }, + }, + }, + }}, + + applyPatchOperation{ + "create instance passes since oldself is null", + myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ + "field": "value", + }}, + + expectError{ + applyPatchOperation{ + "update field fails, since oldself is not null", + myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ + "field": "value2", + }, + }, + }, + + expectError{ + applyPatchOperation{ + "noop update field fails, since oldself is not null and transition rules are not ratcheted", + myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ + "field": "value", + }, + }, + }, + }, + }, // Features that should not ratchet { Name: "AllOf_should_not_ratchet",