Skip to content

fix: OCPBUGS-86073: validate duplicate failureDomain names and topology (Nutanix, vSphere) #10561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
chdeshpa-hue wants to merge 1 commit into from
+125 −2
Closed

fix: OCPBUGS-86073: validate duplicate failureDomain names and topology (Nutanix, vSphere) #10561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion pkg/types/nutanix/validation/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package validation
import (
"fmt"
"regexp"
"sort"
"strings"

"k8s.io/apimachinery/pkg/util/validation/field"

Expand Down Expand Up @@ -103,11 +105,28 @@ func ValidatePlatform(p *nutanix.Platform, fldPath *field.Path, c *types.Install
if err != nil {
allErrs = append(allErrs, field.InternalError(fldPath.Child("failureDomain", "name"), fmt.Errorf("fail to compile the pattern %q: %w", pattern, err)))
} else {
for _, fd := range p.FailureDomains {
fdNames := make(map[string]int)
fdTopologies := make(map[string]string)

for idx, fd := range p.FailureDomains {
if !rexp.MatchString(fd.Name) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("failureDomain", "name"), fd.Name, fmt.Sprintf("failureDomain name should match the pattern %q.", pattern)))
}

if prevIdx, exists := fdNames[fd.Name]; exists {
allErrs = append(allErrs, field.Duplicate(fldPath.Child("failureDomains").Index(idx).Child("name"), fmt.Sprintf("failure domain name %q is already used by failureDomains[%d]", fd.Name, prevIdx)))
} else {
fdNames[fd.Name] = idx
}

topoKey := nutanixFailureDomainTopologyKey(fd)
if prevName, exists := fdTopologies[topoKey]; exists {
allErrs = append(allErrs, field.Invalid(fldPath.Child("failureDomains").Index(idx), fd.Name,
fmt.Sprintf("failure domain %q has identical topology (same prismElement and subnets) as %q; this provides no additional fault tolerance", fd.Name, prevName)))
} else {
fdTopologies[topoKey] = fd.Name
}

if fd.PrismElement.UUID == "" {
allErrs = append(allErrs, field.Required(fldPath.Child("failureDomain", "prismElement", "uuid"), "failureDomain prismElement uuid cannot be empty"))
}
Expand Down Expand Up @@ -153,6 +172,15 @@ func validateLoadBalancer(lbType configv1.PlatformLoadBalancerType) bool {
}
}

// nutanixFailureDomainTopologyKey builds a comparable key from the infrastructure-defining
// fields of a failure domain: Prism Element UUID and sorted subnet UUIDs.
func nutanixFailureDomainTopologyKey(fd nutanix.FailureDomain) string {
subnets := make([]string, len(fd.SubnetUUIDs))
copy(subnets, fd.SubnetUUIDs)
sort.Strings(subnets)
return fmt.Sprintf("pe=%s;subnets=%s", fd.PrismElement.UUID, strings.Join(subnets, ","))
}

// validateSubnets validates the input subnetUUIDs meet the configuration requirements.
func validateSubnets(fldPath *field.Path, subnetUUIDs []string) field.ErrorList {
var errs field.ErrorList
Expand Down
59 changes: 59 additions & 0 deletions pkg/types/nutanix/validation/platform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,65 @@ func TestValidatePlatform(t *testing.T) {
}(),
expectedError: `^test-path\.failureDomain\.dataSourceImages: Required value: failureDomain "fd-1": both the dataSourceImage's uuid and name are empty, you need to configure one\.$`,
},
{
name: "failureDomain with duplicate name",
platform: func() *nutanix.Platform {
p := validPlatform()
p.FailureDomains = []nutanix.FailureDomain{
{
Name: "fd-1",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid-1", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe-1", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid-1"},
},
{
Name: "fd-1",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid-2", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe-2", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid-2"},
},
}
return p
}(),
expectedError: `test-path\.failureDomains\[1\]\.name: Duplicate value: "failure domain name \\"fd-1\\" is already used by failureDomains\[0\]"`,
},
{
name: "failureDomain with duplicate topology same prismElement and subnet",
platform: func() *nutanix.Platform {
p := validPlatform()
p.FailureDomains = []nutanix.FailureDomain{
{
Name: "fd-1",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid"},
},
{
Name: "fd-2",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid"},
},
}
return p
}(),
expectedError: `test-path\.failureDomains\[1\]: Invalid value: "fd-2": failure domain "fd-2" has identical topology \(same prismElement and subnets\) as "fd-1"; this provides no additional fault tolerance`,
},
{
name: "valid failureDomain with different prismElements",
platform: func() *nutanix.Platform {
p := validPlatform()
p.FailureDomains = []nutanix.FailureDomain{
{
Name: "fd-1",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid-1", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe-1", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid"},
},
{
Name: "fd-2",
PrismElement: nutanix.PrismElement{UUID: "fd-pe-uuid-2", Endpoint: nutanix.PrismEndpoint{Address: "fd-pe-2", Port: 9440}},
SubnetUUIDs: []string{"fd-subnet-uuid"},
},
}
return p
}(),
},
{
name: "valid failureDomain",
platform: func() *nutanix.Platform {
Expand Down
26 changes: 26 additions & 0 deletions pkg/types/vsphere/validation/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net"
"path/filepath"
"regexp"
"sort"
"strings"

"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -163,6 +164,7 @@ func validateFailureDomains(p *vsphere.Platform, platformFldPath *field.Path, fl
var associatedVCenter *vsphere.VCenter

zoneNames := make(map[string]string)
fdTopologies := make(map[string]string)

for index, failureDomain := range p.FailureDomains {
if regionName, ok := zoneNames[failureDomain.Zone]; !ok {
Expand All @@ -171,6 +173,14 @@ func validateFailureDomains(p *vsphere.Platform, platformFldPath *field.Path, fl
allErrs = append(allErrs, field.Invalid(fldPath.Child("zone"), failureDomain.Zone, fmt.Sprintf("cannot be used more than once for the failure domain region %q", failureDomain.Region)))
}

topoKey := vsphereFailureDomainTopologyKey(failureDomain)
if prevName, exists := fdTopologies[topoKey]; exists {
allErrs = append(allErrs, field.Invalid(fldPath.Index(index), failureDomain.Name,
fmt.Sprintf("failure domain %q has identical topology (same server, datacenter, computeCluster, datastore, networks, resourcePool) as %q; this provides no additional fault tolerance", failureDomain.Name, prevName)))
} else {
fdTopologies[topoKey] = failureDomain.Name
}
Comment on lines +176 to +182
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Canonicalize topology paths before duplicate-key comparison.

Duplicate-topology detection uses raw computeCluster / datastore / resourcePool strings, so syntactically different-but-equivalent paths can bypass this check.

Proposed fix 
 func vsphereFailureDomainTopologyKey(fd vsphere.FailureDomain) string {
 	networks := make([]string, len(fd.Topology.Networks))
 	copy(networks, fd.Topology.Networks)
 	sort.Strings(networks)
+
+	normalizePath := func(p string) string {
+		if p == "" {
+			return ""
+		}
+		return filepath.Clean(p)
+	}
+
 	return fmt.Sprintf("server=%s;dc=%s;cluster=%s;ds=%s;nets=%s;rp=%s",
 		fd.Server,
 		fd.Topology.Datacenter,
-		fd.Topology.ComputeCluster,
-		fd.Topology.Datastore,
+		normalizePath(fd.Topology.ComputeCluster),
+		normalizePath(fd.Topology.Datastore),
 		strings.Join(networks, ","),
-		fd.Topology.ResourcePool)
+		normalizePath(fd.Topology.ResourcePool))
 }

Also applies to: 356-367

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/types/vsphere/validation/platform.go` around lines 176 - 182, The
duplicate-topology check uses raw strings so equivalent vSphere inventory paths
can slip through; modify vsphereFailureDomainTopologyKey (and the other
occurrence around lines 356-367) to canonicalize computeCluster, datastore,
resourcePool (and any other inventory path fields used to build topoKey) before
composing the topoKey: add or call a helper like canonicalizeVSpherePath(path
string) that normalizes trailing/leading slashes, resolves redundant segments,
and lowercases or otherwise normalizes case where appropriate, then use those
canonicalized values when populating fdTopologies and when comparing prevName;
update references to fdTopologies[topoKey] and the key construction in both
places (vsphereFailureDomainTopologyKey and its duplicate) so comparisons use
the canonical form.


if failureDomain.ZoneType == "" && failureDomain.RegionType == "" {
logrus.Debug("using the defaults regionType is Datacenter and zoneType is ComputeCluster")
}
Expand Down Expand Up @@ -340,6 +350,22 @@ func validateFailureDomains(p *vsphere.Platform, platformFldPath *field.Path, fl
return allErrs
}

// vsphereFailureDomainTopologyKey builds a comparable key from the infrastructure-defining
// fields of a failure domain topology: server, datacenter, compute cluster, datastore,
// networks, and resource pool.
func vsphereFailureDomainTopologyKey(fd vsphere.FailureDomain) string {
networks := make([]string, len(fd.Topology.Networks))
copy(networks, fd.Topology.Networks)
sort.Strings(networks)
return fmt.Sprintf("server=%s;dc=%s;cluster=%s;ds=%s;nets=%s;rp=%s",
fd.Server,
fd.Topology.Datacenter,
fd.Topology.ComputeCluster,
fd.Topology.Datastore,
strings.Join(networks, ","),
fd.Topology.ResourcePool)
}

// validateDiskType checks that the specified diskType is valid.
func validateDiskType(p *vsphere.Platform, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down
12 changes: 11 additions & 1 deletion pkg/types/vsphere/validation/platform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ func TestValidatePlatform(t *testing.T) {

for i := range p.FailureDomains {
p.FailureDomains[i].Topology.ComputeCluster = "/test-datacenter/host/HostClusterFolder/test-cluster"
p.FailureDomains[i].Topology.ResourcePool = "/test-datacenter/host/HostClusterFolder/test-cluster/Resources/test-resourcepool"
p.FailureDomains[i].Topology.ResourcePool = fmt.Sprintf("/test-datacenter/host/HostClusterFolder/test-cluster/Resources/test-resourcepool-%d", i)
p.FailureDomains[i].Topology.Datastore = "/test-datacenter/datastore/StorageFolder/test-datastore"
p.FailureDomains[i].Topology.Folder = "/test-datacenter/vm/VMFolder/test-folder"
}
Expand Down Expand Up @@ -552,6 +552,16 @@ func TestValidatePlatform(t *testing.T) {
}(),
expectedError: `test-path\.failureDomains\.name\[1\]: Duplicate value: "test-east-1a"`,
},
{
name: "Multi-zone platform failureDomain duplicate topology",
platform: func() *vsphere.Platform {
p := validPlatform()
p.FailureDomains[1].Topology = p.FailureDomains[0].Topology
p.FailureDomains[1].Server = p.FailureDomains[0].Server
return p
}(),
expectedError: `test-path\.failureDomains\[1\]: Invalid value: "test-east-2a": failure domain "test-east-2a" has identical topology .* as "test-east-1a"; this provides no additional fault tolerance`,
},
{
name: "Multi-zone platform failureDomain zone missing tag category",
platform: func() *vsphere.Platform {
Expand Down