Fix NSX-T LogicalSwitchUUID to DPVG lookup

A supported but very, very rare NSX-T configuration is for the same
LogicalSwitchUUID to be shared between multiple DPVGs in a Datacenter.
To support this, shortly before WCP 1.0 release, a change was made in
WCP and then ported over here that used a "all the hosts in the CCR"
check to determine which DVPG is the correct one to use. Ever since,
this check in WCP and here have been peculiar and the details as to
why mostly lost.

This is all host check strict in the sense that it will fail if a host
is in MM or otherwise disconnected. Fortunately, within a CCR, the
LogicalSwitchUUID to DVPG mapping is unique, so use that to simplify
the lookup logic.

The possibility of multiple DVPGs for each CCR will be a pain when we
start to do network placement.

Remove the NSX-T OpaqueNetwork support since that was only used pre-1.0
and never in a shipped release.

Author: bryanv

pkg/vmprovider/providers/vsphere/network/network_provider.go

@@ -19,6 +19,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/mo"
 	vimtypes "github.com/vmware/govmomi/vim25/types"
@@ -367,7 +368,7 @@ func (np *netOpNetworkProvider) getNetworkRef(ctx goctx.Context, networkType, ne
 	case VdsNetworkType:
 		return np.networkForPortGroupID(networkID)
 	case NsxtNetworkType:
-		return searchNsxtNetworkReference(ctx, np.finder, np.cluster, networkID)
+		return searchNsxtNetworkReference(ctx, np.cluster, networkID)
 	default:
 		return nil, fmt.Errorf("unsupported NetOP network type %s", networkType)
 	}
@@ -603,7 +604,7 @@ func (np *nsxtNetworkProvider) createEthernetCard(
 		return nil, err
 	}
 
-	networkRef, err := searchNsxtNetworkReference(vmCtx, np.finder, np.cluster, vnetIf.Status.ProviderStatus.NsxLogicalSwitchID)
+	networkRef, err := searchNsxtNetworkReference(vmCtx, np.cluster, vnetIf.Status.ProviderStatus.NsxLogicalSwitchID)
 	if err != nil {
 		// Log message used by VMC LINT. Refer to before making changes
 		vmCtx.Logger.Error(err, "Failed to search for nsx-t network associated with vnetIf", "vnetIf", vnetIf)
@@ -727,108 +728,51 @@ func (np *nsxtNetworkProvider) getNetplanEthernet(vnetIf *ncpv1alpha1.VirtualNet
 	return eth
 }
 
-// matchOpaqueNetwork takes the network ID, returns whether the opaque network matches the networkID.
-func matchOpaqueNetwork(ctx goctx.Context, network object.NetworkReference, networkID string) bool {
-	obj, ok := network.(*object.OpaqueNetwork)
-	if !ok {
-		return false
-	}
-
-	var opaqueNet mo.OpaqueNetwork
-	if err := obj.Properties(ctx, obj.Reference(), []string{"summary"}, &opaqueNet); err != nil {
-		return false
-	}
-
-	summary, _ := opaqueNet.Summary.(*vimtypes.OpaqueNetworkSummary)
-	return summary.OpaqueNetworkId == networkID
-}
-
-// matchDistributedPortGroup takes the network ID, returns whether the distributed port group matches the networkID.
-func matchDistributedPortGroup(
+// searchNsxtNetworkReference takes in nsx-t logical switch UUID and returns the reference of the network.
+func searchNsxtNetworkReference(
 	ctx goctx.Context,
-	network object.NetworkReference,
-	networkID string,
-	hostMoIDs []vimtypes.ManagedObjectReference) bool {
+	ccr *object.ClusterComputeResource,
+	networkID string) (object.NetworkReference, error) {
 
-	obj, ok := network.(*object.DistributedVirtualPortgroup)
-	if !ok {
-		return false
-	}
-
-	var configInfo []vimtypes.ObjectContent
-	err := obj.Properties(ctx, obj.Reference(), []string{"config.logicalSwitchUuid", "host"}, &configInfo)
-	if err != nil {
-		return false
+	var obj mo.ClusterComputeResource
+	if err := ccr.Properties(ctx, ccr.Reference(), []string{"network"}, &obj); err != nil {
+		return nil, err
 	}
 
-	if len(configInfo) > 0 {
-		// Check "logicalSwitchUuid" property
-		lsIDMatch := false
-		for _, dynamicProperty := range configInfo[0].PropSet {
-			if dynamicProperty.Name == "config.logicalSwitchUuid" && dynamicProperty.Val == networkID {
-				lsIDMatch = true
-				break
-			}
-		}
-
-		// logicalSwitchUuid did not match
-		if !lsIDMatch {
-			return false
-		}
-
-		foundAllHosts := false
-		for _, dynamicProperty := range configInfo[0].PropSet {
-			// In the case of a single NSX Overlay Transport Zone for all the clusters and DVS's,
-			// multiple DVPGs(associated with different DVS's) will have the same "logicalSwitchUuid".
-			// So matching "logicalSwitchUuid" is necessary condition, but not sufficient.
-			// Checking if the DPVG has all the hosts in the cluster, along with the above would be sufficient
-			if dynamicProperty.Name == "host" {
-				if hosts, ok := dynamicProperty.Val.(vimtypes.ArrayOfManagedObjectReference); ok {
-					foundAllHosts = true
-					dvsHostSet := make(map[string]bool, len(hosts.ManagedObjectReference))
-					for _, dvsHost := range hosts.ManagedObjectReference {
-						dvsHostSet[dvsHost.Value] = true
-					}
-
-					for _, hostMoRef := range hostMoIDs {
-						if _, ok := dvsHostSet[hostMoRef.Value]; !ok {
-							foundAllHosts = false
-							break
-						}
-					}
-				}
-			}
+	var dvpgsMoRefs []vimtypes.ManagedObjectReference
+	for _, n := range obj.Network {
+		if n.Type == "DistributedVirtualPortgroup" {
+			dvpgsMoRefs = append(dvpgsMoRefs, n.Reference())
 		}
+	}
 
-		// Implicit that lsID Matches at this point
-		return foundAllHosts
+	if len(dvpgsMoRefs) == 0 {
+		return nil, fmt.Errorf("ClusterComputeResource %s has no DVPGs", ccr.Reference().Value)
 	}
-	return false
-}
 
-// searchNsxtNetworkReference takes in nsx-t logical switch UUID and returns the reference of the network.
-func searchNsxtNetworkReference(ctx goctx.Context, finder *find.Finder, cluster *object.ClusterComputeResource, networkID string) (object.NetworkReference, error) {
-	networks, err := finder.NetworkList(ctx, "*")
+	var dvpgs []mo.DistributedVirtualPortgroup
+	err := property.DefaultCollector(ccr.Client()).Retrieve(ctx, dvpgsMoRefs, []string{"config.logicalSwitchUuid"}, &dvpgs)
 	if err != nil {
 		return nil, err
 	}
 
-	// Get the list of ESX host moRef objects for this cluster
-	// TODO: ps: []string{"host"} instead of nil?
-	var computeResource mo.ComputeResource
-	if err := cluster.Properties(ctx, cluster.Reference(), nil, &computeResource); err != nil {
-		return nil, err
+	var dvpgMoRefs []vimtypes.ManagedObjectReference
+	for _, dvpg := range dvpgs {
+		if dvpg.Config.LogicalSwitchUuid == networkID {
+			dvpgMoRefs = append(dvpgMoRefs, dvpg.Reference())
+		}
 	}
 
-	for _, network := range networks {
-		if matchDistributedPortGroup(ctx, network, networkID, computeResource.Host) {
-			return network, nil
-		}
-		if matchOpaqueNetwork(ctx, network, networkID) {
-			return network, nil
-		}
+	switch len(dvpgMoRefs) {
+	case 1:
+		return object.NewDistributedVirtualPortgroup(ccr.Client(), dvpgMoRefs[0]), nil
+	case 0:
+		return nil, fmt.Errorf("no DVPG with NSX-T network ID %q found", networkID)
+	default:
+		// The LogicalSwitchUuid is supposed to be unique per CCR, so this is likely an NCP
+		// misconfiguration, and we don't know which one to pick.
+		return nil, fmt.Errorf("multiple DVPGs (%d) with NSX-T network ID %q found", len(dvpgMoRefs), networkID)
 	}
-	return nil, fmt.Errorf("opaque network with ID '%s' not found", networkID)
 }
 
 // ToCidrNotation takes ip and mask as ip addresses and returns a cidr notation.

pkg/vmprovider/providers/vsphere/network/network_provider_test.go

@@ -578,7 +578,7 @@ var _ = Describe("NetworkProvider", func() {
 
 				It("should return an error", func() {
 					_, err := np.EnsureNetworkInterface(vmCtx, &vmCtx.VM.Spec.NetworkInterfaces[0])
-					Expect(err).To(MatchError(fmt.Sprintf("opaque network with ID '%s' not found", doesNotExist)))
+					Expect(err).To(MatchError(fmt.Sprintf("no DVPG with NSX-T network ID %q found", doesNotExist)))
 				})
 			})