From d081fd56a2aba4663f11dd5e27dbcf64c0ec6638 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Tue, 25 Jun 2024 18:37:34 +0200 Subject: [PATCH] Validate stored messageExpressions with the correct CEL environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../apiextensions/validation/validation.go | 2 +- .../validation/validation_test.go | 56 +++++++++++++------ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 7133776fbbe..a9a2bed004e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -179,7 +179,7 @@ func findPreexistingExpressionsInSchema(schema *apiextensions.JSONSchemaProps, e for _, v := range s.XValidations { expressions.rules.Insert(v.Rule) if len(v.MessageExpression) > 0 { - expressions.messageExpressions.Insert(v.Rule) + expressions.messageExpressions.Insert(v.MessageExpression) } } return false diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index cf8edcdc49a..6c11979b314 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -7282,7 +7282,7 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { } func TestValidateCustomResourceDefinitionValidationRuleCompatibility(t *testing.T) { - allValidationsErrors := []validationMatch{ + allRuleValidationsErrors := []validationMatch{ invalid("spec", "validation", "openAPIV3Schema", "properties[x]", "x-kubernetes-validations[0]", "rule"), invalid("spec", "validation", "openAPIV3Schema", "properties[obj]", "x-kubernetes-validations[0]", "rule"), invalid("spec", "validation", "openAPIV3Schema", "properties[obj]", "properties[a]", "x-kubernetes-validations[0]", "rule"), @@ -7291,23 +7291,46 @@ func TestValidateCustomResourceDefinitionValidationRuleCompatibility(t *testing. invalid("spec", "validation", "openAPIV3Schema", "properties[map]", "x-kubernetes-validations[0]", "rule"), invalid("spec", "validation", "openAPIV3Schema", "properties[map]", "additionalProperties", "x-kubernetes-validations[0]", "rule"), } + allMessageExpressionValidationsErrors := []validationMatch{ + invalid("spec", "validation", "openAPIV3Schema", "properties[x]", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[obj]", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[obj]", "properties[a]", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[array]", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[array]", "items", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[map]", "x-kubernetes-validations[0]", "messageExpression"), + invalid("spec", "validation", "openAPIV3Schema", "properties[map]", "additionalProperties", "x-kubernetes-validations[0]", "messageExpression"), + } tests := []struct { - name string - storedRule string - updatedRule string - errors []validationMatch + name string + storedRule string + updatedRule string + storedMessageExpression string + updatedMessageExpression string + errors []validationMatch }{ { - name: "functions declared for storage mode allowed if expression is unchanged from what is stored", - storedRule: "test() == true", - updatedRule: "test() == true", + name: "functions declared for storage mode allowed if expressions are unchanged from what is stored", + storedRule: "test() == true", + updatedRule: "test() == true", + storedMessageExpression: "'test: %s'.format([test()])", + updatedMessageExpression: "'test: %s'.format([test()])", }, { - name: "functions declared for storage mode not allowed if expression is changed", - storedRule: "test() == false", - updatedRule: "test() == true", - errors: allValidationsErrors, + name: "functions declared for storage mode not allowed if rule expression is changed", + storedRule: "test() == false", + updatedRule: "test() == true", // rule was changed + storedMessageExpression: "'test: %s'.format([test()])", + updatedMessageExpression: "'test: %s'.format([test()])", + errors: allRuleValidationsErrors, + }, + { + name: "functions declared for storage mode not allowed if message expression is changed", + storedRule: "test() == true", + updatedRule: "test() == true", + storedMessageExpression: "'test: %s'.format([test()])", + updatedMessageExpression: "'test - updated: %s'.format([test()])", // messageExpression was changed + errors: allMessageExpressionValidationsErrors, }, } @@ -7322,10 +7345,11 @@ func TestValidateCustomResourceDefinitionValidationRuleCompatibility(t *testing. } for _, tc := range tests { - fn := func(rule string) *apiextensions.CustomResourceDefinition { + fn := func(rule, messageExpression string) *apiextensions.CustomResourceDefinition { validationRules := []apiextensions.ValidationRule{ { - Rule: rule, + Rule: rule, + MessageExpression: messageExpression, }, } return &apiextensions.CustomResourceDefinition{ @@ -7382,8 +7406,8 @@ func TestValidateCustomResourceDefinitionValidationRuleCompatibility(t *testing. Status: apiextensions.CustomResourceDefinitionStatus{StoredVersions: []string{"version"}}, } } - old := fn(tc.storedRule) - resource := fn(tc.updatedRule) + old := fn(tc.storedRule, tc.storedMessageExpression) + resource := fn(tc.updatedRule, tc.updatedMessageExpression) t.Run(tc.name, func(t *testing.T) { ctx := context.TODO()