KEP-4008: CRDValidationRatcheting: Add support for optional `oldSelf` #121034

alexzielenski · 2023-10-06T18:49:57Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds `optionalOldSelf` to `x-kubernetes-validations` to support ratcheting CRD schema constraints

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://kep.k8s.io/4008

/cc @cici37

k8s-ci-robot · 2023-10-06T18:49:59Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

apelisse

I'd love to see a test.

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

alexzielenski · 2023-10-10T21:28:33Z

tests added

jiahuif · 2023-10-12T16:35:43Z

/assign
/assign @cici37
/triage accepted

alexzielenski · 2023-11-03T19:14:17Z

/retest

jpbetz

Two minor comments, then the implementation LGTM. I glanced at the API changes as well and don't have any concerns, but will defer to API review for that.

jpbetz · 2023-11-03T19:50:27Z

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

+			DeclTypes: []*apiservercel.DeclType{
+				scopedType,
+			},


Curious if this was strictly needed since the decl is already added to the environment in the prior version options? If we don't need to declare it again maybe omit it? (if we do need to declare it again, I'll want to dig in to figure out why..)

Was able to remove it. It needed to be added in an earlier implementation when I had called extend multiple times like the following pseudocode:

selfEnvSet := baseEnv.Extend({ // self definition DeclTypes: [scopedType] }) oldSelfEnvSet := selfEnvSet.Extend({ // oldself definition }) optionalOldSelfEnvSet := selfEnvSet.Extend({ // optionalOldSelf definition })

This caused field resolution errors that weren't resolved until we either added the DeclType to each call to Extend, or made it the form it is currently.

@jpbetz is it expected that the definition wouldn't be available in successive calls to Extend?

jpbetz · 2023-11-03T19:51:18Z

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

+	// (e.g. self and oldSelf) to CEL values. One activation must be produced
+	// for


Last sentence is incomplete?

whoops. added the rest:

// 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.

jpbetz · 2023-11-03T19:56:57Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go

+	// - During a create, `oldSelf` evaluates to `null`
+	// - During an update, `oldSelf` evaluates to `null` when the old object is


Just caught up on the latest code. The use of CEL Optionals here is really nice.

liggitt · 2023-11-03T19:18:01Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go

+	// `transition rule`.
+	//
+	// Transition rules by default are applied only on UPDATE requests and are
+	// skipped if and old value could not be found. You can opt a transition


Suggested change

// skipped if and old value could not be found. You can opt a transition

// skipped if an old value could not be found. You can opt a transition

liggitt · 2023-11-03T19:18:55Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go

+	// When enabled, if the old object is missing the value or is being
+	// created for the first time, then `oldSelf` will be a CEL optional.


when enabled, isn't oldSelf always a CEL optional?

Rephrased:

// When enabled `oldSelf` will be a CEL optional whose value will be // `None` if there is no old value, or when the object is initially created. // // You may check for presence of oldSelf using `oldSelf.hasValue()` and // unwrap it after checking using `oldSelf.value()`. Check the CEL // documentation for Optional types for more information: // https://pkg.go.dev/github.com/google/cel-go/cel#OptionalTypes //

liggitt · 2023-11-03T19:21:24Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go

@@ -210,6 +210,14 @@ type ValidationRule struct {
 	//   - 'map': `X + Y` performs a merge where the array positions of all keys in `X` are preserved but the values
 	//     are overwritten by values in `Y` when the key sets of `X` and `Y` intersect. Elements in `Y` with
 	//     non-intersecting keys are appended, retaining their partial order.
+	//
+	// If `rule` makes use of the `oldSelf` variable it is implicitly a
+	// `transition rule`.


By default, the `oldSelf` variable is the same type as `self`. When `optionalOldSelf` is true, the `oldSelf` variable is a CEL optional variable whose value() is the same type as `self`. See the documentation for the `optionalOldSelf` field for details.

liggitt · 2023-11-03T19:35:44Z

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

 	va := &validationActivation{
 		self: UnstructuredToVal(obj, sts),
 	}
+	optionalVA := &validationActivation{
+		self:       va.self,
+		hasOldSelf: true,


this is worth a comment

Suggested change

hasOldSelf: true,

hasOldSelf: true, // this means the oldSelf variable is defined for CEL to reference, not that it has a value

liggitt · 2023-11-03T19:36:00Z

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

@@ -622,21 +632,44 @@ type validationActivation struct {
 	hasOldSelf    bool
 }

-func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation {
+func validationActivationWithOldSelf(sts *schema.Structural, obj, oldObj interface{}) (interpreter.Activation, interpreter.Activation) {


godoc or name the return values

added names to the return values

liggitt · 2023-11-03T19:37:43Z

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

+	return va, optionalVA
+}
+
+func validationActivationWithOptionalOldSelf(sts *schema.Structural, obj, oldObj interface{}) interpreter.Activation {


this looks unused, drop it?

liggitt · 2023-11-03T19:58:23Z

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

 			// transition rules are evaluated only if there is a comparable existing value
 			continue
+		} else if compiled.UsesOldSelf && allowsNilOldSelf {


it's confusing to make this an else block and make it rely on the feature gate... a rule that sets optionalOldSelf: true should always use optionalOldSelfActivation

I'd suggest separating the calculation for which activation to use (based solely on the rule field) and whether to evaluate, based on both the feature gate and the rule:

optionalOldSelfRule := pointer.BoolDeref(rule.OptionalOldSelf, false) if compiled.UsesOldSelf && oldObj == nil { if !utilfeature.DefaultFeatureGate.Enabled(features.CRDValidationRatcheting) { continue } if !optionalOldSelfRule { continue } } ruleActivation := activation if optionalOldSelfRule { ruleActivation = optionalOldSelfActivation }

restructured as suggested

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

liggitt · 2023-11-03T20:01:06Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types_jsonschema.go

copy doc updates to v1 as well

should be there now

liggitt · 2023-11-03T20:06:24Z

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

there should be type checking unit tests on the optional oldSelf to make sure we catch type stuff:

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go index 18df5a42475..a774e818b9d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation_test.go @@ -26,13 +26,18 @@ import ( "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" + "k8s.io/utils/pointer" + 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" ) const ( @@ -151,6 +156,8 @@ 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 @@ -625,6 +632,92 @@ func TestCelCompilation(t *testing.T) { matchesAll(noError(), transitionRule(true)), }, }, + { + 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: pointer.Bool(true)}, + {Rule: "self >= oldSelf.orValue(1)", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "oldSelf.hasValue() ? self >= oldSelf.value() : true", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self >= oldSelf", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self >= oldSelf.orValue('')", OptionalOldSelf: pointer.Bool(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": schema.Structural{Generic: schema.Generic{Type: "integer"}}, + "b": schema.Structural{Generic: schema.Generic{Type: "boolean"}}, + "s": schema.Structural{Generic: schema.Generic{Type: "string"}}, + "a": schema.Structural{ + Generic: schema.Generic{Type: "array"}, + Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}}, + }, + "o": schema.Structural{ + Generic: schema.Generic{Type: "object"}, + Properties: map[string]schema.Structural{ + "i": schema.Structural{Generic: schema.Generic{Type: "integer"}}, + "b": schema.Structural{Generic: schema.Generic{Type: "boolean"}}, + "s": schema.Structural{Generic: schema.Generic{Type: "string"}}, + "a": schema.Structural{ + Generic: schema.Generic{Type: "array"}, + Items: &schema.Structural{Generic: schema.Generic{Type: "integer"}}, + }, + "o": schema.Structural{ + Generic: schema.Generic{Type: "object"}, + }, + }, + }, + }, + Extensions: schema.Extensions{ + XValidations: apiextensions.ValidationRules{ + {Rule: "self.i >= oldSelf.i.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.s == oldSelf.s.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.b == oldSelf.b.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o == oldSelf.o.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.i >= oldSelf.o.i.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.s == oldSelf.o.s.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.b == oldSelf.o.b.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.o == oldSelf.o.o.value()", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.i >= oldSelf.o.i.orValue(1)", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "oldSelf.hasValue() ? self.o.i >= oldSelf.o.i.value() : true", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.i >= oldSelf.o.i", OptionalOldSelf: pointer.Bool(true)}, + {Rule: "self.o.i >= oldSelf.o.s.orValue(0)", OptionalOldSelf: pointer.Bool(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: "whitespace-only rule", input: schema.Structural{ diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 236564f7d36..63119ebaa22 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -3667,6 +3667,18 @@ func TestOptionalOldSelf(t *testing.T) { "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{}{

liggitt · 2023-11-03T20:07:52Z

looks like some TestValidateCustomResourceDefinitionValidation unit test failures as well

liggitt · 2023-11-03T20:18:58Z

I know we validate a default specified in openapi against any CEL rules provided:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go

Lines 76 to 126 in 263ab25

    
           	if insideMeta { 
        
           		obj, _, err := f(runtime.DeepCopyJSONValue(s.Default.Object)) 
        
           		if err != nil { 
        
           			// this should never happen. f(s.Default.Object) only gives an error if f is the 
        
           			// root object func, but the default value is not a map. But then we wouldn't be 
        
           			// in this case. 
        
           			return nil, fmt.Errorf("failed to validate default value inside metadata: %v", err), remainingCost 
        
           		} 
        
           		// check ObjectMeta/TypeMeta and everything else 
        
           		if err := schemaobjectmeta.Coerce(nil, obj, rootSchema, true, false); err != nil { 
        
           			allErrs = append(allErrs, field.Invalid(pth.Child("default"), s.Default.Object, fmt.Sprintf("must result in valid metadata: %v", err))) 
        
           		} else if errs := schemaobjectmeta.Validate(nil, obj, rootSchema, true); len(errs) > 0 { 
        
           			allErrs = append(allErrs, field.Invalid(pth.Child("default"), s.Default.Object, fmt.Sprintf("must result in valid metadata: %v", errs.ToAggregate()))) 
        
           		} else if errs := apiservervalidation.ValidateCustomResource(pth.Child("default"), s.Default.Object, validator); len(errs) > 0 { 
        
           			allErrs = append(allErrs, errs...) 
        
           		} else if celValidator := cel.NewValidator(s, isResourceRoot, celconfig.PerCallLimit); celValidator != nil { 
        
           			celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) 
        
           			remainingCost = rmCost 
        
           			allErrs = append(allErrs, celErrs...) 
        
           			if remainingCost < 0 { 
        
           				return allErrs, nil, remainingCost 
        
           			} 
        
           		} 
        
           	} else { 
        
           		// check whether default is pruned 
        
           		if requirePrunedDefaults { 
        
           			pruned := runtime.DeepCopyJSONValue(s.Default.Object) 
        
           			pruning.Prune(pruned, s, s.XEmbeddedResource) 
        
           			if !reflect.DeepEqual(pruned, s.Default.Object) { 
        
           				allErrs = append(allErrs, field.Invalid(pth.Child("default"), s.Default.Object, "must not have unknown fields")) 
        
           			} 
        
           		} 
        
           		// check ObjectMeta/TypeMeta and everything else 
        
           		if err := schemaobjectmeta.Coerce(pth.Child("default"), s.Default.Object, s, s.XEmbeddedResource, false); err != nil { 
        
           			allErrs = append(allErrs, err) 
        
           		} else if errs := schemaobjectmeta.Validate(pth.Child("default"), s.Default.Object, s, s.XEmbeddedResource); len(errs) > 0 { 
        
           			allErrs = append(allErrs, errs...) 
        
           		} else if errs := apiservervalidation.ValidateCustomResource(pth.Child("default"), s.Default.Object, validator); len(errs) > 0 { 
        
           			allErrs = append(allErrs, errs...) 
        
           		} else if celValidator := cel.NewValidator(s, isResourceRoot, celconfig.PerCallLimit); celValidator != nil { 
        
           			celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) 
        
           			remainingCost = rmCost 
        
           			allErrs = append(allErrs, celErrs...) 
        
           			if remainingCost < 0 { 
        
           				return allErrs, nil, remainingCost 
        
           			} 
        
           		} 
        
           	} 
        
           }

It looks like we're currently passing s.Default.Object as both the old and new object, to run as many validation rules as possible.

If the ratcheting validation feature is enabled, and it passed that first validation check, should we also validate with s.Default.Object as the new object and nil as the old object, to make sure defaults pass any ratcheting rules that behave differently on create?

alexzielenski · 2023-11-03T21:12:24Z

If the ratcheting validation feature is enabled, and it passed that first validation check, should we also validate with s.Default.Object as the new object and nil as the old object, to make sure defaults pass any ratcheting rules that behave differently on create?

I think that'd be prudent. Ive added it and a test for it

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

liggitt · 2023-11-03T21:24:58Z

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

 	}

 	return compResults, nil
 }

-func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (*environment.EnvSet, error) {
+func prepareEnvSet(baseEnvSet *environment.EnvSet, declType *apiservercel.DeclType) (*environment.EnvSet, *environment.EnvSet, error) {


doc or name the return vars

named return vars

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

liggitt · 2023-11-03T21:33:43Z

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

@@ -96,6 +98,11 @@ func validate(ctx context.Context, pth *field.Path, s *structuralschema.Structur
 				if remainingCost < 0 {
 					return allErrs, nil, remainingCost
 				}
+
+				if len(celErrs) == 0 && utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) {


needs a comment explaining why we're calling this again

also, I think we need to capture the cost of both types of runs and take whichever leaves less remaining cost

celErrs, rmCost := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, s.Default.Object, remainingCost) allErrs = append(allErrs, celErrs...) if len(celErrs) == 0 && utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { celErrs, rmCostWithoutOldObject := celValidator.Validate(ctx, pth.Child("default"), s, s.Default.Object, nil, remainingCost) allErrs = append(allErrs, celErrs...) if rmCostWithoutOldObject < rmCost { rmCost = rmCostWithoutOldObject } } remainingCost = rmCost if remainingCost < 0 { return allErrs, nil, remainingCost }

same comment applies below

Applied feedback as suggested and added comments

liggitt · 2023-11-03T21:53:08Z

/lgtm
/approve

k8s-ci-robot · 2023-11-03T21:53:15Z

LGTM label has been added.

Git tree hash: 12b79c4256148f1e4495ff3df690d37574308a91

k8s-ci-robot · 2023-11-03T21:53:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, jpbetz, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [jpbetz,liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ng rules work

apelisse · 2023-11-03T23:22:23Z

Blind reapply /lgtm

…

On Fri, Nov 3, 2023 at 3:45 PM Kubernetes Prow Robot < ***@***.***> wrote: New changes are detected. LGTM label has been removed. — Reply to this email directly, view it on GitHub <#121034 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOXDLEIOC5A7Y5LHCJRWC3YCVXZXAVCNFSM6AAAAAA5WG6SX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTGIYDQNJQGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

k8s-ci-robot · 2023-11-03T23:22:31Z

LGTM label has been added.

Git tree hash: 5ea11e89f552ad0224f32363d568455aa988e3c5

k8s-ci-robot requested a review from cici37 October 6, 2023 18:49

apelisse reviewed Oct 6, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go Outdated Show resolved Hide resolved

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from a866a71 to bddcf76 Compare October 10, 2023 20:55

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2023

alexzielenski marked this pull request as ready for review October 10, 2023 21:28

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2023

k8s-ci-robot assigned cici37 Oct 12, 2023

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 12, 2023

k8s-ci-robot assigned jiahuif Oct 12, 2023

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 12, 2023

alexzielenski mentioned this pull request Oct 16, 2023

CRD Validation Ratcheting kubernetes/enhancements#4008

Open

12 tasks

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from bddcf76 to 65d0f25 Compare October 18, 2023 16:39

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from 53e8924 to 26facf7 Compare November 3, 2023 18:52

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from 26facf7 to 0ac3857 Compare November 3, 2023 19:14

jpbetz reviewed Nov 3, 2023

View reviewed changes

liggitt reviewed Nov 3, 2023

View reviewed changes

alexzielenski commented Nov 3, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go Show resolved Hide resolved

liggitt reviewed Nov 3, 2023

View reviewed changes

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from 0101780 to 77942a4 Compare November 3, 2023 21:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2023

alexzielenski added 4 commits November 3, 2023 15:23

ratcheting-cel: add optionalOldSelf field

5edb27a

ratcheting-cel: use Optional[T] for oldSelf when optionalOldSelf is true

eef1515

validate defaults across an update from nil to ensure create ratcheti…

bba0c9a

…ng rules work

codegen

42a70cd

alexzielenski force-pushed the apiserver/apiextensions/ratcheting-oldself-create branch from 77942a4 to 42a70cd Compare November 3, 2023 22:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2023

k8s-ci-robot requested a review from liggitt November 3, 2023 22:45

k8s-ci-robot assigned apelisse Nov 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2023

k8s-ci-robot merged commit 22866ca into kubernetes:master Nov 4, 2023
14 checks passed

neolit123 mentioned this pull request Nov 7, 2023

flaky tests in k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions: validation #121754

Closed

jpbetz mentioned this pull request Dec 15, 2023

Bug fix: Enable CRD field dropping for validation ratcheting #122329

Merged

		// (e.g. self and oldSelf) to CEL values. One activation must be produced
		// for

		// - During a create, `oldSelf` evaluates to `null`
		// - During an update, `oldSelf` evaluates to `null` when the old object is

	// skipped if and old value could not be found. You can opt a transition
	// skipped if an old value could not be found. You can opt a transition

		// When enabled, if the old object is missing the value or is being
		// created for the first time, then `oldSelf` will be a CEL optional.

	hasOldSelf: true,
	hasOldSelf: true, // this means the oldSelf variable is defined for CEL to reference, not that it has a value

KEP-4008: CRDValidationRatcheting: Add support for optional oldSelf #121034

KEP-4008: CRDValidationRatcheting: Add support for optional oldSelf #121034

Conversation

alexzielenski commented Oct 6, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 6, 2023

apelisse left a comment

Choose a reason for hiding this comment

alexzielenski commented Oct 10, 2023

jiahuif commented Oct 12, 2023

alexzielenski commented Nov 3, 2023

jpbetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 3, 2023

liggitt commented Nov 3, 2023

alexzielenski commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 3, 2023

k8s-ci-robot commented Nov 3, 2023

k8s-ci-robot commented Nov 3, 2023

apelisse commented Nov 3, 2023 via email

k8s-ci-robot commented Nov 3, 2023

KEP-4008: CRDValidationRatcheting: Add support for optional `oldSelf` #121034

KEP-4008: CRDValidationRatcheting: Add support for optional `oldSelf` #121034

alexzielenski commented Oct 6, 2023 •

edited