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 2 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
74 changes: 64 additions & 10 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,23 @@ import (
"github.com/docker/machine/libmachine/state"
)

const (
DefaultProtocol = iota
Copy link
Member

Choose a reason for hiding this comment

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

which is? this is not clear... as what defines the default ?

Copy link
Author

@laozc laozc 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.

It would depend on the adapter and DHCP setting.
I believe it's the reason why sometimes an IPv6 address gets returned by the driver and fails minikube.
Do you have any better naming for this option?

Copy link
Member

Choose a reason for hiding this comment

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

it also happens with the Defautl Switch, while this should always AFAIK return a valid IPv4 addres... but this is not the case. In case of failures in thenetworking stack we have seen IPv6 being returned as a link-local.

Copy link
Member

Choose a reason for hiding this comment

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

I would not say protocol, but just Default.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good.

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
}

const (
Expand All @@ -35,6 +43,7 @@ const (
defaultVLanID = 0
defaultDisableDynamicMemory = false
defaultSwitchID = "c08cb7b8-9b3c-408e-8e30-5e16a3aeb444"
defaultTimeout = time.Minute * 10
Copy link
Member

Choose a reason for hiding this comment

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

awfully long time out...

Copy link
Author

Choose a reason for hiding this comment

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

How long do you think it's appropriate? 3 or 5 min?

Copy link
Member

Choose a reason for hiding this comment

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

a default of 3 is long enough, since when more is needed, the caller should override this.

Copy link
Author

Choose a reason for hiding this comment

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

Sure.
Let me make the timeout configurable if needed.

)

// NewDriver creates a new Hyper-v driver with default settings.
Expand Down Expand Up @@ -99,6 +108,11 @@ 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",
},
}
}

Expand Down Expand Up @@ -328,12 +342,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) >= defaultTimeout {
return "", errors.New("timeout waiting for IP")
}

time.Sleep(1 * time.Second)
}
}
Expand All @@ -342,6 +361,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 +372,10 @@ func (d *Driver) waitStopped() error {
return nil
}

if time.Since(start) >= defaultTimeout {
return errors.New("timeout waiting for stopped")
}

time.Sleep(1 * time.Second)
}
}
Expand Down Expand Up @@ -428,6 +452,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, ":") < 2
}

func (d *Driver) GetIP() (string, error) {
s, err := d.GetState()
if err != nil {
Expand All @@ -437,7 +465,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 +475,33 @@ 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:
for _, ipStr := range resp {
ip := net.ParseIP(ipStr)
if ip.IsGlobalUnicast() {
return ipStr, nil
}
}
}

return "", nil
}

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