fix: avoid use of vNIC IP in guest.TransferURL if there are multiple

InitiateFileTransfer{To,From}Guest methods return an ESX host's inventory name (HostSystem.Name).
This name was used to add the host to vCenter and cannot be changed
(unless the host is removed from inventory and added back with another name).
The name used when adding to VC may not resolvable by this client's DNS, so we prefer an ESX management IP.
However, if there is more than one management vNIC, we don't know which IP(s) the client has a route to.
Leave the hostname as-is in that case or if the env var has disabled the preference.

guest/file_manager.go

@@ -21,8 +21,10 @@ import (
 	"fmt"
 	"net"
 	"net/url"
+	"os"
 	"sync"
 
+	"github.com/vmware/govmomi/internal"
 	"github.com/vmware/govmomi/property"
 	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/methods"
@@ -119,6 +121,9 @@ func (m FileManager) DeleteFile(ctx context.Context, auth types.BaseGuestAuthent
 	return err
 }
 
+// escape hatch to disable the preference to use ESX host management IP for guest file transfer
+var useGuestTransferIP = os.Getenv("GOVMOMI_USE_GUEST_TRANSFER_IP") != "false"
+
 // TransferURL rewrites the url with a valid hostname and adds the host's thumbprint.
 // The InitiateFileTransfer{From,To}Guest methods return a URL with the host set to "*" when connected directly to ESX,
 // but return the address of VM's runtime host when connected to vCenter.
@@ -179,29 +184,19 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 			host.Name, host.Self, host.Runtime.ConnectionState)
 	}
 
-	// prefer an ESX management IP, as the hostname used when adding to VC may not be valid for this client
-	// See also object.HostSystem.ManagementIPs which we can't use here due to import cycle
-	for _, nc := range host.Config.VirtualNicManagerInfo.NetConfig {
-		if nc.NicType != string(types.HostVirtualNicManagerNicTypeManagement) {
-			continue
-		}
-		for ix := range nc.CandidateVnic {
-			for _, selectedVnicKey := range nc.SelectedVnic {
-				if nc.CandidateVnic[ix].Key != selectedVnicKey {
-					continue
-				}
-				ip := net.ParseIP(nc.CandidateVnic[ix].Spec.Ip.IpAddress)
-				if ip != nil {
-					mname = ip.String()
-					m.mu.Lock()
-					m.hosts[name] = mname
-					m.mu.Unlock()
-
-					name = mname
-					break
-				}
-			}
-		}
+	// InitiateFileTransfer{To,From}Guest methods return an ESX host's inventory name (HostSystem.Name).
+	// This name was used to add the host to vCenter and cannot be changed (unless the host is removed from inventory and added back with another name).
+	// The name used when adding to VC may not resolvable by this client's DNS, so we prefer an ESX management IP.
+	// However, if there is more than one management vNIC, we don't know which IP(s) the client has a route to.
+	// Leave the hostname as-is in that case or if the env var has disabled the preference.
+	ips := internal.HostSystemManagementIPs(host.Config.VirtualNicManagerInfo.NetConfig)
+	if len(ips) == 1 && useGuestTransferIP {
+		mname = ips[0].String()
+		m.mu.Lock()
+		m.hosts[name] = mname
+		m.mu.Unlock()
+
+		name = mname
 	}
 
 	turl.Host = net.JoinHostPort(name, port)

guest/file_manager_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/vmware/govmomi/guest"
 	"github.com/vmware/govmomi/simulator"
 	"github.com/vmware/govmomi/vim25"
+	"github.com/vmware/govmomi/vim25/types"
 )
 
 func TestTranferURL(t *testing.T) {
@@ -45,6 +46,24 @@ func TestTranferURL(t *testing.T) {
 			t.Errorf("hostname=%s", u.Hostname())
 		}
 
+		// hostname should be returned by TransferURL when there are multiple management IPs
+		for _, nc := range host.Config.VirtualNicManagerInfo.NetConfig {
+			if nc.NicType == string(types.HostVirtualNicManagerNicTypeManagement) {
+				host.Config.VirtualNicManagerInfo.NetConfig = append(host.Config.VirtualNicManagerInfo.NetConfig, nc)
+				break
+			}
+		}
+
+		turl = "https://esx2:443/foo/bar"
+		u, err = m.TransferURL(ctx, turl)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if u.Hostname() != "esx2" {
+			t.Errorf("hostname=%s", u.Hostname())
+		}
+
+		// error should be returned if HostSystem.Config is nil
 		host.Config = nil
 		m, err = ops.FileManager(ctx)
 		if err != nil {

internal/helpers.go

@@ -17,9 +17,11 @@ limitations under the License.
 package internal
 
 import (
+	"net"
 	"path"
 
 	"github.com/vmware/govmomi/vim25/mo"
+	"github.com/vmware/govmomi/vim25/types"
 )
 
 // InventoryPath composed of entities by Name
@@ -36,3 +38,26 @@ func InventoryPath(entities []mo.ManagedEntity) string {
 
 	return val
 }
+
+func HostSystemManagementIPs(config []types.VirtualNicManagerNetConfig) []net.IP {
+	var ips []net.IP
+
+	for _, nc := range config {
+		if nc.NicType != string(types.HostVirtualNicManagerNicTypeManagement) {
+			continue
+		}
+		for ix := range nc.CandidateVnic {
+			for _, selectedVnicKey := range nc.SelectedVnic {
+				if nc.CandidateVnic[ix].Key != selectedVnicKey {
+					continue
+				}
+				ip := net.ParseIP(nc.CandidateVnic[ix].Spec.Ip.IpAddress)
+				if ip != nil {
+					ips = append(ips, ip)
+				}
+			}
+		}
+	}
+
+	return ips
+}

internal/helpers_test.go

@@ -0,0 +1,32 @@
+/*
+Copyright (c) 2021 VMware, Inc. All Rights Reserved.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package internal_test
+
+import (
+	"testing"
+
+	"github.com/vmware/govmomi/internal"
+	"github.com/vmware/govmomi/simulator/esx"
+)
+
+func TestHostSystemManagementIPs(t *testing.T) {
+	ips := internal.HostSystemManagementIPs(esx.HostSystem.Config.VirtualNicManagerInfo.NetConfig)
+
+	if len(ips) != 1 {
+		t.Fatalf("no mgmt ip found")
+	}
+	if ips[0].String() != "127.0.0.1" {
+		t.Fatalf("Expected management ip %s, got %s", "127.0.0.1", ips[0].String())
+	}
+}

object/host_system.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net"
 
+	"github.com/vmware/govmomi/internal"
 	"github.com/vmware/govmomi/vim25"
 	"github.com/vmware/govmomi/vim25/methods"
 	"github.com/vmware/govmomi/vim25/mo"
@@ -81,24 +82,7 @@ func (h HostSystem) ManagementIPs(ctx context.Context) ([]net.IP, error) {
 		return nil, err
 	}
 
-	var ips []net.IP
-	for _, nc := range mh.Config.VirtualNicManagerInfo.NetConfig {
-		if nc.NicType != string(types.HostVirtualNicManagerNicTypeManagement) {
-			continue
-		}
-		for ix := range nc.CandidateVnic {
-			for _, selectedVnicKey := range nc.SelectedVnic {
-				if nc.CandidateVnic[ix].Key != selectedVnicKey {
-					continue
-				}
-				ip := net.ParseIP(nc.CandidateVnic[ix].Spec.Ip.IpAddress)
-				if ip != nil {
-					ips = append(ips, ip)
-				}
-			}
-		}
-	}
-	return ips, nil
+	return internal.HostSystemManagementIPs(mh.Config.VirtualNicManagerInfo.NetConfig), nil
 }
 
 func (h HostSystem) Disconnect(ctx context.Context) (*Task, error) {