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

[sig-windows] update winkernel to only use dualstack if the node and config supports it #101047

Merged

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Apr 13, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:

Overlay (VXLAN) networks on Windows do not support dual-stack networking today, so we should not use the dualstack proxy. Additionally the dual stack proxier should only be created if the host supports it.

This fixes a couple issues:

  • --bind-address flag was not respected for Windows single stack
  • dual-stack should not be used when the networking stack does not support it
  • dual-stack should not be used when overlay is enabled as it is not supported
  • unspecified ip addresses are not supported in overlay, if address is unspecified will attempt to get bind address for mac address which is required in overlay mode

Which issue(s) this PR fixes:

Fixes #100966

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix winkernel kube-proxy to only use dual stack when host and networking supports it

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Apr 13, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 13, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 13, 2021
@k8s-ci-robot k8s-ci-robot requested review from bowei and dims Apr 13, 2021
pkg/util/node/node.go Outdated Show resolved Hide resolved
cmd/kube-proxy/app/server_windows.go Outdated Show resolved Hide resolved
cmd/kube-proxy/app/server_windows.go Outdated Show resolved Hide resolved
cmd/kube-proxy/app/server_windows.go Outdated Show resolved Hide resolved
@jsturtevant jsturtevant force-pushed the issue-100966-dualstack-windows branch from dd8565c to d3de2d4 Compare Apr 14, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 14, 2021
@jsturtevant jsturtevant force-pushed the issue-100966-dualstack-windows branch 3 times, most recently from c3d8513 to 754856b Compare Apr 14, 2021
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Apr 14, 2021

Updated the PR description for the things addressed.

I've tested this on a WS 2019 overlay host, 2019 l2bridge host and WS 2004 host.

On the WS 2004 host the if supportedFeatures.IPv6DualStack return false which was surprising. It looks like the WS 2004 host is version is not with in the range specified as supported: https://github.com/microsoft/hcsshim/blob/d9474d26c57bed6081b3941dd7980d5e8457148e/hcn/hcnglobals.go#L56-L60

cmd /c ver

Microsoft Windows [Version 10.0.19041.867]

Add logging:

I0414 22:35:15.143135    3372 proxier.go:202] hns version%!(EXTRA hcn.Version={11 10})                                                                                                    
W0414 22:35:15.143135    3372 proxier.go:203] This version of Windows does not support dual-stack. Falling back to single-stack    

I am going to follow up to find out if what the correct version of support should be because the docs state: 2004 (kernel version 10.0.19041.610)

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Apr 14, 2021

/test pull-kubernetes-e2e-aks-engine-azure-windows
/test pull-kubernetes-e2e-aks-engine-windows-containerd

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Apr 14, 2021

pull-kubernetes-e2e-aks-engine-azure-windows was renamed
/test pull-kubernetes-e2e-aks-engine-windows-dockershim

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Apr 15, 2021

the failure on pull-kubernetes-e2e-aks-engine-windows-dockershim was an issue with cni: Azure/azure-container-networking#774 (comment)

/test pull-kubernetes-e2e-aks-engine-windows-dockershim

@jsturtevant jsturtevant marked this pull request as ready for review Apr 15, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2021
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Apr 15, 2021

/retest

klog.InfoS("failed to find an ip. You may need set the --bind-address flag", "err", err)
}
}

interfaces, _ := net.Interfaces() //TODO create interfaces
Copy link
Member

@aojea aojea Jul 8, 2021

Choose a reason for hiding this comment

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

This is a todo, it has a performance problem to iterate over the interfaces first and later over the addreses
#103116

Copy link
Contributor Author

@jsturtevant jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

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

I created #103588 to track. fyi @sbangari

@@ -610,6 +692,15 @@ func NewProxier(
return nil, fmt.Errorf("source-vip flag not set")
}

if nodeIP.IsUnspecified() {
Copy link
Member

@aojea aojea Jul 8, 2021

Choose a reason for hiding this comment

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

Since we are using detectNodeIP that deals with unspecified, is it possible to get an unspecified here?
I couldn't check all code paths, just asking

Copy link
Contributor Author

@jsturtevant jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

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

@jsturtevant jsturtevant force-pushed the issue-100966-dualstack-windows branch from a5a4d01 to d5d9327 Compare Jul 8, 2021
@sbangari
Copy link
Contributor

sbangari commented Jul 8, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2021
return false
}

return v.AtLeast(version.MustParseSemantic("11.10.0"))
Copy link
Contributor

@sbangari sbangari Jul 8, 2021

Choose a reason for hiding this comment

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

We should try fixing this in hcsshim to avoid this version check across other components (like here in kube-proxy and possibly CNI's) ... we can merge this PR but should revert this change back once this is fixed in hcsshim

Copy link
Contributor Author

@jsturtevant jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

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

In 1.23 I can update the hcsshim library and this logic will no longer be required because it will have the correct checks internally.

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Jul 8, 2021

/test pull-kubernetes-integration

1 similar comment
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Jul 8, 2021

/test pull-kubernetes-integration

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Jul 8, 2021

@aojea could you take another look?

@aojea
Copy link
Member

aojea commented Jul 9, 2021

@aojea could you take another look?

I can't say much about the windows logic and I'm out until next week, I think that overall is ok, so don't wait for me if you want to get it merged today

@sbangari
Copy link
Contributor

sbangari commented Jul 15, 2021

/approve

@aojea
Copy link
Member

aojea commented Jul 15, 2021

/approve
kube-proxy bits

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, jsturtevant, sbangari

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2021
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Jul 16, 2021

/retest

@k8s-ci-robot k8s-ci-robot merged commit 76b0906 into kubernetes:master Jul 16, 2021
15 checks passed
SIG-Windows automation moved this from In Review (v1.22) to Done (v1.21) Jul 16, 2021
Kube-proxy cleanup automation moved this from In Review to Done/Closed Jul 16, 2021
@aravindhp
Copy link

aravindhp commented Jul 16, 2021

@jsturtevant thanks for fixing this. Shouldn't this be backported to all affected releases?

@neolit123
Copy link
Member

neolit123 commented Jul 19, 2021

@jsturtevant i'm assuming we need to backport this to 1.21?

@marosset
Copy link
Contributor

marosset commented Jul 19, 2021

Yes, this should get backported to 1.21.

@marosset
Copy link
Contributor

marosset commented Jul 19, 2021

I added a card on our board to track backporting
https://github.com/orgs/kubernetes/projects/8#card-65265573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging this pull request may close these issues.

winkernel kube-proxy problems when IPv6DualStack is enabled (ie, in 1.21)