Skip to content

Commit

Permalink
ratcheting-cel: use Optional[T] for oldSelf when optionalOldSelf is true
Browse files Browse the repository at this point in the history 
Kubernetes-commit: eef15158152702c6315b1959fc3c28d08087dc26
  • Loading branch information
@k8s-publishing-bot
Alexander Zielenski authored and k8s-publishing-bot committed Nov 3, 2023
1 parent 0889c57 commit efc67b4
Show file tree
Hide file tree
Showing 5 changed files with 616 additions and 16 deletions.
42 changes: 38 additions & 4 deletions pkg/apiserver/schema/cel/compilation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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.
Expand All @@ -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) {
Expand Down
91 changes: 91 additions & 0 deletions pkg/apiserver/schema/cel/compilation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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{
Expand Down
56 changes: 44 additions & 12 deletions pkg/apiserver/schema/cel/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit efc67b4

Please sign in to comment.