Merge pull request #5756 from sbueringer/pr-fix-nullable-variables

🐛 ClusterClass: fix nullable variables
kubernetes-sigs · Nov 30, 2021 · 45f6697 · 45f6697
2 parents 0fb3192 + 40af388
commit 45f6697
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 5 deletions.
diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go
@@ -162,8 +162,10 @@ type ClusterVariable struct {
 	// from the ClusterClass.
 	// Note: We have to use apiextensionsv1.JSON instead of a custom JSON type, because controller-tools has a
 	// hard-coded schema for apiextensionsv1.JSON which cannot be produced by another type via controller-tools,
-	// i.e. it's not possible to have no type field.
+	// i.e. it is not possible to have no type field.
 	// Ref: https://github.com/kubernetes-sigs/controller-tools/blob/d0e03a142d0ecdd5491593e941ee1d6b5d91dba6/pkg/crd/known_types.go#L106-L111
+	// Note: This field has to be nullable, so allow setting nullable variables to "null".
+	// +nullable
 	Value apiextensionsv1.JSON `json:"value"`
 }
 

diff --git a/config/crd/bases/cluster.x-k8s.io_clusters.yaml b/config/crd/bases/cluster.x-k8s.io_clusters.yaml
@@ -892,7 +892,10 @@ spec:
                             instead of a custom JSON type, because controller-tools
                             has a hard-coded schema for apiextensionsv1.JSON which
                             cannot be produced by another type via controller-tools,
-                            i.e. it''s not possible to have no type field. Ref: https://github.com/kubernetes-sigs/controller-tools/blob/d0e03a142d0ecdd5491593e941ee1d6b5d91dba6/pkg/crd/known_types.go#L106-L111'
+                            i.e. it is not possible to have no type field. Ref: https://github.com/kubernetes-sigs/controller-tools/blob/d0e03a142d0ecdd5491593e941ee1d6b5d91dba6/pkg/crd/known_types.go#L106-L111
+                            Note: This field has to be nullable, so allow setting
+                            nullable variables to "null".'
+                          nullable: true
                           x-kubernetes-preserve-unknown-fields: true
                       required:
                       - name

diff --git a/internal/topology/variables/cluster_variable_defaulting_test.go b/internal/topology/variables/cluster_variable_defaulting_test.go
@@ -188,6 +188,38 @@ func Test_DefaultClusterVariables(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Don't default variables set to null",
+			clusterClassVariables: []clusterv1.ClusterClassVariable{
+				{
+					Name:     "cpu",
+					Required: true,
+					Schema: clusterv1.VariableSchema{
+						OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+							Type:     "integer",
+							Default:  &apiextensionsv1.JSON{Raw: []byte(`1`)},
+							Nullable: true,
+						},
+					},
+				},
+			},
+			clusterVariables: []clusterv1.ClusterVariable{
+				{
+					Name: "cpu",
+					Value: apiextensionsv1.JSON{
+						Raw: nil, // A JSON with a nil value is the result of setting the variable value to "null" via YAML.
+					},
+				},
+			},
+			want: []clusterv1.ClusterVariable{
+				{
+					Name: "cpu",
+					Value: apiextensionsv1.JSON{
+						Raw: nil, // A JSON with a nil value is the result of setting the variable value to "null" via YAML.
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

diff --git a/internal/topology/variables/cluster_variable_validation.go b/internal/topology/variables/cluster_variable_validation.go
@@ -91,9 +91,13 @@ func validateClusterVariablesDefined(clusterVariables map[string]*clusterv1.Clus
 func validateClusterVariable(clusterVariable *clusterv1.ClusterVariable, clusterClassVariable *clusterv1.ClusterClassVariable, fldPath *field.Path) field.ErrorList {
 	// Parse JSON value.
 	var variableValue interface{}
-	if err := json.Unmarshal(clusterVariable.Value.Raw, &variableValue); err != nil {
-		return field.ErrorList{field.Invalid(fldPath, string(clusterVariable.Value.Raw),
-			fmt.Sprintf("variable %q could not be parsed: %v", clusterVariable.Name, err))}
+	// Only try to unmarshal the clusterVariable if it is not nil, otherwise the variableValue is nil.
+	// Note: A clusterVariable with a nil value is the result of setting the variable value to "null" via YAML.
+	if clusterVariable.Value.Raw != nil {
+		if err := json.Unmarshal(clusterVariable.Value.Raw, &variableValue); err != nil {
+			return field.ErrorList{field.Invalid(fldPath, string(clusterVariable.Value.Raw),
+				fmt.Sprintf("variable %q could not be parsed: %v", clusterVariable.Name, err))}
+		}
 	}
 
 	// Convert schema to Kubernetes APIExtensions Schema.

diff --git a/internal/topology/variables/cluster_variable_validation_test.go b/internal/topology/variables/cluster_variable_validation_test.go
@@ -340,6 +340,45 @@ func Test_ValidateClusterVariable(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Error if string is null",
+			clusterClassVariable: &clusterv1.ClusterClassVariable{
+				Name:     "location",
+				Required: true,
+				Schema: clusterv1.VariableSchema{
+					OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+						Type:     "string",
+						Nullable: false,
+					},
+				},
+			},
+			clusterVariable: &clusterv1.ClusterVariable{
+				Name: "location",
+				Value: apiextensionsv1.JSON{
+					Raw: nil, // A JSON with a nil value is the result of setting the variable value to "null" via YAML.
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Valid nullable string",
+			clusterClassVariable: &clusterv1.ClusterClassVariable{
+				Name:     "location",
+				Required: true,
+				Schema: clusterv1.VariableSchema{
+					OpenAPIV3Schema: clusterv1.JSONSchemaProps{
+						Type:     "string",
+						Nullable: true,
+					},
+				},
+			},
+			clusterVariable: &clusterv1.ClusterVariable{
+				Name: "location",
+				Value: apiextensionsv1.JSON{
+					Raw: nil, // A JSON with a nil value is the result of setting the variable value to "null" via YAML.
+				},
+			},
+		},
 
 		{
 			name: "Valid enum",