From dc7f3bc226c8bf76cd05a940917bc53bfcdbe151 Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Thu, 28 Mar 2024 10:34:49 +0000 Subject: [PATCH] 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, }