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.

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

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

Closed
wants to merge 1 commit into from

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Nov 6, 2019

Signed-off-by: Zhongcheng Lao Zhongcheng.Lao@microsoft.com
Fixes: #3963

This is to fix the issue by only asking the Hyper-V driver to return IPv4 address for minikube.
Note: The change to Hyper-V driver is still under review. Will update the dependency once the change is merged in driver.

Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @laozc. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 6, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: laozc
To complete the pull request process, please assign josedonizetti
You can assign the PR to them by writing /assign @josedonizetti in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

I am not so sure this should be handled by the driver as it would apply to ALL hypervisors, although you have only seen this now for HyperV as the Default Switch or External Switch can return an IPv6 address in case of an error. This has also been observed for others, like VirtualBox, etc.

@tstromberg
Copy link
Contributor

I am not so sure this should be handled by the driver as it would apply to ALL hypervisors, although you have only seen this now for HyperV as the Default Switch or External Switch can return an IPv6 address in case of an error. This has also been observed for others, like VirtualBox, etc.

Is there an alternative approach you recommend?

@tstromberg
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 12, 2019
@tstromberg
Copy link
Contributor

tstromberg commented Nov 13, 2019

@gbraad - I'm not sure how else to hint to the driver to return an IPv6 network for DHCP.

@laozc - Sounds reasonable, but I would like to wait to see if this can get merged into the machine-drivers fork first instead of changing dependencies. Is there a PR for this change that you can link to for status updates?

@laozc
Copy link
Contributor Author

laozc commented Nov 14, 2019

@tstromberg Thanks. The dependency change is just for quick verification when the machine-drivers change (machine-drivers/machine#23) has not been changed. I'm still working on verifying some of the IPv6 stack behavior in the Hyper-V driver. Once it's done I will push for a new change for both PR.

@gbraad
Copy link
Contributor

gbraad commented Nov 19, 2019

You can disable IPv6 as DHCP using:

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

This way, only IPv4 will be assigned to a host on the Default Switch

@gbraad
Copy link
Contributor

gbraad commented Nov 19, 2019

@gbraad - I'm not sure how else to hint to the driver to return an IPv6 network for DHCP.

That is kind of the problem. This issue could occur for any hypervisor that is used, if the network layer is configured to use IPv6. It is a generic issue in how libmachine does not handle multipe IP address assignment to the VM. At the moment it just return the first address from the first interface. The question would be use a workaround in code, to accomodate a possible timing issue or error in the Hyper-V networking stack? Or in this case instruct people to configure the network to prevent his to happen? Or refactor parts of the machine code to allow the use of multiple network addresses. I agree, the workaround seems the simplest, but at the moment the PR that is proposed does not match default behaviour and might cause unexpected other issues. So far, disabling the IPv6 assignment on the switch sounds like in line with what Libvirt and Hyperkit do at the moment...

... but, for how long? Would we in the end start putting preferred options in each and every driver we use? It would sounds more logical to solve the overall issue in that case and do the final selection in the consumer.

@gbraad
Copy link
Contributor

gbraad commented Nov 19, 2019

I took some more time and went over issues for Minikube on Windows, and noticed a pattern of network related issues. For instance Windows 10 Enterprise does not come with a Default Switch option, but this is now assumed as the fallback option in your codebase (as the machine-drivers/machine was made to default to this switch). To avoid this, in CRC we have the option of a named alternative, that is hardcoded by name, and when not found we try to detect the Default Switch. This happens before the VM is created. So if not found, we fail early (and not assume a possible outcome from the libmachine library).

Also, to solve #3963, which is caused by the IPv6 configuration, to use the Default Switch, as it might prevent misconfigurations. However, while it might ease the deployment, this is not a worry-free solution. It is based on the ICS Internet Connection Sharing option and so far I have seen issues with unanswered DHCPRequests, re-use not being tolerated as a DHCPOffer is expected (therefore any communication might fail), and the fact that the DHCP addressfamily implementations seem to operate as two different servers, which could cause an issue with IPv6 being assigned first and/or IPv4 never being offered at all. So the code suggested to machine is only partially solving this in case the timing might have offered both but in the wrong order.

@tstromberg
Copy link
Contributor

Please re-propose when we can do this without switching to a new forked machine driver.

@tstromberg tstromberg closed this Dec 9, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2019
@k8s-ci-robot
Copy link
Contributor

@laozc: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hyperv: cannot use "fe80::215:5dff:fe45:3110" as the bind address for the API Server (IPv6)
5 participants