Skip to content

Commit f091343

Browse files
dlipovetskydkoshkinjimmidyson
authored
fix: Correct the CSI handler logic (#603)
**What problem does this PR solve?**: - Return an error when the CSI variable is defined, but the Providers list is empty. - Return an error when the CSI provider is defined, but its StorageClassConfig list is empty. - Return an error when any CSI provider is unknown. - Return an error when DefaultStorage is empty. - Return an error when the DefaultStorage Provider Name does not match a set provider. - Return an error when the DefaultStorage StorageClassConfig Name does not match the name of a StorageClassConfig for the default provider. Adds unit tests to verify this logic, and more. Covers the majority of the generic CSI handler `AfterControlPlaneInitialized` hook. **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> Adds a unit test; I think this is the first unit test in CAREN to exercise a handler request-response flow. **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. --> I noticed these issues as I was studying the CSI handler in order to write the ServiceLoadbalancer handler. I'll be adding a unit test to #592 as well. --------- Co-authored-by: Dimitri Koshkin <[email protected]> Co-authored-by: Jimmi Dyson <[email protected]>
1 parent 9aeb638 commit f091343

File tree

11 files changed

+354
-50
lines changed

11 files changed

+354
-50
lines changed

‎api/v1alpha1/addon_types.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,12 @@ type DefaultStorage struct {
120120
}
121121

122122
type CSI struct {
123-
// +kubebuilder:validation:Optional
124-
Providers []CSIProvider `json:"providers,omitempty"`
123+
// +kubebuilder:validation:MinItems=1
124+
// +kubebuilder:validation:Required
125+
Providers []CSIProvider `json:"providers"`
125126

126-
// +kubebuilder:validation:Optional
127-
DefaultStorage *DefaultStorage `json:"defaultStorage,omitempty"`
127+
// +kubebuilder:validation:Required
128+
DefaultStorage DefaultStorage `json:"defaultStorage"`
128129
}
129130

130131
type CSIProvider struct {
@@ -133,8 +134,9 @@ type CSIProvider struct {
133134
// +kubebuilder:validation:Enum=aws-ebs;nutanix
134135
Name string `json:"name"`
135136

137+
// StorageClassConfig is a list of storage class configurations for this CSI provider.
136138
// +kubebuilder:validation:Optional
137-
StorageClassConfig []StorageClassConfig `json:"storageClassConfig,omitempty"`
139+
StorageClassConfig []StorageClassConfig `json:"storageClassConfig"`
138140

139141
// Addon strategy used to deploy the CSI provider to the workload cluster.
140142
// +kubebuilder:validation:Required

‎api/v1alpha1/crds/caren.nutanix.com_awsclusterconfigs.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ spec:
139139
- nutanix
140140
type: string
141141
storageClassConfig:
142+
description: StorageClassConfig is a list of storage
143+
class configurations for this CSI provider.
142144
items:
143145
properties:
144146
allowExpansion:
@@ -189,7 +191,11 @@ spec:
189191
- name
190192
- strategy
191193
type: object
194+
minItems: 1
192195
type: array
196+
required:
197+
- defaultStorage
198+
- providers
193199
type: object
194200
nfd:
195201
description: NFD tells us to enable or disable the node feature

‎api/v1alpha1/crds/caren.nutanix.com_dockerclusterconfigs.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ spec:
140140
- nutanix
141141
type: string
142142
storageClassConfig:
143+
description: StorageClassConfig is a list of storage
144+
class configurations for this CSI provider.
143145
items:
144146
properties:
145147
allowExpansion:
@@ -190,7 +192,11 @@ spec:
190192
- name
191193
- strategy
192194
type: object
195+
minItems: 1
193196
type: array
197+
required:
198+
- defaultStorage
199+
- providers
194200
type: object
195201
nfd:
196202
description: NFD tells us to enable or disable the node feature

‎api/v1alpha1/crds/caren.nutanix.com_nutanixclusterconfigs.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ spec:
140140
- nutanix
141141
type: string
142142
storageClassConfig:
143+
description: StorageClassConfig is a list of storage
144+
class configurations for this CSI provider.
143145
items:
144146
properties:
145147
allowExpansion:
@@ -190,7 +192,11 @@ spec:
190192
- name
191193
- strategy
192194
type: object
195+
minItems: 1
193196
type: array
197+
required:
198+
- defaultStorage
199+
- providers
194200
type: object
195201
nfd:
196202
description: NFD tells us to enable or disable the node feature

‎api/v1alpha1/zz_generated.deepcopy.go

Lines changed: 1 addition & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎common/pkg/capi/clustertopology/variables/json.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// Copyright 2024 D2iQ, Inc. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package variables
5+
6+
import (
7+
"encoding/json"
8+
"fmt"
9+
10+
v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
11+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
12+
)
13+
14+
func MarshalToClusterVariable[T any](name string, obj T) (*clusterv1.ClusterVariable, error) {
15+
marshaled, err := json.Marshal(obj)
16+
if err != nil {
17+
return nil, fmt.Errorf("failed to marshal variable value %q: %w", name, err)
18+
}
19+
return &clusterv1.ClusterVariable{
20+
Name: name,
21+
Value: v1.JSON{Raw: marshaled},
22+
}, nil
23+
}
24+
25+
func UnmarshalClusterVariable[T any](clusterVariable *clusterv1.ClusterVariable, obj *T) error {
26+
err := json.Unmarshal(clusterVariable.Value.Raw, obj)
27+
if err != nil {
28+
return fmt.Errorf("error unmarshalling variable: %w", err)
29+
}
30+
31+
return nil
32+
}

‎pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func New(
5858
func (a *AWSEBS) Apply(
5959
ctx context.Context,
6060
provider v1alpha1.CSIProvider,
61-
defaultStorageConfig *v1alpha1.DefaultStorage,
61+
defaultStorageConfig v1alpha1.DefaultStorage,
6262
req *runtimehooksv1.AfterControlPlaneInitializedRequest,
6363
_ logr.Logger,
6464
) error {

‎pkg/handlers/generic/lifecycle/csi/handler.go

Lines changed: 63 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ type CSIProvider interface {
2727
Apply(
2828
context.Context,
2929
v1alpha1.CSIProvider,
30-
*v1alpha1.DefaultStorage,
30+
v1alpha1.DefaultStorage,
3131
*runtimehooksv1.AfterControlPlaneInitializedRequest,
3232
logr.Logger,
3333
) error
@@ -74,68 +74,67 @@ func (c *CSIHandler) AfterControlPlaneInitialized(
7474
)
7575
varMap := variables.ClusterVariablesToVariablesMap(req.Cluster.Spec.Topology.Variables)
7676
resp.SetStatus(runtimehooksv1.ResponseStatusSuccess)
77-
csiProviders, err := variables.Get[v1alpha1.CSI](
77+
csi, err := variables.Get[v1alpha1.CSI](
7878
varMap,
7979
c.variableName,
8080
c.variablePath...)
8181
if err != nil {
82-
if variables.IsNotFoundError(err) ||
83-
csiProviders.Providers == nil ||
84-
len(csiProviders.Providers) == 0 {
85-
log.V(4).Info(
86-
fmt.Sprintf(
87-
"Skipping CSI handler, no providers given in %v",
88-
csiProviders,
89-
),
90-
)
82+
if variables.IsNotFoundError(err) {
83+
log.Info("Skipping CSI handler, the cluster does not define the CSI variable")
9184
return
9285
}
93-
log.Error(
94-
err,
95-
"failed to read CSI provider from cluster definition",
96-
)
86+
msg := "failed to read the CSI variable from the cluster"
87+
log.Error(err, msg)
9788
resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
98-
resp.SetMessage(
99-
fmt.Sprintf("failed to read CSI provider from cluster definition: %v",
100-
err,
101-
),
102-
)
89+
resp.SetMessage(fmt.Sprintf("%s: %v", msg, err))
10390
return
10491
}
105-
if len(csiProviders.Providers) == 1 &&
106-
len(csiProviders.Providers[0].StorageClassConfig) == 1 &&
107-
csiProviders.DefaultStorage == nil {
108-
csiProviders.DefaultStorage = &v1alpha1.DefaultStorage{
109-
ProviderName: csiProviders.Providers[0].Name,
110-
StorageClassConfigName: csiProviders.Providers[0].StorageClassConfig[0].Name,
111-
}
92+
93+
// This is defensive, because the API validation requires at least one provider.
94+
if len(csi.Providers) == 0 {
95+
msg := "The list of CSI providers must include at least one provider"
96+
log.Error(nil, msg)
97+
resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
98+
resp.SetMessage(msg)
99+
return
112100
}
113101

114-
// There's a 1:N mapping of infra to CSI providers. The user chooses the provider.
115-
for _, provider := range csiProviders.Providers {
102+
if err := validateDefaultStorage(csi); err != nil {
103+
log.Error(err, "")
104+
resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
105+
resp.SetMessage(err.Error())
106+
return
107+
}
108+
109+
// There's a 1:N mapping of infra to CSI providers. The user chooses the providers.
110+
for _, provider := range csi.Providers {
116111
handler, ok := c.ProviderHandler[provider.Name]
117112
if !ok {
118-
log.V(4).Info(
119-
fmt.Sprintf(
120-
"Skipping CSI handler, for provider given in %s. Provider handler not given.",
121-
provider.Name,
122-
),
113+
log.Error(
114+
nil,
115+
"CSI provider is unknown",
116+
"name",
117+
provider.Name,
118+
)
119+
resp.SetStatus(runtimehooksv1.ResponseStatusFailure)
120+
resp.SetMessage(
121+
fmt.Sprintf("CSI provider %q is unknown", provider.Name),
123122
)
124-
continue
123+
return
125124
}
126125
log.Info(fmt.Sprintf("Creating CSI provider %s", provider.Name))
127126
err = handler.Apply(
128127
ctx,
129128
provider,
130-
csiProviders.DefaultStorage,
129+
csi.DefaultStorage,
131130
req,
132131
log,
133132
)
134133
if err != nil {
135134
log.Error(
136135
err,
137136
fmt.Sprintf(
138-
"failed to delpoy %s CSI driver",
137+
"failed to deploy %s CSI driver",
139138
provider.Name,
140139
),
141140
)
@@ -149,3 +148,30 @@ func (c *CSIHandler) AfterControlPlaneInitialized(
149148
}
150149
}
151150
}
151+
152+
func validateDefaultStorage(csi v1alpha1.CSI) error {
153+
// Verify that the default storage references a defined provider, and one of the
154+
// storage class configs for that provider.
155+
{
156+
storageClassConfigsByProviderName := map[string][]v1alpha1.StorageClassConfig{}
157+
for _, provider := range csi.Providers {
158+
storageClassConfigsByProviderName[provider.Name] = provider.StorageClassConfig
159+
}
160+
configs, ok := storageClassConfigsByProviderName[csi.DefaultStorage.ProviderName]
161+
if !ok {
162+
return fmt.Errorf("the DefaultStorage Provider name must be the name of a configured provider")
163+
}
164+
defaultStorageClassConfigNameInProvider := false
165+
for _, config := range configs {
166+
if csi.DefaultStorage.StorageClassConfigName == config.Name {
167+
defaultStorageClassConfigNameInProvider = true
168+
break
169+
}
170+
}
171+
if !defaultStorageClassConfigNameInProvider {
172+
//nolint:lll // Long message.
173+
return fmt.Errorf("the DefaultStorage StorageClassConfig name must be the name of a StorageClassConfig for the default provider")
174+
}
175+
}
176+
return nil
177+
}

0 commit comments

Comments
 (0)