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

laozc · 2019-11-06T03:52:59Z

Description

Downstream projects like minikube may only take IPv4 addresses. This PR addresses the issue by providing a new option to limit the IP address to be returned in GetIP.

Related issue(s)

kubernetes/minikube#3963

Signed-off-by: Zhongcheng Lao <[email protected]>

gbraad · 2019-11-06T06:06:41Z

drivers/hyperv/hyperv.go

@@ -16,16 +17,23 @@ import (
 	"github.com/docker/machine/libmachine/state"
 )

+const (
+	DefaultProtocol = iota


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

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?

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.

I would not say protocol, but just Default.

Sounds good.

gbraad · 2019-11-06T06:07:00Z

drivers/hyperv/hyperv.go

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


awfully long time out...

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

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

Sure.
Let me make the timeout configurable if needed.

gbraad

Code for checking between IPv4 and IPv6 can be simplified. A lot of duplicated parsing happens.

gbraad · 2019-11-06T06:11:20Z

drivers/hyperv/hyperv.go

@@ -428,6 +452,10 @@ func (d *Driver) Kill() error {
 	return nil
 }

+func isIPv4(address string) bool {


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?

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.

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

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.

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.

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.

gbraad · 2019-11-06T06:12:22Z

drivers/hyperv/hyperv.go

+	case PreferIPv4:
+		for _, ipStr := range resp {
+			ip := net.ParseIP(ipStr)
+			if isIPv4(ipStr) && ip.To4() != nil && ip.IsGlobalUnicast() {


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

gbraad · 2019-11-06T06:14:09Z

Be aware, this does not only apply to Minikube, but also Minishift and CRC. Please make sure code does not break on our end either.

laozc · 2019-11-06T06:29:52Z

Be aware, this does not only apply to Minikube, but also Minishift and CRC. Please make sure code does not break on our end either.

Sure. Thanks for reviewing.
At the moment it defaults to the old behavior.
There will be another PR to ask minikube to use IPv4 explicitly.
The only concern I believe would be the case that the code only allows global unicast addresses.
Do you think I should remove this check in the default behavior?

gbraad · 2019-11-06T06:37:43Z

the code only allows global unicast addresses.

I noticed this, and it would not be wise to enforce this when the choice is between IPv4 and IPv6 as protocol. I believe the unicast or multicast restriction should come from the caller and not the driver. It is not the responsibility of the client... to be honest, I also do not believe the driver should actually enforce the protocol.

for Minishift and CRC the issue with IPv6 is mostly due to the SSH clients not acting correctly, and the control plane for OpenShift 3 didn't handle this correctly. I would almost say the issue should be solved in a layer higher, machine itself, to handle this. The problem is, machine does not habndle multiple addresses well... :-s

WDYT?

laozc · 2019-11-06T07:16:41Z

I agree with you that this should not be handled at the driver layer.
The current machine design assumes that the first IP returned by hypervisor would be a "good" IP but I don't see that could always stand true.

It would be great that the driver just returns a list of IPs configured on the host and upper layers choose the IP they want. Each application could have its own logic on how to choose an IP instead of asking the driver for one. But it doesn't seem like this could be done without more changes.

Not sure if it could be done on this project.

gbraad · 2019-11-06T07:25:07Z

We are allowed to make changes that make sense. At the moment drivers provide and API check, which means we can alter and raise this.

I want to know what @tstromberg thinks of these changes here.

In CRC we can modify and implement new options, but in Minishift we do not perform any maintenance :-s.

@praveenkumar, WDYT about driver specific changes? It is the easiest to perform at this point, but will certainly not solve the whole "IPv6 is an issue".

Signed-off-by: Zhongcheng Lao <[email protected]>

gbraad · 2019-11-19T09:24:28Z

drivers/hyperv/hyperv.go

+		for _, ipStr := range resp {
+			ip := net.ParseIP(ipStr)
+			if ip.IsGlobalUnicast() {
+				if preferredIP == "" {


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?

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

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

There are two differences before this change

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.

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.

gbraad · 2019-11-19T09:56:19Z

Did you also check if this behaviour on the Default Switch can be prevented with the AddressFamily and disabling DHCP on IPv6:

PS> Set-NetIPInterface -InterfaceAlias 'vEthernet (Default Switch)' -AddressFamily IPv6 -dhcp disable

gbraad · 2019-11-19T16:22:02Z

It is possible to bypass the whole IPv6 situation by disabling this on the Default Switch as mentioned above.

Remove struct member BundleName from hyperv driver struct

laozc force-pushed the network-protocol branch from 26873ec to 40d2cf2 Compare November 6, 2019 03:54

laozc added 2 commits November 6, 2019 11:57

Adds preferable network protocol for Hyper-V driver

cfb21a3

Signed-off-by: Zhongcheng Lao <[email protected]>

Adds timeout when waiting for IP and stopped

e5b2079

Signed-off-by: Zhongcheng Lao <[email protected]>

laozc force-pushed the network-protocol branch from 40d2cf2 to e5b2079 Compare November 6, 2019 03:58

gbraad reviewed Nov 6, 2019

View reviewed changes

gbraad requested changes Nov 6, 2019

View reviewed changes

gbraad requested review from praveenkumar and tstromberg November 6, 2019 06:15

gbraad self-assigned this Nov 6, 2019

gbraad added the enhancement New feature or request label Nov 6, 2019

gbraad mentioned this pull request Nov 6, 2019

Startup failure: Error getting config for native Go SSH: open The system cannot find the file specified. crc-org/crc#784

Closed

laozc mentioned this pull request Nov 14, 2019

Limit only IPv4 address used for API server with Hyper-V driver kubernetes/minikube#5841

Closed

Adds wait timeout option for Hyper-V driver

d3b4e7c

Signed-off-by: Zhongcheng Lao <[email protected]>

laozc force-pushed the network-protocol branch from 422f294 to 6e1f733 Compare November 15, 2019 13:04

Prefer IPv4 over v6 and global unicast IP by default in Hyper-V driver

d615d62

Signed-off-by: Zhongcheng Lao <[email protected]>

laozc force-pushed the network-protocol branch from 6e1f733 to d615d62 Compare November 15, 2019 13:28

gbraad reviewed Nov 19, 2019

View reviewed changes

anjannath pushed a commit to anjannath/machine that referenced this pull request Aug 29, 2022

Merge pull request machine-drivers#23 from anjannath/hyperv_bundlename

a42716b

Remove struct member BundleName from hyperv driver struct

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laozc commented Nov 6, 2019

gbraad Nov 6, 2019

laozc Nov 6, 2019 •

edited

Loading

gbraad Nov 6, 2019

gbraad Nov 6, 2019

laozc Nov 6, 2019

gbraad Nov 6, 2019

laozc Nov 6, 2019

gbraad Nov 6, 2019

laozc Nov 6, 2019

gbraad left a comment

gbraad Nov 6, 2019

laozc Nov 6, 2019

gbraad Nov 6, 2019 •

edited

Loading

gbraad Nov 6, 2019 •

edited

Loading

laozc Nov 6, 2019

laozc Nov 15, 2019

gbraad Nov 6, 2019

gbraad commented Nov 6, 2019

laozc commented Nov 6, 2019

gbraad commented Nov 6, 2019

laozc commented Nov 6, 2019

gbraad commented Nov 6, 2019

gbraad Nov 19, 2019

gbraad Nov 19, 2019

laozc Nov 20, 2019

gbraad commented Nov 19, 2019

gbraad commented Nov 19, 2019

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

Are you sure you want to change the base?

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

Conversation

laozc commented Nov 6, 2019

Description

Related issue(s)

Choose a reason for hiding this comment

laozc Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

gbraad Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Nov 6, 2019

laozc commented Nov 6, 2019

gbraad commented Nov 6, 2019

laozc commented Nov 6, 2019

gbraad commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Nov 19, 2019

gbraad commented Nov 19, 2019

laozc Nov 6, 2019 •

edited

Loading

gbraad Nov 6, 2019 •

edited

Loading

gbraad Nov 6, 2019 •

edited

Loading