Skip to content

Commit b6e27c7

Browse files
thunderboltsidthunderboltsid
committed
fix(webhook): Update cluster webhook to validate failure domains
Use XOR instead of OR for cluster and subnet presence check against failure domains on control plane and worker overrides.
1 parent 94671c9 commit b6e27c7

File tree

3 files changed

+118
-52
lines changed

3 files changed

+118
-52
lines changed

‎api/v1alpha1/nutanix_node_types.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,12 @@ type NutanixMachineDetails struct {
8484
// +kubebuilder:validation:Optional
8585
GPUs []capxv1.NutanixGPU `json:"gpus,omitempty"`
8686
}
87+
88+
// HasClusterAndSubnets checks if cluster and subnets are configured in the machine details.
89+
// It returns (hasCluster, hasSubnets).
90+
func (md *NutanixMachineDetails) HasClusterAndSubnets() (hasCluster, hasSubnets bool) {
91+
hasCluster = md.Cluster != nil &&
92+
(md.Cluster.IsName() || md.Cluster.IsUUID())
93+
hasSubnets = len(md.Subnets) > 0
94+
return hasCluster, hasSubnets
95+
}

‎pkg/webhook/cluster/nutanix_validator.go‎

Lines changed: 82 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,60 @@ func (a *nutanixValidator) validate(
9797
return admission.Allowed("")
9898
}
9999

100+
// validateFailureDomainXORMachineDetails validates the XOR behavior between failure domains
101+
// and cluster/subnets configuration. Either failure domains are set OR cluster and subnets
102+
// are set with machineDetails, but not both.
103+
func validateFailureDomainXORMachineDetails(
104+
fldPath *field.Path,
105+
hasFailureDomains bool,
106+
machineDetails *v1alpha1.NutanixMachineDetails,
107+
failureDomainTerm string, // "failureDomains" for control plane, "failureDomain" for workers
108+
) field.ErrorList {
109+
fldErrs := field.ErrorList{}
110+
111+
// Determine the correct verb based on singular/plural
112+
verb := "is"
113+
if failureDomainTerm == "failureDomains" {
114+
verb = "are"
115+
}
116+
117+
hasCluster, hasSubnets := machineDetails.HasClusterAndSubnets()
118+
119+
if !hasFailureDomains {
120+
// No failure domains -> cluster/subnets MUST be set
121+
if !hasCluster {
122+
fldErrs = append(fldErrs, field.Required(
123+
fldPath.Child("cluster"),
124+
fmt.Sprintf("\"cluster\" must be set when %s %s not configured.", failureDomainTerm, verb),
125+
))
126+
}
127+
128+
if !hasSubnets {
129+
fldErrs = append(fldErrs, field.Required(
130+
fldPath.Child("subnets"),
131+
fmt.Sprintf("\"subnets\" must be set when %s %s not configured.", failureDomainTerm, verb),
132+
))
133+
}
134+
} else {
135+
// Failure domains present -> cluster/subnets MUST NOT be set
136+
if hasCluster {
137+
fldErrs = append(fldErrs, field.Forbidden(
138+
fldPath.Child("cluster"),
139+
fmt.Sprintf("\"cluster\" must not be set when %s %s configured.", failureDomainTerm, verb),
140+
))
141+
}
142+
143+
if hasSubnets {
144+
fldErrs = append(fldErrs, field.Forbidden(
145+
fldPath.Child("subnets"),
146+
fmt.Sprintf("\"subnets\" must not be set when %s %s configured.", failureDomainTerm, verb),
147+
))
148+
}
149+
}
150+
151+
return fldErrs
152+
}
153+
100154
// validateTopologyFailureDomainConfig validates the failure domain related configuration in cluster topology.
101155
func validateTopologyFailureDomainConfig(
102156
clusterConfig *variables.ClusterConfigSpec,
@@ -117,8 +171,8 @@ func validateTopologyFailureDomainConfig(
117171
return fldErrs.ToAggregate()
118172
}
119173

120-
// validateControlPlaneFailureDomainConfig validates that either failureDomains are set,
121-
// or cluster and subnets are set with machineDetails, for control plane.
174+
// validateControlPlaneFailureDomainConfig validates XOR behavior: either failureDomains are set
175+
// OR cluster and subnets are set with machineDetails, but not both, for control plane.
122176
func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterConfigSpec) field.ErrorList {
123177
fldErrs := field.ErrorList{}
124178
fldPath := field.NewPath(
@@ -132,31 +186,28 @@ func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterCon
132186
"machineDetails",
133187
)
134188

135-
if clusterConfig.ControlPlane != nil &&
136-
clusterConfig.ControlPlane.Nutanix != nil &&
137-
len(clusterConfig.ControlPlane.Nutanix.FailureDomains) == 0 {
189+
if clusterConfig.ControlPlane != nil && clusterConfig.ControlPlane.Nutanix != nil {
138190
machineDetails := clusterConfig.ControlPlane.Nutanix.MachineDetails
139-
if machineDetails.Cluster == nil || (!machineDetails.Cluster.IsName() && !machineDetails.Cluster.IsUUID()) {
140-
fldErrs = append(fldErrs, field.Required(
141-
fldPath.Child("cluster"),
142-
"\"cluster\" must be set when failureDomains are not configured.",
143-
))
144-
}
145-
146-
if len(machineDetails.Subnets) == 0 {
147-
fldErrs = append(fldErrs, field.Required(
148-
fldPath.Child("subnets"),
149-
"\"subnets\" must be set when failureDomains are not configured.",
150-
))
151-
}
191+
hasFailureDomains := len(clusterConfig.ControlPlane.Nutanix.FailureDomains) > 0
192+
193+
fldErrs = append(
194+
fldErrs,
195+
validateFailureDomainXORMachineDetails(
196+
fldPath,
197+
hasFailureDomains,
198+
&machineDetails,
199+
"failureDomains",
200+
)...)
152201
}
153202

154203
return fldErrs
155204
}
156205

157-
// validateWorkerFailureDomainConfig validates either failureDomains is set,
158-
// or cluster and subnets are set with machineDetails, for workers.
159-
func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorList {
206+
// validateWorkerFailureDomainConfig validates XOR behavior: either failureDomain is set
207+
// OR cluster and subnets are set with machineDetails, but not both, for workers.
208+
func validateWorkerFailureDomainConfig(
209+
cluster *clusterv1.Cluster,
210+
) field.ErrorList {
160211
fldErrs := field.ErrorList{}
161212
workerConfigVarPath := field.NewPath("spec", "topology", "variables", "workerConfig")
162213
workerConfigMDVarOverridePath := field.NewPath(
@@ -179,10 +230,7 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
179230
if cluster.Spec.Topology.Workers != nil {
180231
for i := range cluster.Spec.Topology.Workers.MachineDeployments {
181232
md := cluster.Spec.Topology.Workers.MachineDeployments[i]
182-
if md.FailureDomain != nil && *md.FailureDomain != "" {
183-
// failureDomain is configured so we can skip checking the cluster and subnet in machineDetails
184-
continue
185-
}
233+
hasFailureDomain := md.FailureDomain != nil && *md.FailureDomain != ""
186234

187235
// Get the machineDetails from the overrides variable "workerConfig" if it is configured,
188236
// otherwise use the defaultWorkerConfig if it is configured.
@@ -211,19 +259,15 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
211259
}
212260

213261
machineDetails := workerConfig.Nutanix.MachineDetails
214-
if machineDetails.Cluster == nil ||
215-
(!machineDetails.Cluster.IsName() && !machineDetails.Cluster.IsUUID()) {
216-
fldErrs = append(fldErrs, field.Required(
217-
wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
218-
"\"cluster\" must be set when failureDomain is not configured.",
219-
))
220-
}
221-
if len(machineDetails.Subnets) == 0 {
222-
fldErrs = append(fldErrs, field.Required(
223-
wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
224-
"\"subnets\" must be set when failureDomain is not configured.",
225-
))
226-
}
262+
263+
fldErrs = append(
264+
fldErrs,
265+
validateFailureDomainXORMachineDetails(
266+
wcfgPath.Child("value", "nutanix", "machineDetails"),
267+
hasFailureDomain,
268+
&machineDetails,
269+
"failureDomain",
270+
)...)
227271
}
228272
}
229273

‎pkg/webhook/cluster/nutanix_validator_test.go‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -270,13 +270,6 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
270270
expectedErr: false,
271271
expectedErrMessages: nil,
272272
},
273-
{
274-
name: "controlPlane failureDomains configured and machineDetail cluster and subnets set",
275-
clusterConfig: fakeClusterConfigSpec(true, true, true),
276-
cluster: fakeCluster(t, true, false, false, workerConfigNone),
277-
expectedErr: false,
278-
expectedErrMessages: nil,
279-
},
280273
{
281274
name: "worker failureDomain not configured and missing 'cluster' in workerConfig overrides only",
282275
clusterConfig: fakeClusterConfigSpec(true, false, false),
@@ -312,13 +305,6 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
312305
expectedErr: false,
313306
expectedErrMessages: nil,
314307
},
315-
{
316-
name: "worker failureDomain configured and machineDetail cluster and subnets set in workerConfig overrides only", //nolint:lll // name is long.
317-
clusterConfig: fakeClusterConfigSpec(true, false, false),
318-
cluster: fakeCluster(t, true, true, true, workerConfigOverridesOnly),
319-
expectedErr: false,
320-
expectedErrMessages: nil,
321-
},
322308
{
323309
name: "worker failureDomain not configured and missing machineDetail 'cluster' and 'subnets' in workerConfig default only", //nolint:lll // name is long.
324310
clusterConfig: fakeClusterConfigSpec(true, false, false),
@@ -353,6 +339,33 @@ func TestValidateTopologyFailureDomainConfig(t *testing.T) {
353339
expectedErr: false,
354340
expectedErrMessages: nil,
355341
},
342+
{
343+
name: "controlPlane failureDomains configured with cluster/subnets set violates XOR",
344+
clusterConfig: fakeClusterConfigSpec(true, true, true),
345+
cluster: fakeCluster(t, true, false, false, workerConfigNone),
346+
expectedErr: true,
347+
expectedErrMessages: []string{
348+
"spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.cluster: Forbidden: \"cluster\" must not be set when failureDomains are configured.", //nolint:lll // Message is long.
349+
"spec.topology.variables.clusterConfig.value.controlPlane.nutanix.machineDetails.subnets: Forbidden: \"subnets\" must not be set when failureDomains are configured.", //nolint:lll // Message is long.
350+
},
351+
},
352+
{
353+
name: "worker failureDomain configured with cluster/subnets set in variables overrides violates XOR",
354+
clusterConfig: fakeClusterConfigSpec(true, false, false),
355+
cluster: fakeCluster(t, true, true, true, workerConfigOverridesOnly),
356+
expectedErr: true,
357+
expectedErrMessages: []string{
358+
"spec.topology.workers.machineDeployments.variables.overrides.workerConfig.value.nutanix.machineDetails.cluster: Forbidden: \"cluster\" must not be set when failureDomain is configured.", //nolint:lll // Message is long.
359+
"spec.topology.workers.machineDeployments.variables.overrides.workerConfig.value.nutanix.machineDetails.subnets: Forbidden: \"subnets\" must not be set when failureDomain is configured.", //nolint:lll // Message is long.
360+
},
361+
},
362+
{
363+
name: "default workerConfig variable with cluster/subnets set when control plane has failureDomains should be allowed", //nolint:lll // name is long.
364+
clusterConfig: fakeClusterConfigSpec(true, false, false),
365+
cluster: fakeCluster(t, false, true, true, workerConfigDefaultOnly),
366+
expectedErr: false,
367+
expectedErrMessages: nil,
368+
},
356369
}
357370

358371
for _, tc := range testcases {

0 commit comments

Comments
 (0)