diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index 3f3905bf8a8..128c5b05418 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -38,8 +38,10 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model" "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" + "k8s.io/apiserver/pkg/warning" celconfig "k8s.io/apiserver/pkg/apis/cel" ) @@ -147,6 +149,71 @@ func validator(s *schema.Structural, isResourceRoot bool, declType *cel.DeclType // Most callers can ignore the returned remainingBudget value unless another validate call is going to be made // context is passed for supporting context cancellation during cel validation func (s *Validator) Validate(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { + return s.validate(ctx, fldPath, sts, obj, oldObj, ratchetingOptions{}, costBudget) +} + +func (s *Validator) ValidateWithRatcheting(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { + // May be a worthwhile optimization to share the correlated object instance + // with the OpenAPI schema validator to avoid doing DeepEqual twice + return s.validate(ctx, fldPath, sts, obj, oldObj, ratchetingOptions{ + currentCorrelation: common.NewCorrelatedObject(obj, oldObj, &model.Structural{Structural: sts}), + }, costBudget) +} + +// ratchetingOptions stores the current correlation object and the nearest +// parent which was correlatable. The parent is stored so that we can check at +// the point an error is thrown whether it should be ratcheted using simple +// logic +// Key and Index should be used as normally to traverse to the next node. +type ratchetingOptions struct { + // Current correlation object. If nil, then this node is from an uncorrelatable + // part of the schema + currentCorrelation *common.CorrelatedObject + + // If currentCorrelation is nil, this is the nearest parent to this node + // which was correlatable. If the parent is deepequal to its old value, + // then errors thrown by this node are ratcheted + nearestParentCorrelation *common.CorrelatedObject +} + +// shouldRatchetError returns true if the errors raised by the current node +// should be ratcheted. +// +// Errors for the current node should be ratcheted if one of the following is true: +// 1. The current node is correlatable, and it is equal to its old value +// 2. The current node has a correlatable ancestor, and the ancestor is equal +// to its old value. +func (r ratchetingOptions) shouldRatchetError() bool { + if r.currentCorrelation != nil { + return r.currentCorrelation.CachedDeepEqual() + } + + return r.nearestParentCorrelation.CachedDeepEqual() +} + +// Finds the next node following the field in the tree and returns options using +// that node. If none could be found, then retains a reference to the last +// correlatable ancestor for ratcheting purposes +func (r ratchetingOptions) key(field string) ratchetingOptions { + if r.currentCorrelation == nil { + return r + } + + return ratchetingOptions{currentCorrelation: r.currentCorrelation.Key(field), nearestParentCorrelation: r.currentCorrelation} +} + +// Finds the next node following the index in the tree and returns options using +// that node. If none could be found, then retains a reference to the last +// correlatable ancestor for ratcheting purposes +func (r ratchetingOptions) index(idx int) ratchetingOptions { + if r.currentCorrelation == nil { + return r + } + + return ratchetingOptions{currentCorrelation: r.currentCorrelation.Index(idx), nearestParentCorrelation: r.currentCorrelation} +} + +func (s *Validator) validate(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, correlation ratchetingOptions, costBudget int64) (errs field.ErrorList, remainingBudget int64) { t := time.Now() defer func() { metrics.Metrics.ObserveEvaluation(time.Since(t)) @@ -156,28 +223,31 @@ func (s *Validator) Validate(ctx context.Context, fldPath *field.Path, sts *sche return nil, remainingBudget } - errs, remainingBudget = s.validateExpressions(ctx, fldPath, sts, obj, oldObj, remainingBudget) + errs, remainingBudget = s.validateExpressions(ctx, fldPath, sts, obj, oldObj, correlation, remainingBudget) + if remainingBudget < 0 { return errs, remainingBudget } + switch obj := obj.(type) { case []interface{}: oldArray, _ := oldObj.([]interface{}) var arrayErrs field.ErrorList - arrayErrs, remainingBudget = s.validateArray(ctx, fldPath, sts, obj, oldArray, remainingBudget) + arrayErrs, remainingBudget = s.validateArray(ctx, fldPath, sts, obj, oldArray, correlation, remainingBudget) errs = append(errs, arrayErrs...) return errs, remainingBudget case map[string]interface{}: oldMap, _ := oldObj.(map[string]interface{}) var mapErrs field.ErrorList - mapErrs, remainingBudget = s.validateMap(ctx, fldPath, sts, obj, oldMap, remainingBudget) + mapErrs, remainingBudget = s.validateMap(ctx, fldPath, sts, obj, oldMap, correlation, remainingBudget) errs = append(errs, mapErrs...) return errs, remainingBudget } + return errs, remainingBudget } -func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj interface{}, correlation ratchetingOptions, costBudget int64) (errs field.ErrorList, remainingBudget int64) { // guard against oldObj being a non-nil interface with a nil value if oldObj != nil { v := reflect.ValueOf(oldObj) @@ -263,26 +333,35 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path if len(compiled.NormalizedRuleFieldPath) > 0 { fldPath = fldPath.Child(compiled.NormalizedRuleFieldPath) } + + addErr := func(e *field.Error) { + if !compiled.TransitionRule && correlation.shouldRatchetError() { + warning.AddWarning(ctx, "", e.Error()) + } else { + errs = append(errs, e) + } + } + if compiled.MessageExpression != nil { messageExpression, newRemainingBudget, msgErr := evalMessageExpression(ctx, compiled.MessageExpression, rule.MessageExpression, activation, remainingBudget) if msgErr != nil { if msgErr.Type == cel.ErrorTypeInternal { - errs = append(errs, field.InternalError(fldPath, msgErr)) + addErr(field.InternalError(fldPath, msgErr)) return errs, -1 } else if msgErr.Type == cel.ErrorTypeInvalid { - errs = append(errs, field.Invalid(fldPath, sts.Type, msgErr.Error())) + addErr(field.Invalid(fldPath, sts.Type, msgErr.Error())) return errs, -1 } else { klog.V(2).ErrorS(msgErr, "messageExpression evaluation failed") - errs = append(errs, fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason)) + addErr(fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason)) remainingBudget = newRemainingBudget } } else { - errs = append(errs, fieldErrorForReason(fldPath, sts.Type, messageExpression, rule.Reason)) + addErr(fieldErrorForReason(fldPath, sts.Type, messageExpression, rule.Reason)) remainingBudget = newRemainingBudget } } else { - errs = append(errs, fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason)) + addErr(fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason)) } } } @@ -566,7 +645,7 @@ func (a *validationActivation) Parent() interpreter.Activation { return nil } -func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj map[string]interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj map[string]interface{}, correlation ratchetingOptions, costBudget int64) (errs field.ErrorList, remainingBudget int64) { remainingBudget = costBudget if remainingBudget < 0 { return errs, remainingBudget @@ -585,7 +664,7 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s } var err field.ErrorList - err, remainingBudget = s.AdditionalProperties.Validate(ctx, fldPath.Key(k), sts.AdditionalProperties.Structural, v, oldV, remainingBudget) + err, remainingBudget = s.AdditionalProperties.validate(ctx, fldPath.Key(k), sts.AdditionalProperties.Structural, v, oldV, correlation.key(k), remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget @@ -603,7 +682,7 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s } var err field.ErrorList - err, remainingBudget = sub.Validate(ctx, fldPath.Child(k), &stsProp, v, oldV, remainingBudget) + err, remainingBudget = sub.validate(ctx, fldPath.Child(k), &stsProp, v, oldV, correlation.key(k), remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget @@ -615,7 +694,7 @@ func (s *Validator) validateMap(ctx context.Context, fldPath *field.Path, sts *s return errs, remainingBudget } -func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj []interface{}, costBudget int64) (errs field.ErrorList, remainingBudget int64) { +func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts *schema.Structural, obj, oldObj []interface{}, correlation ratchetingOptions, costBudget int64) (errs field.ErrorList, remainingBudget int64) { remainingBudget = costBudget if remainingBudget < 0 { return errs, remainingBudget @@ -627,7 +706,7 @@ func (s *Validator) validateArray(ctx context.Context, fldPath *field.Path, sts correlatableOldItems := makeMapList(sts, oldObj) for i := range obj { var err field.ErrorList - err, remainingBudget = s.Items.Validate(ctx, fldPath.Index(i), sts.Items, obj[i], correlatableOldItems.Get(obj[i]), remainingBudget) + err, remainingBudget = s.Items.validate(ctx, fldPath.Index(i), sts.Items, obj[i], correlatableOldItems.Get(obj[i]), correlation.index(i), remainingBudget) errs = append(errs, err...) if remainingBudget < 0 { return errs, remainingBudget 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 899ef4e97e0..2b662534a53 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 @@ -17,22 +17,29 @@ limitations under the License. package cel import ( + "bytes" "context" "flag" "fmt" "math" "strings" + "sync" "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/klog/v2" "k8s.io/kube-openapi/pkg/validation/strfmt" + 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" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apimachinery/pkg/util/yaml" celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/warning" ) // TestValidationExpressions tests CEL integration with custom resource values and OpenAPIv3. @@ -3014,6 +3021,590 @@ func TestValidateFieldPath(t *testing.T) { } } +// FixTabsOrDie counts the number of tab characters preceding the first +// line in the given yaml object. It removes that many tabs from every +// line. It panics (it's a test funvion) if some line has fewer tabs +// than the first line. +// +// The purpose of this is to make it easier to read tests. +func FixTabsOrDie(in string) string { + lines := bytes.Split([]byte(in), []byte{'\n'}) + if len(lines[0]) == 0 && len(lines) > 1 { + lines = lines[1:] + } + // Create prefix made of tabs that we want to remove. + var prefix []byte + for _, c := range lines[0] { + if c != '\t' { + break + } + prefix = append(prefix, byte('\t')) + } + // Remove prefix from all tabs, fail otherwise. + for i := range lines { + line := lines[i] + // It's OK for the last line to be blank (trailing \n) + if i == len(lines)-1 && len(line) <= len(prefix) && bytes.TrimSpace(line) == nil { + lines[i] = []byte{} + break + } + if !bytes.HasPrefix(line, prefix) { + minRange := i - 5 + maxRange := i + 5 + if minRange < 0 { + minRange = 0 + } + if maxRange > len(lines) { + maxRange = len(lines) + } + panic(fmt.Errorf("line %d doesn't start with expected number (%d) of tabs (%v-%v):\n%v", i, len(prefix), minRange, maxRange, string(bytes.Join(lines[minRange:maxRange], []byte{'\n'})))) + } + lines[i] = line[len(prefix):] + } + + joined := string(bytes.Join(lines, []byte{'\n'})) + + // Convert rest of tabs to spaces since yaml doesnt like yabs + // (assuming 2 space alignment) + return strings.ReplaceAll(joined, "\t", " ") +} + +// Creates a *spec.Schema Schema by decoding the given YAML. Panics on error +func mustSchema(source string) *schema.Structural { + source = FixTabsOrDie(source) + d := yaml.NewYAMLOrJSONDecoder(strings.NewReader(source), 4096) + props := &apiextensions.JSONSchemaProps{} + if err := d.Decode(props); err != nil { + panic(err) + } + convertedProps := &apiextensionsinternal.JSONSchemaProps{} + if err := apiextensions.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(props, convertedProps, nil); err != nil { + panic(err) + } + + res, err := schema.NewStructural(convertedProps) + if err != nil { + panic(err) + } + return res +} + +// Creates an *unstructured by decoding the given YAML. Panics on error +func mustUnstructured(source string) interface{} { + source = FixTabsOrDie(source) + d := yaml.NewYAMLOrJSONDecoder(strings.NewReader(source), 4096) + var res interface{} + if err := d.Decode(&res); err != nil { + panic(err) + } + return res +} + +type warningRecorder struct { + mu sync.Mutex + warnings []string +} + +// AddWarning adds a warning to recorder. +func (r *warningRecorder) AddWarning(agent, text string) { + r.mu.Lock() + defer r.mu.Unlock() + r.warnings = append(r.warnings, text) +} + +func (r *warningRecorder) Warnings() []string { + r.mu.Lock() + defer r.mu.Unlock() + + warnings := make([]string, len(r.warnings)) + copy(warnings, r.warnings) + return warnings +} + +func TestRatcheting(t *testing.T) { + cases := []struct { + name string + schema *schema.Structural + oldObj interface{} + newObj interface{} + + // Errors that should occur when evaluating this operation with + // ratcheting feature enabled + errors []string + + // Errors that should occur when evaluating this operation with + // ratcheting feature disabled + // These errors should be raised as warnings when ratcheting is enabled + warnings []string + + runtimeCostBudget int64 + }{ + { + name: "normal CEL expression", + schema: mustSchema(` + type: object + properties: + foo: + type: string + x-kubernetes-validations: + - rule: self == "bar" + message: "gotta be baz" + `), + oldObj: mustUnstructured(` + foo: baz + `), + newObj: mustUnstructured(` + foo: baz + `), + warnings: []string{ + `root.foo: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "normal CEL expression thats a descendent of an atomic array whose parent is totally unchanged", + schema: mustSchema(` + type: array + x-kubernetes-list-type: atomic + items: + type: object + properties: + bar: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + `), + // CEL error comes from uncorrelatable portion of the schema, + // but it should be ratcheted anyway because it is the descendent + // of an unchanged correlatable node + oldObj: mustUnstructured(` + - bar: bar + `), + newObj: mustUnstructured(` + - bar: bar + `), + warnings: []string{ + `root[0].bar: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "normal CEL expression thats a descendent of a set whose parent is totally unchanged", + schema: mustSchema(` + type: array + x-kubernetes-list-type: set + items: + type: number + x-kubernetes-validations: + - rule: int(self) % 2 == 1 + message: "gotta be odd" + `), + // CEL error comes from uncorrelatable portion of the schema, + // but it should be ratcheted anyway because it is the descendent + // of an unchanged correlatable node + oldObj: mustUnstructured(` + - 1 + - 2 + `), + newObj: mustUnstructured(` + - 1 + - 2 + `), + warnings: []string{ + `root[1]: Invalid value: "number": gotta be odd`, + }, + }, + { + name: "normal CEL expression thats a descendent of a set and one of its siblings has changed", + schema: mustSchema(` + type: object + properties: + stringField: + type: string + setArray: + type: array + x-kubernetes-list-type: set + items: + type: number + x-kubernetes-validations: + - rule: int(self) % 2 == 1 + message: "gotta be odd" + `), + oldObj: mustUnstructured(` + stringField: foo + setArray: + - 1 + - 3 + - 2 + `), + newObj: mustUnstructured(` + stringField: changed but ratcheted + setArray: + - 1 + - 3 + - 2 + `), + warnings: []string{ + `root.setArray[2]: Invalid value: "number": gotta be odd`, + }, + }, + { + name: "descendent of a map list whose parent is unchanged", + schema: mustSchema(` + type: array + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: ["key"] + items: + type: object + properties: + key: + type: string + value: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + `), + oldObj: mustUnstructured(` + - key: foo + value: notbaz + - key: bar + value: notbaz + `), + newObj: mustUnstructured(` + - key: foo + value: notbaz + - key: bar + value: notbaz + - key: baz + value: baz + `), + warnings: []string{ + `root[0].value: Invalid value: "string": gotta be baz`, + `root[1].value: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "descendent of a map list whose siblings have changed", + schema: mustSchema(` + type: array + x-kubernetes-list-type: map + x-kubernetes-list-map-keys: ["key"] + items: + type: object + properties: + key: + type: string + value: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + `), + oldObj: mustUnstructured(` + - key: foo + value: notbaz + - key: bar + value: notbaz + `), + newObj: mustUnstructured(` + - key: foo + value: baz + - key: bar + value: notbaz + `), + warnings: []string{ + `root[1].value: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "descendent of a map whose parent is totally unchanged", + schema: mustSchema(` + type: object + properties: + stringField: + type: string + mapField: + type: object + properties: + foo: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + mapField: + type: object + properties: + bar: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be nested baz" + `), + oldObj: mustUnstructured(` + stringField: foo + mapField: + foo: notbaz + mapField: + bar: notbaz + `), + newObj: mustUnstructured(` + stringField: foo + mapField: + foo: notbaz + mapField: + bar: notbaz + `), + warnings: []string{ + `root.mapField.foo: Invalid value: "string": gotta be baz`, + `root.mapField.mapField.bar: Invalid value: "string": gotta be nested baz`, + }, + }, + { + name: "descendent of a map whose siblings have changed", + schema: mustSchema(` + type: object + properties: + stringField: + type: string + mapField: + type: object + properties: + foo: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + mapField: + type: object + properties: + bar: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + otherBar: + type: string + x-kubernetes-validations: + - rule: self == "otherBaz" + message: "gotta be otherBaz" + `), + oldObj: mustUnstructured(` + stringField: foo + mapField: + foo: baz + mapField: + bar: notbaz + otherBar: nototherBaz + `), + newObj: mustUnstructured(` + stringField: foo + mapField: + foo: notbaz + mapField: + bar: notbaz + otherBar: otherBaz + `), + errors: []string{ + // Didn't get ratcheted because we changed its value from baz to notbaz + `root.mapField.foo: Invalid value: "string": gotta be baz`, + }, + warnings: []string{ + // Ratcheted because its value remained the same, even though it is invalid + `root.mapField.mapField.bar: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "normal CEL expression thats a descendent of an atomic array whose siblings has changed", + schema: mustSchema(` + type: object + properties: + stringField: + type: string + atomicArray: + type: array + x-kubernetes-list-type: atomic + items: + type: object + properties: + bar: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + `), + oldObj: mustUnstructured(` + stringField: foo + atomicArray: + - bar: bar + `), + newObj: mustUnstructured(` + stringField: changed but ratcheted + atomicArray: + - bar: bar + `), + warnings: []string{ + `root.atomicArray[0].bar: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "we can't ratchet a normal CEL expression from an uncorrelatable part of the schema whose parent nodes has changed", + schema: mustSchema(` + type: array + x-kubernetes-list-type: atomic + items: + type: object + properties: + bar: + type: string + x-kubernetes-validations: + - rule: self == "baz" + message: "gotta be baz" + `), + // CEL error comes from uncorrelatable portion of the schema, + // but it should be ratcheted anyway because it is the descendent + // or an unchanged correlatable node + oldObj: mustUnstructured(` + - bar: bar + `), + newObj: mustUnstructured(` + - bar: bar + - bar: baz + `), + errors: []string{ + `root[0].bar: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "transition rules never ratchet for correlatable schemas", + schema: mustSchema(` + type: object + properties: + foo: + type: string + x-kubernetes-validations: + - rule: oldSelf != "bar" && self == "baz" + message: gotta be baz + `), + oldObj: mustUnstructured(` + foo: bar + `), + newObj: mustUnstructured(` + foo: bar + `), + errors: []string{ + `root.foo: Invalid value: "string": gotta be baz`, + }, + }, + { + name: "changing field path does not change ratcheting logic", + schema: mustSchema(` + type: object + x-kubernetes-validations: + - rule: self.foo == "baz" + message: gotta be baz + fieldPath: ".foo" + properties: + bar: + type: string + foo: + type: string + `), + oldObj: mustUnstructured(` + foo: bar + `), + // Fieldpath is on unchanged field `foo`, but since rule is on the + // changed parent object we still get an error + newObj: mustUnstructured(` + foo: bar + bar: invalid + `), + errors: []string{ + `root.foo: Invalid value: "object": gotta be baz`, + }, + }, + { + name: "cost budget errors are not ratcheted", + schema: mustSchema(` + type: string + minLength: 5 + x-kubernetes-validations: + - rule: self == "baz" + message: gotta be baz + `), + oldObj: "unchanged", + newObj: "unchanged", + runtimeCostBudget: 1, + errors: []string{ + `validation failed due to running out of cost budget, no further validation rules will be run`, + }, + }, + { + name: "compile errors are not ratcheted", + schema: mustSchema(` + type: string + x-kubernetes-validations: + - rule: asdausidyhASDNJm + message: gotta be baz + `), + oldObj: "unchanged", + newObj: "unchanged", + errors: []string{ + `rule compile error: compilation failed: ERROR: :1:1: undeclared reference to 'asdausidyhASDNJm'`, + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + validator := NewValidator(c.schema, false, celconfig.PerCallLimit) + require.NotNil(t, validator) + recorder := &warningRecorder{} + ctx := warning.WithWarningRecorder(context.TODO(), recorder) + budget := c.runtimeCostBudget + if budget == 0 { + budget = celconfig.RuntimeCELCostBudget + } + errs, _ := validator.ValidateWithRatcheting( + ctx, + field.NewPath("root"), + c.schema, + c.newObj, + c.oldObj, + budget, + ) + + require.Len(t, errs, len(c.errors), "must have expected number of errors") + require.Len(t, recorder.Warnings(), len(c.warnings), "must have expected number of warnings") + + // Check that the expected errors were raised + for _, expectedErr := range c.errors { + found := false + for _, err := range errs { + if strings.Contains(err.Error(), expectedErr) { + found = true + break + } + } + + assert.True(t, found, "expected error %q not found", expectedErr) + } + + // Check that the ratcheting disabled errors were raised as warnings + for _, expectedWarning := range c.warnings { + found := false + for _, warning := range recorder.Warnings() { + if warning == expectedWarning { + found = true + break + } + } + assert.True(t, found, "expected warning %q not found", expectedWarning) + } + + }) + } +} + func genString(n int, c rune) string { b := strings.Builder{} for i := 0; i < n; i++ { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go index 77f2d51bf88..403535de5f3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go @@ -26,6 +26,7 @@ import ( structurallisttype "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype" schemaobjectmeta "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta" "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -257,6 +258,9 @@ func (a customResourceStrategy) ValidateUpdate(ctx context.Context, obj, old run if celValidator := a.celValidator; celValidator != nil { if has, err := hasBlockingErr(errs); has { errs = append(errs, err) + } else if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { + err, _ := celValidator.ValidateWithRatcheting(ctx, nil, a.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget) + errs = append(errs, err...) } else { err, _ := celValidator.Validate(ctx, nil, a.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget) errs = append(errs, err...) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index d0f850e97cc..6d0b2c54944 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -17,6 +17,7 @@ limitations under the License. package integration_test import ( + "bytes" "context" "encoding/json" "errors" @@ -36,6 +37,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" + utilyaml "k8s.io/apimachinery/pkg/util/yaml" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" featuregatetesting "k8s.io/component-base/featuregate/testing" @@ -95,11 +97,50 @@ var fakeRESTMapper map[schema.GroupVersionResource]string = map[schema.GroupVers myCRDV1Beta1: "MyCoolCRD", } +// FixTabsOrDie counts the number of tab characters preceding the first +// line in the given yaml object. It removes that many tabs from every +// line. It panics (it's a test function) if some line has fewer tabs +// than the first line. +// +// The purpose of this is to make it easier to read tests. +func FixTabsOrDie(in string) string { + lines := bytes.Split([]byte(in), []byte{'\n'}) + if len(lines[0]) == 0 && len(lines) > 1 { + lines = lines[1:] + } + // Create prefix made of tabs that we want to remove. + var prefix []byte + for _, c := range lines[0] { + if c != '\t' { + break + } + prefix = append(prefix, byte('\t')) + } + // Remove prefix from all tabs, fail otherwise. + for i := range lines { + line := lines[i] + // It's OK for the last line to be blank (trailing \n) + if i == len(lines)-1 && len(line) <= len(prefix) && bytes.TrimSpace(line) == nil { + lines[i] = []byte{} + break + } + if !bytes.HasPrefix(line, prefix) { + panic(fmt.Errorf("line %d doesn't start with expected number (%d) of tabs: %v", i, len(prefix), string(line))) + } + lines[i] = line[len(prefix):] + } + joined := string(bytes.Join(lines, []byte{'\n'})) + + // Convert rest of tabs to spaces since yaml doesnt like yabs + // (assuming 2 space alignment) + return strings.ReplaceAll(joined, "\t", " ") +} + type applyPatchOperation struct { description string gvr schema.GroupVersionResource name string - patch map[string]interface{} + patch interface{} } func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { @@ -109,25 +150,33 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { return fmt.Errorf("no mapping found for Gvr %v, add entry to fakeRESTMapper", a.gvr) } - a.patch["kind"] = kind - a.patch["apiVersion"] = a.gvr.GroupVersion().String() - - if meta, ok := a.patch["metadata"]; ok { - mObj := meta.(map[string]interface{}) - mObj["name"] = a.name - mObj["namespace"] = "default" - } else { - a.patch["metadata"] = map[string]interface{}{ - "name": a.name, - "namespace": "default", + patch := &unstructured.Unstructured{} + if obj, ok := a.patch.(map[string]interface{}); ok { + patch.Object = obj + } else if str, ok := a.patch.(string); ok { + str = FixTabsOrDie(str) + if err := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(str), len(str)).Decode(&patch.Object); err != nil { + return err } + } else { + return fmt.Errorf("invalid patch type: %T", a.patch) } - _, err := ctx.DynamicClient.Resource(a.gvr).Namespace("default").Apply(context.TODO(), a.name, &unstructured.Unstructured{ - Object: a.patch, - }, metav1.ApplyOptions{ - FieldManager: "manager", - }) + patch.SetKind(kind) + patch.SetAPIVersion(a.gvr.GroupVersion().String()) + patch.SetName(a.name) + patch.SetNamespace("default") + + _, err := ctx.DynamicClient. + Resource(a.gvr). + Namespace(patch.GetNamespace()). + Apply( + context.TODO(), + patch.GetName(), + patch, + metav1.ApplyOptions{ + FieldManager: "manager", + }) return err @@ -1295,13 +1344,54 @@ func TestRatchetingFunctionality(t *testing.T) { }, { Name: "CEL_transition_rules_should_not_ratchet", + Operations: []ratchetingTestOperation{ + updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ + Type: "object", + XPreserveUnknownFields: ptr(true), + }}, + applyPatchOperation{ + "create instance with strings that do not start with k8s", + myCRDV1Beta1, myCRDInstanceName, + ` + myStringField: myStringValue + myOtherField: myOtherField + `, + }, + updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ + Type: "object", + XPreserveUnknownFields: ptr(true), + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "myStringField": { + Type: "string", + XValidations: apiextensionsv1.ValidationRules{ + { + Rule: "oldSelf != 'myStringValue' || self == 'validstring'", + }, + }, + }, + }, + }}, + expectError{applyPatchOperation{ + "try to change one field to valid value, but unchanged field fails to be ratcheted by transition rule", + myCRDV1Beta1, myCRDInstanceName, + ` + myOtherField: myNewOtherField + myStringField: myStringValue + `, + }}, + applyPatchOperation{ + "change both fields to valid values", + myCRDV1Beta1, myCRDInstanceName, + ` + myStringField: validstring + myOtherField: myNewOtherField + `, + }, + }, }, // Future Functionality, disabled tests { Name: "CEL Add Change Rule", - // Planned future test. CEL Rules are not yet ratcheted in alpha - // implementation of CRD Validation Ratcheting - Disabled: true, Operations: []ratchetingTestOperation{ updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ Type: "object",