From ada27648c04e34174b7d4d851eee3df60f88de2c Mon Sep 17 00:00:00 2001 From: Chauncey Date: Thu, 27 Jun 2024 19:14:24 +0800 Subject: [PATCH] :sparkles: Introduce CEL for ClusterClass Variables (#9239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Introduce CEL for ClusterClass Variables Signed-off-by: chaunceyjiang * feat: Implement CEL validation * refactor: Add comments from previous code reviews * chore: Generate CC manifest after fixing list type annotation (#2) * chore: Fix up CRD manifest * fix: Pass through context to CEL funcs * feat: Add CEL admission cost validation * refactor: Add nolint to unbounded * refactor: Fix up new func signature * build: Fix up go mod for tools * fixup! refactor: Apply review feedback * fixup! build: Regenerate openapi spec * fixup! refactor: Apply review feedback * fixup! fix: Regenerate everything * fixup! fix: Apply review feedback * fixup! fix: More review feedback * fixup! refactor: Address review feedback, especially re recursion * fixup! fix: Check total cost * fixup! refactor: Address review feedback - rename testCtx to ctx * CEL: Various improvements (#3) * resolve compile issue after rebase * Some more improvements (#4) --------- Signed-off-by: chaunceyjiang Co-authored-by: Jimmi Dyson Co-authored-by: Jimmi Dyson Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com> Co-authored-by: Stefan Bueringer --- .golangci.yml | 6 + api/v1beta1/clusterclass_types.go | 130 +- api/v1beta1/zz_generated.deepcopy.go | 30 + api/v1beta1/zz_generated.openapi.go | 90 +- .../cluster.x-k8s.io_clusterclasses.yaml | 274 +++ go.mod | 2 +- hack/tools/go.mod | 7 + hack/tools/go.sum | 3 +- .../topology/cluster/cluster_controller.go | 2 +- .../variables/cluster_variable_defaulting.go | 5 +- .../cluster_variable_defaulting_test.go | 44 +- .../variables/cluster_variable_validation.go | 145 +- .../cluster_variable_validation_test.go | 1293 +++++++++++++- .../clusterclass_variable_validation.go | 385 ++++- .../clusterclass_variable_validation_test.go | 1498 ++++++++++++++++- internal/topology/variables/schema.go | 51 +- internal/topology/variables/schema_test.go | 79 +- internal/topology/variables/utils.go | 6 +- internal/webhooks/cluster.go | 84 +- internal/webhooks/cluster_test.go | 307 +++- internal/webhooks/clusterclass.go | 6 +- internal/webhooks/clusterclass_test.go | 74 +- .../main/clusterclass-quick-start.yaml | 3 + .../handlers/topologymutation/handler.go | 8 +- .../handler_integration_test.go | 2 +- webhooks/alias.go | 6 +- 26 files changed, 4239 insertions(+), 301 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 c8096780583f..fb1bad89edd6 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/api/v1beta1/clusterclass_types.go @@ -427,6 +427,8 @@ type VariableSchema struct { OpenAPIV3Schema JSONSchemaProps `json:"openAPIV3Schema"` } +// Adapted from https://github.com/kubernetes/apiextensions-apiserver/blob/v0.28.5/pkg/apis/apiextensions/v1/types_jsonschema.go#L40 + // JSONSchemaProps is a JSON-Schema following Specification Draft 4 (http://json-schema.org/). // This struct has been initially copied from apiextensionsv1.JSONSchemaProps, but all fields // which are not supported in CAPI have been removed. @@ -461,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 @@ -551,7 +563,123 @@ type JSONSchemaProps struct { // NOTE: Can be set for all types. // +optional Default *apiextensionsv1.JSON `json:"default,omitempty"` -} + + // XValidations describes a list of validation rules written in the CEL expression language. + // +optional + // +listType=map + // +listMapKey=rule + XValidations []ValidationRule `json:"x-kubernetes-validations,omitempty"` +} + +// ValidationRule describes a validation rule written in the CEL expression language. +type ValidationRule struct { + // Rule represents the expression which will be evaluated by CEL. + // ref: https://github.com/google/cel-spec + // 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)`. + // 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(...)`. + // If the Rule is scoped to an array, the elements of the array are accessible via `self[i]` and also by macros and + // functions. + // If the Rule is scoped to a scalar, `self` is bound to the scalar value. + // Examples: + // - Rule scoped to a map of objects: {"rule": "self.components['Widget'].priority < 10"} + // - Rule scoped to a list of integers: {"rule": "self.values.all(value, value >= 0 && value < 100)"} + // - Rule scoped to a string value: {"rule": "self.startsWith('kube')"} + // + // Unknown data preserved in custom resources via x-kubernetes-preserve-unknown-fields is not accessible in CEL + // expressions. This includes: + // - Unknown field values that are preserved by object schemas with x-kubernetes-preserve-unknown-fields. + // - Object properties where the property schema is of an "unknown type". An "unknown type" is recursively defined as: + // - A schema with no type and x-kubernetes-preserve-unknown-fields set to true + // - An array where the items schema is of an "unknown type" + // - An object where the additionalProperties schema is of an "unknown type" + // + // Only property names of the form `[a-zA-Z_.-/][a-zA-Z0-9_.-/]*` are accessible. + // Accessible property names are escaped according to the following rules when accessed in the expression: + // - '__' escapes to '__underscores__' + // - '.' escapes to '__dot__' + // - '-' escapes to '__dash__' + // - '/' escapes to '__slash__' + // - Property names that exactly match a CEL RESERVED keyword escape to '__{keyword}__'. The keywords are: + // "true", "false", "null", "in", "as", "break", "const", "continue", "else", "for", "function", "if", + // "import", "let", "loop", "package", "namespace", "return". + // Examples: + // - Rule accessing a property named "namespace": {"rule": "self.__namespace__ > 0"} + // - Rule accessing a property named "x-prop": {"rule": "self.x__dash__prop > 0"} + // - Rule accessing a property named "redact__d": {"rule": "self.redact__underscores__d > 0"} + // + // + // If `rule` makes use of the `oldSelf` variable it is implicitly a + // `transition rule`. + // + // By default, the `oldSelf` variable is the same type as `self`. + // + // Transition rules by default are applied only on UPDATE requests and are + // skipped if an old value could not be found. + // + // +kubebuilder:validation:Required + Rule string `json:"rule"` + // Message represents the message displayed when validation fails. The message is required if the Rule contains + // line breaks. The message must not contain line breaks. + // If unset, the message is "failed rule: {Rule}". + // e.g. "must be a URL with the host matching spec.host" + // +optional + Message string `json:"message,omitempty"` + // 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 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. + // 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)+")" + // +optional + MessageExpression string `json:"messageExpression,omitempty"` + // Reason provides a machine-readable validation failure reason that is returned to the caller when a request fails this validation rule. + // The currently supported reasons are: "FieldValueInvalid", "FieldValueForbidden", "FieldValueRequired", "FieldValueDuplicate". + // If not set, default to use "FieldValueInvalid". + // All future added reasons must be accepted by clients when reading this value and unknown reasons should be treated as FieldValueInvalid. + // +optional + // +kubebuilder:validation:Enum=FieldValueInvalid;FieldValueForbidden;FieldValueRequired;FieldValueDuplicate + // +kubebuilder:default=FieldValueInvalid + // +default=ref(sigs.k8s.io/cluster-api/api/v1beta1.FieldValueInvalid) + Reason FieldValueErrorReason `json:"reason,omitempty"` + // FieldPath represents the field path returned when the validation fails. + // It must be a relative JSON path (i.e. with array notation) scoped to the location of this x-kubernetes-validations extension in the schema and refer to an existing field. + // e.g. when validation checks if a specific attribute `foo` under a map `testMap`, the fieldPath could be set to `.testMap.foo` + // If the validation checks two lists must have unique attributes, the fieldPath could be set to either of the list: e.g. `.testList` + // It does not support list numeric index. + // It supports child operation to refer to an existing field currently. Refer to [JSONPath support in Kubernetes](https://kubernetes.io/docs/reference/kubectl/jsonpath/) for more info. + // Numeric index of array is not supported. + // For field name which contains special characters, use `['specialName']` to refer the field name. + // e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` + // +optional + FieldPath string `json:"fieldPath,omitempty"` +} + +// FieldValueErrorReason is a machine-readable value providing more detail about why a field failed the validation. +type FieldValueErrorReason string + +const ( + // FieldValueRequired is used to report required values that are not + // provided (e.g. empty strings, null values, or empty arrays). + FieldValueRequired FieldValueErrorReason = "FieldValueRequired" + // FieldValueDuplicate is used to report collisions of values that must be + // unique (e.g. unique IDs). + FieldValueDuplicate FieldValueErrorReason = "FieldValueDuplicate" + // FieldValueInvalid is used to report malformed values (e.g. failed regex + // match, too long, out of bounds). + FieldValueInvalid FieldValueErrorReason = "FieldValueInvalid" + // FieldValueForbidden is used to report valid (as per formatting rules) + // values which would be accepted under some conditions, but which are not + // permitted by the current conditions (such as security policy). + FieldValueForbidden FieldValueErrorReason = "FieldValueForbidden" +) // ClusterClassPatch defines a patch which is applied to customize the referenced templates. type ClusterClassPatch struct { diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index aa62c8fb9f4f..80b25c5197c4 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -808,6 +808,16 @@ func (in *JSONSchemaProps) DeepCopyInto(out *JSONSchemaProps) { *out = new(JSONSchemaProps) (*in).DeepCopyInto(*out) } + if in.MaxProperties != nil { + in, out := &in.MaxProperties, &out.MaxProperties + *out = new(int64) + **out = **in + } + if in.MinProperties != nil { + in, out := &in.MinProperties, &out.MinProperties + *out = new(int64) + **out = **in + } if in.Required != nil { in, out := &in.Required, &out.Required *out = make([]string, len(*in)) @@ -860,6 +870,11 @@ func (in *JSONSchemaProps) DeepCopyInto(out *JSONSchemaProps) { *out = new(apiextensionsv1.JSON) (*in).DeepCopyInto(*out) } + if in.XValidations != nil { + in, out := &in.XValidations, &out.XValidations + *out = make([]ValidationRule, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JSONSchemaProps. @@ -2142,6 +2157,21 @@ func (in *UnhealthyCondition) DeepCopy() *UnhealthyCondition { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ValidationRule) DeepCopyInto(out *ValidationRule) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ValidationRule. +func (in *ValidationRule) DeepCopy() *ValidationRule { + if in == nil { + return nil + } + out := new(ValidationRule) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *VariableSchema) DeepCopyInto(out *VariableSchema) { *out = *in diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 4953306c2985..0ab52afeb13c 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -98,6 +98,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "sigs.k8s.io/cluster-api/api/v1beta1.RemediationStrategy": schema_sigsk8sio_cluster_api_api_v1beta1_RemediationStrategy(ref), "sigs.k8s.io/cluster-api/api/v1beta1.Topology": schema_sigsk8sio_cluster_api_api_v1beta1_Topology(ref), "sigs.k8s.io/cluster-api/api/v1beta1.UnhealthyCondition": schema_sigsk8sio_cluster_api_api_v1beta1_UnhealthyCondition(ref), + "sigs.k8s.io/cluster-api/api/v1beta1.ValidationRule": schema_sigsk8sio_cluster_api_api_v1beta1_ValidationRule(ref), "sigs.k8s.io/cluster-api/api/v1beta1.VariableSchema": schema_sigsk8sio_cluster_api_api_v1beta1_VariableSchema(ref), "sigs.k8s.io/cluster-api/api/v1beta1.WorkersClass": schema_sigsk8sio_cluster_api_api_v1beta1_WorkersClass(ref), "sigs.k8s.io/cluster-api/api/v1beta1.WorkersTopology": schema_sigsk8sio_cluster_api_api_v1beta1_WorkersTopology(ref), @@ -1366,6 +1367,20 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_JSONSchemaProps(ref common.Referen Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.JSONSchemaProps"), }, }, + "maxProperties": { + SchemaProps: spec.SchemaProps{ + Description: "MaxProperties is the maximum amount of entries in a map or properties in an object. NOTE: Can only be set if type is object.", + Type: []string{"integer"}, + Format: "int64", + }, + }, + "minProperties": { + SchemaProps: spec.SchemaProps{ + Description: "MinProperties is the minimum amount of entries in a map or properties in an object. NOTE: Can only be set if type is object.", + Type: []string{"integer"}, + Format: "int64", + }, + }, "required": { SchemaProps: spec.SchemaProps{ Description: "Required specifies which fields of an object are required. NOTE: Can only be set if type is object.", @@ -1490,12 +1505,34 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_JSONSchemaProps(ref common.Referen Ref: ref("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON"), }, }, + "x-kubernetes-validations": { + VendorExtensible: spec.VendorExtensible{ + Extensions: spec.Extensions{ + "x-kubernetes-list-map-keys": []interface{}{ + "rule", + }, + "x-kubernetes-list-type": "map", + }, + }, + SchemaProps: spec.SchemaProps{ + Description: "XValidations describes a list of validation rules written in the CEL expression language.", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Default: map[string]interface{}{}, + Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.ValidationRule"), + }, + }, + }, + }, + }, }, Required: []string{"type"}, }, }, Dependencies: []string{ - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON", "sigs.k8s.io/cluster-api/api/v1beta1.JSONSchemaProps"}, + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON", "sigs.k8s.io/cluster-api/api/v1beta1.JSONSchemaProps", "sigs.k8s.io/cluster-api/api/v1beta1.ValidationRule"}, } } @@ -3689,6 +3726,57 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_UnhealthyCondition(ref common.Refe } } +func schema_sigsk8sio_cluster_api_api_v1beta1_ValidationRule(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "ValidationRule describes a validation rule written in the CEL expression language.", + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "rule": { + SchemaProps: spec.SchemaProps{ + Description: "Rule represents the expression which will be evaluated by CEL. ref: https://github.com/google/cel-spec 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)`. 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(...)`. If the Rule is scoped to an array, the elements of the array are accessible via `self[i]` and also by macros and functions. If the Rule is scoped to a scalar, `self` is bound to the scalar value. Examples: - Rule scoped to a map of objects: {\"rule\": \"self.components['Widget'].priority < 10\"} - Rule scoped to a list of integers: {\"rule\": \"self.values.all(value, value >= 0 && value < 100)\"} - Rule scoped to a string value: {\"rule\": \"self.startsWith('kube')\"}\n\nUnknown data preserved in custom resources via x-kubernetes-preserve-unknown-fields is not accessible in CEL expressions. This includes: - Unknown field values that are preserved by object schemas with x-kubernetes-preserve-unknown-fields. - Object properties where the property schema is of an \"unknown type\". An \"unknown type\" is recursively defined as:\n - A schema with no type and x-kubernetes-preserve-unknown-fields set to true\n - An array where the items schema is of an \"unknown type\"\n - An object where the additionalProperties schema is of an \"unknown type\"\n\nOnly property names of the form `[a-zA-Z_.-/][a-zA-Z0-9_.-/]*` are accessible. Accessible property names are escaped according to the following rules when accessed in the expression: - '__' escapes to '__underscores__' - '.' escapes to '__dot__' - '-' escapes to '__dash__' - '/' escapes to '__slash__' - Property names that exactly match a CEL RESERVED keyword escape to '__{keyword}__'. The keywords are:\n\t \"true\", \"false\", \"null\", \"in\", \"as\", \"break\", \"const\", \"continue\", \"else\", \"for\", \"function\", \"if\",\n\t \"import\", \"let\", \"loop\", \"package\", \"namespace\", \"return\".\nExamples:\n - Rule accessing a property named \"namespace\": {\"rule\": \"self.__namespace__ > 0\"}\n - Rule accessing a property named \"x-prop\": {\"rule\": \"self.x__dash__prop > 0\"}\n - Rule accessing a property named \"redact__d\": {\"rule\": \"self.redact__underscores__d > 0\"}\n\nIf `rule` makes use of the `oldSelf` variable it is implicitly a `transition rule`.\n\nBy default, the `oldSelf` variable is the same type as `self`.\n\nTransition rules by default are applied only on UPDATE requests and are skipped if an old value could not be found.", + Default: "", + Type: []string{"string"}, + Format: "", + }, + }, + "message": { + SchemaProps: spec.SchemaProps{ + Description: "Message represents the message displayed when validation fails. The message is required if the Rule contains line breaks. The message must not contain line breaks. If unset, the message is \"failed rule: {Rule}\". e.g. \"must be a URL with the host matching spec.host\"", + Type: []string{"string"}, + Format: "", + }, + }, + "messageExpression": { + SchemaProps: spec.SchemaProps{ + Description: "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 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. 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)+\")\"", + Type: []string{"string"}, + Format: "", + }, + }, + "reason": { + SchemaProps: spec.SchemaProps{ + Description: "Reason provides a machine-readable validation failure reason that is returned to the caller when a request fails this validation rule. The currently supported reasons are: \"FieldValueInvalid\", \"FieldValueForbidden\", \"FieldValueRequired\", \"FieldValueDuplicate\". If not set, default to use \"FieldValueInvalid\". All future added reasons must be accepted by clients when reading this value and unknown reasons should be treated as FieldValueInvalid.", + Default: FieldValueInvalid, + Type: []string{"string"}, + Format: "", + }, + }, + "fieldPath": { + SchemaProps: spec.SchemaProps{ + Description: "FieldPath represents the field path returned when the validation fails. It must be a relative JSON path (i.e. with array notation) scoped to the location of this x-kubernetes-validations extension in the schema and refer to an existing field. e.g. when validation checks if a specific attribute `foo` under a map `testMap`, the fieldPath could be set to `.testMap.foo` If the validation checks two lists must have unique attributes, the fieldPath could be set to either of the list: e.g. `.testList` It does not support list numeric index. It supports child operation to refer to an existing field currently. Refer to [JSONPath support in Kubernetes](https://kubernetes.io/docs/reference/kubectl/jsonpath/) for more info. Numeric index of array is not supported. For field name which contains special characters, use `['specialName']` to refer the field name. e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']`", + Type: []string{"string"}, + Format: "", + }, + }, + }, + Required: []string{"rule"}, + }, + }, + } +} + func schema_sigsk8sio_cluster_api_api_v1beta1_VariableSchema(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index 45c8cb9e3bd7..be84aeedd5d4 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -1094,6 +1094,12 @@ spec: NOTE: Can only be set if type is string. format: int64 type: integer + maxProperties: + description: |- + MaxProperties is the maximum amount of entries in a map or properties in an object. + NOTE: Can only be set if type is object. + format: int64 + type: integer maximum: description: |- Maximum is the maximum of an integer or number variable. @@ -1114,6 +1120,12 @@ spec: NOTE: Can only be set if type is string. format: int64 type: integer + minProperties: + description: |- + MinProperties is the minimum amount of entries in a map or properties in an object. + NOTE: Can only be set if type is object. + format: int64 + type: integer minimum: description: |- Minimum is the minimum of an integer or number variable. @@ -1158,6 +1170,126 @@ spec: which are not defined in the variable schema. This affects fields recursively, except if nested properties or additionalProperties are specified in the schema. type: boolean + x-kubernetes-validations: + description: XValidations describes a list of validation + rules written in the CEL expression language. + items: + description: ValidationRule describes a validation + rule written in the CEL expression language. + properties: + fieldPath: + description: |- + FieldPath represents the field path returned when the validation fails. + It must be a relative JSON path (i.e. with array notation) scoped to the location of this x-kubernetes-validations extension in the schema and refer to an existing field. + e.g. when validation checks if a specific attribute `foo` under a map `testMap`, the fieldPath could be set to `.testMap.foo` + If the validation checks two lists must have unique attributes, the fieldPath could be set to either of the list: e.g. `.testList` + It does not support list numeric index. + It supports child operation to refer to an existing field currently. Refer to [JSONPath support in Kubernetes](https://kubernetes.io/docs/reference/kubectl/jsonpath/) for more info. + Numeric index of array is not supported. + For field name which contains special characters, use `['specialName']` to refer the field name. + e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` + type: string + message: + description: |- + Message represents the message displayed when validation fails. The message is required if the Rule contains + line breaks. The message must not contain line breaks. + If unset, the message is "failed rule: {Rule}". + e.g. "must be a URL with the host matching spec.host" + type: string + messageExpression: + description: |- + 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 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. + 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)+")" + type: string + reason: + default: FieldValueInvalid + description: |- + Reason provides a machine-readable validation failure reason that is returned to the caller when a request fails this validation rule. + The currently supported reasons are: "FieldValueInvalid", "FieldValueForbidden", "FieldValueRequired", "FieldValueDuplicate". + If not set, default to use "FieldValueInvalid". + All future added reasons must be accepted by clients when reading this value and unknown reasons should be treated as FieldValueInvalid. + enum: + - FieldValueInvalid + - FieldValueForbidden + - FieldValueRequired + - FieldValueDuplicate + type: string + rule: + description: "Rule represents the expression which + will be evaluated by CEL.\nref: https://github.com/google/cel-spec\nThe + Rule is scoped to the location of the x-kubernetes-validations + extension in the schema.\nThe `self` variable + in the CEL expression is bound to the scoped + value.\nIf the Rule is scoped to an object with + properties, the accessible properties of the + object are field selectable\nvia `self.field` + and field presence can be checked via `has(self.field)`.\nIf + the Rule is scoped to an object with additionalProperties + (i.e. a map) the value of the map\nare accessible + via `self[mapKey]`, map containment can be checked + via `mapKey in self` and all entries of the + map\nare accessible via CEL macros and functions + such as `self.all(...)`.\nIf the Rule is scoped + to an array, the elements of the array are accessible + via `self[i]` and also by macros and\nfunctions.\nIf + the Rule is scoped to a scalar, `self` is bound + to the scalar value.\nExamples:\n- Rule scoped + to a map of objects: {\"rule\": \"self.components['Widget'].priority + < 10\"}\n- Rule scoped to a list of integers: + {\"rule\": \"self.values.all(value, value >= + 0 && value < 100)\"}\n- Rule scoped to a string + value: {\"rule\": \"self.startsWith('kube')\"}\n\n\nUnknown + data preserved in custom resources via x-kubernetes-preserve-unknown-fields + is not accessible in CEL\nexpressions. This + includes:\n- Unknown field values that are preserved + by object schemas with x-kubernetes-preserve-unknown-fields.\n- + Object properties where the property schema + is of an \"unknown type\". An \"unknown type\" + is recursively defined as:\n - A schema with + no type and x-kubernetes-preserve-unknown-fields + set to true\n - An array where the items schema + is of an \"unknown type\"\n - An object where + the additionalProperties schema is of an \"unknown + type\"\n\n\nOnly property names of the form + `[a-zA-Z_.-/][a-zA-Z0-9_.-/]*` are accessible.\nAccessible + property names are escaped according to the + following rules when accessed in the expression:\n- + '__' escapes to '__underscores__'\n- '.' escapes + to '__dot__'\n- '-' escapes to '__dash__'\n- + '/' escapes to '__slash__'\n- Property names + that exactly match a CEL RESERVED keyword escape + to '__{keyword}__'. The keywords are:\n\t \"true\", + \"false\", \"null\", \"in\", \"as\", \"break\", + \"const\", \"continue\", \"else\", \"for\", + \"function\", \"if\",\n\t \"import\", \"let\", + \"loop\", \"package\", \"namespace\", \"return\".\nExamples:\n + \ - Rule accessing a property named \"namespace\": + {\"rule\": \"self.__namespace__ > 0\"}\n - + Rule accessing a property named \"x-prop\": + {\"rule\": \"self.x__dash__prop > 0\"}\n - + Rule accessing a property named \"redact__d\": + {\"rule\": \"self.redact__underscores__d > 0\"}\n\n\nIf + `rule` makes use of the `oldSelf` variable it + is implicitly a\n`transition rule`.\n\n\nBy + default, the `oldSelf` variable is the same + type as `self`.\n\n\nTransition rules by default + are applied only on UPDATE requests and are\nskipped + if an old value could not be found." + type: string + required: + - rule + type: object + type: array + x-kubernetes-list-map-keys: + - rule + x-kubernetes-list-type: map required: - type type: object @@ -1990,6 +2122,12 @@ spec: NOTE: Can only be set if type is string. format: int64 type: integer + maxProperties: + description: |- + MaxProperties is the maximum amount of entries in a map or properties in an object. + NOTE: Can only be set if type is object. + format: int64 + type: integer maximum: description: |- Maximum is the maximum of an integer or number variable. @@ -2010,6 +2148,12 @@ spec: NOTE: Can only be set if type is string. format: int64 type: integer + minProperties: + description: |- + MinProperties is the minimum amount of entries in a map or properties in an object. + NOTE: Can only be set if type is object. + format: int64 + type: integer minimum: description: |- Minimum is the minimum of an integer or number variable. @@ -2054,6 +2198,136 @@ spec: which are not defined in the variable schema. This affects fields recursively, except if nested properties or additionalProperties are specified in the schema. type: boolean + x-kubernetes-validations: + description: XValidations describes a list of + validation rules written in the CEL expression + language. + items: + description: ValidationRule describes a validation + rule written in the CEL expression language. + properties: + fieldPath: + description: |- + FieldPath represents the field path returned when the validation fails. + It must be a relative JSON path (i.e. with array notation) scoped to the location of this x-kubernetes-validations extension in the schema and refer to an existing field. + e.g. when validation checks if a specific attribute `foo` under a map `testMap`, the fieldPath could be set to `.testMap.foo` + If the validation checks two lists must have unique attributes, the fieldPath could be set to either of the list: e.g. `.testList` + It does not support list numeric index. + It supports child operation to refer to an existing field currently. Refer to [JSONPath support in Kubernetes](https://kubernetes.io/docs/reference/kubectl/jsonpath/) for more info. + Numeric index of array is not supported. + For field name which contains special characters, use `['specialName']` to refer the field name. + e.g. for attribute `foo.34$` appears in a list `testList`, the fieldPath could be set to `.testList['foo.34$']` + type: string + message: + description: |- + Message represents the message displayed when validation fails. The message is required if the Rule contains + line breaks. The message must not contain line breaks. + If unset, the message is "failed rule: {Rule}". + e.g. "must be a URL with the host matching spec.host" + type: string + messageExpression: + description: |- + 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 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. + 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)+")" + type: string + reason: + default: FieldValueInvalid + description: |- + Reason provides a machine-readable validation failure reason that is returned to the caller when a request fails this validation rule. + The currently supported reasons are: "FieldValueInvalid", "FieldValueForbidden", "FieldValueRequired", "FieldValueDuplicate". + If not set, default to use "FieldValueInvalid". + All future added reasons must be accepted by clients when reading this value and unknown reasons should be treated as FieldValueInvalid. + enum: + - FieldValueInvalid + - FieldValueForbidden + - FieldValueRequired + - FieldValueDuplicate + type: string + rule: + description: "Rule represents the expression + which will be evaluated by CEL.\nref: + https://github.com/google/cel-spec\nThe + Rule is scoped to the location of the + x-kubernetes-validations extension in + the schema.\nThe `self` variable in the + CEL expression is bound to the scoped + value.\nIf the Rule is scoped to an object + with properties, the accessible properties + of the object are field selectable\nvia + `self.field` and field presence can be + checked via `has(self.field)`.\nIf the + Rule is scoped to an object with additionalProperties + (i.e. a map) the value of the map\nare + accessible via `self[mapKey]`, map containment + can be checked via `mapKey in self` and + all entries of the map\nare accessible + via CEL macros and functions such as `self.all(...)`.\nIf + the Rule is scoped to an array, the elements + of the array are accessible via `self[i]` + and also by macros and\nfunctions.\nIf + the Rule is scoped to a scalar, `self` + is bound to the scalar value.\nExamples:\n- + Rule scoped to a map of objects: {\"rule\": + \"self.components['Widget'].priority < + 10\"}\n- Rule scoped to a list of integers: + {\"rule\": \"self.values.all(value, value + >= 0 && value < 100)\"}\n- Rule scoped + to a string value: {\"rule\": \"self.startsWith('kube')\"}\n\n\nUnknown + data preserved in custom resources via + x-kubernetes-preserve-unknown-fields is + not accessible in CEL\nexpressions. This + includes:\n- Unknown field values that + are preserved by object schemas with x-kubernetes-preserve-unknown-fields.\n- + Object properties where the property schema + is of an \"unknown type\". An \"unknown + type\" is recursively defined as:\n - + A schema with no type and x-kubernetes-preserve-unknown-fields + set to true\n - An array where the items + schema is of an \"unknown type\"\n - + An object where the additionalProperties + schema is of an \"unknown type\"\n\n\nOnly + property names of the form `[a-zA-Z_.-/][a-zA-Z0-9_.-/]*` + are accessible.\nAccessible property names + are escaped according to the following + rules when accessed in the expression:\n- + '__' escapes to '__underscores__'\n- '.' + escapes to '__dot__'\n- '-' escapes to + '__dash__'\n- '/' escapes to '__slash__'\n- + Property names that exactly match a CEL + RESERVED keyword escape to '__{keyword}__'. + The keywords are:\n\t \"true\", \"false\", + \"null\", \"in\", \"as\", \"break\", \"const\", + \"continue\", \"else\", \"for\", \"function\", + \"if\",\n\t \"import\", \"let\", \"loop\", + \"package\", \"namespace\", \"return\".\nExamples:\n + \ - Rule accessing a property named \"namespace\": + {\"rule\": \"self.__namespace__ > 0\"}\n + \ - Rule accessing a property named \"x-prop\": + {\"rule\": \"self.x__dash__prop > 0\"}\n + \ - Rule accessing a property named \"redact__d\": + {\"rule\": \"self.redact__underscores__d + > 0\"}\n\n\nIf `rule` makes use of the + `oldSelf` variable it is implicitly a\n`transition + rule`.\n\n\nBy default, the `oldSelf` + variable is the same type as `self`.\n\n\nTransition + rules by default are applied only on UPDATE + requests and are\nskipped if an old value + could not be found." + type: string + required: + - rule + type: object + type: array + x-kubernetes-list-map-keys: + - rule + x-kubernetes-list-type: map required: - type type: object diff --git a/go.mod b/go.mod index 5637656ecda7..eacd5e29977c 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/flatcar/ignition v0.36.2 github.com/go-logr/logr v1.4.2 github.com/gobuffalo/flect v1.0.2 + github.com/google/cel-go v0.17.8 github.com/google/go-cmp v0.6.0 github.com/google/go-github/v53 v53.2.0 github.com/google/gofuzz v1.2.0 @@ -88,7 +89,6 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.4 // indirect github.com/google/btree v1.0.1 // indirect - github.com/google/cel-go v0.17.8 // indirect github.com/google/gnostic-models v0.6.8 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/pprof v0.0.0-20240424215950-a892ee059fd6 // indirect diff --git a/hack/tools/go.mod b/hack/tools/go.mod index 31ccb969ec64..a3bf5ec9f29d 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -36,8 +36,15 @@ require ( require ( cloud.google.com/go/auth v0.5.1 // indirect cloud.google.com/go/auth/oauth2adapt v0.2.2 // indirect + github.com/cenkalti/backoff/v4 v4.2.1 // indirect + github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect github.com/moby/docker-image-spec v1.3.1 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 // indirect + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.20.0 // indirect + go.opentelemetry.io/otel/sdk v1.24.0 // indirect + go.opentelemetry.io/proto/otlp v1.0.0 // indirect + sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.0 // indirect ) require ( diff --git a/hack/tools/go.sum b/hack/tools/go.sum index b3e25e22af0e..42a03faac3bb 100644 --- a/hack/tools/go.sum +++ b/hack/tools/go.sum @@ -146,6 +146,8 @@ github.com/gobuffalo/flect v1.0.2/go.mod h1:A5msMlrHtLqh9umBSnvabjsMrCcCpAyzglnD github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= +github.com/golang/glog v1.2.0 h1:uCdmnmatrKCgMBlM4rMuJZWOkPDqdbZPnrMXDY4gI68= +github.com/golang/glog v1.2.0/go.mod h1:6AhwSGph0fcJtXVM/PEHPqZlFeoLxhs7/t5UDAwmO+w= github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da h1:oI5xCqsCo564l8iNU+DwB5epxmsaqB+rhGL0m5jtYqE= github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= @@ -202,7 +204,6 @@ github.com/googleapis/gax-go/v2 v2.12.4 h1:9gWcmF85Wvq4ryPFvGFaOgPIs1AQX0d0bcbGw github.com/googleapis/gax-go/v2 v2.12.4/go.mod h1:KYEYLorsnIGDi/rPC8b5TdlB9kbKoFubselGIoBMCwI= github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho= github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk= -github.com/grpc-ecosystem/grpc-gateway v1.16.0 h1:gmcG1KaJ57LophUzW0Hy8NmPhnMZb4M0+kPpLofRdBo= github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 h1:YBftPWNWd4WwGqtY2yeZL2ef8rHAxPBD8KFhJpmcqms= github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0/go.mod h1:YN5jB8ie0yfIUg6VvR9Kz84aCaG7AsGZnLjhHbUqwPg= github.com/hashicorp/hcl v1.0.1-vault-5 h1:kI3hhbbyzr4dldA8UdTb7ZlVVlI2DACdCfz31RPDgJM= diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index c3415c4b8426..4200b49b257f 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -234,7 +234,7 @@ func (r *Reconciler) reconcile(ctx context.Context, s *scope.Scope) (ctrl.Result // Default and Validate the Cluster variables based on information from the ClusterClass. // This step is needed as if the ClusterClass does not exist at Cluster creation some fields may not be defaulted or // validated in the webhook. - if errs := webhooks.DefaultAndValidateVariables(s.Current.Cluster, clusterClass); len(errs) > 0 { + if errs := webhooks.DefaultAndValidateVariables(ctx, s.Current.Cluster, nil, clusterClass); len(errs) > 0 { return ctrl.Result{}, apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), s.Current.Cluster.Name, errs) } 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 e9cc24c97496..51010d2321d9 100644 --- a/internal/topology/variables/cluster_variable_validation.go +++ b/internal/topology/variables/cluster_variable_validation.go @@ -17,36 +17,39 @@ limitations under the License. package variables import ( - "encoding/json" + "context" "fmt" "strings" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" structuralpruning "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning" "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/validation/field" + celconfig "k8s.io/apiserver/pkg/apis/cel" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) // ValidateClusterVariables validates ClusterVariables based on the definitions in ClusterClass `.status.variables`. -func ValidateClusterVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { - return validateClusterVariables(values, definitions, true, fldPath) +func ValidateClusterVariables(ctx context.Context, values, oldValues []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { + return validateClusterVariables(ctx, values, oldValues, definitions, true, fldPath) } // ValidateControlPlaneVariables validates ControlPLane variables. -func ValidateControlPlaneVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { - return validateClusterVariables(values, definitions, false, fldPath) +func ValidateControlPlaneVariables(ctx context.Context, values, oldValues []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { + return validateClusterVariables(ctx, values, oldValues, definitions, false, fldPath) } // ValidateMachineVariables validates MachineDeployment and MachinePool variables. -func ValidateMachineVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { - return validateClusterVariables(values, definitions, false, fldPath) +func ValidateMachineVariables(ctx context.Context, values, oldValues []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { + return validateClusterVariables(ctx, values, oldValues, definitions, false, fldPath) } // validateClusterVariables validates variable values according to the corresponding definition. -func validateClusterVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, validateRequired bool, fldPath *field.Path) field.ErrorList { +func validateClusterVariables(ctx context.Context, values, oldValues []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, validateRequired bool, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList // Get a map of ClusterVariable values. This function validates that: @@ -61,6 +64,17 @@ func validateClusterVariables(values []clusterv1.ClusterVariable, definitions [] return append(allErrs, field.Invalid(fldPath, "["+strings.Join(valueStrings, ",")+"]", fmt.Sprintf("cluster variables not valid: %s", err))) } + // Get a map of old ClusterVariable values. We know they are all valid and not duplicate names, etc. as previous + // validation has already asserted that. + oldValuesMap, err := newValuesIndex(oldValues) + if err != nil { + var valueStrings []string + for _, v := range values { + valueStrings = append(valueStrings, fmt.Sprintf("Name: %s DefinitionFrom: %s", v.Name, v.DefinitionFrom)) + } + return append(allErrs, field.Invalid(fldPath, "["+strings.Join(valueStrings, ",")+"]", fmt.Sprintf("old cluster variables not valid: %s", err))) + } + // Get an index of definitions for each variable name and definition from the ClusterClass variable. defIndex := newDefinitionsIndex(definitions) @@ -70,19 +84,35 @@ func validateClusterVariables(values []clusterv1.ClusterVariable, definitions [] } 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 definitionFrom defined in the old Cluster then pass this in + // to cluster variable validation. + 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(value.DeepCopy(), &clusterv1.ClusterClassVariable{ - Name: value.Name, - Required: definition.Required, - Schema: definition.Schema, - }, fldPath)...) + allErrs = append(allErrs, ValidateClusterVariable( + ctx, + value.DeepCopy(), + oldValue, + &clusterv1.ClusterClassVariable{ + Name: value.Name, + Required: definition.Required, + Schema: definition.Schema, + }, fldPath)...) } return allErrs @@ -124,7 +154,7 @@ func validateRequiredVariables(values map[string]map[string]clusterv1.ClusterVar } // ValidateClusterVariable validates a clusterVariable. -func ValidateClusterVariable(value *clusterv1.ClusterVariable, definition *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { +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. @@ -152,22 +182,23 @@ func ValidateClusterVariable(value *clusterv1.ClusterVariable, definition *clust // 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 } - return validateUnknownFields(fldPath, value, variableValue, apiExtensionsSchema) -} - -// 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, clusterVariable *clusterv1.ClusterVariable, variableValue interface{}, variableSchema *apiextensions.JSONSchemaProps) field.ErrorList { // Structural schema pruning does not work with scalar values, // so we wrap the schema and the variable in objects. // : wrappedVariable := map[string]interface{}{ - clusterVariable.Name: variableValue, + "variableValue": variableValue, } // type: object // properties: @@ -175,15 +206,64 @@ func validateUnknownFields(fldPath *field.Path, clusterVariable *clusterv1.Clust wrappedSchema := &apiextensions.JSONSchemaProps{ Type: "object", Properties: map[string]apiextensions.JSONSchemaProps{ - clusterVariable.Name: *variableSchema, + "variableValue": *apiExtensionsSchema, }, } ss, err := structuralschema.NewStructural(wrappedSchema) if err != nil { - return field.ErrorList{field.Invalid(fldPath, "", - fmt.Sprintf("failed defaulting variable %q: %v", clusterVariable.Name, err))} + return field.ErrorList{ + field.InternalError(fldPath, + 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, 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`. + if celValidator == nil { + return nil + } + + // 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{}{ + "variableValue": oldVariableValue, + } + } + + // 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.Replace(validationError.Field, "value.variableValue", "value", 1) + allErrs = append(allErrs, validationError) + } + return allErrs } + return nil +} + +// 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, 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. @@ -194,9 +274,14 @@ func validateUnknownFields(fldPath *field.Path, clusterVariable *clusterv1.Clust // 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, ","), clusterVariable.Name)), + 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 8c91011ad434..8c24469feb8d 100644 --- a/internal/topology/variables/cluster_variable_validation_test.go +++ b/internal/topology/variables/cluster_variable_validation_test.go @@ -19,22 +19,25 @@ 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" + ctrl "sigs.k8s.io/controller-runtime" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +var ctx = ctrl.SetupSignalHandler() + func Test_ValidateClusterVariables(t *testing.T) { tests := []struct { name string definitions []clusterv1.ClusterClassStatusVariable values []clusterv1.ClusterVariable validateRequired bool - wantErr bool + wantErrs []validationMatch }{ + // Basic cases { name: "Pass for a number of valid values.", definitions: []clusterv1.ClusterClassStatusVariable{ @@ -110,8 +113,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", @@ -203,8 +209,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. @@ -302,8 +311,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", @@ -332,8 +344,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", @@ -368,8 +383,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", @@ -413,8 +431,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", @@ -544,8 +567,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", @@ -583,8 +609,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", @@ -627,16 +656,10 @@ func Test_ValidateClusterVariables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := validateClusterVariables(tt.values, 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) }) } } @@ -646,8 +669,9 @@ func Test_ValidateClusterVariable(t *testing.T) { name string clusterClassVariable *clusterv1.ClusterClassVariable clusterVariable *clusterv1.ClusterVariable - wantErr bool + wantErrs []validationMatch }{ + // Scalars { name: "Valid integer", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -668,8 +692,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, @@ -688,8 +715,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, @@ -707,10 +737,12 @@ 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, @@ -748,8 +780,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, @@ -768,8 +803,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, @@ -810,8 +848,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, @@ -855,8 +896,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, @@ -877,6 +923,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // Objects { name: "Valid object", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -901,8 +948,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, @@ -925,8 +975,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, @@ -949,8 +1002,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, @@ -978,6 +1036,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{ @@ -1047,8 +1149,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, @@ -1077,6 +1182,7 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, + // Maps { name: "Valid map", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1104,8 +1210,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, @@ -1134,6 +1243,39 @@ 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}}`), + }, + }, + }, + // Arrays { name: "Valid array", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -1160,8 +1302,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, @@ -1186,8 +1331,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, @@ -1209,8 +1357,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, @@ -1232,8 +1383,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, @@ -1280,8 +1434,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, @@ -1305,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{ @@ -1331,8 +1489,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, @@ -1356,8 +1517,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, @@ -1432,8 +1596,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, @@ -1462,8 +1629,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, @@ -1522,19 +1692,978 @@ func Test_ValidateClusterVariable(t *testing.T) { }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := ValidateClusterVariable(tt.clusterVariable, tt.clusterClassVariable, - field.NewPath("spec", "topology", "variables")) + // CEL + { + name: "Valid CEL expression: scalar: using self", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self <= 1", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`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, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self <= 1", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`99`), + }, + }, + }, + { + 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, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + 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", + 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", + }}, + }, + }, + }, + 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 (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: "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{ + 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) { + gotErrs := ValidateClusterVariable(ctx, tt.clusterVariable, nil, tt.clusterClassVariable, + field.NewPath("spec", "topology", "variables").Key(tt.clusterClassVariable.Name)) + + checkErrors(t, tt.wantErrs, gotErrs) + }) + } +} + +func Test_ValidateClusterVariable_CELTransitions(t *testing.T) { + tests := []struct { + name string + clusterClassVariable *clusterv1.ClusterClassVariable + clusterVariable *clusterv1.ClusterVariable + oldClusterVariable *clusterv1.ClusterVariable + wantErrs []validationMatch + }{ + { + name: "Valid transition if old value is not set", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + { + name: "Valid transition if old value is less than new value via CEL expression", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + oldClusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + { + 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, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + oldClusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + { + 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, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + Message: "new value must be greater than old value", + }}, + }, + }, + }, + clusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + oldClusterVariable: &clusterv1.ClusterVariable{ + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + { + 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) { + gotErrs := ValidateClusterVariable(ctx, tt.clusterVariable, tt.oldClusterVariable, tt.clusterClassVariable, + field.NewPath("spec", "topology", "variables").Key(tt.clusterVariable.Name)) + + checkErrors(t, tt.wantErrs, gotErrs) + }) + } +} + +func Test_ValidateMachineVariables(t *testing.T) { + tests := []struct { + name string + definitions []clusterv1.ClusterClassStatusVariable + values []clusterv1.ClusterVariable + 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{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: ptr.To[int64](1), + }, + }, + }, + }, + }, + { + Name: "zone", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + MinLength: ptr.To[int64](1), + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + // cpu is missing in the values but is required in definition. + { + Name: "zone", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"longerThanOneCharacter"`), + }, + }, + }, + }, + { + 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. + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east-1"`), + }, + }, + }, + }, + { + 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", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + // This definition does not exist. + DefinitionFrom: "non-existent-patch", + }, + }, + }, + { + 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", + DefinitionsConflict: true, + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, + { + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), // not a string + }, + DefinitionFrom: "inline", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"one"`), // not an integer + }, + DefinitionFrom: "somepatch", + }, + }, + }, + // CEL + { + name: "Valid integer with CEL expression", + definitions: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self <= 1", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + }, + { + 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", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self <= 1", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`99`), + }, + }, + }, + }, + { + 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", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self >= 1", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + }, + { + name: "Valid transition if old value is not set", + definitions: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + }, + { + name: "Valid transition if old value is less than new value via CEL expression", + definitions: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + oldValues: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + }, + { + 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", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > oldSelf", + }}, + }, + }, + }, + }, + }, + }, + values: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`0`), + }, + }, + }, + oldValues: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + 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 d498441e3afb..806167855a34 100644 --- a/internal/topology/variables/clusterclass_variable_validation.go +++ b/internal/topology/variables/clusterclass_variable_validation.go @@ -19,16 +19,24 @@ package variables import ( "context" "fmt" + "math" + "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" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" structuraldefaulting "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting" "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" + celconfig "k8s.io/apiserver/pkg/apis/cel" + apiservercel "k8s.io/apiserver/pkg/cel" + "k8s.io/apiserver/pkg/cel/environment" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) @@ -36,31 +44,51 @@ import ( const ( // builtinsName is the name of the builtin variable. builtinsName = "builtin" + + // 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 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) + + 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), ), @@ -73,7 +101,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. @@ -83,7 +111,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 } @@ -122,13 +150,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. @@ -144,35 +189,93 @@ 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(), true), } + if oldAPIExtensionsSchema != nil { + opts.preexistingExpressions = findPreexistingExpressions(oldAPIExtensionsSchema) + } + + 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 & messageExpression cost total exceeding cost limit for entire OpenAPIv3 schema" + allErrs = append(allErrs, field.Forbidden(expensive.Path, costErrorMsg)) + } - allErrs = append(allErrs, validateSchema(apiExtensionsSchema, fldPath)...) + 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)) + } + } + + // 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 } -func validateSchema(schema *apiextensions.JSONSchemaProps, fldPath *field.Path) field.ErrorList { +var supportedValidationReason = sets.NewString( + string(clusterv1.FieldValueRequired), + string(clusterv1.FieldValueForbidden), + string(clusterv1.FieldValueInvalid), + string(clusterv1.FieldValueDuplicate), +) + +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. @@ -186,38 +289,254 @@ 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 mutual exclusive")) + allErrs = append(allErrs, field.Forbidden(fldPath, "additionalProperties and properties are mutually exclusive")) } - allErrs = append(allErrs, validateSchema(schema.AdditionalProperties.Schema, fldPath.Child("additionalProperties"))...) + 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))...) + allErrs = append(allErrs, validateSchema(ctx, &p, fldPath.Child("properties").Key(propertyName), opts, celContext.ChildPropertyContext(&p, propertyName), uncorrelatablePath)...) } if schema.Items != nil { - allErrs = append(allErrs, validateSchema(schema.Items.Schema, fldPath.Child("items"))...) + // 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(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. + 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 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))) + } + } + } + } + } + } + + // Return if we found some errors in the CEL validations. + if len(allErrs) > 0 { + return allErrs + } + + // 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 schema.Example != nil { + errs, _ := celValidator.Validate(ctx, fldPath.Child("example"), ss, *schema.Example, nil, celconfig.RuntimeCELCostBudget) + allErrs = append(allErrs, errs...) + } + 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...) + } + } } return allErrs } + +// 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. + +// validationOptions groups several validation options, to avoid passing multiple bool parameters to methods. +type validationOptions struct { + preexistingExpressions preexistingExpressions + + celEnvironmentSet *environment.EnvSet +} + +type preexistingExpressions struct { + rules sets.Set[string] + messageExpressions sets.Set[string] +} + +func (pe preexistingExpressions) RuleEnv(envSet *environment.EnvSet, expression string) *celgo.Env { + if pe.rules.Has(expression) { + return envSet.StoredExpressionsEnv() + } + return envSet.NewExpressionsEnv() +} + +func (pe preexistingExpressions) MessageExpressionEnv(envSet *environment.EnvSet, expression string) *celgo.Env { + if pe.messageExpressions.Has(expression) { + return envSet.StoredExpressionsEnv() + } + 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 + }) +} + +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 +// would exceed math.MaxUint, in which case math.MaxUint is returned. +func multiplyWithOverflowGuard(baseCost, cardinality uint64) uint64 { + if baseCost == 0 { + // an empty rule can return 0, so guard for that here + return 0 + } else if math.MaxUint/baseCost < cardinality { + return math.MaxUint + } + return baseCost * cardinality +} + +// unbounded uses nil to represent an unbounded cardinality value. +var unbounded *uint64 = nil //nolint:revive // Using as a named variable to provide the meaning of nil in this context. + +func getExpressionCost(cr cel.CompilationResult, cardinalityCost *apiextensionsvalidation.CELSchemaContext) uint64 { + if cardinalityCost.MaxCardinality != unbounded { + return multiplyWithOverflowGuard(cr.MaxCost, *cardinalityCost.MaxCardinality) + } + return multiplyWithOverflowGuard(cr.MaxCost, cr.MaxCardinality) +} + +func getCostErrorMessage(costName string, expressionCost, costLimit uint64) string { + exceedFactor := float64(expressionCost) / float64(costLimit) + var factor string + if exceedFactor > 100.0 { + // if exceedFactor is greater than 2 orders of magnitude, the rule is likely O(n^2) or worse + // and will probably never validate without some set limits + // also in such cases the cost estimation is generally large enough to not add any value + factor = "more than 100x" + } else if exceedFactor < 1.5 { + factor = fmt.Sprintf("%fx", exceedFactor) // avoid reporting "exceeds budge by a factor of 1.0x" + } else { + factor = fmt.Sprintf("%.1fx", exceedFactor) + } + return fmt.Sprintf("%s exceeds budget by factor of %s (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)", costName, factor) +} diff --git a/internal/topology/variables/clusterclass_variable_validation_test.go b/internal/topology/variables/clusterclass_variable_validation_test.go index 0ab6b6c8c677..3fcc9347068e 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 ( - "context" + "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,10 +30,12 @@ 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 }{ + // Basics { name: "Error if multiple variables share a name", clusterClassVariables: []clusterv1.ClusterClassVariable{ @@ -56,7 +58,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 +105,246 @@ 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", + }}, + }, + }, + }, + }, + }, + // 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", + 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: "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(context.TODO(), + 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 +353,9 @@ func Test_ValidateClusterClassVariable(t *testing.T) { tests := []struct { name string clusterClassVariable *clusterv1.ClusterClassVariable - wantErr bool + wantErrs []validationMatch }{ + // Scalars { name: "Valid integer schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -149,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{ @@ -172,7 +392,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", @@ -185,7 +408,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 (.)", @@ -198,8 +424,12 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + invalid("\"path.tovariable\": variable name cannot contain \".\"", + "spec.variables[path.tovariable].name"), + }, }, + // Metadata { name: "Valid variable metadata", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -223,7 +453,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", @@ -236,12 +466,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", @@ -254,8 +487,12 @@ 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"), + }, }, + // Defaults { name: "Valid default value regular string", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -278,15 +515,41 @@ 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"), + }, }, + // Examples { name: "Valid example value regular string", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -309,15 +572,41 @@ 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"), + }, }, + // Enums { name: "Valid enum values", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -341,15 +630,42 @@ 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]"), + }, }, + // Variable types { name: "fail on variable type is null", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -360,7 +676,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", @@ -372,7 +691,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", @@ -384,8 +706,68 @@ 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"), + }, }, + // Objects { name: "Valid object schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -433,8 +815,12 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"invalidType\"", + "spec.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type"), + }, }, + // Maps { name: "Valid map schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -488,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", @@ -516,8 +905,12 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, - wantErr: true, + wantErrs: []validationMatch{ + forbidden("Forbidden: additionalProperties and properties are mutually exclusive", + "spec.variables[httpProxy].schema.openAPIV3Schema"), + }, }, + // Arrays { name: "Valid array schema", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -548,8 +941,12 @@ 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"), + }, }, + // Required { name: "pass on variable with required set true with a default defined", clusterClassVariable: &clusterv1.ClusterClassVariable{ @@ -563,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{ @@ -588,7 +986,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", @@ -635,7 +1056,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", @@ -661,7 +1085,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", @@ -720,7 +1147,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", @@ -750,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{ @@ -776,7 +1206,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", @@ -823,7 +1256,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", @@ -849,10 +1285,13 @@ 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", + 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{ @@ -876,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{ @@ -907,7 +1347,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", @@ -960,7 +1403,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", @@ -991,7 +1437,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", @@ -1023,20 +1472,931 @@ func Test_ValidateClusterClassVariable(t *testing.T) { }, }, }, + // CEL + { + name: "pass if x-kubernetes-validations has valid rule", + clusterClassVariable: &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: invalid CEL expression", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "cpu", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + XValidations: []clusterv1.ValidationRule{{ + Rule: "this does not compile", + }}, + }, + }, + }, + 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"), + }, + }, + // CEL: uncorrelatable paths (in arrays) + { + 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: "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"), + }, + }, + // CEL: cost + { + 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": { + // 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](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) + Type: "integer", + XValidations: []clusterv1.ValidationRule{{Rule: "self > 50 && self < 100"}}, + }, + }, + }, + }, + }, + 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 rule & messageExpression cost total exceeding cost limit", + "spec.variables[cpu].schema.openAPIV3Schema.properties[array].items.x-kubernetes-validations[0].rule"), + 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 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{ + 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"), + }, + }, + // CEL: validation of defaults + { + 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", + }}, + }, + }, + }, + }, + // CEL: validation of examples + { + 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"), + }, + }, + // CEL: validation of enums + { + 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]"), + }, + }, + // CEL: reason + { + name: "fail if x-kubernetes-validations has invalid reason", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{ + { + Rule: "self.a > 0", + Reason: clusterv1.FieldValueErrorReason("InternalError"), + FieldPath: ".a", + }, + }, + Properties: map[string]clusterv1.JSONSchemaProps{ + "a": { + Type: "number", + XValidations: []clusterv1.ValidationRule{ + { + Rule: "true", + Reason: clusterv1.FieldValueRequired, + }, + { + Rule: "true", + Reason: clusterv1.FieldValueInvalid, + }, + { + Rule: "true", + Reason: clusterv1.FieldValueDuplicate, + }, + { + Rule: "true", + Reason: clusterv1.FieldValueForbidden, + }, + }, + }, + }, + }, + }, + }, + wantErrs: []validationMatch{ + unsupported("Unsupported value: \"InternalError\"", + "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{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{ + // Valid + { + Rule: "true", + FieldPath: ".foo['b.c']['c\\a']", + }, + { + 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]", // referring to an item of a list with a numeric index is not supported + }, + { + Rule: "true", + FieldPath: " ", + }, + { + Rule: "true", + FieldPath: ".", + }, + { + Rule: "true", + FieldPath: "..", + }, + }, + Properties: map[string]clusterv1.JSONSchemaProps{ + "a.c": { + Type: "number", + }, + "special.character.34$": { + Type: "number", + }, + "foo": { + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "b.c": { + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "c\a": { + Type: "number", + }, + }, + }, + }, + }, + "list": { + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "a": { + Type: "number", + }, + }, + }, + }, + }, + }, + }, + }, + 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"), + }, + }, + { + name: "fail if x-kubernetes-validations has invalid fieldPath", + clusterClassVariable: &clusterv1.ClusterClassVariable{ + Name: "var", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + XValidations: []clusterv1.ValidationRule{ + { + Rule: "self.a.b.c > 0.0", + FieldPath: ".list[0].b", + }, + { + Rule: "self.a.b.c > 0.0", + FieldPath: ".list[0.b", + }, + { + Rule: "self.a.b.c > 0.0", + FieldPath: ".list0].b", + }, + { + Rule: "self.a.b.c > 0.0", + FieldPath: ".a.c", + }, + { + 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": { + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "b": { + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "c": { + Type: "number", + }, + }, + }, + }, + }, + "list": { + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "a": { + Type: "number", + }, + }, + }, + }, + }, + }, + }, + }, + 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"), + }, + }, + // CEL: message + { + 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", + 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{ + "a": { + Type: "number", + }, + }, + }, + }, + }, + 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"), + }, + }, + // CEL: messageExpression + { + 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 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 rule & messageExpression 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(context.TODO(), + 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()) - return - } - g.Expect(errList).To(BeEmpty()) + checkErrors(t, tt.wantErrs, gotErrs) }) } } + +func checkErrors(t *testing.T, wantErrs []validationMatch, gotErrs field.ErrorList) { + t.Helper() + + var foundMismatch bool + if len(gotErrs) != len(wantErrs) { + foundMismatch = true + } else { + for i := range wantErrs { + if !wantErrs[i].matches(gotErrs[i]) { + foundMismatch = true + break + } + } + } + + 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 { + Field *field.Path + Type field.ErrorType + containsString string +} + +func (v validationMatch) matches(err *field.Error) bool { + 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(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(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(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 ffff13091835..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, @@ -163,5 +162,33 @@ func convertToAPIExtensionsJSONSchemaProps(schema *clusterv1.JSONSchemaProps, fl } } + if schema.XValidations != nil { + props.XValidations = convertToAPIExtensionsXValidations(schema.XValidations) + } + return props, allErrs } + +func convertToAPIExtensionsXValidations(validationRules []clusterv1.ValidationRule) apiextensions.ValidationRules { + apiExtValidationRules := make(apiextensions.ValidationRules, 0, len(validationRules)) + + for _, validationRule := range validationRules { + var reason *apiextensions.FieldValueErrorReason + if validationRule.Reason != "" { + reason = ptr.To(apiextensions.FieldValueErrorReason(validationRule.Reason)) + } + + apiExtValidationRules = append( + apiExtValidationRules, + apiextensions.ValidationRule{ + Rule: validationRule.Rule, + Message: validationRule.Message, + MessageExpression: validationRule.MessageExpression, + Reason: reason, + FieldPath: validationRule.FieldPath, + }, + ) + } + + return apiExtValidationRules +} diff --git a/internal/topology/variables/schema_test.go b/internal/topology/variables/schema_test.go index 11b625690bc9..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), }, }, }, @@ -186,6 +186,53 @@ func Test_convertToAPIExtensionsJSONSchemaProps(t *testing.T) { }, }, }, + { + name: "pass for schema validation with CEL validation rules", + schema: &clusterv1.JSONSchemaProps{ + Items: &clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: ptr.To[int64](1), + Format: "uri", + MinLength: ptr.To[int64](2), + MaxLength: ptr.To[int64](4), + XValidations: []clusterv1.ValidationRule{{ + Rule: "self > 0", + Message: "value must be greater than 0", + MessageExpression: "value must be greater than 0", + FieldPath: "a.field.path", + }, { + Rule: "self > 0", + Message: "value must be greater than 0", + MessageExpression: "value must be greater than 0", + FieldPath: "a.field.path", + Reason: clusterv1.FieldValueErrorReason("a reason"), + }}, + }, + }, + want: &apiextensions.JSONSchemaProps{ + Items: &apiextensions.JSONSchemaPropsOrArray{ + Schema: &apiextensions.JSONSchemaProps{ + Type: "integer", + Minimum: ptr.To[float64](1), + Format: "uri", + MinLength: ptr.To[int64](2), + MaxLength: ptr.To[int64](4), + XValidations: apiextensions.ValidationRules{{ + Rule: "self > 0", + Message: "value must be greater than 0", + MessageExpression: "value must be greater than 0", + FieldPath: "a.field.path", + }, { + Rule: "self > 0", + Message: "value must be greater than 0", + MessageExpression: "value must be greater than 0", + FieldPath: "a.field.path", + Reason: ptr.To(apiextensions.FieldValueErrorReason("a reason")), + }}, + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { 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.go b/internal/webhooks/cluster.go index 0f066af8c68b..3b9c49be20d5 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -51,6 +51,10 @@ import ( // SetupWebhookWithManager sets up Cluster webhooks. func (webhook *Cluster) SetupWebhookWithManager(mgr ctrl.Manager) error { + if webhook.decoder == nil { + webhook.decoder = admission.NewDecoder(mgr.GetScheme()) + } + return ctrl.NewWebhookManagedBy(mgr). For(&clusterv1.Cluster{}). WithDefaulter(webhook). @@ -70,6 +74,8 @@ type ClusterCacheTrackerReader interface { type Cluster struct { Client client.Reader Tracker ClusterCacheTrackerReader + + decoder admission.Decoder } var _ webhook.CustomDefaulter = &Cluster{} @@ -122,10 +128,21 @@ func (webhook *Cluster) Default(ctx context.Context, obj runtime.Object) error { return apierrors.NewInternalError(errors.Wrapf(err, "Cluster %s can't be defaulted. ClusterClass %s can not be retrieved", cluster.Name, cluster.GetClassKey().Name)) } + // Validate cluster class variables transitions that may be enforced by CEL validation rules on variables. + // If no request found in context, then this has not come via a webhook request, so skip validation of old cluster. + var oldCluster *clusterv1.Cluster + req, err := admission.RequestFromContext(ctx) + + if err == nil && len(req.OldObject.Raw) > 0 { + oldCluster = &clusterv1.Cluster{} + if err := webhook.decoder.DecodeRaw(req.OldObject, oldCluster); err != nil { + return apierrors.NewBadRequest(errors.Wrap(err, "failed to decode old cluster object").Error()) + } + } + // Doing both defaulting and validating here prevents a race condition where the ClusterClass could be // different in the defaulting and validating webhook. - allErrs = append(allErrs, DefaultAndValidateVariables(cluster, clusterClass)...) - + allErrs = append(allErrs, DefaultAndValidateVariables(ctx, cluster, oldCluster, clusterClass)...) if len(allErrs) > 0 { return apierrors.NewInvalid(clusterv1.GroupVersion.WithKind("Cluster").GroupKind(), cluster.Name, allErrs) } @@ -718,21 +735,58 @@ func validateCIDRBlocks(fldPath *field.Path, cidrs []string) field.ErrorList { // DefaultAndValidateVariables defaults and validates variables in the Cluster and MachineDeployment/MachinePool topologies based // on the definitions in the ClusterClass. -func DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { +func DefaultAndValidateVariables(ctx context.Context, cluster, oldCluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList allErrs = append(allErrs, DefaultVariables(cluster, clusterClass)...) + // Capture variables from old cluster if it is present to be used in validation for transitions that may be specified + // via CEL validation rules. + var ( + oldClusterVariables, oldCPOverrides []clusterv1.ClusterVariable + oldMDVariables map[string][]clusterv1.ClusterVariable + oldMPVariables map[string][]clusterv1.ClusterVariable + ) + if oldCluster != nil { + oldClusterVariables = oldCluster.Spec.Topology.Variables + if oldCluster.Spec.Topology.ControlPlane.Variables != nil { + oldCPOverrides = oldCluster.Spec.Topology.ControlPlane.Variables.Overrides + } + + oldMDVariables = make(map[string][]clusterv1.ClusterVariable, len(oldCluster.Spec.Topology.Workers.MachineDeployments)) + for _, md := range oldCluster.Spec.Topology.Workers.MachineDeployments { + if md.Variables != nil { + oldMDVariables[md.Name] = md.Variables.Overrides + } + } + + oldMPVariables = make(map[string][]clusterv1.ClusterVariable, len(oldCluster.Spec.Topology.Workers.MachinePools)) + for _, mp := range oldCluster.Spec.Topology.Workers.MachinePools { + if mp.Variables != nil { + oldMPVariables[mp.Name] = mp.Variables.Overrides + } + } + } + // Variables must be validated in the defaulting webhook. Variable definitions are stored in the ClusterClass status // and are patched in the ClusterClass reconcile. // Validate cluster-wide variables. - allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables, + allErrs = append(allErrs, variables.ValidateClusterVariables( + ctx, + cluster.Spec.Topology.Variables, + oldClusterVariables, + clusterClass.Status.Variables, field.NewPath("spec", "topology", "variables"))...) // Validate ControlPlane variable overrides. if cluster.Spec.Topology.ControlPlane.Variables != nil && len(cluster.Spec.Topology.ControlPlane.Variables.Overrides) > 0 { - allErrs = append(allErrs, variables.ValidateControlPlaneVariables(cluster.Spec.Topology.ControlPlane.Variables.Overrides, clusterClass.Status.Variables, - field.NewPath("spec", "topology", "controlPlane", "variables", "overrides"))...) + allErrs = append(allErrs, variables.ValidateControlPlaneVariables( + ctx, + cluster.Spec.Topology.ControlPlane.Variables.Overrides, + oldCPOverrides, + clusterClass.Status.Variables, + field.NewPath("spec", "topology", "controlPlane", "variables", "overrides"))..., + ) } if cluster.Spec.Topology.Workers != nil { @@ -742,8 +796,13 @@ func DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clust if md.Variables == nil || len(md.Variables.Overrides) == 0 { continue } - allErrs = append(allErrs, variables.ValidateMachineVariables(md.Variables.Overrides, clusterClass.Status.Variables, - field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("variables", "overrides"))...) + allErrs = append(allErrs, variables.ValidateMachineVariables( + ctx, + md.Variables.Overrides, + oldMDVariables[md.Name], + clusterClass.Status.Variables, + field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("variables", "overrides"))..., + ) } // Validate MachinePool variable overrides. @@ -752,8 +811,13 @@ func DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clust if mp.Variables == nil || len(mp.Variables.Overrides) == 0 { continue } - allErrs = append(allErrs, variables.ValidateMachineVariables(mp.Variables.Overrides, clusterClass.Status.Variables, - field.NewPath("spec", "topology", "workers", "machinePools").Index(i).Child("variables", "overrides"))...) + allErrs = append(allErrs, variables.ValidateMachineVariables( + ctx, + mp.Variables.Overrides, + oldMPVariables[mp.Name], + clusterClass.Status.Variables, + field.NewPath("spec", "topology", "workers", "machinePools").Index(i).Child("variables", "overrides"))..., + ) } } return allErrs diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index ede22d3f79c8..3d58c29cc411 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,268 @@ 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"). + 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 +1215,42 @@ 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) + + // Add old cluster to request if oldTopology is set. + webhookCtx := ctx + 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..b38f40784e1b 100644 --- a/internal/webhooks/clusterclass_test.go +++ b/internal/webhooks/clusterclass_test.go @@ -1004,10 +1004,7 @@ func TestClusterClassValidation(t *testing.T) { expectErr: false, }, - /* - UPDATE Tests - */ - + // update tests { name: "update pass in case of no changes", old: builder.ClusterClass(metav1.NamespaceDefault, "class1"). @@ -1769,6 +1766,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])", + }, + }, }, }, }, diff --git a/test/extension/handlers/topologymutation/handler_integration_test.go b/test/extension/handlers/topologymutation/handler_integration_test.go index cd5326a5edc9..73f1553bf3e3 100644 --- a/test/extension/handlers/topologymutation/handler_integration_test.go +++ b/test/extension/handlers/topologymutation/handler_integration_test.go @@ -117,7 +117,7 @@ func TestHandler(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) // Run variable defaulting and validation on the Cluster object. - errs := clusterWebhook.DefaultAndValidateVariables(s.Current.Cluster, s.Blueprint.ClusterClass) + errs := clusterWebhook.DefaultAndValidateVariables(ctx, s.Current.Cluster, nil, s.Blueprint.ClusterClass) g.Expect(errs.ToAggregate()).ToNot(HaveOccurred()) // Return the desired state. diff --git a/webhooks/alias.go b/webhooks/alias.go index 1d1271bb1530..1b71158d62d7 100644 --- a/webhooks/alias.go +++ b/webhooks/alias.go @@ -17,6 +17,8 @@ limitations under the License. package webhooks import ( + "context" + "k8s.io/apimachinery/pkg/util/validation/field" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -50,10 +52,10 @@ func (webhook *Cluster) SetupWebhookWithManager(mgr ctrl.Manager) error { // This method can be used when testing the behavior of the desired state computation of // the Cluster topology controller (because variables are always defaulted and validated // before the desired state is computed). -func (webhook *Cluster) DefaultAndValidateVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { +func (webhook *Cluster) DefaultAndValidateVariables(ctx context.Context, cluster, oldCluster *clusterv1.Cluster, clusterClass *clusterv1.ClusterClass) field.ErrorList { // As of today this func is not a method on internal/webhooks.Cluster because it doesn't use // any of its fields. But it seems more consistent and future-proof to expose it as a method. - return webhooks.DefaultAndValidateVariables(cluster, clusterClass) + return webhooks.DefaultAndValidateVariables(ctx, cluster, oldCluster, clusterClass) } // ClusterClass implements a validation and defaulting webhook for ClusterClass.