CEL: Various improvements (#3)

.golangci.yml

@@ -290,6 +290,12 @@ issues:
     - stylecheck
     path: util/defaulting/defaulting.go
     text: should not use dot imports
+  # Large parts of this file are duplicate from k/k. Let's ignore "emptyStringTest" to reduce the noise in diffs
+  # and to avoid making mistakes by diverging from upstream just because of this purely stylistic linter finding.
+  - linters:
+      - gocritic
+    text: "emptyStringTest"
+    path: internal/topology/variables/clusterclass_variable_validation.go
   # Append should be able to assign to a different var/slice.
   - linters:
     - gocritic

api/v1beta1/clusterclass_types.go

@@ -463,6 +463,16 @@ type JSONSchemaProps struct {
 	// +kubebuilder:validation:Schemaless
 	AdditionalProperties *JSONSchemaProps `json:"additionalProperties,omitempty"`
 
+	// MaxProperties is the maximum amount of entries in a map or properties in an object.
+	// NOTE: Can only be set if type is object.
+	// +optional
+	MaxProperties *int64 `json:"maxProperties,omitempty"`
+
+	// MinProperties is the minimum amount of entries in a map or properties in an object.
+	// NOTE: Can only be set if type is object.
+	// +optional
+	MinProperties *int64 `json:"minProperties,omitempty"`
+
 	// Required specifies which fields of an object are required.
 	// NOTE: Can only be set if type is object.
 	// +optional
@@ -568,8 +578,7 @@ type ValidationRule struct {
 	// The Rule is scoped to the location of the x-kubernetes-validations extension in the schema.
 	// The `self` variable in the CEL expression is bound to the scoped value.
 	// If the Rule is scoped to an object with properties, the accessible properties of the object are field selectable
-	// via `self.field` and field presence can be checked via `has(self.field)`. Null valued fields are treated as
-	// absent fields in CEL expressions.
+	// via `self.field` and field presence can be checked via `has(self.field)`.
 	// If the Rule is scoped to an object with additionalProperties (i.e. a map) the value of the map
 	// are accessible via `self[mapKey]`, map containment can be checked via `mapKey in self` and all entries of the map
 	// are accessible via CEL macros and functions such as `self.all(...)`.
@@ -623,10 +632,9 @@ type ValidationRule struct {
 	// MessageExpression declares a CEL expression that evaluates to the validation failure message that is returned when this rule fails.
 	// Since messageExpression is used as a failure message, it must evaluate to a string.
 	// If both message and messageExpression are present on a rule, then messageExpression will be used if validation
-	// fails. If messageExpression results in a runtime error, the runtime error is logged, and the validation failure message is produced
+	// fails. If messageExpression results in a runtime error, the validation failure message is produced
 	// as if the messageExpression field were unset. If messageExpression evaluates to an empty string, a string with only spaces, or a string
-	// that contains line breaks, then the validation failure message will also be produced as if the messageExpression field were unset, and
-	// the fact that messageExpression produced an empty string/string with only spaces/string with line breaks will be logged.
+	// that contains line breaks, then the validation failure message will also be produced as if the messageExpression field were unset.
 	// messageExpression has access to all the same variables as the rule; the only difference is the return type.
 	// Example:
 	// "x must be less than max ("+string(self.max)+")"

internal/topology/variables/cluster_variable_defaulting.go

@@ -67,10 +67,13 @@ func defaultClusterVariables(values []clusterv1.ClusterVariable, definitions []c
 	// Default all variables.
 	defaultedValues := []clusterv1.ClusterVariable{}
 	for _, variable := range allVariables {
+		// Add variable name as key, this makes it easier to read the field path.
+		fldPath := fldPath.Key(variable.Name)
+
 		// Get the variable definition from the ClusterClass. If the variable is not defined add an error.
 		definition, err := defIndex.get(variable.Name, variable.DefinitionFrom)
 		if err != nil {
-			allErrs = append(allErrs, field.Required(fldPath, err.Error()))
+			allErrs = append(allErrs, field.Invalid(fldPath, string(variable.Value.Raw), err.Error()))
 			continue
 		}
 

internal/topology/variables/cluster_variable_defaulting_test.go

@@ -33,10 +33,14 @@ func Test_DefaultClusterVariables(t *testing.T) {
 		values          []clusterv1.ClusterVariable
 		createVariables bool
 		want            []clusterv1.ClusterVariable
-		wantErr         bool
+		wantErrs        []validationMatch
 	}{
 		{
-			name:        "Return error if variable is not defined in ClusterClass",
+			name: "Return error if variable is not defined in ClusterClass",
+			wantErrs: []validationMatch{
+				invalid("Invalid value: \"1\": no definitions found for variable \"cpu\"",
+					"spec.topology.variables[cpu]"),
+			},
 			definitions: []clusterv1.ClusterClassStatusVariable{},
 			values: []clusterv1.ClusterVariable{
 				{
@@ -47,7 +51,6 @@ func Test_DefaultClusterVariables(t *testing.T) {
 				},
 			},
 			createVariables: true,
-			wantErr:         true,
 		},
 		{
 			name: "Default one variable of each valid type",
@@ -556,8 +559,11 @@ func Test_DefaultClusterVariables(t *testing.T) {
 			},
 		},
 		{
-			name:    "Error if a value is set with empty and non-empty definitionFrom.",
-			wantErr: true,
+			name: "Error if a value is set with empty and non-empty definitionFrom.",
+			wantErrs: []validationMatch{
+				invalid("cluster variables not valid: variable \"cpu\" is defined with a mix of empty and non-empty values for definitionFrom",
+					"spec.topology.variables"),
+			},
 			definitions: []clusterv1.ClusterClassStatusVariable{
 				{
 					Name: "cpu",
@@ -600,8 +606,11 @@ func Test_DefaultClusterVariables(t *testing.T) {
 			},
 		},
 		{
-			name:    "Error if a value is set twice with the same definitionFrom.",
-			wantErr: true,
+			name: "Error if a value is set twice with the same definitionFrom.",
+			wantErrs: []validationMatch{
+				invalid("cluster variables not valid: variable \"cpu\" is defined more than once",
+					"spec.topology.variables"),
+			},
 			definitions: []clusterv1.ClusterClassStatusVariable{
 				{
 					Name: "cpu",
@@ -648,14 +657,10 @@ func Test_DefaultClusterVariables(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			vars, errList := defaultClusterVariables(tt.values, tt.definitions, tt.createVariables,
+			vars, gotErrs := defaultClusterVariables(tt.values, tt.definitions, tt.createVariables,
 				field.NewPath("spec", "topology", "variables"))
 
-			if tt.wantErr {
-				g.Expect(errList).NotTo(BeEmpty())
-				return
-			}
-			g.Expect(errList).To(BeEmpty())
+			checkErrors(t, tt.wantErrs, gotErrs)
 			g.Expect(vars).To(BeComparableTo(tt.want))
 		})
 	}
@@ -668,7 +673,7 @@ func Test_DefaultClusterVariable(t *testing.T) {
 		clusterClassVariable *statusVariableDefinition
 		createVariable       bool
 		want                 *clusterv1.ClusterVariable
-		wantErr              bool
+		wantErrs             []validationMatch
 	}{
 		{
 			name: "Default new integer variable",
@@ -1255,15 +1260,10 @@ func Test_DefaultClusterVariable(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			defaultedVariable, errList := defaultValue(tt.clusterVariable, tt.clusterClassVariable,
-				field.NewPath("spec", "topology", "variables").Index(0), tt.createVariable)
-
-			if tt.wantErr {
-				g.Expect(errList).NotTo(BeEmpty())
-				return
-			}
-			g.Expect(errList).To(BeEmpty())
+			defaultedVariable, gotErrs := defaultValue(tt.clusterVariable, tt.clusterClassVariable,
+				field.NewPath("spec", "topology", "variables").Key(tt.clusterClassVariable.Name), tt.createVariable)
 
+			checkErrors(t, tt.wantErrs, gotErrs)
 			g.Expect(defaultedVariable).To(BeComparableTo(tt.want))
 		})
 	}

internal/topology/variables/cluster_variable_validation.go

@@ -84,26 +84,30 @@ func validateClusterVariables(ctx context.Context, values, oldValues []clusterv1
 	}
 
 	for _, value := range values {
+		// Add variable name as key, this makes it easier to read the field path.
+		fldPath := fldPath.Key(value.Name)
+
 		// Values must have an associated definition and must have a non-empty definitionFrom if there are conflicting definitions.
 		definition, err := defIndex.get(value.Name, value.DefinitionFrom)
 		if err != nil {
-			allErrs = append(allErrs, field.Required(fldPath, err.Error())) // TODO: consider if to add ClusterClass name
+			allErrs = append(allErrs, field.Invalid(fldPath, string(value.Value.Raw), err.Error())) // TODO: consider if to add ClusterClass name
 			continue
 		}
 
-		// If there is an old variable matching this name and definition from defined in the old Cluster then pass this in
+		// If there is an old variable matching this name and definitionFrom defined in the old Cluster then pass this in
 		// to cluster variable validation.
-		var oldValue clusterv1.ClusterVariable
-		oldValuesForName, found := oldValuesMap[value.Name]
-		if found {
-			oldValue = oldValuesForName[value.DefinitionFrom]
+		var oldValue *clusterv1.ClusterVariable
+		if oldValuesForName, found := oldValuesMap[value.Name]; found {
+			if v, found := oldValuesForName[value.DefinitionFrom]; found {
+				oldValue = &v
+			}
 		}
 
 		// Values must be valid according to the schema in their definition.
 		allErrs = append(allErrs, ValidateClusterVariable(
 			ctx,
 			value.DeepCopy(),
-			&oldValue,
+			oldValue,
 			&clusterv1.ClusterClassVariable{
 				Name:     value.Name,
 				Required: definition.Required,
@@ -153,7 +157,6 @@ func validateRequiredVariables(values map[string]map[string]clusterv1.ClusterVar
 func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.ClusterVariable, definition *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList {
 	// Parse JSON value.
 	var variableValue interface{}
-
 	// Only try to unmarshal the clusterVariable if it is not nil, otherwise the variableValue is nil.
 	// Note: A clusterVariable with a nil value is the result of setting the variable value to "null" via YAML.
 	if value.Value.Raw != nil {
@@ -179,23 +182,31 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu
 
 	// Validate variable against the schema.
 	// NOTE: We're reusing a library func used in CRD validation.
-	if err := validation.ValidateCustomResource(fldPath, variableValue, validator); err != nil {
-		return err
+	if validationErrors := validation.ValidateCustomResource(fldPath.Child("value"), variableValue, validator); len(validationErrors) > 0 {
+		var allErrs field.ErrorList
+		for _, validationError := range validationErrors {
+			// Set correct value in the field error. ValidateCustomResource sets the type instead of the value.
+			validationError.BadValue = string(value.Value.Raw)
+			// Fixup detail message.
+			validationError.Detail = strings.TrimPrefix(validationError.Detail, " in body ")
+			allErrs = append(allErrs, validationError)
+		}
+		return allErrs
 	}
 
 	// Structural schema pruning does not work with scalar values,
 	// so we wrap the schema and the variable in objects.
 	// <variable-name>: <variable-value>
 	wrappedVariable := map[string]interface{}{
-		value.Name: variableValue,
+		"variableValue": variableValue,
 	}
 	// type: object
 	// properties:
 	//   <variable-name>: <variable-schema>
 	wrappedSchema := &apiextensions.JSONSchemaProps{
 		Type: "object",
 		Properties: map[string]apiextensions.JSONSchemaProps{
-			definition.Name: *apiExtensionsSchema,
+			"variableValue": *apiExtensionsSchema,
 		},
 	}
 	ss, err := structuralschema.NewStructural(wrappedSchema)
@@ -205,10 +216,12 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu
 				fmt.Errorf("failed to create structural schema for variable %q; ClusterClass should be checked: %v", value.Name, err))} // TODO: consider if to add ClusterClass name
 	}
 
-	if err := validateUnknownFields(fldPath, definition.Name, wrappedVariable, ss); err != nil {
+	if err := validateUnknownFields(fldPath, value, wrappedVariable, ss); err != nil {
 		return err
 	}
 
+	// Note: k/k CR validation also uses celconfig.PerCallLimit when creating the validator for a custom resource.
+	// The current PerCallLimit gives roughly 0.1 second for each expression validation call.
 	celValidator := cel.NewValidator(ss, false, celconfig.PerCallLimit)
 	// celValidation will be nil if there are no CEL validations specified in the schema
 	// under `x-kubernetes-validations`.
@@ -218,22 +231,30 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu
 
 	// Only extract old variable value if there are CEL validations and if the old variable is not nil.
 	var oldWrappedVariable map[string]interface{}
-
 	if oldValue != nil && oldValue.Value.Raw != nil {
 		var oldVariableValue interface{}
-
 		if err := json.Unmarshal(oldValue.Value.Raw, &oldVariableValue); err != nil {
 			return field.ErrorList{field.Invalid(fldPath.Child("value"), string(oldValue.Value.Raw),
 				fmt.Sprintf("old value of variable %q could not be parsed: %v", value.Name, err))}
 		}
 
 		oldWrappedVariable = map[string]interface{}{
-			definition.Name: oldVariableValue,
+			"variableValue": oldVariableValue,
 		}
 	}
 
-	if err, _ := celValidator.Validate(ctx, fldPath, ss, wrappedVariable, oldWrappedVariable, celconfig.RuntimeCELCostBudget); err != nil {
-		return err
+	// Note: k/k CRD validation also uses celconfig.RuntimeCELCostBudget for the Validate call.
+	// The current RuntimeCELCostBudget gives roughly 1 second for the validation of a variable value.
+	if validationErrors, _ := celValidator.Validate(ctx, fldPath.Child("value"), ss, wrappedVariable, oldWrappedVariable, celconfig.RuntimeCELCostBudget); len(validationErrors) > 0 {
+		var allErrs field.ErrorList
+		for _, validationError := range validationErrors {
+			// Set correct value in the field error. ValidateCustomResource sets the type instead of the value.
+			validationError.BadValue = string(value.Value.Raw)
+			// Drop "variableValue" from the path.
+			validationError.Field = strings.TrimSuffix(validationError.Field, ".variableValue")
+			allErrs = append(allErrs, validationError)
+		}
+		return allErrs
 	}
 
 	return nil
@@ -242,7 +263,7 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu
 // validateUnknownFields validates the given variableValue for unknown fields.
 // This func returns an error if there are variable fields in variableValue that are not defined in
 // variableSchema and if x-kubernetes-preserve-unknown-fields is not set.
-func validateUnknownFields(fldPath *field.Path, variableName string, wrappedVariable map[string]interface{}, ss *structuralschema.Structural) field.ErrorList {
+func validateUnknownFields(fldPath *field.Path, clusterVariable *clusterv1.ClusterVariable, wrappedVariable map[string]interface{}, ss *structuralschema.Structural) field.ErrorList {
 	// Run Prune to check if it would drop any unknown fields.
 	opts := structuralschema.UnknownFieldPathOptions{
 		// TrackUnknownFieldPaths has to be true so PruneWithOptions returns the unknown fields.
@@ -253,9 +274,14 @@ func validateUnknownFields(fldPath *field.Path, variableName string, wrappedVari
 		// If prune dropped any unknown fields, return an error.
 		// This means that not all variable fields have been defined in the variable schema and
 		// x-kubernetes-preserve-unknown-fields was not set.
+		for i := range prunedUnknownFields {
+			// Drop "variableValue" from the path.
+			prunedUnknownFields[i] = strings.TrimPrefix(prunedUnknownFields[i], "variableValue.")
+		}
+
 		return field.ErrorList{
-			field.Invalid(fldPath, "",
-				fmt.Sprintf("failed validation: %q fields are not specified in the variable schema of variable %q", strings.Join(prunedUnknownFields, ","), variableName)),
+			field.Invalid(fldPath, string(clusterVariable.Value.Raw),
+				fmt.Sprintf("failed validation: %q field(s) are not specified in the variable schema of variable %q", strings.Join(prunedUnknownFields, ","), clusterVariable.Name)),
 		}
 	}