From 2f79163bcc9c1f2c1e4ac1af33217e6bb4bf2332 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Tue, 14 Feb 2023 12:52:43 +0000 Subject: [PATCH] Add handling for variables with conflicting definitions --- .../clusterclass/clusterclass_controller.go | 48 +- .../cluster/cluster_controller_test.go | 5 +- .../variables/cluster_variable_defaulting.go | 185 +++- .../cluster_variable_defaulting_test.go | 759 +++++++++----- .../variables/cluster_variable_validation.go | 219 ++-- .../cluster_variable_validation_test.go | 958 ++++++++++++++---- internal/webhooks/cluster.go | 32 +- internal/webhooks/cluster_test.go | 243 ++++- 8 files changed, 1816 insertions(+), 633 deletions(-) diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index 4d21e56ef177..922d8835f8f1 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/internal/controllers/clusterclass/clusterclass_controller.go @@ -19,6 +19,7 @@ package clusterclass import ( "context" "fmt" + "reflect" "strings" "github.com/pkg/errors" @@ -213,12 +214,10 @@ func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterCla func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error { errs := []error{} - uniqueVars := map[string]bool{} - allVars := []clusterv1.ClusterClassStatusVariable{} + allVars := map[string]*clusterv1.ClusterClassStatusVariable{} // Add inline variable definitions to the ClusterClass status. for _, variable := range clusterClass.Spec.Variables { - uniqueVars[variable.Name] = true - allVars = append(allVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline)) + allVars[variable.Name] = addNewStatusVariable(variable, clusterv1.VariableDefinitionFromInline) } // If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables @@ -243,13 +242,12 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust } if resp.Variables != nil { for _, variable := range resp.Variables { - // TODO: Variables should be validated and deduplicated based on their provided definition. - if _, ok := uniqueVars[variable.Name]; ok { - errs = append(errs, errors.Errorf("duplicate variable name %s discovered in patch: %s", variable.Name, patch.Name)) + // If a variable of the same name already exists add it to the existing definition list. + if _, ok := allVars[variable.Name]; ok { + allVars[variable.Name] = addDefinitionToExistingStatusVariable(variable, patch.Name, allVars[variable.Name]) continue } - uniqueVars[variable.Name] = true - allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name)) + allVars[variable.Name] = addNewStatusVariable(variable, patch.Name) } } } @@ -260,10 +258,15 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust "VariableDiscovery failed: %s", kerrors.NewAggregate(errs)) return errors.Wrapf(kerrors.NewAggregate(errs), "failed to discover variables for ClusterClass %s", clusterClass.Name) } - clusterClass.Status.Variables = allVars + statusVarList := []clusterv1.ClusterClassStatusVariable{} + for _, variable := range allVars { + statusVarList = append(statusVarList, *variable) + } + clusterClass.Status.Variables = statusVarList conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) return nil } + func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) { if len(outdatedRefs) > 0 { var msg []string @@ -288,10 +291,9 @@ func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[ ) } -func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVariable, from string) clusterv1.ClusterClassStatusVariable { - return clusterv1.ClusterClassStatusVariable{ - Name: variable.Name, - // TODO: In a future iteration this should be true where definitions are equal. +func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) *clusterv1.ClusterClassStatusVariable { + return &clusterv1.ClusterClassStatusVariable{ + Name: variable.Name, DefinitionsConflict: false, Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { @@ -302,6 +304,24 @@ func statusVariableFromClusterClassVariable(variable clusterv1.ClusterClassVaria }} } +func addDefinitionToExistingStatusVariable(variable clusterv1.ClusterClassVariable, from string, existingVariable *clusterv1.ClusterClassStatusVariable) *clusterv1.ClusterClassStatusVariable { + combinedVariable := existingVariable.DeepCopy() + newVariableDefinition := clusterv1.ClusterClassStatusVariableDefinition{ + From: from, + Required: variable.Required, + Schema: variable.Schema, + } + combinedVariable.Definitions = append(existingVariable.Definitions, newVariableDefinition) + + // If the new definition is different from any existing definition, set DefinitionsConflict to true. + // If definitions already conflict, no need to check. + if !combinedVariable.DefinitionsConflict && + !reflect.DeepEqual(combinedVariable.Definitions[0], newVariableDefinition) { + combinedVariable.DefinitionsConflict = true + } + return combinedVariable +} + func refString(ref *corev1.ObjectReference) string { return fmt.Sprintf("%s %s/%s", ref.GroupVersionKind().String(), ref.Namespace, ref.Name) } diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index d245f6e77dac..646725d5affe 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -1114,7 +1115,7 @@ func TestReconciler_DefaultCluster(t *testing.T) { Build(), wantCluster: clusterBuilder.DeepCopy(). WithTopology(topologyBase.DeepCopy().WithVariables( - clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}}). + clusterv1.ClusterVariable{Name: "location", Value: apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, DefinitionFrom: ""}). Build()). Build(), }, @@ -1234,7 +1235,7 @@ func TestReconciler_DefaultCluster(t *testing.T) { got := &clusterv1.Cluster{} g.Expect(fakeClient.Get(ctx, client.ObjectKey{Name: tt.initialCluster.Name, Namespace: tt.initialCluster.Namespace}, got)).To(Succeed()) // Compare the spec of the two clusters to ensure that variables are defaulted correctly. - g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue()) + g.Expect(reflect.DeepEqual(got.Spec, tt.wantCluster.Spec)).To(BeTrue(), cmp.Diff(got.Spec, tt.wantCluster.Spec)) }) } } diff --git a/internal/topology/variables/cluster_variable_defaulting.go b/internal/topology/variables/cluster_variable_defaulting.go index 18357ca9f162..872b8e1f1a53 100644 --- a/internal/topology/variables/cluster_variable_defaulting.go +++ b/internal/topology/variables/cluster_variable_defaulting.go @@ -30,54 +30,50 @@ import ( ) // DefaultClusterVariables defaults ClusterVariables. -func DefaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { +func DefaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { return defaultClusterVariables(clusterVariables, clusterClassVariables, true, fldPath) } // DefaultMachineDeploymentVariables defaults MachineDeploymentVariables. -func DefaultMachineDeploymentVariables(machineDeploymentVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { +func DefaultMachineDeploymentVariables(machineDeploymentVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { return defaultClusterVariables(machineDeploymentVariables, clusterClassVariables, false, fldPath) } // defaultClusterVariables defaults variables. // If they do not exist yet, they are created if createVariables is set. -func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, createVariables bool, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { +func defaultClusterVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, createVariables bool, fldPath *field.Path) ([]clusterv1.ClusterVariable, field.ErrorList) { var allErrs field.ErrorList - // Build maps for easier and faster access. - clusterVariablesMap := getClusterVariablesMap(clusterVariables) - clusterClassVariablesMap := getClusterClassVariablesMap(clusterClassVariables) - - // Validate that all variables in the Cluster are defined in the ClusterClass. - // Note: If we don't validate this, we would get a nil pointer dereference below. - allErrs = append(allErrs, validateClusterVariablesDefined(clusterVariables, clusterClassVariablesMap, fldPath)...) - if len(allErrs) > 0 { - return nil, allErrs + // Get a map of ClusterVariables and ensure that variables are not defined more than once in Cluster spec. + valuesMap, err := getValuesMap(values) + if err != nil { + return nil, append(allErrs, field.Invalid(fldPath, values, + fmt.Sprintf("cluster variables not valid: %s", err))) } - // allVariables is used to get a full correctly ordered list of variables. - allVariables := []string{} - // Add any ClusterVariables that already exist. - for _, variable := range clusterVariables { - allVariables = append(allVariables, variable.Name) - } - // Add variables from the ClusterClass, which currently don't exist on the Cluster. - for _, variable := range clusterClassVariables { - // Continue if the ClusterClass variable already exists. - if _, ok := clusterVariablesMap[variable.Name]; ok { + // Get an index for each variable name and definition. + defIndex := newDefinitionsIndex(definitions) + + // Get a correctly ordered list of all variables defined in both the Cluster and the ClusterClass. + allVariables := getAllVariables(values, valuesMap, definitions) + + // Default all variables. + defaultedValues := []clusterv1.ClusterVariable{} + uniqueDefaults := map[string]bool{} + for i, variable := range allVariables { + // 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.Index(i).Child("name"), err.Error())) continue } - allVariables = append(allVariables, variable.Name) - } + // Get the current value of the variable if it is defined in the Cluster spec. + currentValue := getCurrentValue(variable, valuesMap) - // Default all variables. - defaultedClusterVariables := []clusterv1.ClusterVariable{} - for i, variableName := range allVariables { - clusterClassVariable := clusterClassVariablesMap[variableName] - clusterVariable := clusterVariablesMap[variableName] + // Default the variable. + defaultedValue, errs := defaultValue(currentValue, definition, fldPath.Index(i), createVariables) - defaultedClusterVariable, errs := defaultClusterVariable(clusterVariable, clusterClassVariable, fldPath.Index(i), createVariables) if len(errs) > 0 { allErrs = append(allErrs, errs...) continue @@ -86,46 +82,97 @@ func defaultClusterVariables(clusterVariables []clusterv1.ClusterVariable, clust // Continue if there is no defaulted variable. // NOTE: This happens when the variable doesn't exist on the CLuster before and // there is no top-level default value. - if defaultedClusterVariable == nil { + if defaultedValue == nil { continue } - defaultedClusterVariables = append(defaultedClusterVariables, *defaultedClusterVariable) + // Set the definitionFrom field for the defaulted value. + defaultedValue = defaultDefinitionFrom(defaultedValue, definition, valuesMap) + + // Only append the variable if it has not already been defaulted. + defaultedValues = appendIfUnique(defaultedValue, defaultedValues, uniqueDefaults) } if len(allErrs) > 0 { return nil, allErrs } + return defaultedValues, nil +} + +func appendIfUnique(defaultedValue *clusterv1.ClusterVariable, defaultedValues []clusterv1.ClusterVariable, uniqueDefaults map[string]bool) []clusterv1.ClusterVariable { + if _, ok := uniqueDefaults[fmt.Sprintf("%s/%s", defaultedValue.DefinitionFrom, defaultedValue.Name)]; !ok { + uniqueDefaults[fmt.Sprintf("%s/%s", defaultedValue.DefinitionFrom, defaultedValue.Name)] = true + defaultedValues = append(defaultedValues, *defaultedValue) + } + return defaultedValues +} + +// getCurrentValue returns the value of a variable for its definitionFrom, or for an empty definitionFrom if it exists. +func getCurrentValue(variable clusterv1.ClusterVariable, valuesMap map[string]map[string]clusterv1.ClusterVariable) *clusterv1.ClusterVariable { + var currentValue *clusterv1.ClusterVariable + // If the value is set in the Cluster spec get the value. + if valuesWithName, ok := valuesMap[variable.Name]; ok { + if value, ok := valuesWithName[variable.DefinitionFrom]; ok { + currentValue = &value + } + if value, ok := valuesWithName[emptyVariableDefinitionFrom]; ok { + currentValue = &value + } + } + return currentValue +} + +// defaultDefinitionFrom sets the definitionFrom field of a variable based on the variable definition and values. +func defaultDefinitionFrom(defaultedValue *clusterv1.ClusterVariable, definition *statusVariableDefinition, values map[string]map[string]clusterv1.ClusterVariable) *clusterv1.ClusterVariable { + // If definitionFrom is non-empty don't make any changes. + if defaultedValue.DefinitionFrom != emptyVariableDefinitionFrom { + return defaultedValue + } + + // If there are conflicts set the definitionFrom field. + if definition.conflicts { + defaultedValue.DefinitionFrom = definition.From + return defaultedValue + } - return defaultedClusterVariables, nil + // If a value with the same name and a different non-empty definitionsFrom is in the Cluster, set the definitionFrom field. + if valuesForName, ok := values[definition.name]; ok { + for _, value := range valuesForName { + if value.DefinitionFrom != definition.From && value.DefinitionFrom != emptyVariableDefinitionFrom { + defaultedValue.DefinitionFrom = definition.From + return defaultedValue + } + } + } + return defaultedValue } -// defaultClusterVariable defaults a clusterVariable based on the default value in the clusterClassVariable. -func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) { - if clusterVariable == nil { +// defaultValue defaults a clusterVariable based on the default value in the clusterClassVariable. +func defaultValue(currentValue *clusterv1.ClusterVariable, definition *statusVariableDefinition, fldPath *field.Path, createVariable bool) (*clusterv1.ClusterVariable, field.ErrorList) { + if currentValue == nil { // Return if the variable does not exist yet and createVariable is false. if !createVariable { return nil, nil } // Return if the variable does not exist yet and there is no top-level default value. - if clusterClassVariable.Schema.OpenAPIV3Schema.Default == nil { + if definition.Schema.OpenAPIV3Schema.Default == nil { return nil, nil } } // Convert schema to Kubernetes APIExtensions schema. - apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(&clusterClassVariable.Schema.OpenAPIV3Schema, field.NewPath("schema")) + apiExtensionsSchema, errs := convertToAPIExtensionsJSONSchemaProps(&definition.Schema.OpenAPIV3Schema, field.NewPath("schema")) if len(errs) > 0 { return nil, field.ErrorList{field.Invalid(fldPath, "", - fmt.Sprintf("invalid schema in ClusterClass for variable %q: error to convert schema %v", clusterClassVariable.Name, errs))} + fmt.Sprintf("invalid schema in ClusterClass for variable %q: error to convert schema %v", definition.name, errs))} } var value interface{} // If the variable already exists, parse the current value. - if clusterVariable != nil && len(clusterVariable.Value.Raw) > 0 { - if err := json.Unmarshal(clusterVariable.Value.Raw, &value); err != nil { + if currentValue != nil && len(currentValue.Value.Raw) > 0 { + if err := json.Unmarshal(currentValue.Value.Raw, &value); err != nil { return nil, field.ErrorList{field.Invalid(fldPath, "", - fmt.Sprintf("failed to unmarshal variable value %q: %v", string(clusterVariable.Value.Raw), err))} + fmt.Sprintf("failed to unmarshal variable value %q: %v", string(currentValue.Value.Raw), err))} } } @@ -133,7 +180,7 @@ func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterC // so we wrap the schema and the variable in objects. // : wrappedVariable := map[string]interface{}{ - clusterClassVariable.Name: value, + definition.name: value, } // type: object // properties: @@ -141,7 +188,7 @@ func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterC wrappedSchema := &apiextensions.JSONSchemaProps{ Type: "object", Properties: map[string]apiextensions.JSONSchemaProps{ - clusterClassVariable.Name: *apiExtensionsSchema, + definition.name: *apiExtensionsSchema, }, } @@ -149,20 +196,58 @@ func defaultClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterC ss, err := structuralschema.NewStructural(wrappedSchema) if err != nil { return nil, field.ErrorList{field.Invalid(fldPath, "", - fmt.Sprintf("failed defaulting variable %q: %v", clusterVariable.Name, err))} + fmt.Sprintf("failed defaulting variable %q: %v", currentValue.Name, err))} } structuraldefaulting.Default(wrappedVariable, ss) // Marshal the defaulted value. - defaultedVariableValue, err := json.Marshal(wrappedVariable[clusterClassVariable.Name]) + defaultedVariableValue, err := json.Marshal(wrappedVariable[definition.name]) if err != nil { return nil, field.ErrorList{field.Invalid(fldPath, "", - fmt.Sprintf("failed to marshal default value of variable %q: %v", clusterClassVariable.Name, err))} + fmt.Sprintf("failed to marshal default value of variable %q: %v", definition.name, err))} } - return &clusterv1.ClusterVariable{ - Name: clusterClassVariable.Name, + v := &clusterv1.ClusterVariable{ + Name: definition.name, Value: apiextensionsv1.JSON{ Raw: defaultedVariableValue, }, - }, nil + } + + // If the currentValue exists set definitionFrom on the defaulted value + if currentValue != nil { + v.DefinitionFrom = currentValue.DefinitionFrom + } + + return v, nil +} + +// getAllVariables returns a correctly ordered list of all variables defined in the ClusterClass and the Cluster. +func getAllVariables(values []clusterv1.ClusterVariable, valuesMap map[string]map[string]clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable) []clusterv1.ClusterVariable { + // allVariables is used to get a full correctly ordered list of variables. + allVariables := []clusterv1.ClusterVariable{} + + // Add any values that already exist. + allVariables = append(allVariables, values...) + + // Add variables from the ClusterClass, which currently don't exist on the Cluster. + for _, variable := range definitions { + for _, definition := range variable.Definitions { + if _, ok := valuesMap[variable.Name]; ok { + // Continue if the Cluster variable with a definitionFrom this ClusterClass variable exists. + if _, ok := valuesMap[variable.Name][definition.From]; ok { + continue + } + // Continue if the Cluster variable with an empty definitionFrom exists. The user intention here is to + // use the default value from a ClusterClass variable with no conflicting variables. + if _, ok := valuesMap[variable.Name][emptyVariableDefinitionFrom]; ok { + continue + } + } + allVariables = append(allVariables, clusterv1.ClusterVariable{ + Name: variable.Name, + DefinitionFrom: definition.From, + }) + } + } + return allVariables } diff --git a/internal/topology/variables/cluster_variable_defaulting_test.go b/internal/topology/variables/cluster_variable_defaulting_test.go index f8d38c953c9c..dcf81fd8e9b6 100644 --- a/internal/topology/variables/cluster_variable_defaulting_test.go +++ b/internal/topology/variables/cluster_variable_defaulting_test.go @@ -29,7 +29,7 @@ import ( func Test_DefaultClusterVariables(t *testing.T) { tests := []struct { name string - clusterClassVariables []clusterv1.ClusterClassVariable + clusterClassVariables []clusterv1.ClusterClassStatusVariable clusterVariables []clusterv1.ClusterVariable createVariables bool want []clusterv1.ClusterVariable @@ -37,7 +37,7 @@ func Test_DefaultClusterVariables(t *testing.T) { }{ { name: "Return error if variable is not defined in ClusterClass", - clusterClassVariables: []clusterv1.ClusterClassVariable{}, + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{}, clusterVariables: []clusterv1.ClusterVariable{ { Name: "cpu", @@ -51,45 +51,69 @@ func Test_DefaultClusterVariables(t *testing.T) { }, { name: "Default one variable of each valid type", - clusterClassVariables: []clusterv1.ClusterClassVariable{ + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, }, }, }, { - Name: "location", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + Name: "location", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, }, }, }, { - Name: "count", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "number", - Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + Name: "count", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "number", + Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + }, + }, }, }, }, { - Name: "correct", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "boolean", - Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + Name: "correct", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "boolean", + Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + }, + }, }, }, }, @@ -125,14 +149,20 @@ func Test_DefaultClusterVariables(t *testing.T) { }, { name: "Don't default variable if variable creation is disabled", - clusterClassVariables: []clusterv1.ClusterClassVariable{ + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, }, }, }, @@ -143,24 +173,36 @@ func Test_DefaultClusterVariables(t *testing.T) { }, { name: "Don't default variables that are set", - clusterClassVariables: []clusterv1.ClusterClassVariable{ + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, }, }, }, { - Name: "correct", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "boolean", - Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + Name: "correct", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "boolean", + Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + }, + }, }, }, }, @@ -193,23 +235,34 @@ func Test_DefaultClusterVariables(t *testing.T) { }, { name: "Don't add variables that have no default schema", - clusterClassVariables: []clusterv1.ClusterClassVariable{ + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, }, }, }, { - Name: "correct", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "boolean", + Name: "correct", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "boolean", + }, + }, }, }, }, @@ -225,6 +278,160 @@ func Test_DefaultClusterVariables(t *testing.T) { }, }, }, + { + name: "Default multiple variables with the same name from multiple conflicting definitions", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + DefinitionsConflict: true, + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`2`)}, + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{}, + createVariables: true, + want: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: clusterv1.VariableDefinitionFromInline, + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: "somepatch", + }, + }, + }, + { + name: "Don't default if a non-conflicting variable is defined without definitionFrom", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + createVariables: true, + want: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + }, + { + name: "Default non-specified definitions if only some are specified with definitionFrom", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: clusterv1.VariableDefinitionFromInline, + }, + }, + createVariables: true, + want: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: clusterv1.VariableDefinitionFromInline, + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -247,20 +454,23 @@ func Test_DefaultClusterVariable(t *testing.T) { tests := []struct { name string clusterVariable *clusterv1.ClusterVariable - clusterClassVariable *clusterv1.ClusterClassVariable + clusterClassVariable *statusVariableDefinition + conflict bool createVariable bool want *clusterv1.ClusterVariable wantErr bool }{ { name: "Default new integer variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "cpu", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, }, }, }, @@ -274,13 +484,15 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default new integer variable if variable creation is disabled", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "cpu", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, }, }, }, @@ -289,13 +501,15 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing integer variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "cpu", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Default: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, }, }, }, @@ -315,13 +529,16 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new string variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "location", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "location", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, }, }, }, @@ -335,13 +552,16 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing string variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "location", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "location", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, }, }, }, @@ -361,13 +581,16 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new number variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "number", - Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "cpu", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "number", + Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + }, }, }, }, @@ -381,13 +604,15 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing number variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "number", - Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "cpu", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "number", + Default: &apiextensionsv1.JSON{Raw: []byte(`0.1`)}, + }, }, }, }, @@ -407,13 +632,15 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new boolean variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "correct", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "boolean", - Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "correct", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "boolean", + Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + }, }, }, }, @@ -427,13 +654,15 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing boolean variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "correct", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "boolean", - Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + clusterClassVariable: &statusVariableDefinition{ + name: "correct", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "boolean", + Default: &apiextensionsv1.JSON{Raw: []byte(`true`)}, + }, }, }, }, @@ -453,22 +682,24 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new object variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", - }, - "noProxy": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -484,23 +715,25 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default new object variable if there is no top-level default", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - Default: &apiextensionsv1.JSON{Raw: []byte(`false`)}, - }, - "url": { - Type: "string", - }, - "noProxy": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + Default: &apiextensionsv1.JSON{Raw: []byte(`false`)}, + }, + "url": { + Type: "string", + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -511,22 +744,24 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing object variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", - }, - "noProxy": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -548,23 +783,26 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default nested fields of existing object variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, - }, - "noProxy": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Default: &apiextensionsv1.JSON{Raw: []byte(`{"enabled": false}`)}, + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -586,25 +824,27 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new map variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - Default: &apiextensionsv1.JSON{Raw: []byte(`{"proxy":{"enabled":false}}`)}, - AdditionalProperties: &clusterv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, - }, - "noProxy": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "object", + Default: &apiextensionsv1.JSON{Raw: []byte(`{"proxy":{"enabled":false}}`)}, + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -621,24 +861,26 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default nested fields of existing map variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "httpProxy", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", - AdditionalProperties: &clusterv1.JSONSchemaProps{ + clusterClassVariable: &statusVariableDefinition{ + name: "httpProxy", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "object", - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", - Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, - }, - "noProxy": { - Type: "string", + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"https://example.com"`)}, + }, + "noProxy": { + Type: "string", + }, }, }, }, @@ -661,24 +903,26 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Default new array variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "testVariable", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "array", - Items: &clusterv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "testVariable", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + }, }, }, + Default: &apiextensionsv1.JSON{Raw: []byte(`[{"enabled":false,"url":"123"},{"enabled":false,"url":"456"}]`)}, }, - Default: &apiextensionsv1.JSON{Raw: []byte(`[{"enabled":false,"url":"123"},{"enabled":false,"url":"456"}]`)}, }, }, }, @@ -692,24 +936,27 @@ func Test_DefaultClusterVariable(t *testing.T) { }, { name: "Don't default existing array variable", - clusterClassVariable: &clusterv1.ClusterClassVariable{ - Name: "testVariable", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "array", - Items: &clusterv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]clusterv1.JSONSchemaProps{ - "enabled": { - Type: "boolean", - }, - "url": { - Type: "string", + clusterClassVariable: &statusVariableDefinition{ + name: "testVariable", + ClusterClassStatusVariableDefinition: &clusterv1.ClusterClassStatusVariableDefinition{ + + Required: true, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "array", + Items: &clusterv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]clusterv1.JSONSchemaProps{ + "enabled": { + Type: "boolean", + }, + "url": { + Type: "string", + }, }, }, + Default: &apiextensionsv1.JSON{Raw: []byte(`[{"enabled":false,"url":"123"},{"enabled":false,"url":"456"}]`)}, }, - Default: &apiextensionsv1.JSON{Raw: []byte(`[{"enabled":false,"url":"123"},{"enabled":false,"url":"456"}]`)}, }, }, }, @@ -732,7 +979,7 @@ func Test_DefaultClusterVariable(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - defaultedVariable, errList := defaultClusterVariable(tt.clusterVariable, tt.clusterClassVariable, + defaultedVariable, errList := defaultValue(tt.clusterVariable, tt.clusterClassVariable, field.NewPath("spec", "topology", "variables").Index(0), tt.createVariable) if tt.wantErr { diff --git a/internal/topology/variables/cluster_variable_validation.go b/internal/topology/variables/cluster_variable_validation.go index fc1f39ccf0ec..19f88283bd59 100644 --- a/internal/topology/variables/cluster_variable_validation.go +++ b/internal/topology/variables/cluster_variable_validation.go @@ -21,87 +21,114 @@ import ( "fmt" "strings" + "github.com/pkg/errors" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" structuralpruning "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning" "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation/field" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -// ValidateClusterVariables validates ClusterVariables. -func ValidateClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { - return validateClusterVariables(clusterVariables, clusterClassVariables, true, fldPath) +const ( + // emptyVariableDefinitionFrom is the definitionFrom value used when none is supplied by the user. + emptyVariableDefinitionFrom = "" +) + +type statusVariableDefinition struct { + *clusterv1.ClusterClassStatusVariableDefinition + name string + conflicts bool +} + +// 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) } // ValidateMachineDeploymentVariables validates ValidateMachineDeploymentVariables. -func ValidateMachineDeploymentVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { - return validateClusterVariables(clusterVariables, clusterClassVariables, false, fldPath) +func ValidateMachineDeploymentVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, fldPath *field.Path) field.ErrorList { + return validateClusterVariables(values, definitions, false, fldPath) } -// validateClusterVariables validates variables via the schemas in the corresponding clusterClassVariable. -func validateClusterVariables(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables []clusterv1.ClusterClassVariable, validateRequired bool, fldPath *field.Path) field.ErrorList { +// validateClusterVariables validates variable values according to the corresponding definition. +func validateClusterVariables(values []clusterv1.ClusterVariable, definitions []clusterv1.ClusterClassStatusVariable, validateRequired bool, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - // Build maps for easier and faster access. - clusterVariablesMap := getClusterVariablesMap(clusterVariables) - clusterClassVariablesMap := getClusterClassVariablesMap(clusterClassVariables) - - // Validate: - // * required variables from the ClusterClass exist on the Cluster. - // * all variables in the Cluster are defined in the ClusterClass. - if validateRequired { - allErrs = append(allErrs, validateRequiredClusterVariables(clusterVariablesMap, clusterClassVariablesMap, fldPath)...) - } - allErrs = append(allErrs, validateClusterVariablesDefined(clusterVariables, clusterClassVariablesMap, fldPath)...) - if len(allErrs) > 0 { - return allErrs + // Get a map of ClusterVariable values and ensure that variables are not defined more than once in Cluster spec. + valuesMap, err := getValuesMap(values) + if err != nil { + return append(allErrs, field.Invalid(fldPath, values, fmt.Sprintf("cluster variables not valid: %s", err))) } - // Validate all variables from the Cluster. - for i := range clusterVariables { - clusterVariable := clusterVariables[i] + // Get an index of definitions for each variable name and definition from the ClusterClass variable. + defIndex := newDefinitionsIndex(definitions) - // Get schema. - // Nb. We already validated above in validateClusterVariablesDefined that there is a - // corresponding ClusterClass variable, so we don't have to check it again. - clusterClassVariable := clusterClassVariablesMap[clusterVariable.Name] - - allErrs = append(allErrs, ValidateClusterVariable(&clusterVariable, clusterClassVariable, fldPath.Index(i))...) + // Required variables definitions must exist as values on the Cluster. + if validateRequired { + allErrs = append(allErrs, validateRequiredVariables(valuesMap, defIndex, fldPath)...) } - return allErrs -} - -// validateRequiredClusterVariables validates all required variables from the ClusterClass exist in the Cluster. -func validateRequiredClusterVariables(clusterVariables map[string]*clusterv1.ClusterVariable, clusterClassVariables map[string]*clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { - var allErrs field.ErrorList - for variableName, clusterClassVariable := range clusterClassVariables { - if !clusterClassVariable.Required { + for i, value := range values { + // Values must have an associated definition. + definition, err := defIndex.get(value.Name, value.DefinitionFrom) + if err != nil { + allErrs = append(allErrs, field.Required(fldPath.Index(i).Child("name"), err.Error())) // TODO: consider if to add ClusterClass name continue } - if _, ok := clusterVariables[variableName]; !ok { - allErrs = append(allErrs, field.Required(fldPath, - fmt.Sprintf("required variable with name %q must be defined", variableName))) // TODO: consider if to use "Clusters with ClusterClass %q must have a variable with name %q" + // Values must set a non-empty DefinitionFrom if they have definition conflicts. + if value.DefinitionFrom == emptyVariableDefinitionFrom && definition.conflicts { + allErrs = append(allErrs, field.Required(fldPath.Index(i).Child("name"), + fmt.Sprintf("variable with name %q is not valid it requires a `definitionFrom` in cluster `spec.variables`", value.Name))) // TODO: consider if to add ClusterClass name + continue } + + // 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.Index(i))...) } return allErrs } -// validateClusterVariablesDefined validates all variables from the Cluster are defined in the ClusterClass. -func validateClusterVariablesDefined(clusterVariables []clusterv1.ClusterVariable, clusterClassVariables map[string]*clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList { +// validateRequiredVariables validates all required variables from the ClusterClass exist in the Cluster. +func validateRequiredVariables(values map[string]map[string]clusterv1.ClusterVariable, definitions definitionsIndex, fldPath *field.Path) field.ErrorList { var allErrs field.ErrorList - for i, clusterVariable := range clusterVariables { - if _, ok := clusterClassVariables[clusterVariable.Name]; !ok { - return field.ErrorList{field.Invalid(fldPath.Index(i).Child("name"), clusterVariable.Name, - fmt.Sprintf("variable with name %q is not defined in ClusterClass `status.variables`", clusterVariable.Name))} // TODO: consider if to add ClusterClass name + for name, perPatchDefs := range definitions { + for _, def := range perPatchDefs { + // Check the required value for the specific variable definition. If the variable is not required continue. + if !def.Required { + continue + } + + // If there is no variable with this name defined in the Cluster add an error and continue. + valuesForName, found := values[name] + if !found { + allErrs = append(allErrs, field.Required(fldPath, + fmt.Sprintf("required variable with name %q must be defined", name))) // TODO: consider if to use "Clusters with ClusterClass %q must have a variable with name %q" + continue + } + + // If there are no definition conflicts and the variable is set with an empty "DefinitionFrom" field return here. + // This is a valid way for users to define a required value for variables across all variable definitions. + if _, ok := valuesForName[emptyVariableDefinitionFrom]; ok && !def.conflicts { + continue + } + + // If the variable is not set for the specific definitionFrom add an error. + if v, ok := valuesForName[def.From]; !ok { + allErrs = append(allErrs, field.Required(fldPath, + fmt.Sprintf("required variable with name %q from %q must be defined", v.Name, v.DefinitionFrom))) // TODO: consider if to use "Clusters with ClusterClass %q must have a variable with name %q" + } } } - return allErrs } @@ -187,18 +214,96 @@ func validateUnknownFields(fldPath *field.Path, clusterVariable *clusterv1.Clust return nil } -func getClusterVariablesMap(clusterVariables []clusterv1.ClusterVariable) map[string]*clusterv1.ClusterVariable { - variablesMap := map[string]*clusterv1.ClusterVariable{} - for i := range clusterVariables { - variablesMap[clusterVariables[i].Name] = &clusterVariables[i] +func getValuesMap(values []clusterv1.ClusterVariable) (map[string]map[string]clusterv1.ClusterVariable, error) { + valuesMap := map[string]map[string]clusterv1.ClusterVariable{} + errs := []error{} + for _, value := range values { + c := value + _, ok := valuesMap[c.Name] + if !ok { + valuesMap[c.Name] = map[string]clusterv1.ClusterVariable{} + } + // 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)) + continue + } + // Add the variable. + valuesMap[c.Name][c.DefinitionFrom] = c + } + if len(errs) > 0 { + return nil, kerrors.NewAggregate(errs) + } + // Validate that the variables do not have incompatible values in their `definitionFrom` fields. + if err := validateValuesDefinitionFrom(valuesMap); err != nil { + return nil, err + } + + return valuesMap, nil +} + +// validateValuesDefinitionFrom validates that variables are not defined with both an empty DefinitionFrom and a +// non-empty DefinitionFrom. +func validateValuesDefinitionFrom(values map[string]map[string]clusterv1.ClusterVariable) error { + var errs []error + for k, definitions := range values { + for _, d := range definitions { + if d.DefinitionFrom == emptyVariableDefinitionFrom { + continue + } + if _, ok := definitions[emptyVariableDefinitionFrom]; ok { + errs = append(errs, errors.Errorf("variable %q is defined with a mix of empty and non-empty values for definitionFrom", k)) + } + } + } + if len(errs) > 0 { + return kerrors.NewAggregate(errs) + } + return nil +} + +type definitionsIndex map[string]map[string]*statusVariableDefinition + +func (i definitionsIndex) store(definition clusterv1.ClusterClassStatusVariable) { + for _, d := range definition.Definitions { + if _, ok := i[definition.Name]; !ok { + i[definition.Name] = map[string]*statusVariableDefinition{} + } + i[definition.Name][d.From] = &statusVariableDefinition{ + name: definition.Name, + conflicts: definition.DefinitionsConflict, + ClusterClassStatusVariableDefinition: d.DeepCopy(), + } + } +} + +func (i definitionsIndex) get(name, definitionFrom string) (*statusVariableDefinition, error) { + // If no variable with this name exists return an error. + if _, ok := i[name]; !ok { + return nil, errors.Errorf("no definitions found for variable %q", name) + } + + // If the definition exists for the specific definitionFrom, return it. + if def, ok := i[name][definitionFrom]; ok { + return def, nil + } + + // If definitionFrom is empty and there are no conflicts return the first definition. + if definitionFrom == emptyVariableDefinitionFrom { + for _, def := range i[name] { + if !def.conflicts { + return def, nil + } + return nil, errors.Errorf("variable %q must be defined with a non-empty `definitionFrom`", name) + } } - return variablesMap + return nil, errors.Errorf("no definitions found for variable %q from %q", name, definitionFrom) } -func getClusterClassVariablesMap(clusterClassVariables []clusterv1.ClusterClassVariable) map[string]*clusterv1.ClusterClassVariable { - variablesMap := map[string]*clusterv1.ClusterClassVariable{} - for i := range clusterClassVariables { - variablesMap[clusterClassVariables[i].Name] = &clusterClassVariables[i] +func newDefinitionsIndex(definitions []clusterv1.ClusterClassStatusVariable) definitionsIndex { + i := definitionsIndex{} + for _, def := range definitions { + i.store(def) } - return variablesMap + return i } diff --git a/internal/topology/variables/cluster_variable_validation_test.go b/internal/topology/variables/cluster_variable_validation_test.go index 85cd98fa9da9..3b60c3deb613 100644 --- a/internal/topology/variables/cluster_variable_validation_test.go +++ b/internal/topology/variables/cluster_variable_validation_test.go @@ -27,215 +27,6 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -func Test_ValidateClusterVariables(t *testing.T) { - tests := []struct { - name string - clusterClassVariables []clusterv1.ClusterClassVariable - clusterVariables []clusterv1.ClusterVariable - validateRequired bool - wantErr bool - }{ - { - name: "Pass for a number of valid variables.", - clusterClassVariables: []clusterv1.ClusterClassVariable{ - { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Minimum: pointer.Int64(1), - }, - }, - }, - { - Name: "zone", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - MinLength: pointer.Int64(1), - }, - }, - }, - { - Name: "location", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - Enum: []apiextensionsv1.JSON{ - {Raw: []byte(`"us-east-1"`)}, - {Raw: []byte(`"us-east-2"`)}, - }, - }, - }, - }, - }, - - clusterVariables: []clusterv1.ClusterVariable{ - { - Name: "cpu", - Value: apiextensionsv1.JSON{ - Raw: []byte(`1`), - }, - }, - { - Name: "zone", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"longerThanOneCharacter"`), - }, - }, - { - Name: "location", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"us-east-1"`), - }, - }, - }, - validateRequired: true, - }, - { - name: "Error if required ClusterClassVariable is not defined in ClusterVariables.", - clusterClassVariables: []clusterv1.ClusterClassVariable{ - { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Minimum: pointer.Int64(1), - }, - }, - }, - { - Name: "zone", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - MinLength: pointer.Int64(1), - }, - }, - }, - }, - - clusterVariables: []clusterv1.ClusterVariable{ - // cpu is missing in the ClusterVariables but is required in ClusterClassVariables. - { - Name: "zone", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"longerThanOneCharacter"`), - }, - }, - }, - validateRequired: true, - wantErr: true, - }, - { - name: "Pass if required ClusterClassVariable is not defined in ClusterVariables but required validation is disabled.", - clusterClassVariables: []clusterv1.ClusterClassVariable{ - { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Minimum: pointer.Int64(1), - }, - }, - }, - { - Name: "zone", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - MinLength: pointer.Int64(1), - }, - }, - }, - }, - - clusterVariables: []clusterv1.ClusterVariable{ - // cpu is missing in the ClusterVariables and is required in ClusterClassVariables, - // but required validation is disabled. - { - Name: "zone", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"longerThanOneCharacter"`), - }, - }, - }, - validateRequired: false, - }, - { - name: "Error if ClusterVariable defined which has no ClusterClassVariable definition.", - clusterClassVariables: []clusterv1.ClusterClassVariable{ - { - Name: "cpu", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "integer", - Minimum: pointer.Int64(1), - }, - }, - }, - { - Name: "zone", - Required: true, - Schema: clusterv1.VariableSchema{ - OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "string", - MinLength: pointer.Int64(1), - }, - }, - }, - }, - - clusterVariables: []clusterv1.ClusterVariable{ - // location is defined here but not in the ClusterClassVariables - { - Name: "location", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"us-east-1"`), - }, - }, - { - Name: "cpu", - Value: apiextensionsv1.JSON{ - Raw: []byte(`1`), - }, - }, - - { - Name: "zone", - Value: apiextensionsv1.JSON{ - Raw: []byte(`"longerThanOneCharacter"`), - }, - }, - }, - validateRequired: true, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - errList := validateClusterVariables(tt.clusterVariables, tt.clusterClassVariables, - tt.validateRequired, field.NewPath("spec", "topology", "variables")) - - if tt.wantErr { - g.Expect(errList).NotTo(BeEmpty()) - return - } - g.Expect(errList).To(BeEmpty()) - }) - } -} - func Test_ValidateClusterVariable(t *testing.T) { tests := []struct { name string @@ -1133,3 +924,752 @@ func Test_ValidateClusterVariable(t *testing.T) { }) } } + +func Test_ValidateClusterVariables(t *testing.T) { + tests := []struct { + name string + clusterClassVariables []clusterv1.ClusterClassStatusVariable + clusterVariables []clusterv1.ClusterVariable + validateRequired bool + wantErr bool + }{ + { + name: "Pass for a number of valid variables.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: pointer.Int64(1), + }, + }, + }, + }, + }, + { + Name: "zone", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + MinLength: pointer.Int64(1), + }, + }, + }, + }, + }, + { + Name: "location", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Enum: []apiextensionsv1.JSON{ + {Raw: []byte(`"us-east-1"`)}, + {Raw: []byte(`"us-east-2"`)}, + }, + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + { + Name: "zone", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"longerThanOneCharacter"`), + }, + }, + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east-1"`), + }, + }, + }, + validateRequired: true, + }, + { + name: "Error if required ClusterClassVariable is not defined in ClusterVariables.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: pointer.Int64(1), + }, + }, + }, + }, + }, + { + Name: "zone", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + MinLength: pointer.Int64(1), + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + // cpu is missing in the ClusterVariables but is required in ClusterClassVariables. + { + Name: "zone", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"longerThanOneCharacter"`), + }, + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Pass if required ClusterClassVariable is not defined in ClusterVariables but required validation is disabled.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: pointer.Int64(1), + }, + }, + }, + }, + }, + { + Name: "zone", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + MinLength: pointer.Int64(1), + }, + }, + }, + }, + }, + }, + + clusterVariables: []clusterv1.ClusterVariable{ + // cpu is missing in the ClusterVariables and is required in ClusterClassVariables, + // but required validation is disabled. + { + Name: "zone", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"longerThanOneCharacter"`), + }, + }, + }, + validateRequired: false, + }, + { + name: "Error if ClusterVariable defined which has no ClusterClassVariable definition.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + Minimum: pointer.Int64(1), + }, + }, + }, + }, + }, + { + Name: "zone", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + MinLength: pointer.Int64(1), + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + // location is defined here but not in the ClusterClassVariables + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east-1"`), + }, + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + + { + Name: "zone", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"longerThanOneCharacter"`), + }, + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if variable should have a DefinitionFrom field as it has conflicting definitions.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + 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", + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if variable DefinitionFrom field does not match an existing patch.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "non-existent-patch", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if a required variable with conflicting definitions isn't defined for each separate DefinitionFrom.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + DefinitionsConflict: true, + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + }, + }, + From: clusterv1.VariableDefinitionFromInline, + Required: true, + }, + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + Required: true, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Pass if a required variable does not have conflicting definitions but is defined with separate values for each definition in the Cluster.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: clusterv1.VariableDefinitionFromInline, + Required: true, + }, + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + Required: true, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: "inline", + }, + }, + validateRequired: true, + wantErr: false, + }, + { + name: "Fail if a variable is defined twice in the Cluster with the same definitionFrom.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: "somepatch", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if a required variable does not have conflicting definitions but has ambiguous definitions in the Cluster.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: clusterv1.VariableDefinitionFromInline, + Required: true, + }, + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + Required: true, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: "", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if a variable is defined twice in the Cluster where one definition has no definitionFrom.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + // Note: this is a different case than the above because of the ordering of the variables. + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + DefinitionFrom: "somepatch", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail if a variable without conflicting definitions provides two separate values without DefinitionFrom in the Cluster variables.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: clusterv1.VariableDefinitionFromInline, + Required: true, + }, + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + Required: true, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`2`), + }, + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Pass if a required variable with compatible definitions is defined once with no DefinitionFrom.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: clusterv1.VariableDefinitionFromInline, + Required: true, + }, + { + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "integer", + }, + }, + From: "somepatch", + Required: true, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + }, + }, + validateRequired: true, + wantErr: false, + }, + { + name: "Pass with two variables with correct values for their associated definition when definitions conflict.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + 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", + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"one"`), + }, + DefinitionFrom: "inline", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "somepatch", + }, + }, + validateRequired: true, + }, + { + name: "Fail with two variables with incorrect values for a their associated definition when definitions conflict.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + 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", + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"one"`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`1`), + }, + DefinitionFrom: "inline", + }, + }, + validateRequired: true, + wantErr: true, + }, + { + name: "Fail with two variables with incorrect values for a their associated definition when definitions conflict.", + clusterClassVariables: []clusterv1.ClusterClassStatusVariable{ + { + Name: "cpu", + // There are conflicting definitions which means Cluster Variables should include a `from` field. + 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", + }, + }, + }, + }, + }, + }, + clusterVariables: []clusterv1.ClusterVariable{ + { + Name: "cpu", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"one"`), + }, + DefinitionFrom: "", + }, + }, + validateRequired: true, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + errList := validateClusterVariables(tt.clusterVariables, tt.clusterClassVariables, + tt.validateRequired, field.NewPath("spec", "topology", "variables")) + + if tt.wantErr { + g.Expect(errList).NotTo(BeEmpty()) + return + } + g.Expect(errList).To(BeEmpty()) + }) + } +} diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index 162d6843a632..f627b4e97899 100644 --- a/internal/webhooks/cluster.go +++ b/internal/webhooks/cluster.go @@ -459,10 +459,7 @@ func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.Cluste if clusterClass == nil { return field.ErrorList{field.InternalError(field.NewPath(""), errors.New("ClusterClass can not be nil"))} } - - defaultedVariables, errs := variables.DefaultClusterVariables(cluster.Spec.Topology.Variables, - //TODO: Update this function to directly us ClusterClassStatusVariables to take care of multiple definitions per variable name. - clusterClassStatusVariablesToVariables(clusterClass.Status.Variables), + defaultedVariables, errs := variables.DefaultClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables, field.NewPath("spec", "topology", "variables")) if len(errs) > 0 { allErrs = append(allErrs, errs...) @@ -476,9 +473,7 @@ func DefaultVariables(cluster *clusterv1.Cluster, clusterClass *clusterv1.Cluste if md.Variables == nil || len(md.Variables.Overrides) == 0 { continue } - defaultedVariables, errs := variables.DefaultMachineDeploymentVariables(md.Variables.Overrides, - //TODO: Update this function to directly us ClusterClassStatusVariables to take care of multiple definitions per variable name. - clusterClassStatusVariablesToVariables(clusterClass.Status.Variables), + defaultedVariables, errs := variables.DefaultMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables, field.NewPath("spec", "topology", "workers", "machineDeployments").Index(i).Child("variables", "overrides")) if len(errs) > 0 { allErrs = append(allErrs, errs...) @@ -505,9 +500,7 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl allErrs = append(allErrs, validateMachineHealthChecks(cluster, clusterClass)...) // Check if the variables defined in the ClusterClass are valid. - allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, - //TODO: Update this function to directly us ClusterClassStatusVariables to take care of multiple definitions per variable name. - clusterClassStatusVariablesToVariables(clusterClass.Status.Variables), + allErrs = append(allErrs, variables.ValidateClusterVariables(cluster.Spec.Topology.Variables, clusterClass.Status.Variables, fldPath.Child("variables"))...) if cluster.Spec.Topology.Workers != nil { @@ -516,9 +509,7 @@ func ValidateClusterForClusterClass(cluster *clusterv1.Cluster, clusterClass *cl if md.Variables == nil || len(md.Variables.Overrides) == 0 { continue } - allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, - //TODO: Update this function to directly us ClusterClassStatusVariables to take care of multiple definitions per variable name. - clusterClassStatusVariablesToVariables(clusterClass.Status.Variables), + allErrs = append(allErrs, variables.ValidateMachineDeploymentVariables(md.Variables.Overrides, clusterClass.Status.Variables, fldPath.Child("workers", "machineDeployments").Index(i).Child("variables", "overrides"))...) } } @@ -546,18 +537,3 @@ func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster }) return clusterClass, clusterClassGetErr } - -// TODO: This function will not be needed once per-definition defaulting and validation is implemented. -func clusterClassStatusVariablesToVariables(vars []clusterv1.ClusterClassStatusVariable) []clusterv1.ClusterClassVariable { - var ccVars []clusterv1.ClusterClassVariable - for _, v := range vars { - for _, d := range v.Definitions { - ccVars = append(ccVars, clusterv1.ClusterClassVariable{ - Required: d.Required, - Schema: d.Schema, - Name: v.Name, - }) - } - } - return ccVars -} diff --git a/internal/webhooks/cluster_test.go b/internal/webhooks/cluster_test.go index dfbd3efc3039..f8bff26f0a53 100644 --- a/internal/webhooks/cluster_test.go +++ b/internal/webhooks/cluster_test.go @@ -75,6 +75,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "string", @@ -106,6 +107,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "string", @@ -147,6 +149,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "string", @@ -161,6 +164,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "number", @@ -201,6 +205,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "string", @@ -252,6 +257,7 @@ func TestClusterDefaultVariables(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "object", @@ -323,28 +329,225 @@ func TestClusterDefaultVariables(t *testing.T) { }, }, }, + { + name: "Use one value for multiple definitions when variables don't conflict", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "location", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "anotherpatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + }, + }, + ). + Build(), + topology: &clusterv1.Topology{}, + expect: &clusterv1.Topology{ + Variables: []clusterv1.ClusterVariable{ + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + }, + }, + }, + }, + { + name: "Add defaults for each definitionFrom if variable is defined for some definitionFrom", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "location", + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "anotherpatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + }, + }, + ). + Build(), + topology: &clusterv1.Topology{ + Variables: []clusterv1.ClusterVariable{ + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-west"`), + }, + DefinitionFrom: "somepatch", + }, + }, + }, + expect: &clusterv1.Topology{ + Variables: []clusterv1.ClusterVariable{ + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-west"`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + DefinitionFrom: clusterv1.VariableDefinitionFromInline, + }, + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + DefinitionFrom: "anotherpatch", + }, + }, + }, + }, + { + name: "set definitionFrom on defaults when variables conflict", + clusterClass: builder.ClusterClass(metav1.NamespaceDefault, "class1"). + WithStatusVariables(clusterv1.ClusterClassStatusVariable{ + Name: "location", + DefinitionsConflict: true, + Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ + { + Required: true, + From: clusterv1.VariableDefinitionFromInline, + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "somepatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + { + Required: true, + From: "anotherpatch", + Schema: clusterv1.VariableSchema{ + OpenAPIV3Schema: clusterv1.JSONSchemaProps{ + Type: "string", + Default: &apiextensionsv1.JSON{Raw: []byte(`"us-east"`)}, + }, + }, + }, + }, + }, + ). + Build(), + topology: &clusterv1.Topology{}, + expect: &clusterv1.Topology{ + Variables: []clusterv1.ClusterVariable{ + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + DefinitionFrom: clusterv1.VariableDefinitionFromInline, + }, + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + DefinitionFrom: "somepatch", + }, + { + Name: "location", + Value: apiextensionsv1.JSON{ + Raw: []byte(`"us-east"`), + }, + DefinitionFrom: "anotherpatch", + }, + }, + }, + }, } for _, tt := range tests { - // 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" + 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" - cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). - WithTopology(tt.topology). - Build() + cluster := builder.Cluster(metav1.NamespaceDefault, "cluster1"). + WithTopology(tt.topology). + Build() - // Mark this condition to true so the webhook sees the ClusterClass as up to date. - conditions.MarkTrue(tt.clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) - fakeClient := fake.NewClientBuilder(). - WithObjects(tt.clusterClass). - 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} + // Mark this condition to true so the webhook sees the ClusterClass as up to date. + conditions.MarkTrue(tt.clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) + fakeClient := fake.NewClientBuilder(). + WithObjects(tt.clusterClass). + 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} - t.Run(tt.name, func(t *testing.T) { // Test if defaulting works in combination with validation. util.CustomDefaultValidateTest(ctx, cluster, webhook)(t) // Test defaulting. @@ -797,6 +1000,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer", @@ -827,6 +1031,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer", @@ -852,6 +1057,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer", @@ -881,6 +1087,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer", @@ -917,6 +1124,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: true, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer", @@ -953,6 +1161,7 @@ func TestClusterTopologyValidation(t *testing.T) { Definitions: []clusterv1.ClusterClassStatusVariableDefinition{ { Required: false, + From: clusterv1.VariableDefinitionFromInline, Schema: clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "integer",