fix: Correct the CSI handler logic

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

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

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

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

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
+}

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 {

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,68 +74,107 @@ 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",
+			"failed to read the CSI variable from the cluster",
 		)
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
 		resp.SetMessage(
-			fmt.Sprintf("failed to read CSI provider from cluster definition: %v",
+			fmt.Sprintf("failed to read the CSI variable from the cluster: %v",
 				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 {
+		log.Error(
+			err,
+			"The list of CSI providers must include at least one provider.",
+		)
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage("the list of CSI providers must include at least one provider")
+		return
+	}
+
+	// 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 {
+			log.Error(
+				err,
+				"The default Storage provider name must be the name of a chosen default provider.",
+			)
+			resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+			resp.SetMessage(
+				"The default Storage provider name must be the name of a chosen provider",
+			)
+			return
+		}
+		defaultStorageClassConfigNameInProvider := false
+		for _, config := range configs {
+			if csi.DefaultStorage.StorageClassConfigName == config.Name {
+				defaultStorageClassConfigNameInProvider = true
+				break
+			}
+		}
+		if !defaultStorageClassConfigNameInProvider {
+			log.Error(
+				err,
+				"The default Storage StrorageClassConfig name must be the name of a StrorageClassConfig of the default provider.",
+			)
+			resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+			resp.SetMessage(
+				"The default Storage StrorageClassConfig name must be the name of a StrorageClassConfig of the default provider.",
+			)
+			return
 		}
 	}
 
-	// There's a 1:N mapping of infra to CSI providers. The user chooses the provider.
-	for _, provider := range csiProviders.Providers {
+	// 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(
+				err,
+				"CSI provider is unknown",
+				"name",
+				provider.Name,
 			)
-			continue
+			resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+			resp.SetMessage(
+				fmt.Sprintf("CSI provider %q is unknown", provider.Name),
+			)
+			return
 		}
 		log.Info(fmt.Sprintf("Creating CSI provider %s", provider.Name))
 		err = handler.Apply(
 			ctx,
 			provider,
-			csiProviders.DefaultStorage,
+			csi.DefaultStorage,
 			req,
 			log,
 		)
 		if err != nil {
 			log.Error(
 				err,
 				fmt.Sprintf(
-					"failed to delpoy %s CSI driver",
+					"failed to deploy %s CSI driver",
 					provider.Name,
 				),
 			)