Skip to content

Commit ac54929

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 ac54929

File tree

2 files changed

+102
-49
lines changed

2 files changed

+102
-49
lines changed

‎pkg/webhook/cluster/nutanix_validator.go‎

Lines changed: 75 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,8 @@ func validateTopologyFailureDomainConfig(
117117
return fldErrs.ToAggregate()
118118
}
119119

120-
// validateControlPlaneFailureDomainConfig validates that either failureDomains are set,
121-
// or cluster and subnets are set with machineDetails, for control plane.
120+
// validateControlPlaneFailureDomainConfig validates XOR behavior: either failureDomains are set
121+
// OR cluster and subnets are set with machineDetails, but not both, for control plane.
122122
func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterConfigSpec) field.ErrorList {
123123
fldErrs := field.ErrorList{}
124124
fldPath := field.NewPath(
@@ -132,31 +132,54 @@ func validateControlPlaneFailureDomainConfig(clusterConfig *variables.ClusterCon
132132
"machineDetails",
133133
)
134134

135-
if clusterConfig.ControlPlane != nil &&
136-
clusterConfig.ControlPlane.Nutanix != nil &&
137-
len(clusterConfig.ControlPlane.Nutanix.FailureDomains) == 0 {
135+
if clusterConfig.ControlPlane != nil && clusterConfig.ControlPlane.Nutanix != nil {
138136
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-
}
137+
hasFailureDomains := len(clusterConfig.ControlPlane.Nutanix.FailureDomains) > 0
138+
hasCluster := machineDetails.Cluster != nil &&
139+
(machineDetails.Cluster.IsName() || machineDetails.Cluster.IsUUID())
140+
hasSubnets := len(machineDetails.Subnets) > 0
141+
142+
if !hasFailureDomains {
143+
// No failure domains -> cluster/subnets MUST be set
144+
if !hasCluster {
145+
fldErrs = append(fldErrs, field.Required(
146+
fldPath.Child("cluster"),
147+
"\"cluster\" must be set when failureDomains are not configured.",
148+
))
149+
}
150+
151+
if !hasSubnets {
152+
fldErrs = append(fldErrs, field.Required(
153+
fldPath.Child("subnets"),
154+
"\"subnets\" must be set when failureDomains are not configured.",
155+
))
156+
}
157+
} else {
158+
// Failure domains present -> cluster/subnets MUST NOT be set
159+
if hasCluster {
160+
fldErrs = append(fldErrs, field.Forbidden(
161+
fldPath.Child("cluster"),
162+
"\"cluster\" must not be set when failureDomains are configured.",
163+
))
164+
}
145165

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-
))
166+
if hasSubnets {
167+
fldErrs = append(fldErrs, field.Forbidden(
168+
fldPath.Child("subnets"),
169+
"\"subnets\" must not be set when failureDomains are configured.",
170+
))
171+
}
151172
}
152173
}
153174

154175
return fldErrs
155176
}
156177

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 {
178+
// validateWorkerFailureDomainConfig validates XOR behavior: either failureDomain is set
179+
// OR cluster and subnets are set with machineDetails, but not both, for workers.
180+
func validateWorkerFailureDomainConfig(
181+
cluster *clusterv1.Cluster,
182+
) field.ErrorList {
160183
fldErrs := field.ErrorList{}
161184
workerConfigVarPath := field.NewPath("spec", "topology", "variables", "workerConfig")
162185
workerConfigMDVarOverridePath := field.NewPath(
@@ -179,10 +202,7 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
179202
if cluster.Spec.Topology.Workers != nil {
180203
for i := range cluster.Spec.Topology.Workers.MachineDeployments {
181204
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-
}
205+
hasFailureDomain := md.FailureDomain != nil && *md.FailureDomain != ""
186206

187207
// Get the machineDetails from the overrides variable "workerConfig" if it is configured,
188208
// otherwise use the defaultWorkerConfig if it is configured.
@@ -211,18 +231,38 @@ func validateWorkerFailureDomainConfig(cluster *clusterv1.Cluster) field.ErrorLi
211231
}
212232

213233
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-
))
234+
hasCluster := machineDetails.Cluster != nil &&
235+
(machineDetails.Cluster.IsName() || machineDetails.Cluster.IsUUID())
236+
hasSubnets := len(machineDetails.Subnets) > 0
237+
238+
if !hasFailureDomain {
239+
// No failure domain -> cluster/subnets MUST be set
240+
if !hasCluster {
241+
fldErrs = append(fldErrs, field.Required(
242+
wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
243+
"\"cluster\" must be set when failureDomain is not configured.",
244+
))
245+
}
246+
if !hasSubnets {
247+
fldErrs = append(fldErrs, field.Required(
248+
wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
249+
"\"subnets\" must be set when failureDomain is not configured.",
250+
))
251+
}
252+
} else {
253+
// Failure domain present -> cluster/subnets MUST NOT be set
254+
if hasCluster {
255+
fldErrs = append(fldErrs, field.Forbidden(
256+
wcfgPath.Child("value", "nutanix", "machineDetails", "cluster"),
257+
"\"cluster\" must not be set when failureDomain is configured.",
258+
))
259+
}
260+
if hasSubnets {
261+
fldErrs = append(fldErrs, field.Forbidden(
262+
wcfgPath.Child("value", "nutanix", "machineDetails", "subnets"),
263+
"\"subnets\" must not be set when failureDomain is configured.",
264+
))
265+
}
226266
}
227267
}
228268
}

‎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)