From 8834caa97bbd30274faa0f039cf1d9803c23b46f 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 15:06:07 +0200 Subject: [PATCH] Some more improvements (#4) --- .../variables/cluster_variable_validation.go | 2 +- .../cluster_variable_validation_test.go | 48 +++++++++- .../clusterclass_variable_validation.go | 4 +- .../clusterclass_variable_validation_test.go | 87 +++++++++++-------- internal/webhooks/cluster_test.go | 4 +- internal/webhooks/clusterclass_test.go | 1 - 6 files changed, 102 insertions(+), 44 deletions(-) diff --git a/internal/topology/variables/cluster_variable_validation.go b/internal/topology/variables/cluster_variable_validation.go index 1507234b8ace..51010d2321d9 100644 --- a/internal/topology/variables/cluster_variable_validation.go +++ b/internal/topology/variables/cluster_variable_validation.go @@ -251,7 +251,7 @@ func ValidateClusterVariable(ctx context.Context, value, oldValue *clusterv1.Clu // 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") + validationError.Field = strings.Replace(validationError.Field, "value.variableValue", "value", 1) allErrs = append(allErrs, validationError) } return allErrs diff --git a/internal/topology/variables/cluster_variable_validation_test.go b/internal/topology/variables/cluster_variable_validation_test.go index 18d6804b6803..8c24469feb8d 100644 --- a/internal/topology/variables/cluster_variable_validation_test.go +++ b/internal/topology/variables/cluster_variable_validation_test.go @@ -37,6 +37,7 @@ func Test_ValidateClusterVariables(t *testing.T) { validateRequired bool wantErrs []validationMatch }{ + // Basic cases { name: "Pass for a number of valid values.", definitions: []clusterv1.ClusterClassStatusVariable{ @@ -670,6 +671,7 @@ func Test_ValidateClusterVariable(t *testing.T) { clusterVariable *clusterv1.ClusterVariable wantErrs []validationMatch }{ + // Scalars { name: "Valid integer", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -735,7 +737,6 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, - { name: "Fails, expected integer got string", wantErrs: []validationMatch{ @@ -922,6 +923,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // Objects { name: "Valid object", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1180,6 +1182,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // Maps { name: "Valid map", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1272,6 +1275,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // Arrays { name: "Valid array", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1458,6 +1462,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // x-kubernetes-preserve-unknown-fields { name: "Valid object with x-kubernetes-preserve-unknown-fields", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1687,7 +1692,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, - + // CEL { name: "Valid CEL expression: scalar: using self", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1984,6 +1989,43 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + { + name: "Invalid CEL expression: objects (nested)", + wantErrs: []validationMatch{ + invalid("Invalid value: \"{\\\"objectField\\\": {\\\"field\\\": 2}}\": failed rule: self.field <= 1", + "spec.topology.variables[cpu].value.objectField"), + }, + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XPreserveUnknownFields: true, + Properties: map[string]clusterv1.JSONSchemaProps{ + "objectField": { + 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(`{"objectField": {"field": 2}}`), + }, + }, + }, { name: "Valid CEL expression: objects: defined field can be accessed", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -2263,6 +2305,7 @@ func Test_ValidateMachineVariables(t *testing.T) { oldValues []clusterv1.ClusterVariable wantErrs []validationMatch }{ + // Basic cases { name: "Pass when no value for required definition (required variables are not required for overrides)", definitions: []clusterv1.ClusterClassStatusVariable{ @@ -2405,6 +2448,7 @@ func Test_ValidateMachineVariables(t *testing.T) { }, }, }, + // CEL { name: "Valid integer with CEL expression", definitions: []clusterv1.ClusterClassStatusVariable{ diff --git a/internal/topology/variables/clusterclass_variable_validation.go b/internal/topology/variables/clusterclass_variable_validation.go index 7f753530c30c..806167855a34 100644 --- a/internal/topology/variables/clusterclass_variable_validation.go +++ b/internal/topology/variables/clusterclass_variable_validation.go @@ -250,11 +250,11 @@ func validateRootSchema(ctx context.Context, oldClusterClassVariables, clusterCl if celContext != nil && celContext.TotalCost != nil { if celContext.TotalCost.Total > StaticEstimatedCRDCostLimit { for _, expensive := range celContext.TotalCost.MostExpensive { - costErrorMsg := "contributed to estimated cost total exceeding cost limit for entire OpenAPIv3 schema" + costErrorMsg := "contributed to estimated rule & messageExpression cost total exceeding cost limit for entire OpenAPIv3 schema" allErrs = append(allErrs, field.Forbidden(expensive.Path, costErrorMsg)) } - costErrorMsg := getCostErrorMessage("x-kubernetes-validations estimated cost total for entire OpenAPIv3 schema", celContext.TotalCost.Total, StaticEstimatedCRDCostLimit) + costErrorMsg := getCostErrorMessage("x-kubernetes-validations estimated rule & messageExpression cost total for entire OpenAPIv3 schema", celContext.TotalCost.Total, StaticEstimatedCRDCostLimit) allErrs = append(allErrs, field.Forbidden(fldPath, costErrorMsg)) } } diff --git a/internal/topology/variables/clusterclass_variable_validation_test.go b/internal/topology/variables/clusterclass_variable_validation_test.go index bb77de6a3445..3fcc9347068e 100644 --- a/internal/topology/variables/clusterclass_variable_validation_test.go +++ b/internal/topology/variables/clusterclass_variable_validation_test.go @@ -35,6 +35,7 @@ func Test_ValidateClusterClassVariables(t *testing.T) { oldClusterClassVariables []clusterv1.ClusterClassVariable wantErrs []validationMatch }{ + // Basics { name: "Error if multiple variables share a name", clusterClassVariables: []clusterv1.ClusterClassVariable{ @@ -120,6 +121,25 @@ func Test_ValidateClusterClassVariables(t *testing.T) { }, }, }, + // CEL + { + 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])", + }}, + }, + }, + }, + }, + }, + // CEL compatibility version { name: "fail if x-kubernetes-validations has invalid rule: " + "new rule that uses opts that are not available with the current compatibility version", @@ -215,23 +235,6 @@ func Test_ValidateClusterClassVariables(t *testing.T) { "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", @@ -352,6 +355,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { clusterClassVariable *clusterv1.ClusterClassVariable wantErrs []validationMatch }{ + // Scalars { name: "Valid integer schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -376,18 +380,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - { - name: "Valid variable name", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "validVariable", - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - MinLength: ptr.To[int64](1), - }, - }, - }, - }, + // Variable names { name: "fail on variable name is builtin", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -436,6 +429,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[path.tovariable].name"), }, }, + // Metadata { name: "Valid variable metadata", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -498,6 +492,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[variable].metadata.annotations"), }, }, + // Defaults { name: "Valid default value regular string", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -554,6 +549,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.items.default"), }, }, + // Examples { name: "Valid example value regular string", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -610,6 +606,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.items.example"), }, }, + // Enums { name: "Valid enum values", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -668,6 +665,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.items.enum[1]"), }, }, + // Variable types { name: "fail on variable type is null", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -769,6 +767,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.additionalProperties.type"), }, }, + // Objects { name: "Valid object schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -821,6 +820,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type"), }, }, + // Maps { name: "Valid map schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -910,6 +910,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[httpProxy].schema.openAPIV3Schema"), }, }, + // Arrays { name: "Valid array schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -945,6 +946,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[arrayVariable].schema.openAPIV3Schema.items.default"), }, }, + // Required { name: "pass on variable with required set true with a default defined", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -958,6 +960,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, + // Defaults { name: "pass on variable with a default that is valid by the given schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1177,7 +1180,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - + // Examples { name: "pass on variable with an example that is valid by the given schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1288,7 +1291,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, { - name: "pass if variable is an object with a top level default value valid by the given schema", + name: "pass if variable is an object with a top level example value valid by the given schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ Name: "var", Schema: clusterv1.VariableSchema{ @@ -1312,6 +1315,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, + // Enums { name: "pass on variable with an enum with all variables valid by the given schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1468,6 +1472,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, + // CEL { name: "pass if x-kubernetes-validations has valid rule", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1500,6 +1505,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[cpu].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), }, }, + // CEL: uncorrelatable paths (in arrays) { name: "fail if x-kubernetes-validations has invalid rule: oldSef cannot be used in arrays", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1550,6 +1556,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[cpu].schema.openAPIV3Schema.items.items.x-kubernetes-validations[0].rule"), }, }, + // CEL: cost { name: "fail if x-kubernetes-validations has invalid rules: cost total exceeds total limit", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1589,14 +1596,15 @@ func Test_ValidateClusterClassVariable(t *testing.T) { 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", + forbidden("contributed to estimated rule & messageExpression 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", + forbidden("contributed to estimated rule & messageExpression 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", + forbidden("x-kubernetes-validations estimated rule & messageExpression cost total for entire OpenAPIv3 schema exceeds budget by factor of more than 100x", "spec.variables[cpu].schema.openAPIV3Schema"), }, }, + // CEL: rule { name: "fail if x-kubernetes-validations has invalid rule: rule is not specified", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1622,6 +1630,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].rule"), }, }, + // CEL: validation of defaults { name: "pass if x-kubernetes-validations has valid rule: valid default value", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1753,6 +1762,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, + // CEL: validation of examples { name: "pass if x-kubernetes-validations has valid rule: valid example value", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1839,6 +1849,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[cpu].schema.openAPIV3Schema.properties[nestedField].example"), }, }, + // CEL: validation of enums { name: "pass if x-kubernetes-validations has valid rule: valid enum value", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1929,6 +1940,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[cpu].schema.openAPIV3Schema.properties[nestedField].enum[1]"), }, }, + // CEL: reason { name: "fail if x-kubernetes-validations has invalid reason", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1974,6 +1986,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[0].reason"), }, }, + // CEL: field path { name: "fail if x-kubernetes-validations has invalid fieldPath for array", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -2150,6 +2163,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[5].fieldPath"), }, }, + // CEL: message { name: "fail if x-kubernetes-validations has invalid message", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -2200,6 +2214,7 @@ func Test_ValidateClusterClassVariable(t *testing.T) { "spec.variables[var].schema.openAPIV3Schema.x-kubernetes-validations[4].message"), }, }, + // CEL: messageExpression { name: "pass if x-kubernetes-validations has valid messageExpression", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -2300,9 +2315,9 @@ func Test_ValidateClusterClassVariable(t *testing.T) { 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", + forbidden("contributed to estimated rule & messageExpression 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", + forbidden("x-kubernetes-validations estimated rule & messageExpression cost total for entire OpenAPIv3 schema exceeds budget by factor of more than 100x", "spec.variables[cpu].schema.openAPIV3Schema"), }, }, diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index 38f844da8378..3d58c29cc411 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -941,6 +941,7 @@ func TestClusterDefaultAndValidateVariables(t *testing.T) { Build()). Build(), }, + // Testing validation of variables with CEL. { name: "should pass when CEL transition rules are skipped", clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). @@ -1220,9 +1221,8 @@ func TestClusterDefaultAndValidateVariables(t *testing.T) { t.Run("default", func(t *testing.T) { g := NewWithT(t) - webhookCtx := ctx - // Add old cluster to request if oldTopology is set. + webhookCtx := ctx if tt.oldTopology != nil { oldCluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). WithTopology(tt.oldTopology). diff --git a/internal/webhooks/clusterclass_test.go b/internal/webhooks/clusterclass_test.go index ac06f41e5c5a..b38f40784e1b 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1005,7 +1005,6 @@ func TestClusterClassValidation(t *testing.T) { }, // update tests - { name: "update pass in case of no changes", old: builder.ClusterClass(metav1.NamespaceDefault, "class1").