From 5c210174cb75e37a0856add0215ceca0ca0dbc6b Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Thu, 28 Mar 2024 10:05:04 +0000 Subject: [PATCH 1/2] test: Ensure defaults from JSON schema are respected Without this, defaults declared in the JSON schema are not included in validation steps, which can lead to invalid failures, while also not allowing for tests that target defaults. --- common/go.mod | 1 + common/pkg/testutils/openapi/convert.go | 2 +- common/pkg/testutils/openapi/validate.go | 13 +++++++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/common/go.mod b/common/go.mod index 7e97d6d90..d65da65ec 100644 --- a/common/go.mod +++ b/common/go.mod @@ -63,6 +63,7 @@ require ( golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect golang.org/x/net v0.19.0 // indirect golang.org/x/oauth2 v0.14.0 // indirect + golang.org/x/sync v0.5.0 // indirect golang.org/x/sys v0.16.0 // indirect golang.org/x/term v0.15.0 // indirect golang.org/x/text v0.14.0 // indirect diff --git a/common/pkg/testutils/openapi/convert.go b/common/pkg/testutils/openapi/convert.go index 922e4f0b5..5eb248617 100644 --- a/common/pkg/testutils/openapi/convert.go +++ b/common/pkg/testutils/openapi/convert.go @@ -14,7 +14,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -// convertToAPIExtensionsJSONSchemaProps converts a clusterv1.JSONSchemaProps to apiextensions.JSONSchemaProp. +// ConvertToAPIExtensionsJSONSchemaProps converts a clusterv1.JSONSchemaProps to apiextensions.JSONSchemaProp. // NOTE: This is used whenever we want to use one of the upstream libraries, as they use apiextensions.JSONSchemaProp. // NOTE: If new fields are added to clusterv1.JSONSchemaProps (e.g. to support complex types), the corresponding // schema validation must be added to validateRootSchema too. diff --git a/common/pkg/testutils/openapi/validate.go b/common/pkg/testutils/openapi/validate.go index 9ef8f4ea8..bf776db6d 100644 --- a/common/pkg/testutils/openapi/validate.go +++ b/common/pkg/testutils/openapi/validate.go @@ -10,6 +10,7 @@ import ( "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting" structuralpruning "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning" "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" "k8s.io/apimachinery/pkg/util/validation/field" @@ -62,6 +63,18 @@ func ValidateClusterVariable( )} } + s, err := structuralschema.NewStructural(apiExtensionsSchema) + if err != nil { + return field.ErrorList{field.InternalError(fldPath, + fmt.Errorf( + "failed to create structural schema for variable %q; ClusterClass should be checked: %v", + value.Name, + err, + ), + )} + } + defaulting.Default(variableValue, s) + // Validate variable against the schema. // NOTE: We're reusing a library func used in CRD validation. if err := validation.ValidateCustomResource(fldPath, variableValue, validator); err != nil { From dc7f3bc226c8bf76cd05a940917bc53bfcdbe151 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Thu, 28 Mar 2024 10:34:49 +0000 Subject: [PATCH 2/2] fix: Add schema defaults for CSI Also add tests for the new schema requirements. --- api/v1alpha1/addon_types.go | 52 ++--- api/v1alpha1/zz_generated.deepcopy.go | 48 ++--- pkg/handlers/generic/lifecycle/csi/handler.go | 2 +- .../generic/lifecycle/csi/variables_test.go | 180 ++++++++++++++++++ pkg/handlers/generic/lifecycle/utils/utils.go | 15 +- 5 files changed, 237 insertions(+), 60 deletions(-) create mode 100644 pkg/handlers/generic/lifecycle/csi/variables_test.go diff --git a/api/v1alpha1/addon_types.go b/api/v1alpha1/addon_types.go index de788bf60..4852ef27b 100644 --- a/api/v1alpha1/addon_types.go +++ b/api/v1alpha1/addon_types.go @@ -5,6 +5,7 @@ package v1alpha1 import ( corev1 "k8s.io/api/core/v1" + "k8s.io/utils/ptr" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "github.com/d2iq-labs/cluster-api-runtime-extensions-nutanix/api/variables" @@ -36,7 +37,7 @@ type Addons struct { CPI *CPI `json:"cpi,omitempty"` // +optional - CSIProviders *CSIProviders `json:"csi,omitempty"` + CSI *CSI `json:"csi,omitempty"` } func (Addons) VariableSchema() clusterv1.VariableSchema { @@ -48,7 +49,7 @@ func (Addons) VariableSchema() clusterv1.VariableSchema { "cni": CNI{}.VariableSchema().OpenAPIV3Schema, "nfd": NFD{}.VariableSchema().OpenAPIV3Schema, "clusterAutoscaler": ClusterAutoscaler{}.VariableSchema().OpenAPIV3Schema, - "csi": CSIProviders{}.VariableSchema().OpenAPIV3Schema, + "csi": CSI{}.VariableSchema().OpenAPIV3Schema, "cpi": CPI{}.VariableSchema().OpenAPIV3Schema, }, }, @@ -147,7 +148,7 @@ type DefaultStorage struct { StorageClassConfigName string `json:"storageClassConfigName"` } -type CSIProviders struct { +type CSI struct { // +optional Providers []CSIProvider `json:"providers,omitempty"` // +optional @@ -191,24 +192,34 @@ func (StorageClassConfig) VariableSchema() clusterv1.VariableSchema { } return clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", + Type: "object", + Required: []string{"name", "parameters", "reclaimPolicy", "volumeBindingMode"}, Properties: map[string]clusterv1.JSONSchemaProps{ "name": { Type: "string", Description: "Name of storage class config.", + MinLength: ptr.To(int64(1)), }, "parameters": { - Type: "object", - Description: "Parameters passed into the storage class object.", - XPreserveUnknownFields: true, + Type: "object", + Description: "Parameters passed into the storage class object.", + AdditionalProperties: &clusterv1.JSONSchemaProps{ + Type: "string", + }, + Default: variables.MustMarshal(map[string]string{ + "csi.storage.k8s.io/fstype": "ext4", + "type": "gp3", + }), }, "reclaimPolicy": { - Type: "string", - Enum: variables.MustMarshalValuesToEnumJSON(supportedReclaimPolicies...), + Type: "string", + Enum: variables.MustMarshalValuesToEnumJSON(supportedReclaimPolicies...), + Default: variables.MustMarshal(VolumeReclaimDelete), }, "volumeBindingMode": { - Type: "string", - Enum: variables.MustMarshalValuesToEnumJSON(supportedBindingModes...), + Type: "string", + Enum: variables.MustMarshalValuesToEnumJSON(supportedBindingModes...), + Default: variables.MustMarshal(VolumeBindingImmediate), }, }, }, @@ -219,7 +230,8 @@ func (CSIProvider) VariableSchema() clusterv1.VariableSchema { supportedCSIProviders := []string{CSIProviderAWSEBS, CSIProviderNutanix} return clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ - Type: "object", + Type: "object", + Required: []string{"name", "strategy"}, Properties: map[string]clusterv1.JSONSchemaProps{ "name": { Description: "Name of the CSI Provider", @@ -248,11 +260,8 @@ func (CSIProvider) VariableSchema() clusterv1.VariableSchema { }, }, "storageClassConfig": { - Type: "array", - Items: &clusterv1.JSONSchemaProps{ - Type: "object", - Properties: StorageClassConfig{}.VariableSchema().OpenAPIV3Schema.Properties, - }, + Type: "array", + Items: ptr.To(StorageClassConfig{}.VariableSchema().OpenAPIV3Schema), }, }, }, @@ -281,17 +290,14 @@ func (DefaultStorage) VariableSchema() clusterv1.VariableSchema { } } -func (CSIProviders) VariableSchema() clusterv1.VariableSchema { +func (CSI) VariableSchema() clusterv1.VariableSchema { return clusterv1.VariableSchema{ OpenAPIV3Schema: clusterv1.JSONSchemaProps{ Type: "object", Properties: map[string]clusterv1.JSONSchemaProps{ "providers": { - Type: "array", - Items: &clusterv1.JSONSchemaProps{ - Type: "object", - Properties: CSIProvider{}.VariableSchema().OpenAPIV3Schema.Properties, - }, + Type: "array", + Items: ptr.To(CSIProvider{}.VariableSchema().OpenAPIV3Schema), }, "defaultStorage": DefaultStorage{}.VariableSchema().OpenAPIV3Schema, }, diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 60f0a3189..ecb1a578a 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -9,7 +9,7 @@ package v1alpha1 import ( "github.com/d2iq-labs/cluster-api-runtime-extensions-nutanix/api/external/sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -204,9 +204,9 @@ func (in *Addons) DeepCopyInto(out *Addons) { *out = new(CPI) **out = **in } - if in.CSIProviders != nil { - in, out := &in.CSIProviders, &out.CSIProviders - *out = new(CSIProviders) + if in.CSI != nil { + in, out := &in.CSI, &out.CSI + *out = new(CSI) (*in).DeepCopyInto(*out) } } @@ -252,55 +252,55 @@ func (in *CPI) DeepCopy() *CPI { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CSIProvider) DeepCopyInto(out *CSIProvider) { +func (in *CSI) DeepCopyInto(out *CSI) { *out = *in - if in.StorageClassConfig != nil { - in, out := &in.StorageClassConfig, &out.StorageClassConfig - *out = make([]StorageClassConfig, len(*in)) + if in.Providers != nil { + in, out := &in.Providers, &out.Providers + *out = make([]CSIProvider, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.Credentials != nil { - in, out := &in.Credentials, &out.Credentials - *out = new(v1.SecretReference) + if in.DefaultStorage != nil { + in, out := &in.DefaultStorage, &out.DefaultStorage + *out = new(DefaultStorage) **out = **in } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSIProvider. -func (in *CSIProvider) DeepCopy() *CSIProvider { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSI. +func (in *CSI) DeepCopy() *CSI { if in == nil { return nil } - out := new(CSIProvider) + out := new(CSI) in.DeepCopyInto(out) return out } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *CSIProviders) DeepCopyInto(out *CSIProviders) { +func (in *CSIProvider) DeepCopyInto(out *CSIProvider) { *out = *in - if in.Providers != nil { - in, out := &in.Providers, &out.Providers - *out = make([]CSIProvider, len(*in)) + if in.StorageClassConfig != nil { + in, out := &in.StorageClassConfig, &out.StorageClassConfig + *out = make([]StorageClassConfig, len(*in)) for i := range *in { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.DefaultStorage != nil { - in, out := &in.DefaultStorage, &out.DefaultStorage - *out = new(DefaultStorage) + if in.Credentials != nil { + in, out := &in.Credentials, &out.Credentials + *out = new(v1.SecretReference) **out = **in } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSIProviders. -func (in *CSIProviders) DeepCopy() *CSIProviders { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSIProvider. +func (in *CSIProvider) DeepCopy() *CSIProvider { if in == nil { return nil } - out := new(CSIProviders) + out := new(CSIProvider) in.DeepCopyInto(out) return out } diff --git a/pkg/handlers/generic/lifecycle/csi/handler.go b/pkg/handlers/generic/lifecycle/csi/handler.go index 14209bec3..e7bccfb3b 100644 --- a/pkg/handlers/generic/lifecycle/csi/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/handler.go @@ -72,7 +72,7 @@ func (c *CSIHandler) AfterControlPlaneInitialized( ) varMap := variables.ClusterVariablesToVariablesMap(req.Cluster.Spec.Topology.Variables) resp.SetStatus(runtimehooksv1.ResponseStatusSuccess) - csiProviders, found, err := variables.Get[v1alpha1.CSIProviders]( + csiProviders, found, err := variables.Get[v1alpha1.CSI]( varMap, c.variableName, c.variablePath...) diff --git a/pkg/handlers/generic/lifecycle/csi/variables_test.go b/pkg/handlers/generic/lifecycle/csi/variables_test.go new file mode 100644 index 000000000..d1cae6ffa --- /dev/null +++ b/pkg/handlers/generic/lifecycle/csi/variables_test.go @@ -0,0 +1,180 @@ +// Copyright 2023 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package csi + +import ( + "testing" + + "k8s.io/utils/ptr" + + "github.com/d2iq-labs/cluster-api-runtime-extensions-nutanix/api/v1alpha1" + "github.com/d2iq-labs/cluster-api-runtime-extensions-nutanix/common/pkg/testutils/capitest" + "github.com/d2iq-labs/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/clusterconfig" +) + +func TestVariableValidation(t *testing.T) { + capitest.ValidateDiscoverVariables( + t, + clusterconfig.MetaVariableName, + ptr.To(v1alpha1.GenericClusterConfig{}.VariableSchema()), + false, + clusterconfig.NewVariable, + capitest.VariableTestDef{ + Name: "set with empty CSI", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{}, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with with empty CSI providers slice", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with invalid provider", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "csi-provider", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + }}, + }, + }, + }, + ExpectError: true, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single valid provider with HelmAddon strategy", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyHelmAddon, + }}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single valid provider with CRS strategy", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + }}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single valid provider with empty storage class config", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{}, + }}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single invalid provider with missing name", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{{}}, + }}, + }, + }, + }, + ExpectError: true, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single valid provider using defaults", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{{ + Name: "default", + }}, + }}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single valid provider with single empty specified storage class config", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{{ + Name: "default", + Parameters: map[string]string{}, + }}, + }}, + }, + }, + }, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single invalid provider with invalid reclaim policy", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{{ + Name: "default", + ReclaimPolicy: "invalid", + }}, + }}, + }, + }, + }, + ExpectError: true, + }, + capitest.VariableTestDef{ + Name: "set with single CSIProvider with single invalid provider with invalid reclaim volume binding mode", + Vals: v1alpha1.GenericClusterConfig{ + Addons: &v1alpha1.Addons{ + CSI: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{{ + Name: "aws-ebs", + Strategy: v1alpha1.AddonStrategyClusterResourceSet, + StorageClassConfig: []v1alpha1.StorageClassConfig{{ + Name: "default", + VolumeBindingMode: "invalid", + }}, + }}, + }, + }, + }, + ExpectError: true, + }, + ) +} diff --git a/pkg/handlers/generic/lifecycle/utils/utils.go b/pkg/handlers/generic/lifecycle/utils/utils.go index 7c58c8017..cd2e19113 100644 --- a/pkg/handlers/generic/lifecycle/utils/utils.go +++ b/pkg/handlers/generic/lifecycle/utils/utils.go @@ -6,7 +6,6 @@ package utils import ( "context" "fmt" - "maps" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -30,13 +29,9 @@ const ( ) var ( - defualtStorageClassKey = "storageclass.kubernetes.io/is-default-class" + defaultStorageClassKey = "storageclass.kubernetes.io/is-default-class" defaultStorageClassMap = map[string]string{ - defualtStorageClassKey: "true", - } - defaultParams = map[string]string{ - "csi.storage.k8s.io/fstype": "ext4", - "type": "gp3", + defaultStorageClassKey: "true", } ) @@ -159,10 +154,6 @@ func CreateStorageClass( case v1alpha1.VolumeReclaimRetain: reclaimPolicy = ptr.To(corev1.PersistentVolumeReclaimRetain) } - params := defaultParams - if storageConfig.Parameters != nil { - params = maps.Clone(storageConfig.Parameters) - } sc := storagev1.StorageClass{ TypeMeta: metav1.TypeMeta{ Kind: kindStorageClass, @@ -173,7 +164,7 @@ func CreateStorageClass( Namespace: defaultsNamespace, }, Provisioner: string(provisionerName), - Parameters: params, + Parameters: storageConfig.Parameters, VolumeBindingMode: volumeBindingMode, ReclaimPolicy: reclaimPolicy, }