Repository: vmware/govmomi (go)

Issue:
[BUG] FileManager.TransferURL dual IPv4/IPv6 stack ESXi problem
**Describe the bug**
The FileManager.TransferURL has a problem with dual stack ESXi when the machine it is being called on has only pure IPv6. It can return the IPv4 address overriding the IPv6 address in the URL. The logic for override returns first managed IP without taking stacks into account

https://github.com/vmware/govmomi/blob/master/guest/file_manager.go#:~:text=//%20prefer%20an%20ESX%20management%20IP%2C%20as%20the%20hostname%20used%20when%20adding%20to%20VC%20may%20not%20be%20valid%20for%20this%20client

**To Reproduce**
Steps to reproduce the behavior:

1. Deploy ESXi host with both IPv4 and IPv6
2. Deploy VC with only IPv6 and make it manage the host
3. Try to download file in a VM managed by the VC connecting to the VC directly. Can be executed on the VC for the best result.

**Expected behavior**
I would expect that TransferURL won't change to an IP that is not supported on the machine it is being called. I would expect that the code to check if the URL returned has an IP already and to not replace it.

**Affected version**
Using 0.24.0, but it is resent even in 0.26.x 

**Screenshots/Debug Output**
ESXi network:
vNicInfo = (vim.host.VirtualNicManagerInfo) {
-->             netConfig = (vim.host.VirtualNicManager.NetConfig) [
-->                (vim.host.VirtualNicManager.NetConfig) {
-->                   nicType = "faultToleranceLogging", 
-->                   multiSelectAllowed = true, 
-->                   candidateVnic = (vim.host.VirtualNic) [
-->                      (vim.host.VirtualNic) {
-->                         device = "vmk0", 
-->                         key = "faultToleranceLogging.key-vim.host.VirtualNic-vmk0", 
-->                         portgroup = "Management Network", 
-->                         spec = (vim.host.VirtualNic.Specification) {
-->                            ip = (vim.host.IpConfig) {
-->                               dhcp = true, 
-->                               ipAddress = "10.173.21.231", 
-->                               subnetMask = "255.255.254.0", 
-->                               ipV6Config = (vim.host.IpConfig.IpV6AddressConfiguration) {
-->                                  ipV6Address = (vim.host.IpConfig.IpV6Address) [
-->                                     (vim.host.IpConfig.IpV6Address) {
-->                                        ipAddress = "fe80::266e:96ff:fe4b:71c6", 
-->                                        prefixLength = 64, 
-->                                        origin = "other", 
-->                                        dadState = "preferred", 
-->                                     }, 
-->                                     (vim.host.IpConfig.IpV6Address) {
-->                                        ipAddress = "fd01:1:3:1528::10bf", 
-->                                        prefixLength = 64, 
-->                                        origin = "manual", 
-->                                        dadState = "preferred", 
-->                                     }
-->                                  ], 
-->                                  autoConfigurationEnabled = false, 
-->                                  dhcpV6Enabled = true
-->                               }
-->                            }, 
-->                            mac = "24:6e:96:4b:71:c6", 
-->                            portgroup = "Management Network", 
-->                            mtu = 1500, 
-->                            tsoEnabled = true, 
-->                            netStackInstanceKey = "defaultTcpipStack", 
-->                         }, 
-->                      }, 
-->                      (vim.host.VirtualNic) {
-->                         device = "vmk1", 
-->                         key = "faultToleranceLogging.key-vim.host.VirtualNic-vmk1", 
-->                         portgroup = "iscsi", 
-->                         spec = (vim.host.VirtualNic.Specification) {
-->                            ip = (vim.host.IpConfig) {
-->                               dhcp = false, 
-->                               ipAddress = "192.168.108.178", 
-->                               subnetMask = "255.255.252.0", 
-->                               ipV6Config = (vim.host.IpConfig.IpV6AddressConfiguration) {
-->                                  ipV6Address = (vim.host.IpConfig.IpV6Address) [
-->                                     (vim.host.IpConfig.IpV6Address) {
-->                                        ipAddress = "fe80::250:56ff:fe6c:2630", 
-->                                        prefixLength = 64, 
-->                                        origin = "other", 
-->                                        dadState = "preferred", 
-->                                     }
-->                                  ], 
-->                                  autoConfigurationEnabled = true, 
-->                                  dhcpV6Enabled = true
-->                               }
-->                            }, 
-->                            mac = "00:50:56:6c:26:30", 
-->                            portgroup = "iscsi", 
-->                            mtu = 1500, 
-->                            tsoEnabled = true, 
-->                            netStackInstanceKey = "defaultTcpipStack", 
-->                         }, 
-->                      }, 
-->                      (vim.host.VirtualNic) {
-->                         device = "vmk2", 
-->                         key = "faultToleranceLogging.key-vim.host.VirtualNic-vmk2", 
-->                         portgroup = "New port group", 
-->                         spec = (vim.host.VirtualNic.Specification) {
-->                            ip = (vim.host.IpConfig) {
-->                               dhcp = false, 
-->                               ipAddress = "10.172.208.178", 
-->                               subnetMask = "255.255.252.0", 
-->                               ipV6Config = (vim.host.IpConfig.IpV6AddressConfiguration) {
-->                                  ipV6Address = (vim.host.IpConfig.IpV6Address) [
-->                                     (vim.host.IpConfig.IpV6Address) {
-->                                        ipAddress = "fe80::250:56ff:fe65:3260", 
-->                                        prefixLength = 64, 
-->                                        origin = "other", 
-->                                        dadState = "preferred", 
-->                                     }
-->                                  ], 
-->                                  autoConfigurationEnabled = false, 
-->                                  dhcpV6Enabled = false
-->                               }
-->                            }, 
-->                            mac = "00:50:56:65:32:60", 
-->                            portgroup = "New port group", 
-->                            mtu = 1500, 
-->                            tsoEnabled = true, 
-->                            netStackInstanceKey = "defaultTcpipStack", 
-->                         }, 
-->                      }, 
-->                      (vim.host.VirtualNic) {
-->                         device = "vmk3", 
-->                         key = "faultToleranceLogging.key-vim.host.VirtualNic-vmk3", 
-->                         portgroup = "VLAN-1537", 
-->                         spec = (vim.host.VirtualNic.Specification) {
-->                            ip = (vim.host.IpConfig) {
-->                               dhcp = true, 
-->                               ipAddress = "10.173.23.229", 
-->                               subnetMask = "255.255.255.0", 
-->                               ipV6Config = (vim.host.IpConfig.IpV6AddressConfiguration) {
-->                                  ipV6Address = (vim.host.IpConfig.IpV6Address) [
-->                                     (vim.host.IpConfig.IpV6Address) {
-->                                        ipAddress = "fe80::250:56ff:fe63:989f", 
-->                                        prefixLength = 64, 
-->                                        origin = "other", 
-->                                        dadState = "preferred", 
-->                                     }
-->                                  ], 
-->                                  autoConfigurationEnabled = false, 
-->                                  dhcpV6Enabled = false
-->                               }
-->                            }, 
-->                            mac = "00:50:56:63:98:9f", 
-->                            portgroup = "VLAN-1537", 
-->                            mtu = 1500, 
-->                            tsoEnabled = true, 
-->                            netStackInstanceKey = "defaultTcpipStack", 
-->                         }, 
-->                      }
-->                   ], 
-->                }

Guestops VPXD URL returned to govmomi:

2021-08-17T04:40:49.605Z verbose vpxd[18624] [Originator@6876 sub=Vmomi opID=c00ed04] Invoke done; <<521e7dff-8520-059a-45f0-c11dd10b6160, <TCP '127.0.0.1 : 8085'>, <TCP '127.0.0.1 : 32820'>>, guestOperationsFileManager, vim.vm.guest.FileManager.initiateFileTransferFromGuest>
--> Result:
--> (vim.vm.guest.FileManager.FileTransferInformation) {
-->    attributes = (vim.vm.guest.FileManager.PosixFileAttributes) {
-->       modificationTime = "2021-08-17T04:40:48Z",
-->       accessTime = "2021-08-17T04:40:48Z",
-->       symlinkTarget = <unset>,
-->       ownerId = 0,
-->       groupId = 0,
-->       permissions = 33188
-->    },
-->    size = 1433,
-->    url = "https://[fd01:1:3:1528::10bf]:443/guestFile?id=749&token=52fd0e76-3490-5057-01d0-fea01a710b2d749"

URL returned by FileManager.TransferURL:
"https://10.173.21.231:443/guestFile?id=749&token=52721ca9-feb4-f462-e021-1c20eb1bc7ce749"

**Additional context**
In my situation, the govmomi is executed on the VC that manages the ESXi with dual stack, and deploying ova did work well. Only after it was deployed a file was tried to be downloaded and failed as it was suddenly talking on the IPv4 address while VC is pure IPv6


Pull request:
fix: avoid vNIC mapping in guest.TransferURL if URL.Host is an IP
## Description

No need to map the guest transfer URL host to the ESX vNIC IP if the URL.Host is already an IP address.
In which case we can also avoid property collection of the vNIC config.
Using the original IP also a must when the ESXi host is dual stack, but the client has only pure IPv6.

Changed the host cache value from URL.Hostname() to URL.Host, so IP-literal form is maintained.
e.g. URL.Hostname() is "fe80::1" when URL.Host is "[fe80::1]:80"

Fixes #2553

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [x] My code follows the CONTRIBUTION [guidelines](./../CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged

Patch:
diff --git a/guest/file_manager.go b/guest/file_manager.go
index 922c6d95..9dc0c194 100644
--- a/guest/file_manager.go
+++ b/guest/file_manager.go
@@ -143,14 +143,17 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 
 	name := turl.Hostname()
 	port := turl.Port()
+	isHostname := net.ParseIP(name) == nil
 
 	m.mu.Lock()
 	mname, ok := m.hosts[name]
 	m.mu.Unlock()
 
 	if ok {
-		turl.Host = net.JoinHostPort(mname, port)
+		turl.Host = mname
 		return turl, nil
+	} else {
+		mname = turl.Host
 	}
 
 	c := property.DefaultCollector(m.c)
@@ -169,7 +172,10 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 		"name",
 		"runtime.connectionState",
 		"summary.config.sslThumbprint",
-		"config.virtualNicManagerInfo.netConfig",
+	}
+
+	if isHostname {
+		props = append(props, "config.virtualNicManagerInfo.netConfig")
 	}
 
 	var host mo.HostSystem
@@ -178,28 +184,29 @@ func (m FileManager) TransferURL(ctx context.Context, u string) (*url.URL, error
 		return nil, err
 	}
 
-	if host.Config == nil {
-		return nil, fmt.Errorf("guest TransferURL failed for vm %q (%s): host %q (%s) config==nil, connectionState==%s",
-			vm.Name, vm.Self,
-			host.Name, host.Self, host.Runtime.ConnectionState)
-	}
-
-	// 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
+	if isHostname {
+		if host.Config == nil {
+			return nil, fmt.Errorf("guest TransferURL failed for vm %q (%s): host %q (%s) config==nil, connectionState==%s",
+				vm.Name, vm.Self,
+				host.Name, host.Self, host.Runtime.ConnectionState)
+		}
+
+		// 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 = net.JoinHostPort(ips[0].String(), port)
+
+			turl.Host = mname
+		}
 	}
 
-	turl.Host = net.JoinHostPort(name, port)
+	m.mu.Lock()
+	m.hosts[name] = mname
+	m.mu.Unlock()
 
 	m.c.SetThumbprint(turl.Host, host.Summary.Config.SslThumbprint)
 


Test patch:
diff --git a/guest/file_manager_test.go b/guest/file_manager_test.go
index c950ff25..5ad948fe 100644
--- a/guest/file_manager_test.go
+++ b/guest/file_manager_test.go
@@ -18,6 +18,7 @@ package guest_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/vmware/govmomi/guest"
@@ -37,13 +38,25 @@ func TestTranferURL(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		turl := "https://esx:443/foo/bar"
-		u, err := m.TransferURL(ctx, turl)
-		if err != nil {
-			t.Fatal(err)
-		}
-		if u.Hostname() != "127.0.0.1" {
-			t.Errorf("hostname=%s", u.Hostname())
+		for i := 0; i < 2; i++ { // 2nd time is to validate the cached value
+			turl := "https://esx:443/foo/bar"
+			u, err := m.TransferURL(ctx, turl)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if u.Hostname() != "127.0.0.1" {
+				t.Errorf("hostname=%s", u.Hostname())
+			}
+
+			ipv6 := "[fd01:1:3:1528::10bf]:443"
+			turl = fmt.Sprintf("https://%s/foo/bar", ipv6)
+			u, err = m.TransferURL(ctx, turl)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if u.Host != ipv6 {
+				t.Errorf("host=%s", u.Host)
+			}
 		}
 
 		// hostname should be returned by TransferURL when there are multiple management IPs
@@ -54,13 +67,15 @@ func TestTranferURL(t *testing.T) {
 			}
 		}
 
-		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())
+		for i := 0; i < 2; i++ { // 2nd time is to validate the cached value
+			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
@@ -70,10 +85,10 @@ func TestTranferURL(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		u, err = m.TransferURL(ctx, turl)
+		turl := "https://noconfig:443/foo/bar"
+		u, err := m.TransferURL(ctx, turl)
 		if err == nil {
 			t.Errorf("expected error (url=%s)", u)
 		}
-		t.Log(err)
 	})
 }