Skip to content
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

Jump to bottom

Allows Hyper-V driver to select IP address based on preferred protocol #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allows Hyper-V driver to select IP address based on preferred protocol #23

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cfb21a3
Adds preferable network protocol for Hyper-V driver
laozc Nov 1, 2019
e5b2079
Adds timeout when waiting for IP and stopped
laozc Nov 4, 2019
d3b4e7c
Adds wait timeout option for Hyper-V driver
laozc Nov 14, 2019
d615d62
Prefer IPv4 over v6 and global unicast IP by default in Hyper-V driver
laozc Nov 14, 2019
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
104 changes: 90 additions & 14 deletions drivers/hyperv/hyperv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package hyperv

import (
"encoding/json"
"errors"
"fmt"
"net"
"os"
Expand All @@ -16,16 +17,24 @@ import (
"github.com/docker/machine/libmachine/state"
)

const (
Default = iota
PreferIPv4
PreferIPv6
)

type Driver struct {
*drivers.BaseDriver
Boot2DockerURL string
VSwitch string
DiskSize int
MemSize int
CPU int
MacAddr string
VLanID int
DisableDynamicMemory bool
Boot2DockerURL string
VSwitch string
DiskSize int
MemSize int
CPU int
MacAddr string
VLanID int
DisableDynamicMemory bool
PreferredNetworkProtocol int
WaitTimeoutInSeconds time.Duration
}

const (
Expand All @@ -35,19 +44,22 @@ const (
defaultVLanID = 0
defaultDisableDynamicMemory = false
defaultSwitchID = "c08cb7b8-9b3c-408e-8e30-5e16a3aeb444"
defaultWaitTimeoutInSeconds = 3 * 60
)

// NewDriver creates a new Hyper-v driver with default settings.
func NewDriver(hostName, storePath string) *Driver {
return &Driver{
DiskSize: defaultDiskSize,
MemSize: defaultMemory,
CPU: defaultCPU,
DisableDynamicMemory: defaultDisableDynamicMemory,
BaseDriver: &drivers.BaseDriver{
MachineName: hostName,
StorePath: storePath,
},
DiskSize: defaultDiskSize,
MemSize: defaultMemory,
CPU: defaultCPU,
DisableDynamicMemory: defaultDisableDynamicMemory,
PreferredNetworkProtocol: Default,
WaitTimeoutInSeconds: defaultWaitTimeoutInSeconds,
}
}

Expand Down Expand Up @@ -99,6 +111,17 @@ func (d *Driver) GetCreateFlags() []mcnflag.Flag {
Usage: "Disable dynamic memory management setting",
EnvVar: "HYPERV_DISABLE_DYNAMIC_MEMORY",
},
mcnflag.IntFlag{
Name: "hyperv-preferred-network-protocol",
Usage: "Preferred network protocol (IPv4/v6)",
EnvVar: "HYPERV_PREFERRED_NETWORK_PROTOCOL",
},
mcnflag.IntFlag{
Name: "hyperv-wait-timeout",
Usage: "Wait timeout (in seconds)",
Value: defaultWaitTimeoutInSeconds,
EnvVar: "HYPERV_WAIT_TIMEOUT",
},
}
}

Expand All @@ -112,6 +135,8 @@ func (d *Driver) SetConfigFromFlags(flags drivers.DriverOptions) error {
d.VLanID = flags.Int("hyperv-vlan-id")
d.SSHUser = "docker"
d.DisableDynamicMemory = flags.Bool("hyperv-disable-dynamic-memory")
d.PreferredNetworkProtocol = flags.Int("hyperv-preferred-network-protocol")
d.WaitTimeoutInSeconds = time.Duration(flags.Int("hyperv-wait-timeout"))
d.SetSwarmConfigFromFlags(flags)

return nil
Expand Down Expand Up @@ -328,12 +353,17 @@ func (d *Driver) chooseVirtualSwitch() (string, error) {
func (d *Driver) waitForIP() (string, error) {
log.Infof("Waiting for host to start...")

start := time.Now()
for {
ip, _ := d.GetIP()
if ip != "" {
return ip, nil
}

if time.Since(start) >= d.WaitTimeoutInSeconds*time.Second {
return "", errors.New("timeout waiting for IP")
}

time.Sleep(1 * time.Second)
}
}
Expand All @@ -342,6 +372,7 @@ func (d *Driver) waitForIP() (string, error) {
func (d *Driver) waitStopped() error {
log.Infof("Waiting for host to stop...")

start := time.Now()
for {
s, err := d.GetState()
if err != nil {
Expand All @@ -352,6 +383,10 @@ func (d *Driver) waitStopped() error {
return nil
}

if time.Since(start) >= d.WaitTimeoutInSeconds*time.Second {
return errors.New("timeout waiting for stopped")
}

time.Sleep(1 * time.Second)
}
}
Expand Down Expand Up @@ -428,6 +463,10 @@ func (d *Driver) Kill() error {
return nil
}

func isIPv4(address string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

What we do in Minishift is as follows: https://github.com/minishift/minishift/blob/0efa5d527d77a7651eac75a4861097ab4a203f43/cmd/minishift/cmd/start_preflight.go#L485-L491

func isIPv4(address string) bool {
   return net.ParseIP(ip).To4() != nil
}

should be enough and relies on the framework instead of counting delimiters.

127.1 is a valid IPv4 address... which expands to 127.0.0.1. Does your code work with this?

Copy link
Author

Choose a reason for hiding this comment

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

I thought about using To4() for checking.
However, IPv4 address converted to IPv6 format 0:0:0:0:0:ffff:d1ad:35a7 could be considerred as IPv4 address since To4 can take both v4 and v6 address.

	if len(ip) == IPv6len &&
		isZeros(ip[0:10]) &&
		ip[10] == 0xff &&
		ip[11] == 0xff {
		return ip[12:16]
	}

I'm not sure which format could be accepted by minikube (either converting it back to IPv4 or still using this IPv6 format). Therefore, I used delimiter counting instead of To4() here to avoid this potential issue.

Copy link
Member

@gbraad gbraad Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

does Minikube function with any IPv6 address?
We have seen issues with the SSH clients not accepting the address (espcially on Windows).

Copy link
Member

@gbraad gbraad Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But with To4() the correct result is returned anyway return ip[12:16] so shouldn't that then we used instead on the switch case for IPv4 protocol? We do not mind if IPv6 is returned from the networkadapter, as the network stack should handle this for us anyway when this is instead resolved with the relevant IPv4 format.

Copy link
Author

Choose a reason for hiding this comment

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

I was not sure which format v4 or v6 should be returned in this case as the function asks for a string.
It's not clear that if IPv4 address in v6 format could be accepted in Windows or simply just return a value in IPv4.
Let me do some experiment on this.

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't seem IPv4 address in IPv6 format functions well on Windows.
Thus I would like to keep the code to count on ":" instead of To4 in which case a closer match for IPv4 format can be done and only IPv4 format address could be returned.

return strings.Count(address, ":") < 1
}

func (d *Driver) GetIP() (string, error) {
s, err := d.GetState()
if err != nil {
Expand All @@ -437,7 +476,7 @@ func (d *Driver) GetIP() (string, error) {
return "", drivers.ErrHostIsNotRunning
}

stdout, err := cmdOut("((", "Hyper-V\\Get-VM", d.MachineName, ").networkadapters[0]).ipaddresses[0]")
stdout, err := cmdOut("((", "Hyper-V\\Get-VM", d.MachineName, ").networkadapters[0]).ipaddresses")
if err != nil {
return "", err
}
Expand All @@ -447,7 +486,44 @@ func (d *Driver) GetIP() (string, error) {
return "", fmt.Errorf("IP not found")
}

return resp[0], nil
switch d.PreferredNetworkProtocol {
case PreferIPv4:
for _, ipStr := range resp {
ip := net.ParseIP(ipStr)
if isIPv4(ipStr) && ip.To4() != nil && ip.IsGlobalUnicast() {
Copy link
Member

Choose a reason for hiding this comment

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

See previous comment... it would make this check easier as no To4 is needed, as we already know this.

return ipStr, nil
}
}

case PreferIPv6:
for _, ipStr := range resp {
ip := net.ParseIP(ipStr)
if !isIPv4(ipStr) && ip.IsGlobalUnicast() {
return ipStr, nil
}
}

default:
var preferredIP string
for _, ipStr := range resp {
ip := net.ParseIP(ipStr)
if ip.IsGlobalUnicast() {
if preferredIP == "" {
Copy link
Member

Choose a reason for hiding this comment

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

this would mean that the LAST ip address, that is shared on the switch, will be returned.
So essentially, LIFO (Last in First out)...

When 10.0.0.1, 172.16.0.1 and 192.168.0.1 is given to an interface, we return 192.168.0.1 and not 10.0.0.1 which might serve a larger range?

Copy link
Member

Choose a reason for hiding this comment

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

The reason I bring this up, is that this seems changed behaviour from previously. That would not be the intention of a 'default' fallback.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think it acts much differently than the original one.

There are two differences before this change

  1. Global Unicast will always be excluded

This is intended to exclude fe80:: for IPv6 for most of the cases when I found that fe80:: will be always assigned to the interfaces when IPv6 is enabled. When it tries to wait for SSH connectivity it might end up use this address.
For IPv4 only 169.254 range will be excluded.

  1. Prefer IPv4 over IPv6 in the default case (which may bring some slight confusion on the PreferIPv4 / PreferIPv6 /Default and it seems like to use IPv4Only / IPv6Only / Default would be better)
    This is also been seen in other drivers as well.

When there are IPv4 addresses assigned for the interfaces, it will always the first IPv4 address available based on the addresses reported by Hyper-V. So if the interface says 10.0.0.1, 172.16.0.1, 192.168.0.1, it will return 10.0.0.1. Only when there is no IPv4 addreses found it will return the first IPv6 Global Unique address.

preferredIP = ipStr
}
if isIPv4(ipStr) && ip.To4() != nil {
preferredIP = ipStr
break
}
}
}

if preferredIP != "" {
return preferredIP, nil
}
}

return "", nil
}

func (d *Driver) publicSSHKeyPath() string {
Expand Down