Repository: kubernetes/kubernetes (language: go)

Issue:
CEL stored messageExpressions of CRDs are not validated with the correct CEL environment
### What happened?

Noticed a copy & paste issue when reading through the CEL code.

Basically here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go#L182

This should be 
> expressions.messageExpressions.Insert(v.MessageExpression)
instead of:
> expressions.messageExpressions.Insert(v.Rule)

The impact of this issue should be that messageExpressions are always evaluated with the CEL environment of the compatibility version. But stored (i.e. pre-existing) messageExpressions should be evaluated with a CEL environment with the "max" version instead.

For users of Kubernetes this means that messageExpressions deployed with one minor version of Kubernetes might not work anymore if they rollback e.g. by one minor version.




### What did you expect to happen?

messageExpressions should be treated the same way as rules in that regard.



### How can we reproduce it (as minimally and precisely as possible)?

* Deploy Kubernetes 1.30
* Deploy CRD that uses messageExpression that is using Sets (which became available with Kubernetes 1.30 that is using compatibilty version 1.29)
* Rollback to Kubernetes 1.29
* Update something in the CRD
  * => CRD update will fail because messageExpression is evaluated with the wrong CEL environment (it should still work)



### Anything else we need to know?

_No response_

### Kubernetes version

<details>

At least on Kubernetes main, but I would assume this isuse is there since CEL was implemented

</details>


### Cloud provider

-

### OS version

-

### Install tools

-

### Container runtime (CRI) and version (if applicable)

-

### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
There are no sig labels on this issue. Please add an appropriate label by using one of the following commands:
- `/sig <group-name>`
- `/wg <group-name>`
- `/committee <group-name>`

Please see the [group list](https://git.k8s.io/community/sig-list.md) for a listing of the SIGs, working groups, and committees available.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
cc @liggitt 

Will open a PR in a bit

Fix patch:
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 7133776fbbe32..a9a2bed004e80 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


Test patch:
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 cf8edcdc49ab0..6c11979b314e0 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 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,
 		},
 	}
 
@@ -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()