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

jsturtevant · 2021-04-13T00:48:24Z

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 · 2021-04-13T00:48:26Z

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

pkg/util/node/node.go

cmd/kube-proxy/app/server_windows.go

jsturtevant · 2021-04-14T22:39:15Z

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 · 2021-04-14T22:39:37Z

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

jsturtevant · 2021-04-14T23:34:40Z

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

jsturtevant · 2021-04-15T15:07:37Z

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 · 2021-04-15T17:09:58Z

/retest

aojea · 2021-07-08T15:15:18Z

pkg/proxy/winkernel/proxier.go

+				klog.InfoS("failed to find an ip. You may need set the --bind-address flag", "err", err)
+			}
+		}
+
 		interfaces, _ := net.Interfaces() //TODO create interfaces


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

I created #103588 to track. fyi @sbangari

aojea · 2021-07-08T15:22:30Z

pkg/proxy/winkernel/proxier.go

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

+		if nodeIP.IsUnspecified() {


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

Yes it could be unspecified, in DualStack the bind address doesn't go through detectNodeIp, which only works with IPv4. This is the same on Linux: https://github.com/kubernetes/kubernetes/blob/90b02692e2980d460fa2e53bf4509df1ec10475f/cmd/kube-proxy/app/server_others.go#L239 https://github.com/kubernetes/kubernetes/blob/90b02692e2980d460fa2e53bf4509df1ec10475f/cmd/kube-proxy/app/server_windows.go#L120

sbangari · 2021-07-08T19:25:09Z

/lgtm

sbangari · 2021-07-08T19:28:59Z

pkg/proxy/winkernel/proxier.go

+		return false
+	}
+
+	return v.AtLeast(version.MustParseSemantic("11.10.0"))


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

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 · 2021-07-08T19:58:01Z

/test pull-kubernetes-integration

jsturtevant · 2021-07-08T21:27:20Z

/test pull-kubernetes-integration

jsturtevant · 2021-07-08T23:07:54Z

@aojea could you take another look?

aojea · 2021-07-09T09:05:54Z

@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 · 2021-07-15T22:45:27Z

/approve

aojea · 2021-07-15T22:51:15Z

/approve
kube-proxy bits

k8s-ci-robot · 2021-07-15T22:51:23Z

[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:

~~cmd/kube-proxy/OWNERS~~ [aojea]
~~pkg/proxy/winkernel/OWNERS~~ [aojea,sbangari]

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

jsturtevant · 2021-07-16T02:24:46Z

/retest

aravindhp · 2021-07-16T15:57:44Z

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

neolit123 · 2021-07-19T14:50:00Z

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

marosset · 2021-07-19T15:58:45Z

Yes, this should get backported to 1.21.

marosset · 2021-07-19T16:00:38Z

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

…#101047-upstream-release-1.21 Automated cherry pick of #101047: Only use dualstack if the node and config supports it

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 requested review from bowei and dims April 13, 2021 00:49

aojea reviewed Apr 13, 2021

View reviewed changes

pkg/util/node/node.go Outdated Show resolved Hide resolved

aojea reviewed Apr 13, 2021

View reviewed changes

cmd/kube-proxy/app/server_windows.go Outdated Show resolved Hide resolved

jsturtevant mentioned this pull request Apr 13, 2021

Explictly turn off Dualstack for win overlay kubernetes-sigs/cluster-api-provider-azure#1302

Merged

3 tasks

aramase reviewed Apr 13, 2021

View reviewed changes

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 force-pushed the issue-100966-dualstack-windows branch from dd8565c to d3de2d4 Compare April 14, 2021 18:03

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 force-pushed the issue-100966-dualstack-windows branch 3 times, most recently from c3d8513 to 754856b Compare April 14, 2021 22:21

jsturtevant marked this pull request as ready for review April 15, 2021 15:10

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

aojea reviewed Jul 8, 2021

View reviewed changes

jsturtevant mentioned this pull request Jul 8, 2021

Possible performance problem to iterate over the interfaces first and later over the addreses in winkernel Proxier #103588

Closed

jsturtevant force-pushed the issue-100966-dualstack-windows branch from 90b0269 to a5a4d01 Compare July 8, 2021 18:02

Only use dualstack if the node and config supports it

d5d9327

jsturtevant force-pushed the issue-100966-dualstack-windows branch from a5a4d01 to d5d9327 Compare July 8, 2021 18:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021

sbangari reviewed Jul 8, 2021

View reviewed changes

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

k8s-ci-robot merged commit 76b0906 into kubernetes:master Jul 16, 2021

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

neolit123 mentioned this pull request Jul 19, 2021

Once 1.21 is fixed we should be able to revert to defaults for kubeproxy kubernetes-sigs/sig-windows-tools#148

Closed

jsturtevant mentioned this pull request Jul 19, 2021

Automated cherry pick of #101047: Only use dualstack if the node and config supports it #103783

Merged

jsturtevant mentioned this pull request Jul 29, 2021

Could not find host mac address for 0.0.0.0 kubernetes-sigs/sig-windows-tools#159

Closed

k8s-ci-robot added a commit that referenced this pull request Sep 27, 2021

Merge pull request #103783 from jsturtevant/automated-cherry-pick-of-…

8724967

…#101047-upstream-release-1.21 Automated cherry pick of #101047: Only use dualstack if the node and config supports it

jsturtevant mentioned this pull request Oct 1, 2021

Don't explicity set DualStack for windows kubernetes-sigs/cluster-api-provider-azure#1748

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jsturtevant commented Apr 13, 2021 •

edited

k8s-ci-robot commented Apr 13, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 15, 2021

jsturtevant commented Apr 15, 2021

aojea Jul 8, 2021

jsturtevant Jul 8, 2021

aojea Jul 8, 2021

jsturtevant Jul 8, 2021

sbangari commented Jul 8, 2021

sbangari Jul 8, 2021

jsturtevant Jul 8, 2021

jsturtevant commented Jul 8, 2021

jsturtevant commented Jul 8, 2021

jsturtevant commented Jul 8, 2021

aojea commented Jul 9, 2021

sbangari commented Jul 15, 2021

aojea commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

jsturtevant commented Jul 16, 2021

aravindhp commented Jul 16, 2021 •

edited

neolit123 commented Jul 19, 2021

marosset commented Jul 19, 2021

marosset commented Jul 19, 2021 •

edited

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

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

Conversation

jsturtevant commented Apr 13, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Apr 13, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 14, 2021

jsturtevant commented Apr 15, 2021

jsturtevant commented Apr 15, 2021

aojea Jul 8, 2021

Choose a reason for hiding this comment

jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

aojea Jul 8, 2021

Choose a reason for hiding this comment

jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

sbangari commented Jul 8, 2021

sbangari Jul 8, 2021

Choose a reason for hiding this comment

jsturtevant Jul 8, 2021

Choose a reason for hiding this comment

jsturtevant commented Jul 8, 2021

jsturtevant commented Jul 8, 2021

jsturtevant commented Jul 8, 2021

aojea commented Jul 9, 2021

sbangari commented Jul 15, 2021

aojea commented Jul 15, 2021

k8s-ci-robot commented Jul 15, 2021

jsturtevant commented Jul 16, 2021

aravindhp commented Jul 16, 2021 • edited

neolit123 commented Jul 19, 2021

marosset commented Jul 19, 2021

marosset commented Jul 19, 2021 • edited

jsturtevant commented Apr 13, 2021 •

edited

aravindhp commented Jul 16, 2021 •

edited

marosset commented Jul 19, 2021 •

edited