From 2c20a3a3fc9f3ae8b839d259655567c3d53a83b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20B=C3=BCringer?= <4662360+sbueringer@users.noreply.github.com> Date: Mon, 24 Jun 2024 14:38:34 +0200 Subject: [PATCH] CEL: Various improvements (#3) --- .golangci.yml | 6 + api/v1beta1/clusterclass_types.go | 18 +- .../variables/cluster_variable_defaulting.go | 5 +- .../cluster_variable_defaulting_test.go | 44 +- .../variables/cluster_variable_validation.go | 68 +- .../cluster_variable_validation_test.go | 763 ++++++++-- .../clusterclass_variable_validation.go | 407 ++++-- .../clusterclass_variable_validation_test.go | 1268 +++++++++++++++-- internal/topology/variables/schema.go | 23 +- internal/topology/variables/schema_test.go | 32 +- internal/topology/variables/utils.go | 6 +- internal/webhooks/cluster_test.go | 307 +++- internal/webhooks/clusterclass.go | 6 +- internal/webhooks/clusterclass_test.go | 73 +- .../main/clusterclass-quick-start.yaml | 3 + .../handlers/topologymutation/handler.go | 8 +- 16 files changed, 2542 insertions(+), 495 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 38242b5312cf..6f9b4affc401 100644 --- a/.golangci.yml +++ b/.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 diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index 0005507cb610..fb1bad89edd6 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/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)+")" diff --git a/internal/topology/variables/cluster_variable_defaulting.go b/internal/topology/variables/cluster_variable_defaulting.go index 3f4a38ad344c..1b9d10364218 100644 --- a/internal/topology/variables/cluster_variable_defaulting.go +++ b/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 } diff --git a/internal/topology/variables/cluster_variable_defaulting_test.go b/internal/topology/variables/cluster_variable_defaulting_test.go index a874fb0a55a2..e18f18f5ee54 100644 --- a/internal/topology/variables/cluster_variable_defaulting_test.go +++ b/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)) }) } diff --git a/internal/topology/variables/cluster_variable_validation.go b/internal/topology/variables/cluster_variable_validation.go index 32f7013185a4..1507234b8ace 100644 --- a/internal/topology/variables/cluster_variable_validation.go +++ b/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,15 +182,23 @@ 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. // : wrappedVariable := map[string]interface{}{ - value.Name: variableValue, + "variableValue": variableValue, } // type: object // properties: @@ -195,7 +206,7 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu 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)), } } diff --git a/internal/topology/variables/cluster_variable_validation_test.go b/internal/topology/variables/cluster_variable_validation_test.go index c769a1a0a8f5..18d6804b6803 100644 --- a/internal/topology/variables/cluster_variable_validation_test.go +++ b/internal/topology/variables/cluster_variable_validation_test.go @@ -19,7 +19,6 @@ package variables import ( "testing" - . "github.com/onsi/gomega" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" @@ -36,8 +35,7 @@ func Test_ValidateClusterVariables(t *testing.T) { definitions []clusterv1.ClusterClassStatusVariable values []clusterv1.ClusterVariable validateRequired bool - wantErr bool - wantErrMessage string + wantErrs []validationMatch }{ { name: "Pass for a number of valid values.", @@ -114,8 +112,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Error when no value for required definition.", - wantErr: true, + name: "Error when no value for required definition.", + wantErrs: []validationMatch{ + required("Required value: required variable with name \"cpu\" must be defined", + "spec.topology.variables"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -207,8 +208,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: false, }, { - name: "Error if value has no definition.", - wantErr: true, + name: "Error if value has no definition.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"us-east-1\\\"\": no definitions found for variable \"location\"", + "spec.topology.variables[location]"), + }, definitions: []clusterv1.ClusterClassStatusVariable{}, values: []clusterv1.ClusterVariable{ // location has a value but no definition. @@ -306,8 +310,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail if value DefinitionFrom field does not match any definition.", - wantErr: true, + name: "Fail if value DefinitionFrom field does not match any definition.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"1\": no definitions found for variable \"cpu\" from \"non-existent-patch\"", + "spec.topology.variables[cpu]"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -336,8 +343,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail if a value is set twice with the same definitionFrom.", - wantErr: true, + name: "Fail if a value is set twice with the same definitionFrom.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"[Name: cpu DefinitionFrom: somepatch,Name: cpu DefinitionFrom: somepatch]\": cluster variables not valid: variable \"cpu\" from \"somepatch\" is defined more than once", + "spec.topology.variables"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -372,8 +382,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail if a value is set with empty and non-empty definitionFrom.", - wantErr: true, + name: "Fail if a value is set with empty and non-empty definitionFrom.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"[Name: cpu DefinitionFrom: ,Name: cpu DefinitionFrom: somepatch]\": 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", @@ -417,8 +430,13 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail when values invalid by their definition schema.", - wantErr: true, + name: "Fail when values invalid by their definition schema.", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"1\": must be of type string: \"integer\"", + "spec.topology.variables[cpu].value"), + invalidType("Invalid value: \"\\\"one\\\"\": must be of type integer: \"string\"", + "spec.topology.variables[cpu].value"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -548,8 +566,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail if value doesn't include definitionFrom when definitions conflict.", - wantErr: true, + name: "Fail if value doesn't include definitionFrom when definitions conflict.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"1\": variable \"cpu\" has conflicting definitions. It requires a non-empty `definitionFrom`", + "spec.topology.variables[cpu]"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -587,8 +608,11 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired: true, }, { - name: "Fail if value doesn't include definitionFrom for each required definition when definitions conflict.", - wantErr: true, + name: "Fail if value doesn't include definitionFrom for each required definition when definitions conflict.", + wantErrs: []validationMatch{ + required("Required value: required variable with name \"cpu\" from \"inline\" must be defined", + "spec.topology.variables"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -631,16 +655,10 @@ func Test_ValidateClusterVariables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := validateClusterVariables(ctx, tt.values, nil, tt.definitions, + gotErrs := validateClusterVariables(ctx, tt.values, nil, tt.definitions, tt.validateRequired, field.NewPath("spec", "topology", "variables")) - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } @@ -650,8 +668,7 @@ func Test_ValidateClusterVariable(t *testing.T) { name string clusterClassVariable *clusterv1.ClusterClassVariable clusterVariable *clusterv1.ClusterVariable - wantErr bool - wantErrMessage string + wantErrs []validationMatch }{ { name: "Valid integer", @@ -673,8 +690,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if integer is above Maximum", - wantErr: true, + name: "Error if integer is above Maximum", + wantErrs: []validationMatch{ + invalid("Invalid value: \"99\": should be less than or equal to 10", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -693,8 +713,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if integer is below Minimum", - wantErr: true, + name: "Error if integer is below Minimum", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": should be greater than or equal to 1", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -714,8 +737,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, { - name: "Fails, expected integer got string", - wantErr: true, + name: "Fails, expected integer got string", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"\\\"1\\\"\": must be of type integer: \"string\"", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -753,8 +779,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if string doesn't match pattern ", - wantErr: true, + name: "Error if string doesn't match pattern ", + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"000000a\\\"\": should match '^[0-9]+$'", + "spec.topology.variables[location].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "location", Required: true, @@ -773,8 +802,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if string doesn't match format ", - wantErr: true, + name: "Error if string doesn't match format ", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"\\\"not a URI\\\"\": must be of type uri: \"not a URI\"", + "spec.topology.variables[location].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "location", Required: true, @@ -815,8 +847,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Fails, value does not match one of the enum string values", - wantErr: true, + name: "Fails, value does not match one of the enum string values", + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"\\\"us-east-invalid\\\"\": supported values: \"us-east-1\", \"us-east-2\"", + "spec.topology.variables[location].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "location", Required: true, @@ -860,8 +895,13 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Fails, value does not match one of the enum integer values", - wantErr: true, + name: "Fails, value does not match one of the enum integer values", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"3\": must be of type string: \"integer\"", + "spec.topology.variables[location].value"), + unsupported("Unsupported value: \"3\": supported values: \"1\", \"2\"", + "spec.topology.variables[location].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "location", Required: true, @@ -906,8 +946,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if nested field is invalid", - wantErr: true, + name: "Error if nested field is invalid", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"{\\\"enabled\\\":\\\"not-a-bool\\\"}\": enabled in body must be of type boolean: \"string\"", + "spec.topology.variables[httpProxy].value.enabled"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "httpProxy", Required: true, @@ -930,8 +973,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if object is a bool instead", - wantErr: true, + name: "Error if object is a bool instead", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"\\\"not-a-object\\\"\": must be of type object: \"string\"", + "spec.topology.variables[httpProxy].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "httpProxy", Required: true, @@ -954,8 +1000,13 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if object is missing required field", - wantErr: true, + name: "Error if object is missing required field", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"{\\\"enabled\\\":\\\"true\\\"}\": enabled in body must be of type boolean: \"string\"", + "spec.topology.variables[httpProxy].value.enabled"), + required("Required value", + "spec.topology.variables[httpProxy].value.url"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "httpProxy", Required: true, @@ -983,6 +1034,50 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + { + name: "Error if object has too many properties", + wantErrs: []validationMatch{ + toomany("Too many: \"{\\\"requiredProperty\\\":false,\\\"boolProperty\\\":true,\\\"integerProperty\\\":1,\\\"enumProperty\\\":\\\"enumValue2\\\"}\": must have at most 2 items", + "spec.topology.variables[testObject].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "testObject", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + MaxProperties: ptr.To[int64](2), + Properties: map[string]clusterv1.JSONSchemaProps{ + "requiredProperty": { + Type: "boolean", + }, + "boolProperty": { + Type: "boolean", + }, + "integerProperty": { + Type: "integer", + Minimum: ptr.To[int64](1), + }, + "enumProperty": { + Type: "string", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`"enumValue1"`)}, + {Raw: []byte(`"enumValue2"`)}, + }, + }, + }, + Required: []string{"requiredProperty"}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "testObject", + Value: apiextensionsv1.JSON{ + // Object has 4 properties, allowed are up to 2. + Raw: []byte(`{"requiredProperty":false,"boolProperty":true,"integerProperty":1,"enumProperty":"enumValue2"}`), + }, + }, + }, { name: "Valid object", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1052,8 +1147,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Fails, value does not match one of the enum object values", - wantErr: true, + name: "Fails, value does not match one of the enum object values", + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"{\\\"location\\\": \\\"us-east-2\\\",\\\"url\\\":\\\"wrong-url\\\"}\": supported values: \"{\\\"location\\\":\\\"us-east-1\\\",\\\"url\\\":\\\"us-east-1-url\\\"}\", \"{\\\"location\\\":\\\"us-east-2\\\",\\\"url\\\":\\\"us-east-2-url\\\"}\"", + "spec.topology.variables[enumObject].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "enumObject", Required: true, @@ -1109,8 +1207,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if map is missing a required field", - wantErr: true, + name: "Error if map is missing a required field", + wantErrs: []validationMatch{ + required("Required value", + "spec.topology.variables[httpProxy].value.proxy.url"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "httpProxy", Required: true, @@ -1139,6 +1240,38 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + { + name: "Error if map has too many entries", + wantErrs: []validationMatch{ + toomany("Too many: \"{\\\"proxy\\\":{\\\"enabled\\\":false},\\\"proxy2\\\":{\\\"enabled\\\":false},\\\"proxy3\\\":{\\\"enabled\\\":false}}\": must have at most 2 items", + "spec.topology.variables[httpProxy].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "httpProxy", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + MaxProperties: ptr.To[int64](2), + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "httpProxy", + Value: apiextensionsv1.JSON{ + // Map has 3 entries, allowed are up to 2. + Raw: []byte(`{"proxy":{"enabled":false},"proxy2":{"enabled":false},"proxy3":{"enabled":false}}`), + }, + }, + }, { name: "Valid array", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1165,8 +1298,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if array element is invalid", - wantErr: true, + name: "Error if array element is invalid", + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"[\\\"enumValue1\\\",\\\"enumValueInvalid\\\"]\": supported values: \"enumValue1\", \"enumValue2\"", + "spec.topology.variables[testArray].value.[1]"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testArray", Required: true, @@ -1191,8 +1327,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if array is too large", - wantErr: true, + name: "Error if array is too large", + wantErrs: []validationMatch{ + toomany("Too many: \"[\\\"value1\\\",\\\"value2\\\",\\\"value3\\\",\\\"value4\\\"]\": must have at most 3 items", + "spec.topology.variables[testArray].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testArray", Required: true, @@ -1214,8 +1353,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if array is too small", - wantErr: true, + name: "Error if array is too small", + wantErrs: []validationMatch{ + invalid("Invalid value: \"[\\\"value1\\\",\\\"value2\\\"]\": should have at least 3 items", + "spec.topology.variables[testArray].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testArray", Required: true, @@ -1237,8 +1379,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if array contains duplicate values", - wantErr: true, + name: "Error if array contains duplicate values", + wantErrs: []validationMatch{ + invalid("Invalid value: \"[\\\"value1\\\",\\\"value1\\\"]\": shouldn't contain duplicates", + "spec.topology.variables[testArray].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testArray", Required: true, @@ -1285,8 +1430,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Fails, value does not match one of the enum array values", - wantErr: true, + name: "Fails, value does not match one of the enum array values", + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"[\\\"7\\\",\\\"8\\\",\\\"9\\\"]\": supported values: \"[\\\"1\\\",\\\"2\\\",\\\"3\\\"]\", \"[\\\"4\\\",\\\"5\\\",\\\"6\\\"]\"", + "spec.topology.variables[enumArray].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "enumArray", Required: true, @@ -1336,8 +1484,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if undefined field", - wantErr: true, + name: "Error if undefined field", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"knownProperty\\\":false,\\\"unknownProperty\\\":true}\": failed validation: \"unknownProperty\" field(s) are not specified in the variable schema of variable \"testObject\"", + "spec.topology.variables[testObject]"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testObject", Required: true, @@ -1361,8 +1512,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if undefined field with different casing", - wantErr: true, + name: "Error if undefined field with different casing", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"KnownProperty\\\":false}\": failed validation: \"KnownProperty\" field(s) are not specified in the variable schema of variable \"testObject\"", + "spec.topology.variables[testObject]"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testObject", Required: true, @@ -1437,8 +1591,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if undefined field nested", - wantErr: true, + name: "Error if undefined field nested", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"test\\\": {\\\"knownProperty\\\":false,\\\"unknownProperty\\\":true}}\": failed validation: \"test.unknownProperty\" field(s) are not specified in the variable schema of variable \"testObject\"", + "spec.topology.variables[testObject]"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testObject", Required: true, @@ -1467,8 +1624,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if undefined field nested and x-kubernetes-preserve-unknown-fields one level above", - wantErr: true, + name: "Error if undefined field nested and x-kubernetes-preserve-unknown-fields one level above", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"test\\\": {\\\"knownProperty\\\":false,\\\"unknownProperty\\\":true}}\": failed validation: \"test.unknownProperty\" field(s) are not specified in the variable schema of variable \"testObject\"", + "spec.topology.variables[testObject]"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "testObject", Required: true, @@ -1529,7 +1689,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, { - name: "Valid integer with CEL expression", + name: "Valid CEL expression: scalar: using self", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1550,9 +1710,156 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if integer is above maximum via with CEL expression", - wantErr: true, - wantErrMessage: `failed rule: self <= 1`, + name: "Valid CEL expression: special characters", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.__namespace__", + }, { + Rule: "self.x__dash__prop", + }, { + Rule: "self.redact__underscores__d", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "namespace": { // keyword + Type: "boolean", + }, + "x-prop": { + Type: "boolean", + }, + "redact__d": { + Type: "boolean", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"namespace":true,"x-prop":true,"redact__d":true}`), + }, + }, + }, + { + name: "Valid CEL expression: objects: using self.field, has(self.field)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.field <= 1", + }, { + Rule: "has(self.field)", + }, { + Rule: "!has(self.field2)", // field2 is absent in the value + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "field": { + Type: "integer", + }, + "field2": { + Type: "integer", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field": 1}`), + }, + }, + }, + { + name: "Valid CEL expression: maps: using self[mapKey], mapKey in self, self.all, equality", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self['key1'] == 1", + }, { + Rule: "'key1' in self", + }, { + Rule: "self.all(value, self[value] >= 1)", + }, { + Rule: "self == {'key1':1,'key2':2}", + }, { + Rule: "self == {'key2':2,'key1':1}", // order does not matter + }}, + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"key1":1,"key2":2}`), + }, + }, + }, + { + name: "Valid CEL expression: arrays: using self[i], self.all, equality", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self[0] == 1", + }, { + Rule: "self[1] == 2", + }, { + Rule: "self.all(value, value >= 1)", + }, { + Rule: "self == [1,2]", + }, { + Rule: "self != [2,1]", // order matters + }, { + Rule: "self + [3] == [1,2,3]", + }, { + Rule: "self + [3] == [1,2] + [3]", + }, { + Rule: "self + [3] != [3,1,2]", + }, { + Rule: "[3] + self == [3,1,2]", + }, { + Rule: "[3] + self == [3] + [1,2]", + }, { + Rule: "[3] + self != [1,2,3]", + }}, + Items: &clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`[1,2]`), + }, + }, + }, + { + name: "Error if integer is above maximum via with CEL expression", + wantErrs: []validationMatch{ + invalid("Invalid value: \"99\": failed rule: self <= 1", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1573,9 +1880,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if integer is below minimum via CEL expression", - wantErr: true, - wantErrMessage: `failed rule: self >= 1`, + name: "Error if integer is below minimum via CEL expression", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": failed rule: self >= 1", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1596,9 +1905,11 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, { - name: "Error if integer is below minimum via CEL expression with custom error message", - wantErr: true, - wantErrMessage: `new value must be greater than or equal to 1`, + name: "Error if integer is below minimum via CEL expression with custom error message", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": new value must be greater than or equal to 1", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1619,22 +1930,127 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + { + name: "Error if integer is below minimum via CEL expression with custom error message (messageExpression preferred)", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": new value must be greater than or equal to 1, but got 0", + "spec.topology.variables[cpu].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + Message: "new value must be greater than or equal to 1", + MessageExpression: "'new value must be greater than or equal to 1, but got %d'.format([self])", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + { + name: "Error if integer is below minimum via CEL expression with custom error message (fallback to message if messageExpression fails)", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": new value must be greater than or equal to 1", + "spec.topology.variables[cpu].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + Message: "new value must be greater than or equal to 1", + MessageExpression: "''", // This evaluates to an empty string, and thus message is used instead. + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + { + name: "Valid CEL expression: objects: defined field can be accessed", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XPreserveUnknownFields: true, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.field <= 1", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field": 1,"field3": null}`), // unknown field3 is preserved, but not used in CEL + }, + }, + }, + { + name: "Invalid CEL expression: objects: unknown field cannot be accessed", + wantErrs: []validationMatch{ + invalid("rule compile error: compilation failed: ERROR: :1:5: undefined field 'field3'", + "spec.topology.variables[cpu].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XPreserveUnknownFields: true, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.field <= 1", + }, { + Rule: "self.field3 <= 1", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "field": { + Type: "integer", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field": 1,"field3": null}`), // unknown field3 is preserved, but checking it via CEL fails + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := ValidateClusterVariable(ctx, tt.clusterVariable, nil, tt.clusterClassVariable, - field.NewPath("spec", "topology", "variables")) + gotErrs := ValidateClusterVariable(ctx, tt.clusterVariable, nil, tt.clusterClassVariable, + field.NewPath("spec", "topology", "variables").Key(tt.clusterClassVariable.Name)) - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) - if tt.wantErrMessage != "" { - g.Expect(errList[0].Error()).To(ContainSubstring(tt.wantErrMessage)) - } - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } @@ -1645,7 +2061,7 @@ func Test_ValidateClusterVariable_CELTransitions(t *testing.T) { clusterClassVariable *clusterv1.ClusterClassVariable clusterVariable *clusterv1.ClusterVariable oldClusterVariable *clusterv1.ClusterVariable - wantErrMessage string + wantErrs []validationMatch }{ { name: "Valid transition if old value is not set", @@ -1696,8 +2112,11 @@ func Test_ValidateClusterVariable_CELTransitions(t *testing.T) { }, }, { - name: "Error if integer is not greater than old value via CEL expression", - wantErrMessage: `failed rule: self > oldSelf`, + name: "Error if integer is not greater than old value via CEL expression", + wantErrs: []validationMatch{ + invalid("failed rule: self > oldSelf", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1724,8 +2143,11 @@ func Test_ValidateClusterVariable_CELTransitions(t *testing.T) { }, }, { - name: "Error if integer is not greater than old value via CEL expression with custom error message", - wantErrMessage: `new value must be greater than old value`, + name: "Error if integer is not greater than old value via CEL expression with custom error message", + wantErrs: []validationMatch{ + invalid("new value must be greater than old value", + "spec.topology.variables[cpu].value"), + }, clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Required: true, @@ -1752,20 +2174,83 @@ func Test_ValidateClusterVariable_CELTransitions(t *testing.T) { }, }, }, + { + name: "Pass immutability check if value did not change", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.field == oldSelf.field", + Message: "field is immutable", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "field": { + Type: "string", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field":"value1"}`), + }, + }, + oldClusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field":"value1"}`), + }, + }, + }, + { + name: "Fail immutability check if value changes", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"field\\\":\\\"value2\\\"}\": field is immutable, but field was changed from \"value1\" to \"value2\"", + "spec.topology.variables[cpu].value"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.field == oldSelf.field", + MessageExpression: "'field is immutable, but field was changed from \"%s\" to \"%s\"'.format([oldSelf.field,self.field])", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "field": { + Type: "string", + }, + }, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field":"value2"}`), + }, + }, + oldClusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`{"field":"value1"}`), + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) + gotErrs := ValidateClusterVariable(ctx, tt.clusterVariable, tt.oldClusterVariable, tt.clusterClassVariable, + field.NewPath("spec", "topology", "variables").Key(tt.clusterVariable.Name)) - errList := ValidateClusterVariable(ctx, tt.clusterVariable, tt.oldClusterVariable, tt.clusterClassVariable, - field.NewPath("spec", "topology", "variables")) - - if tt.wantErrMessage != "" { - g.Expect(errList).To(HaveLen(1)) - g.Expect(errList[0].Error()).To(ContainSubstring(tt.wantErrMessage)) - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } @@ -1776,10 +2261,10 @@ func Test_ValidateMachineVariables(t *testing.T) { definitions []clusterv1.ClusterClassStatusVariable values []clusterv1.ClusterVariable oldValues []clusterv1.ClusterVariable - wantErr bool + wantErrs []validationMatch }{ { - name: "Pass when no value for required definition.", + name: "Pass when no value for required definition (required variables are not required for overrides)", definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -1823,8 +2308,11 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Error if value has no definition.", - wantErr: true, + name: "Error if value has no definition.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"us-east-1\\\"\": no definitions found for variable \"location\"", + "spec.topology.workers.machineDeployments[0].variables.overrides[location]"), + }, definitions: []clusterv1.ClusterClassStatusVariable{}, values: []clusterv1.ClusterVariable{ // location has a value but no definition. @@ -1837,8 +2325,11 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Fail if value DefinitionFrom field does not match any definition.", - wantErr: true, + name: "Fail if value DefinitionFrom field does not match any definition.", + wantErrs: []validationMatch{ + invalid("Invalid value: \"1\": no definitions found for variable \"cpu\" from \"non-existent-patch\"", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu]"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -1866,8 +2357,13 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Fail when values invalid by their definition schema.", - wantErr: true, + name: "Fail when values invalid by their definition schema.", + wantErrs: []validationMatch{ + invalidType("Invalid value: \"1\": must be of type string: \"integer\"", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value"), + invalidType("Invalid value: \"\\\"one\\\"\": must be of type integer: \"string\"", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -1896,14 +2392,14 @@ func Test_ValidateMachineVariables(t *testing.T) { { Name: "cpu", Value: apiextensionsv1.JSON{ - Raw: []byte(`1`), + Raw: []byte(`1`), // not a string }, DefinitionFrom: "inline", }, { Name: "cpu", Value: apiextensionsv1.JSON{ - Raw: []byte(`"one"`), + Raw: []byte(`"one"`), // not an integer }, DefinitionFrom: "somepatch", }, @@ -1940,8 +2436,11 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Error if integer is above maximum via with CEL expression", - wantErr: true, + name: "Error if integer is above maximum via with CEL expression", + wantErrs: []validationMatch{ + invalid("Invalid value: \"99\": failed rule: self <= 1", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -1971,8 +2470,11 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Error if integer is below minimum via CEL expression", - wantErr: true, + name: "Error if integer is below minimum via CEL expression", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": failed rule: self >= 1", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -2070,8 +2572,11 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, { - name: "Error if integer is not greater than old value via CEL expression", - wantErr: true, + name: "Error if integer is not greater than old value via CEL expression", + wantErrs: []validationMatch{ + invalid("Invalid value: \"0\": failed rule: self > oldSelf", + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value"), + }, definitions: []clusterv1.ClusterClassStatusVariable{ { Name: "cpu", @@ -2111,16 +2616,10 @@ func Test_ValidateMachineVariables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := ValidateMachineVariables(ctx, tt.values, tt.oldValues, tt.definitions, + gotErrs := ValidateMachineVariables(ctx, tt.values, tt.oldValues, tt.definitions, field.NewPath("spec", "topology", "workers", "machineDeployments").Index(0).Child("variables", "overrides")) - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } diff --git a/internal/topology/variables/clusterclass_variable_validation.go b/internal/topology/variables/clusterclass_variable_validation.go index 51082d456ddc..50878e615c91 100644 --- a/internal/topology/variables/clusterclass_variable_validation.go +++ b/internal/topology/variables/clusterclass_variable_validation.go @@ -23,6 +23,7 @@ import ( "regexp" "strings" + celgo "github.com/google/cel-go/cel" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsvalidation "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" @@ -46,33 +47,49 @@ const ( // StaticEstimatedCostLimit represents the largest-allowed static CEL cost on a per-expression basis. StaticEstimatedCostLimit = apiextensionsvalidation.StaticEstimatedCostLimit - // StaticEstimatedCRDCostLimit represents the largest-allowed total cost for the x-kubernetes-validations rules of a CRD. + // StaticEstimatedCRDCostLimit represents the largest-allowed total cost for the x-kubernetes-validations rules of a variable. StaticEstimatedCRDCostLimit = apiextensionsvalidation.StaticEstimatedCRDCostLimit ) // ValidateClusterClassVariables validates clusterClassVariable. -func ValidateClusterClassVariables(ctx context.Context, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { +func ValidateClusterClassVariables(ctx context.Context, oldClusterClassVariables, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList allErrs = append(allErrs, validateClusterClassVariableNamesUnique(clusterClassVariables, fldPath)...) - for i := range clusterClassVariables { - allErrs = append(allErrs, validateClusterClassVariable(ctx, &clusterClassVariables[i], fldPath.Index(i))...) + oldClusterClassVariablesMap := map[string]clusterv1.ClusterClassVariable{} + for _, variable := range oldClusterClassVariables { + oldClusterClassVariablesMap[variable.Name] = variable + } + + for _, clusterClassVariable := range clusterClassVariables { + // Add variable name as key, this makes it easier to read the field path. + fldPath := fldPath.Key(clusterClassVariable.Name) + + clusterClassVariable := clusterClassVariable + var oldClusterClassVariable *clusterv1.ClusterClassVariable + if v, exists := oldClusterClassVariablesMap[clusterClassVariable.Name]; exists { + oldClusterClassVariable = &v + } + allErrs = append(allErrs, validateClusterClassVariable(ctx, oldClusterClassVariable, &clusterClassVariable, fldPath)...) } return allErrs } // validateClusterClassVariableNamesUnique validates that ClusterClass variable names are unique. -func validateClusterClassVariableNamesUnique(clusterClassVariables []clusterv1.ClusterClassVariable, pathPrefix *field.Path) field.ErrorList { +func validateClusterClassVariableNamesUnique(clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList variableNames := sets.Set[string]{} - for i, clusterClassVariable := range clusterClassVariables { + for _, clusterClassVariable := range clusterClassVariables { + // Add variable name as key, this makes it easier to read the field path. + fldPath := fldPath.Key(clusterClassVariable.Name) + if variableNames.Has(clusterClassVariable.Name) { allErrs = append(allErrs, field.Invalid( - pathPrefix.Index(i).Child("name"), + fldPath.Child("name"), clusterClassVariable.Name, fmt.Sprintf("variable name must be unique. Variable with name %q is defined more than once", clusterClassVariable.Name), ), @@ -85,7 +102,7 @@ func validateClusterClassVariableNamesUnique(clusterClassVariables []clusterv1.C } // validateClusterClassVariable validates a ClusterClassVariable. -func validateClusterClassVariable(ctx context.Context, variable *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { +func validateClusterClassVariable(ctx context.Context, oldVariable, variable *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} // Validate variable name. @@ -95,7 +112,7 @@ func validateClusterClassVariable(ctx context.Context, variable *clusterv1.Clust allErrs = append(allErrs, validateClusterClassVariableMetadata(variable.Metadata, fldPath.Child("metadata"))...) // Validate schema. - allErrs = append(allErrs, validateRootSchema(ctx, variable, fldPath.Child("schema", "openAPIV3Schema"))...) + allErrs = append(allErrs, validateRootSchema(ctx, oldVariable, variable, fldPath.Child("schema", "openAPIV3Schema"))...) return allErrs } @@ -134,13 +151,30 @@ func validateClusterClassVariableMetadata(metadata clusterv1.ClusterClassVariabl var validVariableTypes = sets.Set[string]{}.Insert("object", "array", "string", "number", "integer", "boolean") // validateRootSchema validates the schema. -func validateRootSchema(ctx context.Context, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { +func validateRootSchema(ctx context.Context, oldClusterClassVariables, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - apiExtensionsSchema, allErrs := convertToAPIExtensionsJSONSchemaProps(&clusterClassVariable.Schema.OpenAPIV3Schema, field.NewPath("schema")) - if len(allErrs) > 0 { - return field.ErrorList{field.InternalError(fldPath, - fmt.Errorf("failed to convert schema definition for variable %q; ClusterClass should be checked: %v", clusterClassVariable.Name, allErrs))} // TODO: consider if to add ClusterClass name + apiExtensionsSchema, validationErrors := convertToAPIExtensionsJSONSchemaProps(&clusterClassVariable.Schema.OpenAPIV3Schema, fldPath) + if len(validationErrors) > 0 { + for _, validationError := range validationErrors { + // Add context to the error message. + validationError.Detail = fmt.Sprintf("failed to convert schema definition for variable %q; ClusterClass should be checked: %s", clusterClassVariable.Name, validationError.Detail) // TODO: consider if to add ClusterClass name + allErrs = append(allErrs, validationError) + } + return allErrs + } + + var oldAPIExtensionsSchema *apiextensions.JSONSchemaProps + if oldClusterClassVariables != nil { + oldAPIExtensionsSchema, validationErrors = convertToAPIExtensionsJSONSchemaProps(&oldClusterClassVariables.Schema.OpenAPIV3Schema, fldPath) + if len(validationErrors) > 0 { + for _, validationError := range validationErrors { + // Add context to the error message. + validationError.Detail = fmt.Sprintf("failed to convert old schema definition for variable %q; ClusterClass should be checked: %s", clusterClassVariable.Name, validationError.Detail) // TODO: consider if to add ClusterClass name + allErrs = append(allErrs, validationError) + } + return allErrs + } } // Validate structural schema. @@ -156,43 +190,82 @@ func validateRootSchema(ctx context.Context, clusterClassVariable *clusterv1.Clu } // Get the structural schema for the variable. - ss, err := structuralschema.NewStructural(wrappedSchema) + wrappedStructural, err := structuralschema.NewStructural(wrappedSchema) if err != nil { - return append(allErrs, field.Invalid(fldPath.Child("schema"), "", err.Error())) + return append(allErrs, field.Invalid(fldPath, "", err.Error())) } // Validate the schema. - if validationErrors := structuralschema.ValidateStructural(fldPath.Child("schema"), ss); len(validationErrors) > 0 { - return append(allErrs, validationErrors...) + if validationErrors := structuralschema.ValidateStructural(fldPath, wrappedStructural); len(validationErrors) > 0 { + for _, validationError := range validationErrors { + // Remove .properties[variableSchema], this only exists because we had to wrap via wrappedSchema. + validationError.Field = strings.Replace(validationError.Field, "openAPIV3Schema.properties[variableSchema]", "openAPIV3Schema", 1) + allErrs = append(allErrs, validationError) + } + return allErrs } + ss := wrappedStructural.Properties["variableSchema"] // Validate defaults in the structural schema. - validationErrors, err := structuraldefaulting.ValidateDefaults(ctx, fldPath.Child("schema"), ss, true, true) + // Note: Since convertToAPIExtensionsJSONSchemaProps converts the XValidations, + // this func internally now also uses CEL to validate the defaults values. + validationErrors, err = structuraldefaulting.ValidateDefaults(ctx, fldPath, &ss, false, true) if err != nil { - return append(allErrs, field.Invalid(fldPath.Child("schema"), "", err.Error())) + return append(allErrs, field.Invalid(fldPath, "", err.Error())) } if len(validationErrors) > 0 { return append(allErrs, validationErrors...) } - // If the structural schema is valid, ensure a schema validator can be constructed. - if _, _, err := validation.NewSchemaValidator(apiExtensionsSchema); err != nil { - return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build validator: %v", err))) + celContext := apiextensionsvalidation.RootCELContext(apiExtensionsSchema) + + // Context: + // * EnvSets are CEL environments configured with various options (https://github.com/kubernetes/kubernetes/blob/v1.30.0/staging/src/k8s.io/apiserver/pkg/cel/environment/base.go#L49) + // * Options can be e.g. library functions (string, regex) or types (IP, CIDR) + // * Over time / Kubernetes versions new options might be added and older ones removed + // * When creating an EnvSet two environments are created: + // * one environment based on the passed in Kubernetes version. We use environment.DefaultCompatibilityVersion(), + // which is either n-1 or an even earlier version (called "n-1" in the following) + // * one environment with vMaxUint.MaxUint (called "max" in the following") + // * The following is implemented similar to how Kubernetes implements it for CRD create/update and CR validation. + // * CAPI validation: + // * If a ClusterClass create or update introduces a new CEL expression, we will validate it with the "n-1" env + // * If a ClusterClass update keeps a CEL expression unchanged, it will be validated with the "max" env + // * On Cluster create or update, variable values will always be validated with the "max" env + // * The goal is to only allow adding new CEL expressions that are compatible with "n-1", + // while allowing to use pre-existing CEL expressions that are compatible with "max". + // * We have to do this because: + // * This makes it possible to rollback Cluster API from "m" to "m-1", because Cluster API "m-1" is + // able to use CEL expressions created with Cluster API "m" (because "m" only allows to add CEL expressions + // that can be used by "m-1"). + // * The Kubernetes CEL library assumes this behavior and it's baked into cel.NewValidator (they use n to validate) + // * If the DefaultCompatibilityVersion is an earlier version than "n-1", it means we could roll back even more than 1 version. + opts := &validationOptions{ + celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), + } + if oldAPIExtensionsSchema != nil { + opts.preexistingExpressions = findPreexistingExpressions(oldAPIExtensionsSchema) } - celContext := apiextensionsvalidation.RootCELContext(apiExtensionsSchema) - allErrs = append(allErrs, validateSchema(apiExtensionsSchema, fldPath, celContext, nil)...) - if celContext != nil { - if celContext.TotalCost != nil && celContext.TotalCost.Total > StaticEstimatedCRDCostLimit { + allErrs = append(allErrs, validateSchema(ctx, apiExtensionsSchema, fldPath, opts, celContext, nil)...) + if celContext != nil && celContext.TotalCost != nil { + if celContext.TotalCost.Total > StaticEstimatedCRDCostLimit { for _, expensive := range celContext.TotalCost.MostExpensive { - costErrorMsg := "contributed to estimated rule cost total exceeding cost limit for entire OpenAPIv3 schema" + costErrorMsg := "contributed to estimated cost total exceeding cost limit for entire OpenAPIv3 schema" allErrs = append(allErrs, field.Forbidden(expensive.Path, costErrorMsg)) } - costErrorMsg := getCostErrorMessage("x-kubernetes-validations estimated rule cost total for entire OpenAPIv3 schema", celContext.TotalCost.Total, StaticEstimatedCRDCostLimit) + costErrorMsg := getCostErrorMessage("x-kubernetes-validations estimated cost total for entire OpenAPIv3 schema", celContext.TotalCost.Total, StaticEstimatedCRDCostLimit) allErrs = append(allErrs, field.Forbidden(fldPath, costErrorMsg)) } } + + // If the structural schema is valid, ensure a schema validator can be constructed. + if len(allErrs) == 0 { + if _, _, err := validation.NewSchemaValidator(apiExtensionsSchema); err != nil { + return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build validator: %v", err))) + } + } return allErrs } @@ -203,7 +276,7 @@ var supportedValidationReason = sets.NewString( string(clusterv1.FieldValueDuplicate), ) -func validateSchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, celContext *apiextensionsvalidation.CELSchemaContext, uncorrelatablePath *field.Path) field.ErrorList { +func validateSchema(ctx context.Context, schema *apiextensions.JSONSchemaProps, fldPath *field.Path, opts *validationOptions, celContext *apiextensionsvalidation.CELSchemaContext, uncorrelatablePath *field.Path) field.ErrorList { var allErrs field.ErrorList // Validate that type is one of the validVariableTypes. @@ -217,166 +290,218 @@ func validateSchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, // If the structural schema is valid, ensure a schema validator can be constructed. validator, _, err := validation.NewSchemaValidator(schema) if err != nil { - return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build validator: %v", err))) + return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("failed to build schema validator: %v", err))) } if schema.Example != nil { - if errs := validation.ValidateCustomResource(fldPath, *schema.Example, validator); len(errs) > 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("example"), schema.Example, fmt.Sprintf("invalid value in example: %v", errs))) + if errs := validation.ValidateCustomResource(fldPath.Child("example"), *schema.Example, validator); len(errs) > 0 { + allErrs = append(allErrs, errs...) } } for i, enum := range schema.Enum { if enum != nil { - if errs := validation.ValidateCustomResource(fldPath, enum, validator); len(errs) > 0 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("enum").Index(i), enum, fmt.Sprintf("invalid value in enum: %v", errs))) + if errs := validation.ValidateCustomResource(fldPath.Child("enum").Index(i), enum, validator); len(errs) > 0 { + allErrs = append(allErrs, errs...) } } } if schema.AdditionalProperties != nil { if len(schema.Properties) > 0 { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("additionalProperties"), "additionalProperties and properties are mutually exclusive")) + allErrs = append(allErrs, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive")) } - allErrs = append(allErrs, validateSchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath)...) + allErrs = append(allErrs, validateSchema(ctx, schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"), opts, celContext.ChildAdditionalPropertiesContext(schema.AdditionalProperties.Schema), uncorrelatablePath)...) } for propertyName, propertySchema := range schema.Properties { p := propertySchema - allErrs = append(allErrs, validateSchema(&p, fldPath.Child("properties").Key(propertyName), celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath)...) + allErrs = append(allErrs, validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath)...) } if schema.Items != nil { - // We cannot correlate old/new items on atomic list types, which is the only type supported in ClusterClass variable schema. - if uncorrelatablePath != nil { - uncorrelatablePath = fldPath + // We cannot correlate old/new items on atomic list types, which is the only list type supported in ClusterClass variable schema. + if uncorrelatablePath == nil { + uncorrelatablePath = fldPath.Child("items") } - allErrs = append(allErrs, validateSchema(schema.Items.Schema, fldPath.Child("items"), celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath)...) + allErrs = append(allErrs, validateSchema(ctx, schema.Items.Schema, fldPath.Child("items"), opts, celContext.ChildItemsContext(schema.Items.Schema), uncorrelatablePath)...) } // This validation is duplicated from upstream CRD validation at // https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1178. - for i, rule := range schema.XValidations { - trimmedRule := strings.TrimSpace(rule.Rule) - trimmedMsg := strings.TrimSpace(rule.Message) - trimmedMsgExpr := strings.TrimSpace(rule.MessageExpression) - if trimmedRule == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified")) - } else if rule.Message != "" && trimmedMsg == "" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified")) - } else if rule.Message != "" && len(rule.Message) > 2048 { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must have a maximum length of 2048 characters")) - } else if hasNewlines(trimmedMsg) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks")) - } else if hasNewlines(trimmedRule) && trimmedMsg == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks")) + if len(schema.XValidations) > 0 { + // Return if schema is not a structural schema (this should enver happen, but let's handle it anyway). + ss, err := structuralschema.NewStructural(schema) + if err != nil { + return append(allErrs, field.Invalid(fldPath, "", fmt.Sprintf("schema is not a structural schema: %v", err))) + } + + for i, rule := range schema.XValidations { + trimmedRule := strings.TrimSpace(rule.Rule) + trimmedMsg := strings.TrimSpace(rule.Message) + trimmedMsgExpr := strings.TrimSpace(rule.MessageExpression) + if len(trimmedRule) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), "rule is not specified")) + } else if len(rule.Message) > 0 && len(trimmedMsg) == 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must be non-empty if specified")) + } else if len(rule.Message) > 2048 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message[0:10]+"...", "message must have a maximum length of 2048 characters")) + } else if hasNewlines(trimmedMsg) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), rule.Message, "message must not contain line breaks")) + } else if hasNewlines(trimmedRule) && len(trimmedMsg) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("message"), "message must be specified if rule contains line breaks")) + } + if len(rule.MessageExpression) > 0 && len(trimmedMsgExpr) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified")) + } + if rule.Reason != nil && !supportedValidationReason.Has(string(*rule.Reason)) { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List())) + } + trimmedFieldPath := strings.TrimSpace(rule.FieldPath) + if len(rule.FieldPath) > 0 && len(trimmedFieldPath) == 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be non-empty if specified")) + } + if hasNewlines(rule.FieldPath) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks")) + } + if len(rule.FieldPath) > 0 { + if _, _, err := cel.ValidFieldPath(rule.FieldPath, ss); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, fmt.Sprintf("fieldPath must be a valid path: %v", err))) + } + } } - if rule.MessageExpression != "" && trimmedMsgExpr == "" { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), "messageExpression must be non-empty if specified")) + + // If any schema related validation errors have been found at this level or deeper, skip CEL expression validation. + // Invalid OpenAPISchemas are not always possible to convert into valid CEL DeclTypes, and can lead to CEL + // validation error messages that are not actionable (will go away once the schema errors are resolved) and that + // are difficult for CEL expression authors to understand. + if len(allErrs) == 0 && celContext != nil { + typeInfo, err := celContext.TypeInfo() + if err != nil { + allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to construct type information for x-kubernetes-validations rules: %s", err))) + } else if typeInfo == nil { + allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to retrieve type information for x-kubernetes-validations"))) + } else { + // Note: k/k CRD validation also uses celconfig.PerCallLimit when creating the validator. + // The current PerCallLimit gives roughly 0.1 second for each expression validation call. + compResults, err := cel.Compile(typeInfo.Schema, typeInfo.DeclType, celconfig.PerCallLimit, opts.celEnvironmentSet, opts.preexistingExpressions) + if err != nil { + allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), err)) + } else { + for i, cr := range compResults { + expressionCost := getExpressionCost(cr, celContext) + if expressionCost > StaticEstimatedCostLimit { + costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg)) + } + if celContext.TotalCost != nil { + celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost) + } + if cr.Error != nil { + if cr.Error.Type == apiservercel.ErrorTypeRequired { + allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail)) + } else { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail)) + } + } + if cr.MessageExpressionError != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail)) + } else if cr.MessageExpression != nil { + if cr.MessageExpressionMaxCost > StaticEstimatedCostLimit { + costErrorMsg := getCostErrorMessage("estimated messageExpression cost", cr.MessageExpressionMaxCost, StaticEstimatedCostLimit) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg)) + } + if celContext.TotalCost != nil { + celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), cr.MessageExpressionMaxCost) + } + } + if cr.UsesOldSelf { + if uncorrelatablePath != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, fmt.Sprintf("oldSelf cannot be used on the uncorrelatable portion of the schema within %v", uncorrelatablePath))) + } + } + } + } + } } - if rule.Reason != nil && !supportedValidationReason.Has(string(*rule.Reason)) { - allErrs = append(allErrs, field.NotSupported(fldPath.Child("x-kubernetes-validations").Index(i).Child("reason"), *rule.Reason, supportedValidationReason.List())) + + // Return if we found some errors in the CEL validations. + if len(allErrs) > 0 { + return allErrs } - trimmedFieldPath := strings.TrimSpace(rule.FieldPath) - if rule.FieldPath != "" && trimmedFieldPath == "" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be non-empty if specified")) + + // Validate example & enum values via CEL. + // OpenAPI validation for example & enum values was already done via: validation.ValidateCustomResource + // CEL validation for Default values was already done via: structuraldefaulting.ValidateDefaults + celValidator := cel.NewValidator(ss, false, celconfig.PerCallLimit) + if celValidator == nil { + return append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations"), "", "failed to create CEL validator")) } - if hasNewlines(rule.FieldPath) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks")) + if schema.Example != nil { + errs, _ := celValidator.Validate(ctx, fldPath.Child("example"), ss, *schema.Example, nil, celconfig.RuntimeCELCostBudget) + allErrs = append(allErrs, errs...) } - if rule.FieldPath != "" { - if !pathValid(schema, rule.FieldPath) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be a valid path")) + for i, enum := range schema.Enum { + if enum != nil { + errs, _ := celValidator.Validate(ctx, fldPath.Child("enum").Index(i), ss, enum, nil, celconfig.RuntimeCELCostBudget) + allErrs = append(allErrs, errs...) } } } - // If any schema related validation errors have been found at this level or deeper, skip CEL expression validation. - // Invalid OpenAPISchemas are not always possible to convert into valid CEL DeclTypes, and can lead to CEL - // validation error messages that are not actionable (will go away once the schema errors are resolved) and that - // are difficult for CEL expression authors to understand. - if len(allErrs) > 0 { - return allErrs - } + return allErrs +} - if celContext != nil { - allErrs = append(allErrs, validateCELExpressions(schema, fldPath, celContext, uncorrelatablePath)...) - } +// The following funcs are all duplicated from upstream CRD validation at +// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1317. - return allErrs +// validationOptions groups several validation options, to avoid passing multiple bool parameters to methods. +type validationOptions struct { + preexistingExpressions preexistingExpressions + + celEnvironmentSet *environment.EnvSet } -var newlineMatcher = regexp.MustCompile(`[\n\r]+`) // valid newline chars in CEL grammar -func hasNewlines(s string) bool { - return newlineMatcher.MatchString(s) +type preexistingExpressions struct { + rules sets.Set[string] + messageExpressions sets.Set[string] } -func pathValid(schema *apiextensions.JSONSchemaProps, path string) bool { - // To avoid duplicated code and better maintain, using ValidFieldPath func to check if the path is valid - if ss, err := structuralschema.NewStructural(schema); err == nil { - _, _, err := cel.ValidFieldPath(path, ss) - return err == nil +func (pe preexistingExpressions) RuleEnv(envSet *environment.EnvSet, expression string) *celgo.Env { + if pe.rules.Has(expression) { + return envSet.StoredExpressionsEnv() } - return true + return envSet.NewExpressionsEnv() } -func validateCELExpressions(schema *apiextensions.JSONSchemaProps, fldPath *field.Path, celContext *apiextensionsvalidation.CELSchemaContext, uncorrelatablePath *field.Path) field.ErrorList { - var allErrs field.ErrorList - - typeInfo, err := celContext.TypeInfo() - if typeInfo == nil { - return nil +func (pe preexistingExpressions) MessageExpressionEnv(envSet *environment.EnvSet, expression string) *celgo.Env { + if pe.messageExpressions.Has(expression) { + return envSet.StoredExpressionsEnv() } - if err != nil { - allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), fmt.Errorf("internal error: failed to construct type information for x-kubernetes-validations rules: %s", err))) - } else { - compResults, err := cel.Compile( - typeInfo.Schema, - typeInfo.DeclType, - celconfig.PerCallLimit, - environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()), - cel.NewExpressionsEnvLoader(), - ) - if err != nil { - allErrs = append(allErrs, field.InternalError(fldPath.Child("x-kubernetes-validations"), err)) - } else { - for i, cr := range compResults { - expressionCost := getExpressionCost(cr, celContext) - if expressionCost > StaticEstimatedCostLimit { - costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit) - allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg)) - } - if celContext.TotalCost != nil { - celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), expressionCost) - } - if cr.Error != nil { - if cr.Error.Type == apiservercel.ErrorTypeRequired { - allErrs = append(allErrs, field.Required(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), cr.Error.Detail)) - } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i], cr.Error.Detail)) - } - } - if cr.MessageExpressionError != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail)) - } else if cr.MessageExpression != nil { - if cr.MessageExpressionMaxCost > StaticEstimatedCostLimit { - costErrorMsg := getCostErrorMessage("estimated messageExpression cost", cr.MessageExpressionMaxCost, StaticEstimatedCostLimit) - allErrs = append(allErrs, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg)) - } - if celContext.TotalCost != nil { - celContext.TotalCost.ObserveExpressionCost(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), cr.MessageExpressionMaxCost) - } - } - if cr.UsesOldSelf { - if uncorrelatablePath != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), schema.XValidations[i].Rule, fmt.Sprintf("oldSelf cannot be used on the uncorrelatable portion of the schema within %v", uncorrelatablePath))) - } - } + return envSet.NewExpressionsEnv() +} + +func findPreexistingExpressions(schema *apiextensions.JSONSchemaProps) preexistingExpressions { + expressions := preexistingExpressions{rules: sets.New[string](), messageExpressions: sets.New[string]()} + findPreexistingExpressionsInSchema(schema, expressions) + return expressions +} + +func findPreexistingExpressionsInSchema(schema *apiextensions.JSONSchemaProps, expressions preexistingExpressions) { + apiextensionsvalidation.SchemaHas(schema, func(s *apiextensions.JSONSchemaProps) bool { + for _, v := range s.XValidations { + expressions.rules.Insert(v.Rule) + if len(v.MessageExpression) > 0 { + expressions.messageExpressions.Insert(v.MessageExpression) } } - } + return false + }) +} - return allErrs +var newlineMatcher = regexp.MustCompile(`[\n\r]+`) // valid newline chars in CEL grammar +func hasNewlines(s string) bool { + return newlineMatcher.MatchString(s) } // multiplyWithOverflowGuard returns the product of baseCost and cardinality unless that product diff --git a/internal/topology/variables/clusterclass_variable_validation_test.go b/internal/topology/variables/clusterclass_variable_validation_test.go index 6a195f5fcf06..bb77de6a3445 100644 --- a/internal/topology/variables/clusterclass_variable_validation_test.go +++ b/internal/topology/variables/clusterclass_variable_validation_test.go @@ -17,10 +17,10 @@ limitations under the License. package variables import ( + "fmt" "strings" "testing" - . "github.com/onsi/gomega" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" @@ -30,9 +30,10 @@ import ( func Test_ValidateClusterClassVariables(t *testing.T) { tests := []struct { - name string - clusterClassVariables []clusterv1.ClusterClassVariable - wantErr bool + name string + clusterClassVariables []clusterv1.ClusterClassVariable + oldClusterClassVariables []clusterv1.ClusterClassVariable + wantErrs []validationMatch }{ { name: "Error if multiple variables share a name", @@ -56,7 +57,10 @@ func Test_ValidateClusterClassVariables(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("variable name must be unique. Variable with name \"cpu\" is defined more than once", + "spec.variables[cpu].name"), + }, }, { name: "Pass multiple variable validation", @@ -100,21 +104,244 @@ func Test_ValidateClusterClassVariables(t *testing.T) { }, }, }, + { + name: "pass if x-kubernetes-validations has valid rule", + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rule: " + + "new rule that uses opts that are not available with the current compatibility version", + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("compilation failed: ERROR: :1:3: undeclared reference to 'ip' (in container '')", + "spec.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: " + + "pre-existing rule that uses opts that are not available with the current compatibility version, but with the \"max\" env", + oldClusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }, + }, + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + // but because this rule already existed before, the "max" env is used + // and there the IP func is available. + Rule: "ip(self).family() == 6", + }}, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rule: " + + "pre-existing rule that uses opts that are not available with the current compatibility version, but with the \"max\" env" + + "this would work if the pre-existing rule would be for the same variable", + oldClusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someOtherIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }, + }, + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + // this rule already existed before, but the "max" env is not used + // because the rule didn't exist for the same variable. + Rule: "ip(self).family() == 6", + }}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("compilation failed: ERROR: :1:3: undeclared reference to 'ip' (in container '')", + "spec.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid messageExpression", + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + MessageExpression: "'Expected integer greater or equal to 1, got %d'.format([self])", + }}, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has invalid messageExpression: " + + "new messageExpression that uses opts that are not available with the current compatibility version", + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Note: IP will be only available if the compatibility version is 1.30 + MessageExpression: "'IP family %d'.format([ip(self).family()])", + }}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("messageExpression compilation failed: ERROR: :1:26: undeclared reference to 'ip' (in container '')", + "spec.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid messageExpression: " + + "pre-existing messageExpression that uses opts that are not available with the current compatibility version, but with the \"max\" env", + oldClusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Note: IP will be only available if the compatibility version is 1.30 + MessageExpression: "'IP family %d'.format([ip(self).family()])", + }}, + }, + }, + }, + }, + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Note: IP will be only available if the compatibility version is 1.30 + // but because this messageExpression already existed before, the "max" env is used + // and there the IP func is available. + MessageExpression: "'IP family %d'.format([ip(self).family()])", + }}, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has invalid messageExpression: " + + "pre-existing messageExpression that uses opts that are not available with the current compatibility version, but with the \"max\" env" + + "this would work if the pre-existing messageExpression would be for the same variable", + oldClusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someOtherIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Note: IP will be only available if the compatibility version is 1.30 + MessageExpression: "'IP family %d'.format([ip(self).family()])", + }}, + }, + }, + }, + }, + clusterClassVariables: []clusterv1.ClusterClassVariable{ + { + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Note: IP will be only available if the compatibility version is 1.30 + // this rule already existed before, but the "max" env is not used + // because the rule didn't exist for the same variable. + MessageExpression: "'IP family %d'.format([ip(self).family()])", + }}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("messageExpression compilation failed: ERROR: :1:26: undeclared reference to 'ip' (in container '')", + "spec.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression"), + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := ValidateClusterClassVariables(ctx, + gotErrs := ValidateClusterClassVariables(ctx, + tt.oldClusterClassVariables, tt.clusterClassVariables, field.NewPath("spec", "variables")) - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } @@ -123,8 +350,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { tests := []struct { name string clusterClassVariable *clusterv1.ClusterClassVariable - wantErr bool - errors []validationMatch + wantErrs []validationMatch }{ { name: "Valid integer schema", @@ -173,7 +399,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("\"builtin\" is a reserved variable name", + "spec.variables[builtin].name"), + }, }, { name: "fail on empty variable name", @@ -186,7 +415,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + required("variable name must be defined", + "spec.variables[].name"), + }, }, { name: "fail on variable name containing dot (.)", @@ -199,7 +431,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("\"path.tovariable\": variable name cannot contain \".\"", + "spec.variables[path.tovariable].name"), + }, }, { name: "Valid variable metadata", @@ -224,7 +459,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { { name: "fail on invalid variable label: key does not start with alphanumeric character", clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "path.tovariable", + Name: "variable", Metadata: clusterv1.ClusterClassVariableMetadata{ Labels: map[string]string{ ".label-key": "label-value", @@ -237,12 +472,15 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("\".label-key\": name part must consist of alphanumeric characters", + "spec.variables[variable].metadata.labels"), + }, }, { name: "fail on invalid variable annotation: key does not start with alphanumeric character", clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "path.tovariable", + Name: "variable", Metadata: clusterv1.ClusterClassVariableMetadata{ Annotations: map[string]string{ ".annotation-key": "annotation-value", @@ -255,7 +493,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("\".annotation-key\": name part must consist of alphanumeric characters", + "spec.variables[variable].metadata.annotations"), + }, }, { name: "Valid default value regular string", @@ -279,14 +520,39 @@ func Test_ValidateClusterClassVariable(t *testing.T) { Required: true, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", + Type: "object", Default: &apiextensionsv1.JSON{ Raw: []byte(`"defaultValue": "value"`), // invalid JSON }, }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"defaultValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.default"), + }, + }, + { + name: "fail on default value with invalid JSON (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`"defaultValue": "value"`), // invalid JSON + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"defaultValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.items.default"), + }, }, { name: "Valid example value regular string", @@ -310,14 +576,39 @@ func Test_ValidateClusterClassVariable(t *testing.T) { Required: true, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", + Type: "object", Example: &apiextensionsv1.JSON{ Raw: []byte(`"exampleValue": "value"`), // invalid JSON }, }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"exampleValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.example"), + }, + }, + { + name: "fail on example value with invalid JSON (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Example: &apiextensionsv1.JSON{ + Raw: []byte(`"exampleValue": "value"`), // invalid JSON + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"exampleValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.items.example"), + }, }, { name: "Valid enum values", @@ -342,14 +633,40 @@ func Test_ValidateClusterClassVariable(t *testing.T) { Required: true, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", + Type: "object", Enum: []apiextensionsv1.JSON{ - {Raw: []byte(`"defaultValue": "value"`)}, // invalid JSON + {Raw: []byte(`"enumValue": "value"`)}, // invalid JSON + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"enumValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.enum[0]"), + }, + }, + { + name: "fail on enum value with invalid JSON (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`{"enumValue": "value"}`)}, // valid JSON + {Raw: []byte(`"enumValue": "value"`)}, // invalid JSON + }, }, }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("Invalid value: \"\\\"enumValue\\\": \\\"value\\\"\": failed to convert schema definition for variable \"var\"", + "spec.variables[var].schema.openAPIV3Schema.items.enum[1]"), + }, }, { name: "fail on variable type is null", @@ -361,7 +678,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"null\"", + "spec.variables[var].schema.openAPIV3Schema.type"), + }, }, { name: "fail on variable type is not valid", @@ -373,7 +693,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"invalidVariableType\"", + "spec.variables[var].schema.openAPIV3Schema.type"), + }, }, { name: "fail on variable type length zero", @@ -385,7 +708,66 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + required("Required value: must not be empty for specified object fields", + "spec.variables[var].schema.openAPIV3Schema.type"), + }, + }, + { + name: "fail on variable type length zero (object)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "", + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + required("Required value: must not be empty for specified object fields", + "spec.variables[var].schema.openAPIV3Schema.properties[nestedField].type"), + }, + }, + { + name: "fail on variable type length zero (array)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "", + }, + }, + }, + }, + wantErrs: []validationMatch{ + required("Required value: must not be empty for specified array items", + "spec.variables[var].schema.openAPIV3Schema.items.type"), + }, + }, + { + name: "fail on variable type length zero (map)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "", + }, + }, + }, + }, + wantErrs: []validationMatch{ + required("Required value: must not be empty for specified object fields", + "spec.variables[var].schema.openAPIV3Schema.additionalProperties.type"), + }, }, { name: "Valid object schema", @@ -434,7 +816,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"invalidType\"", + "spec.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type"), + }, }, { name: "Valid map schema", @@ -489,7 +874,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"invalidType\"", + "spec.variables[httpProxy].schema.openAPIV3Schema.additionalProperties.properties[noProxy].type"), + }, }, { name: "fail on object (properties) and map (additionalProperties) both set", @@ -517,7 +905,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + forbidden("Forbidden: additionalProperties and properties are mutually exclusive", + "spec.variables[httpProxy].schema.openAPIV3Schema"), + }, }, { name: "Valid array schema", @@ -549,7 +940,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("default is not valid JSON: invalid character 'i' looking for beginning of value", + "spec.variables[arrayVariable].schema.openAPIV3Schema.items.default"), + }, }, { name: "pass on variable with required set true with a default defined", @@ -589,7 +983,30 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + toolong("Too long: may not be longer than 6", + "spec.variables[var].schema.openAPIV3Schema.default"), + }, + }, + { + name: "fail on variable with a default that is invalid by the given schema (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "string", + MaxLength: ptr.To[int64](6), + Default: &apiextensionsv1.JSON{Raw: []byte(`"veryLongValueIsInvalid"`)}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + toolong("Too long: may not be longer than 6", + "spec.variables[var].schema.openAPIV3Schema.items.default"), + }, }, { name: "pass if variable is an object with default value valid by the given schema", @@ -636,7 +1053,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 1", + "spec.variables[var].schema.openAPIV3Schema.properties[spec].properties[replicas].default"), + }, }, { name: "fail if variable is an object with a top level default value invalidated by the given schema", @@ -662,7 +1082,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 1", + "spec.variables[var].schema.openAPIV3Schema.default.spec.replicas"), + }, }, { name: "pass if variable is an object with a top level default value valid by the given schema", @@ -721,7 +1144,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + required("Required value", + "spec.variables[var].schema.openAPIV3Schema.properties[spec].default.replicas"), + }, }, { name: "pass if field is required below properties and sets a default", @@ -777,7 +1203,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + toolong("Too long: may not be longer than 6", + "spec.variables[var].schema.openAPIV3Schema.example"), + }, }, { name: "pass if variable is an object with an example valid by the given schema", @@ -824,7 +1253,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 0", + "spec.variables[var].schema.openAPIV3Schema.properties[spec].properties[replicas].example"), + }, }, { name: "fail if variable is an object with a top level example value invalidated by the given schema", @@ -850,7 +1282,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 1", + "spec.variables[var].schema.openAPIV3Schema.example.spec.replicas"), + }, }, { name: "pass if variable is an object with a top level default value valid by the given schema", @@ -908,7 +1343,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + toolong("may not be longer than 6", + "spec.variables[var].schema.openAPIV3Schema.enum[0]"), + }, }, { name: "pass if variable is an object with an enum value that is valid by the given schema", @@ -961,7 +1399,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 0", + "spec.variables[var].schema.openAPIV3Schema.properties[spec].properties[replicas].enum[1]"), + }, }, { name: "fail if variable is an object with a top level enum value invalidated by the given schema", @@ -992,7 +1433,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("should be greater than or equal to 1", + "spec.variables[var].schema.openAPIV3Schema.enum[1].spec.replicas"), + }, }, { name: "pass if variable is an object with a top level enum value that is valid by the given schema", @@ -1025,7 +1469,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, { - name: "pass if valid integer schema with CEL expression", + name: "pass if x-kubernetes-validations has valid rule", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Schema: clusterv1.VariableSchema{ @@ -1039,7 +1483,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, { - name: "fail if integer schema with invalid CEL expression", + name: "fail if x-kubernetes-validations has invalid rule: invalid CEL expression", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Schema: clusterv1.VariableSchema{ @@ -1051,37 +1495,90 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[0]", "rule"), + wantErrs: []validationMatch{ + invalid("compilation failed: ERROR: :1:6: Syntax error: mismatched input 'does' expecting \n | this does not compile\n | .....^", + "spec.variables[cpu].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), }, }, { - name: "fail if validation rules cost total exceeds total limit", + name: "fail if x-kubernetes-validations has invalid rule: oldSef cannot be used in arrays", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "cpu", Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]clusterv1.JSONSchemaProps{ - "list": { - Type: "array", - MaxItems: ptr.To[int64](10000000), - Items: &clusterv1.JSONSchemaProps{ + Type: "array", + MaxItems: ptr.To[int64](20), + Items: &clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + Rule: "oldSelf.contains('keyword')", + }}, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"oldSelf.contains('keyword')\": oldSelf cannot be used on the uncorrelatable portion of the schema within spec.variables[cpu].schema.openAPIV3Schema.items", + "spec.variables[cpu].schema.openAPIV3Schema.items.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rule: oldSef cannot be used in arrays (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + MaxItems: ptr.To[int64](10), + Items: &clusterv1.JSONSchemaProps{ + Type: "array", + MaxItems: ptr.To[int64](10), + Items: &clusterv1.JSONSchemaProps{ + Type: "string", + MaxLength: ptr.To[int64](1024), + XValidations: []clusterv1.ValidationRule{{ + Rule: "oldSelf.contains('keyword')", + }}, + }, + }, + }, + }, + }, + // Note: This test verifies that the path in the error message references the first array, not the nested one. + wantErrs: []validationMatch{ + invalid("Invalid value: \"oldSelf.contains('keyword')\": oldSelf cannot be used on the uncorrelatable portion of the schema within spec.variables[cpu].schema.openAPIV3Schema.items", + "spec.variables[cpu].schema.openAPIV3Schema.items.items.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rules: cost total exceeds total limit", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "array": { + // We are not setting MaxItems, so a worst case cardinality will be assumed and we exceed the per-expression limit. + Type: "array", + Items: &clusterv1.JSONSchemaProps{ Type: "string", MaxLength: ptr.To[int64](500000), XValidations: []clusterv1.ValidationRule{{Rule: "self.contains('keyword')"}}, }, }, "map": { - Type: "object", + // Setting MaxProperties and MaxLength allows us to stay below the per-expression limit. + Type: "object", + MaxProperties: ptr.To[int64](10000), AdditionalProperties: &clusterv1.JSONSchemaProps{ Type: "string", - MaxLength: ptr.To[int64](500000), + MaxLength: ptr.To[int64](2048), XValidations: []clusterv1.ValidationRule{{Rule: "self.contains('keyword')"}}, }, }, - "field": { // include a validation rule that does not contribute to total limit being exceeded (i.e. it is less than 1% of the limit) + "field": { + // Include a validation rule that does not contribute to total limit being exceeded (i.e. it is less than 1% of the limit) Type: "integer", XValidations: []clusterv1.ValidationRule{{Rule: "self > 50 && self < 100"}}, }, @@ -1089,14 +1586,347 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - // exceeds per-rule limit and contributes to total limit being exceeded (1 error for each) - forbidden("spec", "variables[0]", "schema", "openAPIV3Schema", "properties[list]", "items", "x-kubernetes-validations[0]", "rule"), - // contributes to total limit being exceeded, but does not exceed per-rule limit - forbidden("spec", "variables[0]", "schema", "openAPIV3Schema", "properties[map]", "additionalProperties", "x-kubernetes-validations[0]", "rule"), - // total limit is exceeded - forbidden("spec", "variables[0]", "schema", "openAPIV3Schema"), + wantErrs: []validationMatch{ + forbidden("estimated rule cost exceeds budget by factor of more than 100x", + "spec.variables[cpu].schema.openAPIV3Schema.properties[array].items.x-kubernetes-validations[0].rule"), + forbidden("contributed to estimated cost total exceeding cost limit", + "spec.variables[cpu].schema.openAPIV3Schema.properties[array].items.x-kubernetes-validations[0].rule"), + forbidden("contributed to estimated cost total exceeding cost limit", + "spec.variables[cpu].schema.openAPIV3Schema.properties[map].additionalProperties.x-kubernetes-validations[0].rule"), + forbidden("x-kubernetes-validations estimated cost total for entire OpenAPIv3 schema exceeds budget by factor of more than 100x", + "spec.variables[cpu].schema.openAPIV3Schema"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rule: rule is not specified", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{ + { + Rule: " ", + }, + }, + Properties: map[string]clusterv1.JSONSchemaProps{ + "a": { + Type: "number", + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + required("rule is not specified", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid default value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid default value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + MessageExpression: "'Expected integer greater or equal to 1, got %d'.format([self])", + }}, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": Expected integer greater or equal to 1, got 0", + "spec.variables[cpu].schema.openAPIV3Schema.default"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid default value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid default value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self >= 1", + "spec.variables[cpu].schema.openAPIV3Schema.properties[nestedField].default"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid rule: invalid default value transition rule", + // For CEL validation of default values, the default value is used as self & oldSelf. + // This is because if a field is defaulted it would usually stay the same on updates, + // so it has to be valid according to transition rules. + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self > oldSelf", + "spec.variables[cpu].schema.openAPIV3Schema.default"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid default value transition rule", + // For CEL validation of default values, the default value is used as self & oldSelf. + // This is because if a field is defaulted it would usually stay the same on updates, + // so it has to be valid according to transition rules. + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self == oldSelf", + }}, + }, + }, + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid example value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Example: &apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid example value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Example: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self >= 1", + "spec.variables[cpu].schema.openAPIV3Schema.example"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid example value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Example: &apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid example value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Example: &apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self >= 1", + "spec.variables[cpu].schema.openAPIV3Schema.properties[nestedField].example"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid enum value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`1`)}, + {Raw: []byte(`2`)}, + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid enum value", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`0`)}, + {Raw: []byte(`1`)}, + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self >= 1", + "spec.variables[cpu].schema.openAPIV3Schema.enum[0]"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid rule: valid enum value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`1`)}, + {Raw: []byte(`2`)}, + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has valid rule: invalid enum value (nested)", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "nestedField": { + Type: "integer", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`1`)}, + {Raw: []byte(`0`)}, + }, + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("Invalid value: \"integer\": failed rule: self >= 1", + "spec.variables[cpu].schema.openAPIV3Schema.properties[nestedField].enum[1]"), }, }, { @@ -1139,9 +1969,9 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - unsupported("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[0]", "reason"), + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"InternalError\"", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].reason"), }, }, { @@ -1152,6 +1982,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "object", XValidations: []clusterv1.ValidationRule{ + // Valid { Rule: "true", FieldPath: ".foo['b.c']['c\\a']", @@ -1160,13 +1991,22 @@ func Test_ValidateClusterClassVariable(t *testing.T) { Rule: "true", FieldPath: "['a.c']", }, + { + Rule: "true", + FieldPath: "['special.character.34$']", + }, + { + Rule: "true", + FieldPath: ".list", // referring to an entire list is supported + }, + // Invalid { Rule: "true", FieldPath: ".a.c", }, { Rule: "true", - FieldPath: ".list[0]", + FieldPath: ".list[0]", // referring to an item of a list with a numeric index is not supported }, { Rule: "true", @@ -1185,6 +2025,9 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "a.c": { Type: "number", }, + "special.character.34$": { + Type: "number", + }, "foo": { Type: "object", Properties: map[string]clusterv1.JSONSchemaProps{ @@ -1213,14 +2056,19 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[2]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[3]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[4]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[4]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[5]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[6]", "fieldPath"), + wantErrs: []validationMatch{ + invalid("Invalid value: \".a.c\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[4].fieldPath"), + invalid("Invalid value: \".list[0]\": fieldPath must be a valid path: expected single quoted string but got 0", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[5].fieldPath"), + invalid("Invalid value: \" \": fieldPath must be non-empty if specified", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[6].fieldPath"), + invalid("Invalid value: \" \": fieldPath must be a valid path: expected [ or . but got: ", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[6].fieldPath"), + invalid("Invalid value: \".\": fieldPath must be a valid path: unexpected end of JSON path", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[7].fieldPath"), + invalid("Invalid value: \"..\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[8].fieldPath"), }, }, { @@ -1251,6 +2099,10 @@ func Test_ValidateClusterClassVariable(t *testing.T) { Rule: "self.a.b.c > 0.0", FieldPath: ".a.b.d", }, + { + Rule: "self.a.b.c > 0.0", + FieldPath: ".a\n", + }, }, Properties: map[string]clusterv1.JSONSchemaProps{ "a": { @@ -1281,28 +2133,52 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[0]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[1]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[2]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[3]", "fieldPath"), - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[4]", "fieldPath"), + wantErrs: []validationMatch{ + invalid("Invalid value: \".list[0].b\": fieldPath must be a valid path: expected single quoted string but got 0", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].fieldPath"), + invalid("Invalid value: \".list[0.b\": fieldPath must be a valid path: expected single quoted string but got 0", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[1].fieldPath"), + invalid("Invalid value: \".list0].b\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[2].fieldPath"), + invalid("Invalid value: \".a.c\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[3].fieldPath"), + invalid("Invalid value: \".a.b.d\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[4].fieldPath"), + invalid("Invalid value: \".a\\n\": fieldPath must not contain line breaks", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[5].fieldPath"), + invalid("Invalid value: \".a\\n\": fieldPath must be a valid path: does not refer to a valid field", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[5].fieldPath"), }, }, { - name: "fail if x-kubernetes-validations message is longer than 2048 characters", + name: "fail if x-kubernetes-validations has invalid message", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "var", Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "object", XValidations: []clusterv1.ValidationRule{ + // Valid { - Rule: "self.a > 0", - Reason: clusterv1.FieldValueInvalid, - FieldPath: ".a", - Message: strings.Repeat("a", 2049), + Rule: "self.a > 0", + Message: "valid message", + }, + // Invalid + { + Rule: "self.a > 0", + Message: " ", + }, + { + Rule: "self.a > 0", + Message: strings.Repeat("a", 2049), + }, + { + Rule: "self.a > 0", + Message: "message with \n line break", + }, + { + Rule: "self.a \n > 0", + Message: "", }, }, Properties: map[string]clusterv1.JSONSchemaProps{ @@ -1313,63 +2189,199 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, - errors: []validationMatch{ - invalid("spec", "variables[0]", "schema", "openAPIV3Schema", "x-kubernetes-validations[0]", "message"), + wantErrs: []validationMatch{ + invalid("Invalid value: \" \": message must be non-empty if specified", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[1].message"), + invalid("Invalid value: \"aaaaaaaaaa...\": message must have a maximum length of 2048 characters", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[2].message"), + invalid("Invalid value: \"message with \\n line break\": message must not contain line breaks", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[3].message"), + required("Required value: message must be specified if rule contains line breaks", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[4].message"), + }, + }, + { + name: "pass if x-kubernetes-validations has valid messageExpression", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self.replicas >= 1", + MessageExpression: "'Expected integer greater or equal to 1, got %d'.format([self.replicas])", + }}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "replicas": { + Type: "integer", + }, + }, + }, + }, + }, + }, + { + name: "fail if x-kubernetes-validations has invalid messageExpression: must be non-empty if specified", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{ + { + Rule: "self.a > 0", + MessageExpression: " ", + }, + }, + Properties: map[string]clusterv1.JSONSchemaProps{ + "a": { + Type: "number", + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + required("Required value: messageExpression must be non-empty if specified", + "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid messageExpression: invalid CEL expression", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + MessageExpression: "'Expected integer greater or equal to 1, got ' + this does not compile", + }}, + }, + }, + }, + wantErrs: []validationMatch{ + invalid("messageExpression compilation failed: ERROR: :1:55: Syntax error: mismatched input 'does' expecting ", + "spec.variables[cpu].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid messageExpression: cost total exceeds total limit", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "array": { + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "string", + MaxLength: ptr.To[int64](500000), + XValidations: []clusterv1.ValidationRule{{ + Rule: "true", + // Using + and string() exceeds the cost limit + MessageExpression: "'contains: ' + string(self.contains('keyword'))", + }}, + }, + }, + "field": { + // Include a messageExpression that does not contribute to total limit being exceeded + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > 50 && self < 100", + MessageExpression: "'Expected integer to be between 50 and 100, got %d'.format([self])", + }}, + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + forbidden("estimated messageExpression cost exceeds budget by factor of more than 100x", + "spec.variables[cpu].schema.openAPIV3Schema.properties[array].items.x-kubernetes-validations[0].messageExpression"), + forbidden("contributed to estimated cost total exceeding cost limit", + "spec.variables[cpu].schema.openAPIV3Schema.properties[array].items.x-kubernetes-validations[0].messageExpression"), + forbidden("x-kubernetes-validations estimated cost total for entire OpenAPIv3 schema exceeds budget by factor of more than 100x", + "spec.variables[cpu].schema.openAPIV3Schema"), }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := validateClusterClassVariable(ctx, + gotErrs := validateClusterClassVariable(ctx, + nil, tt.clusterClassVariable, - field.NewPath("spec", "variables").Index(0)) + field.NewPath("spec", "variables").Key(tt.clusterClassVariable.Name)) - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) + }) + } +} - seenErrs := make([]bool, len(errList)) - for _, expectedError := range tt.errors { - found := false - for i, err := range errList { - if expectedError.matches(err) && !seenErrs[i] { - found = true - seenErrs[i] = true - break - } - } +func checkErrors(t *testing.T, wantErrs []validationMatch, gotErrs field.ErrorList) { + t.Helper() - if !found { - t.Errorf("expected %v at %v, got %v", expectedError.errorType, expectedError.path.String(), errList) - } - } - return + var foundMismatch bool + if len(gotErrs) != len(wantErrs) { + foundMismatch = true + } else { + for i := range wantErrs { + if !wantErrs[i].matches(gotErrs[i]) { + foundMismatch = true + break } - g.Expect(errList).To(BeEmpty()) - }) + } + } + + if foundMismatch { + var wantErrsStr []string + for i := range wantErrs { + wantErrsStr = append(wantErrsStr, fmt.Sprintf("type: %s\ntext: %s\npath: %s", string(wantErrs[i].Type), wantErrs[i].containsString, wantErrs[i].Field)) + } + var gotErrsStr []string + for i := range gotErrs { + gotErrsStr = append(gotErrsStr, fmt.Sprintf("type: %s\ntext: %s\npath: %s", string(gotErrs[i].Type), gotErrs[i].Error(), gotErrs[i].Field)) + } + t.Errorf("expected %d errors, got %d\ngot errors:\n\n%s\n\nwant errors:\n\n%s\n", + len(wantErrs), len(gotErrs), strings.Join(gotErrsStr, "\n\n"), strings.Join(wantErrsStr, "\n\n")) } } type validationMatch struct { - path *field.Path - errorType field.ErrorType + Field *field.Path + Type field.ErrorType containsString string } func (v validationMatch) matches(err *field.Error) bool { - return err.Type == v.errorType && err.Field == v.path.String() && strings.Contains(err.Error(), v.containsString) + return err.Type == v.Type && err.Field == v.Field.String() && strings.Contains(err.Error(), v.containsString) +} + +func toolong(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeTooLong} +} + +func toomany(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeTooMany} +} + +func required(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeRequired} +} + +func invalidType(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeTypeInvalid} } -func invalid(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} +func invalid(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeInvalid} } -func unsupported(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeNotSupported} +func unsupported(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeNotSupported} } -func forbidden(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden} +func forbidden(containsString string, path string) validationMatch { + return validationMatch{containsString: containsString, Field: field.NewPath(strings.Split(path, ".")[0], strings.Split(path, ".")[1:]...), Type: field.ErrorTypeForbidden} } diff --git a/internal/topology/variables/schema.go b/internal/topology/variables/schema.go index 7ec5649d775d..e4690a46ae3d 100644 --- a/internal/topology/variables/schema.go +++ b/internal/topology/variables/schema.go @@ -38,6 +38,8 @@ func convertToAPIExtensionsJSONSchemaProps(schema *clusterv1.JSONSchemaProps, fl props := &apiextensions.JSONSchemaProps{ Type: schema.Type, Required: schema.Required, + MaxProperties: schema.MaxProperties, + MinProperties: schema.MinProperties, MaxItems: schema.MaxItems, MinItems: schema.MinItems, UniqueItems: schema.UniqueItems, @@ -123,10 +125,9 @@ func convertToAPIExtensionsJSONSchemaProps(schema *clusterv1.JSONSchemaProps, fl } if schema.AdditionalProperties != nil { - apiExtensionsSchema, err := convertToAPIExtensionsJSONSchemaProps(schema.AdditionalProperties, fldPath.Child("additionalProperties")) - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("additionalProperties"), "", - fmt.Sprintf("failed to convert schema: %v", err))) + apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(schema.AdditionalProperties, fldPath.Child("additionalProperties")) + if len(errs) > 0 { + allErrs = append(allErrs, errs...) } else { props.AdditionalProperties = &apiextensions.JSONSchemaPropsOrBool{ // Allows must be true to allow "additional properties". @@ -141,10 +142,9 @@ func convertToAPIExtensionsJSONSchemaProps(schema *clusterv1.JSONSchemaProps, fl props.Properties = map[string]apiextensions.JSONSchemaProps{} for propertyName, propertySchema := range schema.Properties { p := propertySchema - apiExtensionsSchema, err := convertToAPIExtensionsJSONSchemaProps(&p, fldPath.Child("properties").Key(propertyName)) - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("properties").Key(propertyName), "", - fmt.Sprintf("failed to convert schema: %v", err))) + apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(&p, fldPath.Child("properties").Key(propertyName)) + if len(errs) > 0 { + allErrs = append(allErrs, errs...) } else { props.Properties[propertyName] = *apiExtensionsSchema } @@ -152,10 +152,9 @@ func convertToAPIExtensionsJSONSchemaProps(schema *clusterv1.JSONSchemaProps, fl } if schema.Items != nil { - apiExtensionsSchema, err := convertToAPIExtensionsJSONSchemaProps(schema.Items, fldPath.Child("items")) - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("items"), "", - fmt.Sprintf("failed to convert schema: %v", err))) + apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(schema.Items, fldPath.Child("items")) + if len(errs) > 0 { + allErrs = append(allErrs, errs...) } else { props.Items = &apiextensions.JSONSchemaPropsOrArray{ Schema: apiExtensionsSchema, diff --git a/internal/topology/variables/schema_test.go b/internal/topology/variables/schema_test.go index ad12b4a8309f..f93eed11558d 100644 --- a/internal/topology/variables/schema_test.go +++ b/internal/topology/variables/schema_test.go @@ -103,10 +103,10 @@ func Test_convertToAPIExtensionsJSONSchemaProps(t *testing.T) { Minimum: ptr.To[int64](1), }, "property2": { - Type: "string", - Format: "uri", - MinLength: ptr.To[int64](2), - MaxLength: ptr.To[int64](4), + Type: "string", + Format: "uri", + MinProperties: ptr.To[int64](2), + MaxProperties: ptr.To[int64](4), }, }, }, @@ -117,10 +117,10 @@ func Test_convertToAPIExtensionsJSONSchemaProps(t *testing.T) { Minimum: ptr.To[float64](1), }, "property2": { - Type: "string", - Format: "uri", - MinLength: ptr.To[int64](2), - MaxLength: ptr.To[int64](4), + Type: "string", + Format: "uri", + MinProperties: ptr.To[int64](2), + MaxProperties: ptr.To[int64](4), }, }, }, @@ -135,10 +135,10 @@ func Test_convertToAPIExtensionsJSONSchemaProps(t *testing.T) { Minimum: ptr.To[int64](1), }, "property2": { - Type: "string", - Format: "uri", - MinLength: ptr.To[int64](2), - MaxLength: ptr.To[int64](4), + Type: "string", + Format: "uri", + MinProperties: ptr.To[int64](2), + MaxProperties: ptr.To[int64](4), }, }, }, @@ -153,10 +153,10 @@ func Test_convertToAPIExtensionsJSONSchemaProps(t *testing.T) { Minimum: ptr.To[float64](1), }, "property2": { - Type: "string", - Format: "uri", - MinLength: ptr.To[int64](2), - MaxLength: ptr.To[int64](4), + Type: "string", + Format: "uri", + MinProperties: ptr.To[int64](2), + MaxProperties: ptr.To[int64](4), }, }, }, diff --git a/internal/topology/variables/utils.go b/internal/topology/variables/utils.go index 600c03f58e52..467ad6da8384 100644 --- a/internal/topology/variables/utils.go +++ b/internal/topology/variables/utils.go @@ -43,7 +43,11 @@ func newValuesIndex(values []clusterv1.ClusterVariable) (map[string]map[string]c } // Check that the variable has not been defined more than once with the same definitionFrom. if _, ok := valuesMap[c.Name][c.DefinitionFrom]; ok { - errs = append(errs, errors.Errorf("variable %q from %q is defined more than once", c.Name, c.DefinitionFrom)) + if c.DefinitionFrom == "" { + errs = append(errs, errors.Errorf("variable %q is defined more than once", c.Name)) + } else { + errs = append(errs, errors.Errorf("variable %q from %q is defined more than once", c.Name, c.DefinitionFrom)) + } continue } // Add the variable. diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index ede22d3f79c8..38f844da8378 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -18,22 +18,26 @@ package webhooks import ( "context" + "encoding/json" "testing" "github.com/blang/semver/v4" . "github.com/onsi/gomega" "github.com/pkg/errors" + admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" @@ -69,11 +73,13 @@ func TestClusterDefaultAndValidateVariables(t *testing.T) { defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)() tests := []struct { - name string - clusterClass *clusterv1.ClusterClass - topology *clusterv1.Topology - expect *clusterv1.Topology - wantErr bool + name string + clusterClass *clusterv1.ClusterClass + topology *clusterv1.Topology + oldTopology *clusterv1.Topology + expect *clusterv1.Topology + wantErr bool + wantErrMessage string }{ { name: "default a single variable to its correct values", @@ -935,14 +941,267 @@ func TestClusterDefaultAndValidateVariables(t *testing.T) { Build()). Build(), }, + { + name: "should pass when CEL transition rules are skipped", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("md1").Build()). + WithWorkerMachinePoolClasses(*builder.MachinePoolClass("mp1").Build()). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-10`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + Build(), + expect: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-10`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + Build(), + }, + { + name: "should pass when CEL transition rules are running", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("md1").Build()). + WithWorkerMachinePoolClasses(*builder.MachinePoolClass("mp1").Build()). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-10`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + Build(), + oldTopology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-6`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-6`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-12`)}, + }). + Build()). + Build(), + expect: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-10`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + Build(), + }, + { + name: "should fail when CEL transition rules are running and failing", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithWorkerMachineDeploymentClasses(*builder.MachineDeploymentClass("md1").Build()). + WithWorkerMachinePoolClasses(*builder.MachinePoolClass("mp1").Build()). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }}).Build(), + topology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-5`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-4`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-10`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`-11`)}, + }). + Build()). + Build(), + oldTopology: builder.ClusterTopology(). + WithClass("foo"). + WithVersion("v1.19.1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`0`)}, + }). + WithControlPlaneVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`0`)}, + }). + WithMachineDeployment(builder.MachineDeploymentTopology("workers1"). + WithClass("md1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`0`)}, + }). + Build()). + WithMachinePool(builder.MachinePoolTopology("workers1"). + WithClass("mp1"). + WithVariables(clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{Raw: []byte(`0`)}, + }). + Build()). + Build(), + wantErr: true, + wantErrMessage: "Cluster.cluster.x-k8s.io \"cluster1\" is invalid: [" + + "spec.topology.variables[cpu].value: Invalid value: \"-5\": failed rule: self > oldSelf, " + + "spec.topology.controlPlane.variables.overrides[cpu].value: Invalid value: \"-4\": failed rule: self > oldSelf, " + + "spec.topology.workers.machineDeployments[0].variables.overrides[cpu].value: Invalid value: \"-10\": failed rule: self > oldSelf, " + + "spec.topology.workers.machinePools[0].variables.overrides[cpu].value: Invalid value: \"-11\": failed rule: self > oldSelf]", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Setting Class and Version here to avoid obfuscating the test cases above. tt.topology.Class = "class1" tt.topology.Version = "v1.22.2" - tt.expect.Class = "class1" - tt.expect.Version = "v1.22.2" + if tt.expect != nil { + tt.expect.Class = "class1" + tt.expect.Version = "v1.22.2" + } cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). WithTopology(tt.topology). @@ -955,17 +1214,43 @@ func TestClusterDefaultAndValidateVariables(t *testing.T) { WithScheme(fakeScheme). Build() // Create the webhook and add the fakeClient as its client. This is required because the test uses a Managed Topology. - webhook := &Cluster{Client: fakeClient} + webhook := &Cluster{Client: fakeClient, decoder: admission.NewDecoder(fakeScheme)} // Test defaulting. t.Run("default", func(t *testing.T) { g := NewWithT(t) + + webhookCtx := ctx + + // Add old cluster to request if oldTopology is set. + if tt.oldTopology != nil { + oldCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithTopology(tt.oldTopology). + Build() + jsonObj, err := json.Marshal(oldCluster) + g.Expect(err).ToNot(HaveOccurred()) + + webhookCtx = admission.NewContextWithRequest(ctx, admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + OldObject: runtime.RawExtension{ + Raw: jsonObj, + Object: oldCluster, + }, + }, + }) + } + + err := webhook.Default(webhookCtx, cluster) if tt.wantErr { - g.Expect(webhook.Default(ctx, cluster)).To(Not(Succeed())) + g.Expect(err).To(HaveOccurred()) + if tt.wantErrMessage != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrMessage)) + } return } - g.Expect(webhook.Default(ctx, cluster)).To(Succeed()) - g.Expect(cluster.Spec.Topology).To(BeEquivalentTo(tt.expect)) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(cluster.Spec.Topology).To(BeComparableTo(tt.expect)) }) // Test if defaulting works in combination with validation. diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 34568da6eef3..db5ec80dadc7 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -163,8 +163,12 @@ func (webhook *ClusterClass) validate(ctx context.Context, oldClusterClass, newC allErrs = append(allErrs, validateNamingStrategies(newClusterClass)...) // Validate variables. + var oldClusterClassVariables []clusterv1.ClusterClassVariable + if oldClusterClass != nil { + oldClusterClassVariables = oldClusterClass.Spec.Variables + } allErrs = append(allErrs, - variables.ValidateClusterClassVariables(ctx, newClusterClass.Spec.Variables, field.NewPath("spec", "variables"))..., + variables.ValidateClusterClassVariables(ctx, oldClusterClassVariables, newClusterClass.Spec.Variables, field.NewPath("spec", "variables"))..., ) // Validate patches. diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index c45625fbd996..ac06f41e5c5a 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1004,9 +1004,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: false, }, - /* - UPDATE Tests - */ + // update tests { name: "update pass in case of no changes", @@ -1769,6 +1767,75 @@ func TestClusterClassValidation(t *testing.T) { Build(), expectErr: true, }, + + // CEL tests + { + name: "fail if x-kubernetes-validations has invalid rule: " + + "new rule that uses opts that are not available with the current compatibility version", + in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithVariables(clusterv1.ClusterClassVariable{ + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }). + Build(), + expectErr: true, + }, + { + name: "pass if x-kubernetes-validations has valid rule: " + + "pre-existing rule that uses opts that are not available with the current compatibility version, but with the \"max\" env", + old: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithVariables(clusterv1.ClusterClassVariable{ + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }). + Build(), + in: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithInfrastructureClusterTemplate( + builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "infra1").Build()). + WithControlPlaneTemplate( + builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cp1"). + Build()). + WithVariables(clusterv1.ClusterClassVariable{ + Name: "someIP", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + XValidations: []clusterv1.ValidationRule{{ + // Note: IP will be only available if the compatibility version is 1.30 + Rule: "ip(self).family() == 6", + }}, + }, + }, + }). + Build(), + expectErr: false, + }, } for _, tt := range tests { diff --git a/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml b/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml index dc9cec1027b7..f15efab8a700 100644 --- a/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml +++ b/test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml @@ -106,6 +106,9 @@ spec: default: "" example: "0" description: "kubeadmControlPlaneMaxSurge is the maximum number of control planes that can be scheduled above or under the desired number of control plane machines." + x-kubernetes-validations: + - rule: "self == \"\" || self != \"\"" + messageExpression: "'just a test expression, got %s'.format([self])" - name: preLoadImages required: false schema: diff --git a/test/extension/handlers/topologymutation/handler.go b/test/extension/handlers/topologymutation/handler.go index ec3c1e217302..3a8ef9602c23 100644 --- a/test/extension/handlers/topologymutation/handler.go +++ b/test/extension/handlers/topologymutation/handler.go @@ -424,8 +424,14 @@ func (h *ExtensionHandlers) DiscoverVariables(ctx context.Context, _ *runtimehoo OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "string", Default: &apiextensionsv1.JSON{Raw: []byte(`""`)}, - Example: &apiextensionsv1.JSON{Raw: []byte(`""`)}, + Example: &apiextensionsv1.JSON{Raw: []byte(`"0"`)}, Description: "kubeadmControlPlaneMaxSurge is the maximum number of control planes that can be scheduled above or under the desired number of control plane machines.", + XValidations: []clusterv1.ValidationRule{ + { + Rule: "self == \"\" || self != \"\"", + MessageExpression: "'just a test expression, got %s'.format([self])", + }, + }, }, }, },