Skip to content

Commit eef1515

Browse files
author
Alexander Zielenski
committed
ratcheting-cel: use Optional[T] for oldSelf when optionalOldSelf is true
1 parent 5edb27a commit eef1515

File tree

5 files changed

+616
-16
lines changed

5 files changed

+616
-16
lines changed

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
"github.com/google/cel-go/cel"
2525
"github.com/google/cel-go/checker"
26+
"github.com/google/cel-go/common/types"
2627

2728
apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
2829
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
@@ -126,7 +127,7 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit
126127
}
127128
celRules := s.Extensions.XValidations
128129

129-
envSet, err := prepareEnvSet(baseEnvSet, declType)
130+
oldSelfEnvSet, optionalOldSelfEnvSet, err := prepareEnvSet(baseEnvSet, declType)
130131
if err != nil {
131132
return nil, err
132133
}
@@ -135,15 +136,20 @@ func Compile(s *schema.Structural, declType *apiservercel.DeclType, perCallLimit
135136
compResults := make([]CompilationResult, len(celRules))
136137
maxCardinality := maxCardinality(declType.MinSerializedSize)
137138
for i, rule := range celRules {
138-
compResults[i] = compileRule(s, rule, envSet, envLoader, estimator, maxCardinality, perCallLimit)
139+
ruleEnvSet := oldSelfEnvSet
140+
if rule.OptionalOldSelf != nil && *rule.OptionalOldSelf {
141+
ruleEnvSet = optionalOldSelfEnvSet
142+
}
143+
compResults[i] = compileRule(s, rule, ruleEnvSet, envLoader, estimator, maxCardinality, perCallLimit)
139144
}
140145

141146
return compResults, nil
142147
}
143148

144-
func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (*environment.EnvSet, error) {
149+
func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (oldSelfEnvSet *environment.EnvSet, optionalOldSelfEnvSet *environment.EnvSet, err error) {
145150
scopedType := declType.MaybeAssignTypeName(generateUniqueSelfTypeName())
146-
return baseEnvSet.Extend(
151+
152+
oldSelfEnvSet, err = baseEnvSet.Extend(
147153
environment.VersionedOptions{
148154
// Feature epoch was actually 1.23, but we artificially set it to 1.0 because these
149155
// options should always be present.
@@ -162,6 +168,34 @@ func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclTy
162168
},
163169
},
164170
)
171+
if err != nil {
172+
return nil, nil, err
173+
}
174+
175+
optionalOldSelfEnvSet, err = baseEnvSet.Extend(
176+
environment.VersionedOptions{
177+
// Feature epoch was actually 1.23, but we artificially set it to 1.0 because these
178+
// options should always be present.
179+
IntroducedVersion: version.MajorMinor(1, 0),
180+
EnvOptions: []cel.EnvOption{
181+
cel.Variable(ScopedVarName, scopedType.CelType()),
182+
},
183+
DeclTypes: []*apiservercel.DeclType{
184+
scopedType,
185+
},
186+
},
187+
environment.VersionedOptions{
188+
IntroducedVersion: version.MajorMinor(1, 24),
189+
EnvOptions: []cel.EnvOption{
190+
cel.Variable(OldScopedVarName, types.NewOptionalType(scopedType.CelType())),
191+
},
192+
},
193+
)
194+
if err != nil {
195+
return nil, nil, err
196+
}
197+
198+
return oldSelfEnvSet, optionalOldSelfEnvSet, nil
165199
}
166200

167201
func compileRule(s *schema.Structural, rule apiextensions.ValidationRule, envSet *environment.EnvSet, envLoader EnvLoader, estimator *library.CostEstimator, maxCardinality uint64, perCallLimit uint64) (compilationResult CompilationResult) {

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,14 @@ import (
2929
apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
3030
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
3131
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model"
32+
apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features"
3233
"k8s.io/apimachinery/pkg/util/version"
3334
celconfig "k8s.io/apiserver/pkg/apis/cel"
3435
"k8s.io/apiserver/pkg/cel"
3536
"k8s.io/apiserver/pkg/cel/environment"
37+
utilfeature "k8s.io/apiserver/pkg/util/feature"
38+
featuregatetesting "k8s.io/component-base/featuregate/testing"
39+
"k8s.io/utils/ptr"
3640
)
3741

3842
const (
@@ -151,12 +155,99 @@ func (v transitionRuleMatcher) String() string {
151155
}
152156

153157
func TestCelCompilation(t *testing.T) {
158+
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, apiextensionsfeatures.CRDValidationRatcheting, true)()
154159
cases := []struct {
155160
name string
156161
input schema.Structural
157162
expectedResults []validationMatcher
158163
unmodified bool
159164
}{
165+
{
166+
name: "optional primitive transition rule type checking",
167+
input: schema.Structural{
168+
Generic: schema.Generic{
169+
Type: "integer",
170+
},
171+
Extensions: schema.Extensions{
172+
XValidations: apiextensions.ValidationRules{
173+
{Rule: "self >= oldSelf.value()", OptionalOldSelf: ptr.To(true)},
174+
{Rule: "self >= oldSelf.orValue(1)", OptionalOldSelf: ptr.To(true)},
175+
{Rule: "oldSelf.hasValue() ? self >= oldSelf.value() : true", OptionalOldSelf: ptr.To(true)},
176+
{Rule: "self >= oldSelf", OptionalOldSelf: ptr.To(true)},
177+
{Rule: "self >= oldSelf.orValue('')", OptionalOldSelf: ptr.To(true)},
178+
},
179+
},
180+
},
181+
expectedResults: []validationMatcher{
182+
matchesAll(noError(), transitionRule(true)),
183+
matchesAll(noError(), transitionRule(true)),
184+
matchesAll(noError(), transitionRule(true)),
185+
matchesAll(invalidError("optional")),
186+
matchesAll(invalidError("orValue")),
187+
},
188+
},
189+
{
190+
name: "optional complex transition rule type checking",
191+
input: schema.Structural{
192+
Generic: schema.Generic{
193+
Type: "object",
194+
},
195+
Properties: map[string]schema.Structural{
196+
"i": {Generic: schema.Generic{Type: "integer"}},
197+
"b": {Generic: schema.Generic{Type: "boolean"}},
198+
"s": {Generic: schema.Generic{Type: "string"}},
199+
"a": {
200+
Generic: schema.Generic{Type: "array"},
201+
Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}},
202+
},
203+
"o": {
204+
Generic: schema.Generic{Type: "object"},
205+
Properties: map[string]schema.Structural{
206+
"i": {Generic: schema.Generic{Type: "integer"}},
207+
"b": {Generic: schema.Generic{Type: "boolean"}},
208+
"s": {Generic: schema.Generic{Type: "string"}},
209+
"a": {
210+
Generic: schema.Generic{Type: "array"},
211+
Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}},
212+
},
213+
"o": {
214+
Generic: schema.Generic{Type: "object"},
215+
},
216+
},
217+
},
218+
},
219+
Extensions: schema.Extensions{
220+
XValidations: apiextensions.ValidationRules{
221+
{Rule: "self.i >= oldSelf.i.value()", OptionalOldSelf: ptr.To(true)},
222+
{Rule: "self.s == oldSelf.s.value()", OptionalOldSelf: ptr.To(true)},
223+
{Rule: "self.b == oldSelf.b.value()", OptionalOldSelf: ptr.To(true)},
224+
{Rule: "self.o == oldSelf.o.value()", OptionalOldSelf: ptr.To(true)},
225+
{Rule: "self.o.i >= oldSelf.o.i.value()", OptionalOldSelf: ptr.To(true)},
226+
{Rule: "self.o.s == oldSelf.o.s.value()", OptionalOldSelf: ptr.To(true)},
227+
{Rule: "self.o.b == oldSelf.o.b.value()", OptionalOldSelf: ptr.To(true)},
228+
{Rule: "self.o.o == oldSelf.o.o.value()", OptionalOldSelf: ptr.To(true)},
229+
{Rule: "self.o.i >= oldSelf.o.i.orValue(1)", OptionalOldSelf: ptr.To(true)},
230+
{Rule: "oldSelf.hasValue() ? self.o.i >= oldSelf.o.i.value() : true", OptionalOldSelf: ptr.To(true)},
231+
{Rule: "self.o.i >= oldSelf.o.i", OptionalOldSelf: ptr.To(true)},
232+
{Rule: "self.o.i >= oldSelf.o.s.orValue(0)", OptionalOldSelf: ptr.To(true)},
233+
},
234+
},
235+
},
236+
expectedResults: []validationMatcher{
237+
matchesAll(noError(), transitionRule(true)),
238+
matchesAll(noError(), transitionRule(true)),
239+
matchesAll(noError(), transitionRule(true)),
240+
matchesAll(noError(), transitionRule(true)),
241+
matchesAll(noError(), transitionRule(true)),
242+
matchesAll(noError(), transitionRule(true)),
243+
matchesAll(noError(), transitionRule(true)),
244+
matchesAll(noError(), transitionRule(true)),
245+
matchesAll(noError(), transitionRule(true)),
246+
matchesAll(noError(), transitionRule(true)),
247+
matchesAll(invalidError("optional")),
248+
matchesAll(invalidError("orValue")),
249+
},
250+
},
160251
{
161252
name: "valid object",
162253
input: schema.Structural{

‎staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go‎

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,15 +32,18 @@ import (
3232
"github.com/google/cel-go/interpreter"
3333

3434
"k8s.io/klog/v2"
35+
"k8s.io/utils/ptr"
3536

3637
apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
3738
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
3839
"k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model"
40+
"k8s.io/apiextensions-apiserver/pkg/features"
3941
"k8s.io/apimachinery/pkg/util/validation/field"
4042
"k8s.io/apiserver/pkg/cel"
4143
"k8s.io/apiserver/pkg/cel/common"
4244
"k8s.io/apiserver/pkg/cel/environment"
4345
"k8s.io/apiserver/pkg/cel/metrics"
46+
utilfeature "k8s.io/apiserver/pkg/util/feature"
4447
"k8s.io/apiserver/pkg/warning"
4548

4649
celconfig "k8s.io/apiserver/pkg/apis/cel"
@@ -65,9 +68,10 @@ type Validator struct {
6568
// custom resource being validated, or the root of an XEmbeddedResource object.
6669
isResourceRoot bool
6770

68-
// celActivationFactory produces an Activation, which resolves identifiers (e.g. self and
69-
// oldSelf) to CEL values.
70-
celActivationFactory func(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation
71+
// celActivationFactory produces a Activations, which resolve identifiers
72+
// (e.g. self and oldSelf) to CEL values. One activation must be produced
73+
// for each of the cases when oldSelf is optional and non-optional.
74+
celActivationFactory func(sts *schema.Structural, obj, oldObj interface{}) (activation interpreter.Activation, optionalOldSelfActivation interpreter.Activation)
7175
}
7276

7377
// 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
122126
additionalPropertiesValidator = validator(s.AdditionalProperties.Structural, s.AdditionalProperties.Structural.XEmbeddedResource, declType.ElemType, perCallLimit)
123127
}
124128
if len(compiledRules) > 0 || err != nil || itemsValidator != nil || additionalPropertiesValidator != nil || len(propertiesValidators) > 0 {
125-
var activationFactory func(*schema.Structural, interface{}, interface{}) interpreter.Activation = validationActivationWithoutOldSelf
129+
activationFactory := validationActivationWithoutOldSelf
126130
for _, rule := range compiledRules {
127131
if rule.UsesOldSelf {
128132
activationFactory = validationActivationWithOldSelf
@@ -289,7 +293,7 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path
289293
if s.isResourceRoot {
290294
sts = model.WithTypeAndObjectMeta(sts)
291295
}
292-
activation := s.celActivationFactory(sts, obj, oldObj)
296+
activation, optionalOldSelfActivation := s.celActivationFactory(sts, obj, oldObj)
293297
for i, compiled := range s.compiledRules {
294298
rule := sts.XValidations[i]
295299
if compiled.Error != nil {
@@ -300,11 +304,29 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path
300304
// rule is empty
301305
continue
302306
}
307+
308+
// If ratcheting is enabled, allow rule with oldSelf to evaluate
309+
// when `optionalOldSelf` is set to true
310+
optionalOldSelfRule := ptr.Deref(rule.OptionalOldSelf, false)
303311
if compiled.UsesOldSelf && oldObj == nil {
304312
// transition rules are evaluated only if there is a comparable existing value
305-
continue
313+
// But if the rule uses optional oldSelf and gate is enabled we allow
314+
// the rule to be evaluated
315+
if !utilfeature.DefaultFeatureGate.Enabled(features.CRDValidationRatcheting) {
316+
continue
317+
}
318+
319+
if !optionalOldSelfRule {
320+
continue
321+
}
322+
}
323+
324+
ruleActivation := activation
325+
if optionalOldSelfRule {
326+
ruleActivation = optionalOldSelfActivation
306327
}
307-
evalResult, evalDetails, err := compiled.Program.ContextEval(ctx, activation)
328+
329+
evalResult, evalDetails, err := compiled.Program.ContextEval(ctx, ruleActivation)
308330
if evalDetails == nil {
309331
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))))
310332
return errs, -1
@@ -622,21 +644,31 @@ type validationActivation struct {
622644
hasOldSelf bool
623645
}
624646

625-
func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation {
647+
func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) (activation interpreter.Activation, optionalOldSelfActivation interpreter.Activation) {
626648
va := &validationActivation{
627649
self: UnstructuredToVal(obj, sts),
628650
}
651+
optionalVA := &validationActivation{
652+
self: va.self,
653+
hasOldSelf: true, // this means the oldSelf variable is defined for CEL to reference, not that it has a value
654+
oldSelf: types.OptionalNone,
655+
}
656+
629657
if oldObj != nil {
630658
va.oldSelf = UnstructuredToVal(oldObj, sts) // +k8s:verify-mutation:reason=clone
631-
va.hasOldSelf = true // +k8s:verify-mutation:reason=clone
659+
va.hasOldSelf = true
660+
661+
optionalVA.oldSelf = types.OptionalOf(va.oldSelf) // +k8s:verify-mutation:reason=clone
632662
}
633-
return va
663+
664+
return va, optionalVA
634665
}
635666

636-
func validationActivationWithoutOldSelf(sts *schema.Structural, obj, _ interface{}) interpreter.Activation {
637-
return &validationActivation{
667+
func validationActivationWithoutOldSelf(sts *schema.Structural, obj, _ interface{}) (interpreter.Activation, interpreter.Activation) {
668+
res := &validationActivation{
638669
self: UnstructuredToVal(obj, sts),
639670
}
671+
return res, res
640672
}
641673

642674
func (a *validationActivation) ResolveName(name string) (interface{}, bool) {

0 commit comments

Comments
 (0)