diff --git a/api/v1alpha1/addon_types.go b/api/v1alpha1/addon_types.go index ac440cb0b..c5f4d738f 100644 --- a/api/v1alpha1/addon_types.go +++ b/api/v1alpha1/addon_types.go @@ -120,11 +120,12 @@ type DefaultStorage struct { } type CSI struct { - // +kubebuilder:validation:Optional - Providers []CSIProvider `json:"providers,omitempty"` + // +kubebuilder:validation:MinItems=1 + // +kubebuilder:validation:Required + Providers []CSIProvider `json:"providers"` - // +kubebuilder:validation:Optional - DefaultStorage *DefaultStorage `json:"defaultStorage,omitempty"` + // +kubebuilder:validation:Required + DefaultStorage DefaultStorage `json:"defaultStorage"` } type CSIProvider struct { @@ -133,8 +134,9 @@ type CSIProvider struct { // +kubebuilder:validation:Enum=aws-ebs;nutanix Name string `json:"name"` + // StorageClassConfig is a list of storage class configurations for this CSI provider. // +kubebuilder:validation:Optional - StorageClassConfig []StorageClassConfig `json:"storageClassConfig,omitempty"` + StorageClassConfig []StorageClassConfig `json:"storageClassConfig"` // Addon strategy used to deploy the CSI provider to the workload cluster. // +kubebuilder:validation:Required diff --git a/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml index 42ec4465d..1f7663da7 100644 --- a/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml +++ b/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml @@ -139,6 +139,8 @@ spec: - nutanix type: string storageClassConfig: + description: StorageClassConfig is a list of storage + class configurations for this CSI provider. items: properties: allowExpansion: @@ -189,7 +191,11 @@ spec: - name - strategy type: object + minItems: 1 type: array + required: + - defaultStorage + - providers type: object nfd: description: NFD tells us to enable or disable the node feature diff --git a/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml index 05e1e316e..a0efd1e87 100644 --- a/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml +++ b/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml @@ -140,6 +140,8 @@ spec: - nutanix type: string storageClassConfig: + description: StorageClassConfig is a list of storage + class configurations for this CSI provider. items: properties: allowExpansion: @@ -190,7 +192,11 @@ spec: - name - strategy type: object + minItems: 1 type: array + required: + - defaultStorage + - providers type: object nfd: description: NFD tells us to enable or disable the node feature diff --git a/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml index b5b8a753b..e3ecdec72 100644 --- a/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml +++ b/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml @@ -140,6 +140,8 @@ spec: - nutanix type: string storageClassConfig: + description: StorageClassConfig is a list of storage + class configurations for this CSI provider. items: properties: allowExpansion: @@ -190,7 +192,11 @@ spec: - name - strategy type: object + minItems: 1 type: array + required: + - defaultStorage + - providers type: object nfd: description: NFD tells us to enable or disable the node feature diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 7b3d6e685..cf6454d02 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -413,11 +413,7 @@ func (in *CSI) DeepCopyInto(out *CSI) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.DefaultStorage != nil { - in, out := &in.DefaultStorage, &out.DefaultStorage - *out = new(DefaultStorage) - **out = **in - } + out.DefaultStorage = in.DefaultStorage } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CSI. diff --git a/common/pkg/capi/clustertopology/variables/json.go b/common/pkg/capi/clustertopology/variables/json.go new file mode 100644 index 000000000..47ebdea9e --- /dev/null +++ b/common/pkg/capi/clustertopology/variables/json.go @@ -0,0 +1,32 @@ +// Copyright 2024 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package variables + +import ( + "encoding/json" + "fmt" + + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +func MarshalToClusterVariable[T any](name string, obj T) (*clusterv1.ClusterVariable, error) { + marshaled, err := json.Marshal(obj) + if err != nil { + return nil, fmt.Errorf("failed to marshal variable value %q: %w", name, err) + } + return &clusterv1.ClusterVariable{ + Name: name, + Value: v1.JSON{Raw: marshaled}, + }, nil +} + +func UnmarshalClusterVariable[T any](clusterVariable *clusterv1.ClusterVariable, obj *T) error { + err := json.Unmarshal(clusterVariable.Value.Raw, obj) + if err != nil { + return fmt.Errorf("error unmarshalling variable: %w", err) + } + + return nil +} diff --git a/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go b/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go index 79fcd1def..08461a812 100644 --- a/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go @@ -58,7 +58,7 @@ func New( func (a *AWSEBS) Apply( ctx context.Context, provider v1alpha1.CSIProvider, - defaultStorageConfig *v1alpha1.DefaultStorage, + defaultStorageConfig v1alpha1.DefaultStorage, req *runtimehooksv1.AfterControlPlaneInitializedRequest, _ logr.Logger, ) error { diff --git a/pkg/handlers/generic/lifecycle/csi/handler.go b/pkg/handlers/generic/lifecycle/csi/handler.go index ab871bd73..9d1aea995 100644 --- a/pkg/handlers/generic/lifecycle/csi/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/handler.go @@ -27,7 +27,7 @@ type CSIProvider interface { Apply( context.Context, v1alpha1.CSIProvider, - *v1alpha1.DefaultStorage, + v1alpha1.DefaultStorage, *runtimehooksv1.AfterControlPlaneInitializedRequest, logr.Logger, ) error @@ -74,60 +74,59 @@ func (c *CSIHandler) AfterControlPlaneInitialized( ) varMap := variables.ClusterVariablesToVariablesMap(req.Cluster.Spec.Topology.Variables) resp.SetStatus(runtimehooksv1.ResponseStatusSuccess) - csiProviders, err := variables.Get[v1alpha1.CSI]( + csi, err := variables.Get[v1alpha1.CSI]( varMap, c.variableName, c.variablePath...) if err != nil { - if variables.IsNotFoundError(err) || - csiProviders.Providers == nil || - len(csiProviders.Providers) == 0 { - log.V(4).Info( - fmt.Sprintf( - "Skipping CSI handler, no providers given in %v", - csiProviders, - ), - ) + if variables.IsNotFoundError(err) { + log.Info("Skipping CSI handler, the cluster does not define the CSI variable") return } - log.Error( - err, - "failed to read CSI provider from cluster definition", - ) + msg := "failed to read the CSI variable from the cluster" + log.Error(err, msg) resp.SetStatus(runtimehooksv1.ResponseStatusFailure) - resp.SetMessage( - fmt.Sprintf("failed to read CSI provider from cluster definition: %v", - err, - ), - ) + resp.SetMessage(fmt.Sprintf("%s: %v", msg, err)) return } - if len(csiProviders.Providers) == 1 && - len(csiProviders.Providers[0].StorageClassConfig) == 1 && - csiProviders.DefaultStorage == nil { - csiProviders.DefaultStorage = &v1alpha1.DefaultStorage{ - ProviderName: csiProviders.Providers[0].Name, - StorageClassConfigName: csiProviders.Providers[0].StorageClassConfig[0].Name, - } + + // This is defensive, because the API validation requires at least one provider. + if len(csi.Providers) == 0 { + msg := "The list of CSI providers must include at least one provider" + log.Error(nil, msg) + resp.SetStatus(runtimehooksv1.ResponseStatusFailure) + resp.SetMessage(msg) + return } - // There's a 1:N mapping of infra to CSI providers. The user chooses the provider. - for _, provider := range csiProviders.Providers { + if err := validateDefaultStorage(csi); err != nil { + log.Error(err, "") + resp.SetStatus(runtimehooksv1.ResponseStatusFailure) + resp.SetMessage(err.Error()) + return + } + + // There's a 1:N mapping of infra to CSI providers. The user chooses the providers. + for _, provider := range csi.Providers { handler, ok := c.ProviderHandler[provider.Name] if !ok { - log.V(4).Info( - fmt.Sprintf( - "Skipping CSI handler, for provider given in %s. Provider handler not given.", - provider.Name, - ), + log.Error( + nil, + "CSI provider is unknown", + "name", + provider.Name, + ) + resp.SetStatus(runtimehooksv1.ResponseStatusFailure) + resp.SetMessage( + fmt.Sprintf("CSI provider %q is unknown", provider.Name), ) - continue + return } log.Info(fmt.Sprintf("Creating CSI provider %s", provider.Name)) err = handler.Apply( ctx, provider, - csiProviders.DefaultStorage, + csi.DefaultStorage, req, log, ) @@ -135,7 +134,7 @@ func (c *CSIHandler) AfterControlPlaneInitialized( log.Error( err, fmt.Sprintf( - "failed to delpoy %s CSI driver", + "failed to deploy %s CSI driver", provider.Name, ), ) @@ -149,3 +148,30 @@ func (c *CSIHandler) AfterControlPlaneInitialized( } } } + +func validateDefaultStorage(csi v1alpha1.CSI) error { + // Verify that the default storage references a defined provider, and one of the + // storage class configs for that provider. + { + storageClassConfigsByProviderName := map[string][]v1alpha1.StorageClassConfig{} + for _, provider := range csi.Providers { + storageClassConfigsByProviderName[provider.Name] = provider.StorageClassConfig + } + configs, ok := storageClassConfigsByProviderName[csi.DefaultStorage.ProviderName] + if !ok { + return fmt.Errorf("the DefaultStorage Provider name must be the name of a configured provider") + } + defaultStorageClassConfigNameInProvider := false + for _, config := range configs { + if csi.DefaultStorage.StorageClassConfigName == config.Name { + defaultStorageClassConfigNameInProvider = true + break + } + } + if !defaultStorageClassConfigNameInProvider { + //nolint:lll // Long message. + return fmt.Errorf("the DefaultStorage StorageClassConfig name must be the name of a StorageClassConfig for the default provider") + } + } + return nil +} diff --git a/pkg/handlers/generic/lifecycle/csi/handler_test.go b/pkg/handlers/generic/lifecycle/csi/handler_test.go new file mode 100644 index 000000000..4d26f502a --- /dev/null +++ b/pkg/handlers/generic/lifecycle/csi/handler_test.go @@ -0,0 +1,230 @@ +// Copyright 2024 D2iQ, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package csi + +import ( + "context" + "fmt" + "testing" + + "github.com/go-logr/logr" + "github.com/google/go-cmp/cmp" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1" + "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/variables" +) + +type fakeCSIProvider struct { + returnedErr error +} + +func (p *fakeCSIProvider) Apply( + ctx context.Context, + provider v1alpha1.CSIProvider, + defaultStorageConfig v1alpha1.DefaultStorage, + req *runtimehooksv1.AfterControlPlaneInitializedRequest, + log logr.Logger, +) error { + return p.returnedErr +} + +var testProviderHandlers = map[string]CSIProvider{ + "test1": &fakeCSIProvider{}, + "test2": &fakeCSIProvider{}, + "broken": &fakeCSIProvider{ + returnedErr: fmt.Errorf("fake error"), + }, +} + +func testReq(csi *v1alpha1.CSI) (*runtimehooksv1.AfterControlPlaneInitializedRequest, error) { + cv, err := variables.MarshalToClusterVariable( + "clusterConfig", + &v1alpha1.GenericClusterConfigSpec{ + Addons: &v1alpha1.Addons{ + CSIProviders: csi, + }, + }, + ) + if err != nil { + return nil, err + } + + return &runtimehooksv1.AfterControlPlaneInitializedRequest{ + Cluster: clusterv1.Cluster{ + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{ + Variables: []clusterv1.ClusterVariable{ + *cv, + }, + }, + }, + }, + }, nil +} + +func Test_AfterControlPlaneInitialized(t *testing.T) { + tests := []struct { + name string + csi *v1alpha1.CSI + wantStatus runtimehooksv1.ResponseStatus + }{ + { + name: "csi variable is optional", + csi: nil, + wantStatus: runtimehooksv1.ResponseStatusSuccess, + }, + { + name: "if csi variable set, must set at least one provider", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{}, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "example", + StorageClassConfigName: "example", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusFailure, + }, + { + name: "default storage Provider name must be the name of a set provider", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{ + { + Name: "test1", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "test1", + }, + }, + }, + }, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "not-test1", + StorageClassConfigName: "test1", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusFailure, + }, + { + name: "default storage StorageClassConfig name must be the name of a StorageClassConfig of the default provider", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{ + { + Name: "test1", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "test1", + }, + }, + }, + { + Name: "test2", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "test2", + }, + }, + }, + }, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "test1", + StorageClassConfigName: "test2", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusFailure, + }, + { + name: "csi provider is unknown", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{ + { + Name: "not-test1-or-test2", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "not-test1-or-test2", + }, + }, + }, + }, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "not-test1-or-test2", + StorageClassConfigName: "not-test1-or-test2", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusFailure, + }, + { + name: "valid csi configuration", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{ + { + Name: "test1", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "test1", + }, + }, + }, + { + Name: "test2", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "test2", + }, + }, + }, + }, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "test2", + StorageClassConfigName: "test2", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusSuccess, + }, + { + name: "csi handler fails to apply", + csi: &v1alpha1.CSI{ + Providers: []v1alpha1.CSIProvider{ + { + Name: "broken", + StorageClassConfig: []v1alpha1.StorageClassConfig{ + { + Name: "broken", + }, + }, + }, + }, + DefaultStorage: v1alpha1.DefaultStorage{ + ProviderName: "broken", + StorageClassConfigName: "broken", + }, + }, + wantStatus: runtimehooksv1.ResponseStatusFailure, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + client := fake.NewClientBuilder().Build() + handler := New(client, testProviderHandlers) + resp := &runtimehooksv1.AfterControlPlaneInitializedResponse{} + + req, err := testReq(tt.csi) + if err != nil { + t.Fatalf("failed to create test request: %s", err) + } + + handler.AfterControlPlaneInitialized(ctx, req, resp) + if diff := cmp.Diff(tt.wantStatus, resp.Status); diff != "" { + t.Errorf( + "response Status mismatch (-want +got):\n%s. Message: %s", + diff, + resp.Message, + ) + } + }) + } +} diff --git a/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go b/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go index dcc17b0a7..b9edce659 100644 --- a/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go @@ -80,7 +80,7 @@ func New( func (n *NutanixCSI) Apply( ctx context.Context, provider v1alpha1.CSIProvider, - defaultStorageConfig *v1alpha1.DefaultStorage, + defaultStorageConfig v1alpha1.DefaultStorage, req *runtimehooksv1.AfterControlPlaneInitializedRequest, log logr.Logger, ) error { diff --git a/pkg/handlers/generic/lifecycle/utils/scs.go b/pkg/handlers/generic/lifecycle/utils/scs.go index b9b372e24..d6ae52123 100644 --- a/pkg/handlers/generic/lifecycle/utils/scs.go +++ b/pkg/handlers/generic/lifecycle/utils/scs.go @@ -63,7 +63,7 @@ func CreateStorageClassOnRemote( cl ctrlclient.Client, configs []v1alpha1.StorageClassConfig, cluster *clusterv1.Cluster, - defaultStorageConfig *v1alpha1.DefaultStorage, + defaultStorageConfig v1alpha1.DefaultStorage, csiProvider string, provisioner v1alpha1.StorageProvisioner, defaultParameters map[string]string,