Merge pull request #123475 from alexzielenski/apiserver/apiextensions/cel-error-fieldpath

bugfix: incorrect fieldpath when using multiple crd validation rules

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

@@ -369,8 +369,9 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path
 			continue
 		}
 		if evalResult != types.True {
+			currentFldPath := fldPath
 			if len(compiled.NormalizedRuleFieldPath) > 0 {
-				fldPath = fldPath.Child(compiled.NormalizedRuleFieldPath)
+				currentFldPath = currentFldPath.Child(compiled.NormalizedRuleFieldPath)
 			}
 
 			addErr := func(e *field.Error) {
@@ -385,22 +386,22 @@ func (s *Validator) validateExpressions(ctx context.Context, fldPath *field.Path
 				messageExpression, newRemainingBudget, msgErr := evalMessageExpression(ctx, compiled.MessageExpression, rule.MessageExpression, activation, remainingBudget)
 				if msgErr != nil {
 					if msgErr.Type == cel.ErrorTypeInternal {
-						addErr(field.InternalError(fldPath, msgErr))
+						addErr(field.InternalError(currentFldPath, msgErr))
 						return errs, -1
 					} else if msgErr.Type == cel.ErrorTypeInvalid {
-						addErr(field.Invalid(fldPath, sts.Type, msgErr.Error()))
+						addErr(field.Invalid(currentFldPath, sts.Type, msgErr.Error()))
 						return errs, -1
 					} else {
 						klog.V(2).ErrorS(msgErr, "messageExpression evaluation failed")
-						addErr(fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason))
+						addErr(fieldErrorForReason(currentFldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason))
 						remainingBudget = newRemainingBudget
 					}
 				} else {
-					addErr(fieldErrorForReason(fldPath, sts.Type, messageExpression, rule.Reason))
+					addErr(fieldErrorForReason(currentFldPath, sts.Type, messageExpression, rule.Reason))
 					remainingBudget = newRemainingBudget
 				}
 			} else {
-				addErr(fieldErrorForReason(fldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason))
+				addErr(fieldErrorForReason(currentFldPath, sts.Type, ruleMessageOrDefault(rule), rule.Reason))
 			}
 		}
 	}

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

@@ -2676,11 +2676,10 @@ func TestReasonAndFldPath(t *testing.T) {
 		return &r
 	}()
 	tests := []struct {
-		name      string
+		name   string
-		schema    *schema.Structural
+		schema *schema.Structural
-		obj       interface{}
+		obj    interface{}
-		errors    []string
+		errors field.ErrorList
-		errorType field.ErrorType
 	}{
 		{name: "Return error based on input reason",
 			obj: map[string]interface{}{
@@ -2691,8 +2690,12 @@ func TestReasonAndFldPath(t *testing.T) {
 			schema: withRulePtr(objectTypePtr(map[string]schema.Structural{
 				"f": withReasonAndFldPath(objectType(map[string]schema.Structural{"m": integerType}), "self.m == 2", "", forbiddenReason),
 			}), "1 == 1"),
-			errorType: field.ErrorTypeForbidden,
+			errors: field.ErrorList{
-			errors:    []string{"root.f: Forbidden"},
+				{
+					Type:  field.ErrorTypeForbidden,
+					Field: "root.f",
+				},
+			},
 		},
 		{name: "Return error default is invalid",
 			obj: map[string]interface{}{
@@ -2703,8 +2706,12 @@ func TestReasonAndFldPath(t *testing.T) {
 			schema: withRulePtr(objectTypePtr(map[string]schema.Structural{
 				"f": withReasonAndFldPath(objectType(map[string]schema.Structural{"m": integerType}), "self.m == 2", "", nil),
 			}), "1 == 1"),
-			errorType: field.ErrorTypeInvalid,
+			errors: field.ErrorList{
-			errors:    []string{"root.f: Invalid"},
+				{
+					Type:  field.ErrorTypeInvalid,
+					Field: "root.f",
+				},
+			},
 		},
 		{name: "Return error based on input fieldPath",
 			obj: map[string]interface{}{
@@ -2715,8 +2722,63 @@ func TestReasonAndFldPath(t *testing.T) {
 			schema: withRulePtr(objectTypePtr(map[string]schema.Structural{
 				"f": withReasonAndFldPath(objectType(map[string]schema.Structural{"m": integerType}), "self.m == 2", ".m", forbiddenReason),
 			}), "1 == 1"),
-			errorType: field.ErrorTypeForbidden,
+			errors: field.ErrorList{
-			errors:    []string{"root.f.m: Forbidden"},
+				{
+					Type:  field.ErrorTypeForbidden,
+					Field: "root.f.m",
+				},
+			},
+		},
+		{
+			name: "multiple rules with custom reason and field path",
+			obj: map[string]interface{}{
+				"field1": "value1",
+				"field2": "value2",
+				"field3": "value3",
+			},
+			schema: &schema.Structural{
+				Generic: schema.Generic{
+					Type: "object",
+				},
+				Properties: map[string]schema.Structural{
+					"field1": stringType,
+					"field2": stringType,
+					"field3": stringType,
+				},
+				Extensions: schema.Extensions{
+					XValidations: apiextensions.ValidationRules{
+						{
+							Rule:      `self.field2 != "value2"`,
+							Reason:    ptr.To(apiextensions.FieldValueDuplicate),
+							FieldPath: ".field2",
+						},
+						{
+							Rule:      `self.field3 != "value3"`,
+							Reason:    ptr.To(apiextensions.FieldValueRequired),
+							FieldPath: ".field3",
+						},
+						{
+							Rule:      `self.field1 != "value1"`,
+							Reason:    ptr.To(apiextensions.FieldValueForbidden),
+							FieldPath: ".field1",
+						},
+					},
+				},
+			},
+			errors: field.ErrorList{
+				{
+					Type:  field.ErrorTypeDuplicate,
+					Field: "root.field2",
+				},
+				{
+					Type:  field.ErrorTypeRequired,
+					Field: "root.field3",
+				},
+				{
+					Type:  field.ErrorTypeForbidden,
+					Field: "root.field1",
+				},
+			},
 		},
 	}
 	for _, tt := range tests {
@@ -2727,30 +2789,14 @@ func TestReasonAndFldPath(t *testing.T) {
 				t.Fatal("expected non nil validator")
 			}
 			errs, _ := celValidator.Validate(ctx, field.NewPath("root"), tt.schema, tt.obj, nil, celconfig.RuntimeCELCostBudget)
-			unmatched := map[string]struct{}{}
+
-			for _, e := range tt.errors {
+			for i := range errs {
-				unmatched[e] = struct{}{}
+				// Ignore unchecked fields for this test
-			}
+				errs[i].Detail = ""
-			for _, err := range errs {
+				errs[i].BadValue = nil
-				if err.Type != tt.errorType {
-					t.Errorf("expected error type %v, but got: %v", tt.errorType, err.Type)
-					continue
-				}
-				matched := false
-				for expected := range unmatched {
-					if strings.Contains(err.Error(), expected) {
-						delete(unmatched, expected)
-						matched = true
-						break
-					}
-				}
-				if !matched {
-					t.Errorf("expected error to contain one of %v, but got: %v", unmatched, err)
-				}
-			}
-			if len(unmatched) > 0 {
-				t.Errorf("expected errors %v", unmatched)
 			}
+
+			require.ElementsMatch(t, tt.errors, errs)
 		})
 	}
 }