fix: Remove providerName from defaultStorage config

jimmidyson · jimmidyson · commit 6527408b34f1 · 2024-06-02T09:17:52.000+01:00
This commit removes the `providerName` from `defaultStorage`
configuration, just requiring that the `storageClassConfigName` exists
for one of the configured providers. This commit also adds validation to
ensure that the storage class configs have unique names across
providers, current behaviour could have led to conflicts if the storage
class name was used in multiple provider configurations.

When adding the `local-path` CSI provisioner for Docker clusters, I
found that the `providerName` in default storage class config was
redundant and led to unnecessary duplication of values. Removing this
unnecessary field reduces the duplication from:

```
defaultStorage:
  providerName: local-path
  storageClassConfigName: local-path
providers:
  - name: local-path
    storageClassConfig:
      - name: local-path
    strategy: HelmAddon
```

To:

```
defaultStorage:
  storageClassConfigName: local-path
providers:
  - name: local-path
    storageClassConfig:
      - name: local-path
    strategy: HelmAddon
```

While this may seem like only a small change, a single line removed, it
is always good to keep APIs as terse as possible while still retaining
strict behaviour. I think this commit achieves that.
diff --git a/api/v1alpha1/addon_types.go b/api/v1alpha1/addon_types.go
@@ -110,11 +110,6 @@ type ClusterAutoscaler struct {
 }
 
 type DefaultStorage struct {
-	// Name of the CSI Provider for the default storage class.
-	// +kubebuilder:validation:Required
-	// +kubebuilder:validation:Enum=aws-ebs;nutanix;local-path
-	ProviderName string `json:"providerName"`
-
 	// Name of storage class config in any of the provider objects.
 	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:MinLength=1
diff --git a/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml
@@ -102,17 +102,11 @@ spec:
                       properties:
                         defaultStorage:
                           properties:
-                            providerName:
-                              description: Name of the CSI Provider for the default storage class.
-                              enum:
-                                - aws-ebs
-                              type: string
                             storageClassConfigName:
                               description: Name of storage class config in any of the provider objects.
                               minLength: 1
                               type: string
                           required:
-                            - providerName
                             - storageClassConfigName
                           type: object
                         providers:
diff --git a/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml
@@ -102,17 +102,11 @@ spec:
                       properties:
                         defaultStorage:
                           properties:
-                            providerName:
-                              description: Name of the CSI Provider for the default storage class.
-                              enum:
-                                - local-path
-                              type: string
                             storageClassConfigName:
                               description: Name of storage class config in any of the provider objects.
                               minLength: 1
                               type: string
                           required:
-                            - providerName
                             - storageClassConfigName
                           type: object
                         providers:
diff --git a/api/v1alpha1/crds/caren.nutanix.com_genericclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_genericclusterconfigs.yaml
@@ -116,21 +116,12 @@ spec:
                     properties:
                       defaultStorage:
                         properties:
-                          providerName:
-                            description: Name of the CSI Provider for the default
-                              storage class.
-                            enum:
-                            - aws-ebs
-                            - nutanix
-                            - local-path
-                            type: string
                           storageClassConfigName:
                             description: Name of storage class config in any of the
                               provider objects.
                             minLength: 1
                             type: string
                         required:
-                        - providerName
                         - storageClassConfigName
                         type: object
                       providers:
diff --git a/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml b/api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml
@@ -102,17 +102,11 @@ spec:
                       properties:
                         defaultStorage:
                           properties:
-                            providerName:
-                              description: Name of the CSI Provider for the default storage class.
-                              enum:
-                                - nutanix
-                              type: string
                             storageClassConfigName:
                               description: Name of storage class config in any of the provider objects.
                               minLength: 1
                               type: string
                           required:
-                            - providerName
                             - storageClassConfigName
                           type: object
                         providers:
diff --git a/examples/capi-quick-start/aws-cluster-calico-crs.yaml b/examples/capi-quick-start/aws-cluster-calico-crs.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: aws-ebs
               storageClassConfigName: aws-ebs
             providers:
             - name: aws-ebs
diff --git a/examples/capi-quick-start/aws-cluster-calico-helm-addon.yaml b/examples/capi-quick-start/aws-cluster-calico-helm-addon.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: aws-ebs
               storageClassConfigName: aws-ebs
             providers:
             - name: aws-ebs
diff --git a/examples/capi-quick-start/aws-cluster-cilium-crs.yaml b/examples/capi-quick-start/aws-cluster-cilium-crs.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: aws-ebs
               storageClassConfigName: aws-ebs
             providers:
             - name: aws-ebs
diff --git a/examples/capi-quick-start/aws-cluster-cilium-helm-addon.yaml b/examples/capi-quick-start/aws-cluster-cilium-helm-addon.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: aws-ebs
               storageClassConfigName: aws-ebs
             providers:
             - name: aws-ebs
diff --git a/examples/capi-quick-start/docker-cluster-calico-crs.yaml b/examples/capi-quick-start/docker-cluster-calico-crs.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: local-path
               storageClassConfigName: local-path
             providers:
             - name: local-path
diff --git a/examples/capi-quick-start/docker-cluster-calico-helm-addon.yaml b/examples/capi-quick-start/docker-cluster-calico-helm-addon.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: local-path
               storageClassConfigName: local-path
             providers:
             - name: local-path
diff --git a/examples/capi-quick-start/docker-cluster-cilium-crs.yaml b/examples/capi-quick-start/docker-cluster-cilium-crs.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: local-path
               storageClassConfigName: local-path
             providers:
             - name: local-path
diff --git a/examples/capi-quick-start/docker-cluster-cilium-helm-addon.yaml b/examples/capi-quick-start/docker-cluster-cilium-helm-addon.yaml
@@ -29,7 +29,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: local-path
               storageClassConfigName: local-path
             providers:
             - name: local-path
diff --git a/examples/capi-quick-start/nutanix-cluster-calico-crs.yaml b/examples/capi-quick-start/nutanix-cluster-calico-crs.yaml
@@ -74,7 +74,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: nutanix
               storageClassConfigName: nutanix-volume
             providers:
             - credentials:
diff --git a/examples/capi-quick-start/nutanix-cluster-calico-helm-addon.yaml b/examples/capi-quick-start/nutanix-cluster-calico-helm-addon.yaml
@@ -74,7 +74,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: nutanix
               storageClassConfigName: nutanix-volume
             providers:
             - credentials:
diff --git a/examples/capi-quick-start/nutanix-cluster-cilium-crs.yaml b/examples/capi-quick-start/nutanix-cluster-cilium-crs.yaml
@@ -74,7 +74,6 @@ spec:
             strategy: ClusterResourceSet
           csi:
             defaultStorage:
-              providerName: nutanix
               storageClassConfigName: nutanix-volume
             providers:
             - credentials:
diff --git a/examples/capi-quick-start/nutanix-cluster-cilium-helm-addon.yaml b/examples/capi-quick-start/nutanix-cluster-cilium-helm-addon.yaml
@@ -74,7 +74,6 @@ spec:
             strategy: HelmAddon
           csi:
             defaultStorage:
-              providerName: nutanix
               storageClassConfigName: nutanix-volume
             providers:
             - credentials:
diff --git a/go.mod b/go.mod
@@ -23,6 +23,7 @@ require (
 	github.com/onsi/gomega v1.33.1
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
+	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
 	gopkg.in/yaml.v2 v2.4.0
 	k8s.io/api v0.29.5
 	k8s.io/apiextensions-apiserver v0.29.5
@@ -128,7 +129,6 @@ require (
 	go.opentelemetry.io/otel/trace v1.22.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/crypto v0.23.0 // indirect
-	golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect
 	golang.org/x/mod v0.17.0 // indirect
 	golang.org/x/net v0.25.0 // indirect
 	golang.org/x/oauth2 v0.18.0 // indirect
diff --git a/hack/examples/patches/aws/csi.yaml b/hack/examples/patches/aws/csi.yaml
@@ -5,7 +5,6 @@
   path: "/spec/topology/variables/0/value/addons/csi"
   value:
     defaultStorage:
-      providerName: aws-ebs
       storageClassConfigName: aws-ebs
     providers:
       - name: aws-ebs
diff --git a/hack/examples/patches/docker/csi.yaml b/hack/examples/patches/docker/csi.yaml
@@ -5,7 +5,6 @@
   path: "/spec/topology/variables/0/value/addons/csi"
   value:
     defaultStorage:
-      providerName: local-path
       storageClassConfigName: local-path
     providers:
       - name: local-path
diff --git a/hack/examples/patches/nutanix/csi.yaml b/hack/examples/patches/nutanix/csi.yaml
@@ -5,7 +5,6 @@
   path: "/spec/topology/variables/0/value/addons/csi"
   value:
     defaultStorage:
-      providerName: nutanix
       storageClassConfigName: nutanix-volume
     providers:
       - name: nutanix
diff --git a/make/addons.mk b/make/addons.mk
@@ -61,15 +61,17 @@ update-addon.kube-vip: ; $(info $(M) updating kube-vip manifests)
 
 .PHONY: generate-helm-configmap
 generate-helm-configmap:
-	go run hack/tools/helm-cm/main.go -kustomize-directory="./hack/addons/kustomize" -output-file="./charts/cluster-api-runtime-extensions-nutanix/templates/helm-config.yaml"
+	go run hack/tools/helm-cm/main.go -kustomize-directory="./hack/addons/kustomize" \
+	  -output-file="./charts/cluster-api-runtime-extensions-nutanix/templates/helm-config.yaml"
 	./hack/addons/add-warning-helm-configmap.sh
 
 # Set only the supported CSI providers for each provider.
 .PHONY: configure-csi-providers.%
 configure-csi-providers.%: CSI_JSONPATH := .spec.versions[].schema.openAPIV3Schema.properties.spec.properties.addons.properties.csi.properties
 configure-csi-providers.%: ; $(info $(M) configuring csi providers for $*clusterconfigs)
 	gojq --yaml-input --yaml-output \
-	  '($(CSI_JSONPATH).providers.items.properties.name.enum, $(CSI_JSONPATH).defaultStorage.properties.providerName.enum) |= $(CSI_PROVIDERS_$(*))' \
+	  '$(CSI_JSONPATH).providers.items.properties.name.enum |= $(CSI_PROVIDERS_$(*))' \
 	  api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml > api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml.tmp
-	cat hack/license-header.yaml.txt <(echo ---) api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml.tmp > api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml
+	cat hack/license-header.yaml.txt <(echo ---) api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml.tmp \
+	  >api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml
 	rm api/v1alpha1/crds/caren.nutanix.com_$(*)clusterconfigs.yaml.tmp
diff --git a/pkg/handlers/generic/lifecycle/csi/handler.go b/pkg/handlers/generic/lifecycle/csi/handler.go
@@ -6,6 +6,7 @@ package csi
 import (
 	"context"
 	"fmt"
+	"slices"
 
 	"github.com/go-logr/logr"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
@@ -98,6 +99,13 @@ func (c *CSIHandler) AfterControlPlaneInitialized(
 		return
 	}
 
+	if err := validateUniqueStorageClassNames(csi); err != nil {
+		log.Error(err, "")
+		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
+		resp.SetMessage(err.Error())
+		return
+	}
+
 	if err := validateDefaultStorage(csi); err != nil {
 		log.Error(err, "")
 		resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
@@ -149,31 +157,37 @@ 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
+	// Verify that the default storage references a defined storage class config.
+	if !slices.ContainsFunc(csi.Providers, func(provider v1alpha1.CSIProvider) bool {
+		return slices.ContainsFunc(
+			provider.StorageClassConfig,
+			func(scc v1alpha1.StorageClassConfig) bool {
+				return scc.Name == csi.DefaultStorage.StorageClassConfigName
+			},
+		)
+	}) {
+		return fmt.Errorf(
+			"the DefaultStorage StorageClassConfig name must be the name of a configured StorageClassConfig",
+		)
+	}
+
+	return nil
+}
+
+func validateUniqueStorageClassNames(csi v1alpha1.CSI) error {
+	storageClassNames := make(map[string]string)
+	for _, provider := range csi.Providers {
+		for _, scc := range provider.StorageClassConfig {
+			if existingProviderDef, ok := storageClassNames[scc.Name]; ok {
+				return fmt.Errorf(
+					"storage class name %q must be unique and is already defined by provider %q",
+					scc.Name,
+					existingProviderDef,
+				)
 			}
-		}
-		if !defaultStorageClassConfigNameInProvider {
-			return fmt.Errorf(
-				"the DefaultStorage StorageClassConfig name must be the name of a StorageClassConfig for the default provider",
-			)
+			storageClassNames[scc.Name] = provider.Name
 		}
 	}
+
 	return nil
 }
diff --git a/pkg/handlers/generic/lifecycle/csi/handler_test.go b/pkg/handlers/generic/lifecycle/csi/handler_test.go
diff --git a/pkg/handlers/generic/lifecycle/utils/scs.go b/pkg/handlers/generic/lifecycle/utils/scs.go
