AWS: support node port health check #43585

foolusion · 2017-03-23T20:14:16Z

What this PR does / why we need it:
if a custom health check is set from the beta annotation on a service it
should be used for the ELB health check. This patch adds support for
that.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Let me know if any tests need to be added.
Release note:

if a custom health check is set from the beta annotation on a service it should be used for the ELB health check. This patch adds support for that.

k8s-ci-robot · 2017-03-23T20:14:23Z

Hi @foolusion. 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 @k8s-bot 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.

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-reviewable · 2017-03-23T20:15:26Z

This change is

jsafrane · 2017-03-24T08:16:14Z

@k8s-bot ok to test

jsafrane · 2017-03-24T08:18:29Z

I don't know anything about load balancers and AWS health checks.
/assign @justinsb
/unassign @jsafrane

foolusion · 2017-03-29T23:00:32Z

@justinsb can you please take a look

foolusion · 2017-04-06T14:16:52Z

@jsafrane @justinsb can I get an update on this?

justinsb · 2017-04-19T05:29:44Z

Can you describe what you're trying to archive @foolusion ?

I think this annotation was intended for use with ESIPP - so it's actually probably good to get it in. But @thockin can you comment on whether we should be supporting this annotation separately from ESIPP, and who is looking after this?

I'll comment on a potential code refactoring directly on the code :-)

(Edit: Or is this in fact all that is needed to support ESIPP on AWS?)

justinsb · 2017-04-19T05:31:27Z

pkg/cloudprovider/providers/aws/aws.go

+			return nil, fmt.Errorf("Failed to ensure health check for localized service %v on node port %v: %v", loadBalancerName, healthCheckNodePort, err)
+		}
+	} else {
+		glog.V(4).Infof("service %v does not need health checks", apiService.Name)


Probably better to say "does not need custom health checks" or "uses default health checks" or similar

justinsb · 2017-04-19T05:34:15Z

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

+
+	expectedTarget := "HTTP:" + strconv.FormatInt(int64(port), 10) + path
+
+	if expectedTarget == orEmpty(actual.Target) &&


AFAICT from this point on the two health-check configuration functions are the same (there's a lot of overlap earlier also!) Can we combine the two tails here into a single function? Maybe even just have one function, and build the expected healthCheck in EnsureLoadBalancer ?

foolusion · 2017-04-19T19:14:34Z

I'm currently using this to eliminate additional hops when using a load balancer in AWS. Currently the ELB only checks if it can make a connection to the service but this would allow more sophisticated health reporting.

I changed the function signature to contain protocol, port, and path. When the service has a health check path and port set it will create an HTTP health check that corresponds to the port and path. If those are not set it will create a standard TCP health check on the first port from the listeners that is not nil. As far as I know, there is no way to tell if a Health Check should be HTTP vs HTTPS.

foolusion · 2017-04-19T21:30:29Z

@justinsb I've combined those two methods and updated the log output. The main change is moving the logic for TCP health check into the aws.go EnsureLoadBalacner method and changing the function signature of ensureLoadBalancerHealthCheck. Please take another look.

emmanuel · 2017-05-26T22:02:32Z

We are very interested in this feature. Is there anything we can do to help get this merged?

thockin · 2017-05-26T23:22:07Z

@MrHohn @nicksardo

…

On Fri, May 26, 2017 at 3:02 PM, Emmanuel Gomez ***@***.***> wrote: We () are very interested in this feature. Is there anything we can do to help get this merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43585 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVDCy37Kew5A32uonzxvOPB1Vehm3ks5r90wKgaJpZM4MnTZZ> .

emmanuel · 2017-05-27T04:03:20Z

I should add that I (used to) work with @foolusion and that I believe he won't be available to speak up for this PR any further (he's off on other, wonderful, adventures). So I'm roughly proposing that I, @dhawal55, @SleepyBrett and @mariusgrigoriu carry this PR forward in his stead (all coworkers of foolusion's, till recently).

MrHohn · 2017-05-27T06:51:30Z

@emmanuel This is great, I'd happy to see folks are extending the OnlyLocal feature (kubernetes/enhancements#27) to AWS :)

MrHohn · 2017-05-27T07:05:02Z

To better clarify current status of ESIPP (External Source IP Preservation):

ESIPP is going to GA in v1.7 - Promotes Source IP preservation for Virtual IPs from Beta to GA #41162.
The beta annotations (service.beta.kubernetes.io/external-traffic and service.beta.kubernetes.io/healthcheck-nodeport) have been promoted as first class fields (Service.Spec.ExternalTrafficPolicy and Service.Spec.HealthCheckNodePort).
For now this feature is only supported on GCE.

I'm not sure how would AWS LB behave when forwarding traffic onto nodes (preserving source IP or not). But only for preventing the second hop, I think having the health check properly set up on LB would be sufficient.

foolusion · 2017-05-27T13:01:56Z

I'm still partially available (when i have wifi), just in GMT timezone now. Let me know if you need me to make any further changes.

thockin · 2017-05-27T16:54:39Z

Just to be clear. ESIPP is what Google called it, because that was the customer demand. It doesn't preserve IP for ELB because ELB never carried the client IP in the first place (except in PROXY headers, which kube-proxy does not deal with). Only local is still useful, but it's not ESIPP. And yes, we really do need it to work on other platforms. Fragmentation bad.

…

On May 27, 2017 12:05 AM, "Zihong Zheng" ***@***.***> wrote: To better clarify current status of ESIPP (External Source IP Preservation): - ESIPP is going to GA in v1.7 - #41162 <#41162>. - The beta annotations (service.beta.kubernetes.io/external-traffic and service.beta.kubernetes.io/healthcheck-nodeport) have been promoted as first class fields (Service.Spec.ExternalTrafficPolicy and Service.Spec.HealthCheckNodePort). - For now this feature is only supported on GCE. I'm not sure how would AWS LB behave when forwarding traffic onto nodes (preserving traffic or not). But only for preventing the second hop, I think having the health check properly set up on LB would be sufficient. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43585 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVKUFHNQ-kSis9SW8vdF4O82f6Sarks5r98s0gaJpZM4MnTZZ> .

emmanuel · 2017-05-29T15:08:03Z

ESIPP sounds lovely and we'd be delighted to have it. That said, we're on AWS and (simply) eliminating the extra hop would be welcome. We're aquainted with PROXY protocol shenanigans for the ESIPP portion.

justinsb · 2017-05-29T17:37:58Z

Got it! This sounds great, and now that it is a field my concerns about using the esipp annotations seem misplaced.

/lgtm

Thanks also for the code changes!

k8s-github-robot · 2017-05-29T17:38:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: foolusion, justinsb

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/aws/OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

justinsb · 2017-05-29T21:02:11Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

k8s-github-robot · 2017-05-29T22:29:51Z

Automatic merge from submit-queue

emmanuel · 2017-05-30T19:03:41Z

What is reasonable to hope for as far as a release of this change? Is this on track to land in 1.7.0? How about a backport to 1.6.x?

emmanuel · 2017-05-30T19:04:58Z

Also, while I'm asking questions... how does this work (w/r/t kube-proxy)? Are NodePort services' iptables rules set up to forward to a host-local pod? If not, how does this eliminate the extra hop?

foolusion · 2017-05-30T19:59:02Z

@emmanuel The beta local only annotation on the service or feature external traffic local only https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/iptables/proxier.go#L193

MrHohn · 2017-05-30T20:20:18Z

Is this on track to land in 1.7.0? How about a backport to 1.6.x?

This is on track for 1.7.0 (code freeze will be on Thursday).

Also, while I'm asking questions... how does this work (w/r/t kube-proxy)? Are NodePort services' iptables rules set up to forward to a host-local pod? If not, how does this eliminate the extra hop?

Exactly, kube-proxy sets up rules to forward relevant traffic to host-local pods only. More details could be found on this proposal.

emmanuel · 2017-09-22T03:25:24Z

With the recent announcement of “Network Load Balancers” on AWS (L4 with source IP/port preservation: https://aws.amazon.com/blogs/aws/new-network-load-balancer-effortless-scaling-to-millions-of-requests-per-second/), true ESIPP is in reach on AWS.

In other news, I believe that this PR can be closed, as this code shipped in 1.7.0.

pkg/cloudprovider/providers/aws: add node port health check

864ea2f

if a custom health check is set from the beta annotation on a service it should be used for the ELB health check. This patch adds support for that.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 23, 2017

k8s-github-robot assigned jsafrane Mar 23, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 23, 2017

k8s-ci-robot assigned justinsb and unassigned jsafrane Mar 24, 2017

justinsb reviewed Apr 19, 2017

View reviewed changes

MrHohn mentioned this pull request May 27, 2017

It's not necessary for Kube Proxy to SNAT UDP Packets #37396

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2017

justinsb changed the title ~~pkg/cloudprovider/providers/aws: add node port health check~~ AWS: support node port health check May 29, 2017

justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 29, 2017

k8s-github-robot merged commit aee0ced into kubernetes:master May 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: support node port health check #43585

AWS: support node port health check #43585

foolusion commented Mar 23, 2017

k8s-ci-robot commented Mar 23, 2017

k8s-reviewable commented Mar 23, 2017

jsafrane commented Mar 24, 2017

jsafrane commented Mar 24, 2017

foolusion commented Mar 29, 2017

foolusion commented Apr 6, 2017

justinsb commented Apr 19, 2017 •

edited

Loading

justinsb Apr 19, 2017

justinsb Apr 19, 2017

foolusion commented Apr 19, 2017

foolusion commented Apr 19, 2017

emmanuel commented May 26, 2017 •

edited

Loading

thockin commented May 26, 2017 via email

emmanuel commented May 27, 2017

MrHohn commented May 27, 2017 •

edited

Loading

MrHohn commented May 27, 2017 •

edited

Loading

foolusion commented May 27, 2017

thockin commented May 27, 2017 via email

emmanuel commented May 29, 2017

justinsb commented May 29, 2017

k8s-github-robot commented May 29, 2017

justinsb commented May 29, 2017

k8s-github-robot commented May 29, 2017

emmanuel commented May 30, 2017 •

edited

Loading

emmanuel commented May 30, 2017

foolusion commented May 30, 2017

MrHohn commented May 30, 2017

emmanuel commented Sep 22, 2017


		expectedTarget := "HTTP:" + strconv.FormatInt(int64(port), 10) + path

		if expectedTarget == orEmpty(actual.Target) &&

AWS: support node port health check #43585

AWS: support node port health check #43585

Conversation

foolusion commented Mar 23, 2017

k8s-ci-robot commented Mar 23, 2017

k8s-reviewable commented Mar 23, 2017

jsafrane commented Mar 24, 2017

jsafrane commented Mar 24, 2017

foolusion commented Mar 29, 2017

foolusion commented Apr 6, 2017

justinsb commented Apr 19, 2017 • edited Loading

justinsb Apr 19, 2017

Choose a reason for hiding this comment

justinsb Apr 19, 2017

Choose a reason for hiding this comment

foolusion commented Apr 19, 2017

foolusion commented Apr 19, 2017

emmanuel commented May 26, 2017 • edited Loading

thockin commented May 26, 2017 via email

emmanuel commented May 27, 2017

MrHohn commented May 27, 2017 • edited Loading

MrHohn commented May 27, 2017 • edited Loading

foolusion commented May 27, 2017

thockin commented May 27, 2017 via email

emmanuel commented May 29, 2017

justinsb commented May 29, 2017

k8s-github-robot commented May 29, 2017

justinsb commented May 29, 2017

k8s-github-robot commented May 29, 2017

emmanuel commented May 30, 2017 • edited Loading

emmanuel commented May 30, 2017

foolusion commented May 30, 2017

MrHohn commented May 30, 2017

emmanuel commented Sep 22, 2017

justinsb commented Apr 19, 2017 •

edited

Loading

emmanuel commented May 26, 2017 •

edited

Loading

MrHohn commented May 27, 2017 •

edited

Loading

MrHohn commented May 27, 2017 •

edited

Loading

emmanuel commented May 30, 2017 •

edited

Loading