Skip to content

Commit

Permalink
Merge pull request #125703 from sbueringer/pr-fix-message-expression-…
Browse files Browse the repository at this point in the history 
…evaluation

Validate CRD stored messageExpressions with the correct CEL environment
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jun 26, 2024
2 parents 7a6062f + d081fd5 commit c6fd466
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 17 deletions.
2 changes: 1 addition & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
56 changes: 40 additions & 16 deletions ...g/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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 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 expression is changed",
storedRule: "test() == false",
updatedRule: "test() == true",
errors: allValidationsErrors,
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,
},
}

Expand All @@ -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{
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit c6fd466

Please sign in to comment.