Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

jeremyje · 2020-12-01T23:49:10Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
In GCE, the current externalTrafficPolicy: Local logic does not work because the nodes that run the pods do not setup load balancer ports. This means that the GCLB does not understand which nodes are serving the pods that can accept traffic. Since all report unhealthy it'll direct traffic to any node. This PR configures the health check ports so that GCLB knows which nodes can handle the traffic.

See #62046 (comment) for details.

Which issue(s) this PR fixes:

Fixes #62046

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`externalTrafficPolicy: local` works on Windows hosts with DSR support.

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

/cc @elweb9858 @daschott

k8s-ci-robot · 2020-12-01T23:49:14Z

@jeremyje: GitHub didn't allow me to request PR reviews from the following users: daschott.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind bug

What this PR does / why we need it:
In GCE, the current externalTrafficPolicy: Local logic does not work because the nodes that run the pods do not setup load balancer ports. This means that the GCLB does not understand which nodes are serving the pods that can accept traffic. Since all report unhealthy it'll direct traffic to any node. This PR configures the health check ports so that GCLB knows which nodes can handle the traffic.

See #62046 (comment) for details.

Which issue(s) this PR fixes:

Fixes #62046

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
`externalTrafficPolicy: local` works on Windows hosts with DSR support.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @elweb9858 @daschott

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 · 2020-12-01T23:49:19Z

Hi @jeremyje. 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 · 2020-12-01T23:50:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jeremyje
To complete the pull request process, please assign andrewsykim after the PR has been reviewed.
You can assign the PR to them by writing /assign @andrewsykim 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:

pkg/proxy/winkernel/OWNERS
test/e2e/network/OWNERS

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

jeremyje · 2020-12-01T23:55:36Z

/sig windows

pjh · 2020-12-02T00:16:11Z

/ok-to-test

pjh · 2020-12-02T00:16:23Z

/priority important-soon

pjh · 2020-12-02T00:16:33Z

/triage accepted

jeremyje · 2020-12-02T05:04:10Z

/retest

jeremyje · 2020-12-02T17:52:22Z

test/e2e/network/service.go

@@ -909,9 +909,10 @@ var _ = SIGDescribe("Services", func() {
 		framework.ExpectNoError(err, "failed to validate endpoints for service %s in namespace: %s", serviceName, ns)
 	})

-	ginkgo.It("should preserve source pod IP for traffic thru service cluster IP [LinuxOnly]", func() {
+	ginkgo.It("should preserve source pod IP for traffic thru service cluster IP", func() {


This test does not pass locally. I'm not sure why yet.

I don't think this test is related. It's creating clusterIP service and check if source ip is preserved. This PR's case is from LoadBalancer.

jeremyje · 2020-12-02T17:53:03Z

pkg/proxy/winkernel/proxier_test.go

@@ -765,6 +769,11 @@ func TestCreateDsrLoadBalancer(t *testing.T) {
 		if svcInfo.localTrafficDSR != true {
 			t.Errorf("Failed to create DSR loadbalancer with local traffic policy")
 		}
+		if len(svcInfo.loadBalancerIngressIPs) == 0 {


This test condition fails. Looking at the structs it's not clear what should actually be compared here or what the expectation is.

The fake needs to be changed a lit.

jeremyje · 2020-12-02T17:56:02Z

pkg/proxy/winkernel/proxier.go

@@ -926,6 +929,7 @@ func (proxier *Proxier) syncProxyRules() {

 	hnsNetworkName := proxier.network.name
 	hns := proxier.hns
+	cbr0HnsEndpoint, _ := hns.getEndpointByName("cbr0")


cbr0 is GCP specific (https://kubernetes.io/docs/concepts/cluster-administration/networking/#google-compute-engine-gce), can this be generalized?

It also looks like this can collide because there could be multiple endpoints with the same name.

yes I don't think we should be putting GCP specific logic here. where is the current cbr0 endpoint being created?

We do not usually apply policies to endpoints other than the one that is part of the service.

What is the actual scenario/data path are you trying to enable with this fix?
Can you please describe? That would help us figure out what is the correct solution here.

It is trying to let the GCE load balancer health check the nodes. The way GCE load balancer works is it doesn't do DNAT. Instead of getting <node-ip>:<health-check-node-port>, the node gets <service-vip>:<health-check-node-port>. so we need to do this to forward the health check package the kube-proxy's internal health check app.

I have another PR open for further review: #99287. Let's move discussion there.

elweb9858 · 2020-12-07T18:50:36Z

@madhanrm PTAL

jeremyje · 2020-12-16T19:56:15Z

/cc @anfernee

k8s-ci-robot · 2021-02-19T22:54:58Z

@jeremyje: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind	`1c31e33`	link	`/test pull-kubernetes-e2e-kind`
pull-kubernetes-e2e-ubuntu-gce-network-policies	`1c31e33`	link	`/test pull-kubernetes-e2e-ubuntu-gce-network-policies`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot · 2021-04-06T02:58:29Z

@jeremyje: 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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Dec 1, 2020

k8s-ci-robot requested a review from elweb9858 December 1, 2020 23:49

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2020

k8s-ci-robot added 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. labels Dec 1, 2020

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Dec 1, 2020

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 Dec 2, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 2, 2020

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 2, 2020

jeremyje commented Dec 2, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2020

immuzz added this to In Progress (v1.21) in SIG-Windows Dec 17, 2020

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local

1c31e33

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2021

anfernee mentioned this pull request Feb 21, 2021

Add HNS Load Balancer Healthchecks for ExternalTrafficPolicy: Local #99287

Merged

immuzz moved this from In Progress (v1.21) to InProgress (v1.22) in SIG-Windows Mar 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2021

jeremyje closed this Apr 14, 2021

SIG-Windows automation moved this from InProgress (v1.22) to Done (v1.21) Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

jeremyje commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

jeremyje commented Dec 1, 2020

pjh commented Dec 2, 2020

pjh commented Dec 2, 2020

pjh commented Dec 2, 2020

jeremyje commented Dec 2, 2020

jeremyje Dec 2, 2020

anfernee Feb 21, 2021

jeremyje Dec 2, 2020

anfernee Feb 21, 2021

jeremyje Dec 2, 2020

jeremyje Dec 2, 2020

elweb9858 Dec 2, 2020

madhanrm Dec 7, 2020

anfernee Feb 21, 2021

elweb9858 commented Dec 7, 2020

jeremyje commented Dec 16, 2020 •

edited

k8s-ci-robot commented Feb 19, 2021

k8s-ci-robot commented Apr 6, 2021

Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

Add HNS Load Balancer Health Checks for ExternalTrafficPolicy: Local #96998

Conversation

jeremyje commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

k8s-ci-robot commented Dec 1, 2020

jeremyje commented Dec 1, 2020

pjh commented Dec 2, 2020

pjh commented Dec 2, 2020

pjh commented Dec 2, 2020

jeremyje commented Dec 2, 2020

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

Choose a reason for hiding this comment

elweb9858 commented Dec 7, 2020

jeremyje commented Dec 16, 2020 • edited

k8s-ci-robot commented Feb 19, 2021

k8s-ci-robot commented Apr 6, 2021

jeremyje commented Dec 16, 2020 •

edited