AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569

ajwdev · 2016-04-20T22:11:24Z

This is a ~~work in progress~~ branch that adds support for the Proxy Protocol with Elastic Load Balancers. The proxy protocol is documented here: http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt. It allows us to pass the "real ip" address of a client to pods behind services.

As it stands now, we create an ELB policy on the load balancer that enables the proxy protocol. We then enumerate each node port assigned to the load balancer and add our newly created policy to it. The manual process is documented here: http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/enable-proxy-protocol.html

Right now, I’m looking to get some feedback on the approach before I dive too much deeper in the code. More precisely, I have questions regarding the following:

Right now I just check that a certain annotation exists on the service regardless of what its value is. Assuming we’re going to enable this feature via an annotation, what is the expected experience? This decision likely depends on the answers to the next questions.
Right now the implementation enables the proxy protocol on every ELB backend. The actual ELB API expects you to add the policy for each configured backend. Do we want the ability to configure the proxy protocol on a per service port basis? For example, if a service exposes TCP 80 and 443, would we want the ability to only enable the proxy protocol on port 443? Does this overcomplicate the implementation? If we wanted to go this direction we could do something like ...

{
  "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol": "tcp:80,tcp:443"
}

I avoided this because I was concerned with scope creep and our organization doesn’t need it, but could/should our implementation be adjusted to just handle ELB policies in general? I hadn’t used the ELB API until I started working on this branch so I don’t know how realistic this is. I also don't know how common this use case is as our organization has used our own load balancing setup prior to Kubernetes. This page has a couple of examples at the bottom: http://docs.aws.amazon.com/cli/latest/reference/elb/create-load-balancer-policy.html

cc @justinsb

This change is

k8s-bot · 2016-04-20T22:12:14Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-04-20T22:12:31Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-04-20T22:13:14Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

justinsb · 2016-04-24T21:45:24Z

About to look at the code, but I thought I would tackle your questions first. My viewpoints (and I emphasize that these only my opinion):

I would do something like this: https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L67-L70 We require a specific value, because that lets us add extensions later.
I would start by just enabling proxy-protocol on all ports. But we can choose the required value so that we can accommodate this feature in future, if we need it. For example, the default might be "*", and then in future we might allow a user to specify just the ports on which it should be enabled "80,443"
It's good to consider it, but my view is that they are sufficiently different that they would be different annotations. They happen to share an API call in AWS, but otherwise they are unrelated. So I think we can scope this only to proxy-protocol or not (I don't know if there's any particular attribute you saw that you felt was particularly closely related - they seemed mostly to be SSL policy stuff?)

justinsb · 2016-04-24T21:46:51Z

pkg/cloudprovider/providers/aws/aws.go

@@ -2218,7 +2231,15 @@ func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string, a
 	}

 	// Build the load balancer itself
-	loadBalancer, err := s.ensureLoadBalancer(serviceName, loadBalancerName, listeners, subnetIDs, securityGroupIDs, internalELB)
+	loadBalancer, err := s.ensureLoadBalancer(


Yeah... probably time we did this, or thought about creating a structure for the values. This is good anyway :-)

justinsb · 2016-04-24T22:07:10Z

Overall this looks really good. I agree that we should DRY the listener policy logic, and I think lazy creation of the policy would be better. Let me know if you disagree, or if you hit any snags with that and I'd be happy to take a look. But I think the big picture is spot-on.

k8s-bot · 2016-05-06T22:10:17Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

justinsb · 2016-05-10T12:47:19Z

ok to test

googlebot · 2016-05-13T22:00:47Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-05-13T22:05:00Z

CLAs look good, thanks!

ajwdev · 2016-05-26T02:21:58Z

Thanks @justinsb. Will I need to squash before we merge into HEAD?

EDIT: Just saw your comment in Slack. I'll squash for cleanliness

justinsb · 2016-05-26T02:26:28Z

I believe so @williamsandrew - would you mind squashing and I'll re-LGTM

ajwdev · 2016-05-26T02:42:09Z

Squashed and re-pushed

justinsb · 2016-05-31T13:15:35Z

@williamsandrew this is failing because of a concurrent merge that went in, that changed the signature of EnsureLoadBalancer, removing the annotations argument. Git didn't detect it as a merge conflict though.

I believe this diff is sufficient:

diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go
index a977d07..17b4853 100644
--- a/pkg/cloudprovider/providers/aws/aws.go
+++ b/pkg/cloudprovider/providers/aws/aws.go
@@ -2187,7 +2187,7 @@ func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string) (

        // Determine if we need to set the Proxy protocol policy
        proxyProtocol := false
-       proxyProtocolAnnotation := annotations[ServiceAnnotationLoadBalancerProxyProtocol]
+       proxyProtocolAnnotation := apiService.Annotations[ServiceAnnotationLoadBalancerProxyProtocol]
        if proxyProtocolAnnotation != "" {
                if proxyProtocolAnnotation != "*" {
                        return nil, fmt.Errorf("annotation %q=%q detected, but the only value supported currently is '*'", ServiceAnnotationLoadBalancerProxyProtocol, proxyProtocolAnnotation)

Any chance you can apply this (or something similar), squash and repush?

Add ELB proxy protocol support via the annotation "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol". This allows servers like Nginx and Haproxy to retrieve the real IP address of a remote client.

justinsb · 2016-05-31T15:40:44Z

Re-adding LGTM after rebase

justinsb · 2016-05-31T16:09:09Z

@k8s-bot unit test this flake #26345

justinsb · 2016-05-31T17:07:25Z

@k8s-bot unit test this flake #26345

k8s-github-robot · 2016-05-31T18:44:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-31T19:26:09Z

GCE e2e build/test passed for commit 01d9cdd.

k8s-github-robot · 2016-05-31T19:37:57Z

Automatic merge from submit-queue

felixbuenemann · 2016-08-11T04:21:15Z

Has there been any work to make this feature work with specific ports only?

I'd love to use this with DEIS Workflow, but can't because it forwards ports to multiple services from an ELB load balancer, but only http and https should use proxy protocol.

ajwdev · 2016-08-11T18:01:54Z

I haven't done any work on it, not sure if anyone else has. I changed jobs and my new team doesn't use Kubernetes (yet). I've considered working on this for a weekend project though. I think it should be pretty straight forward to fix.

therc · 2016-08-11T22:30:01Z

@felixbuenemann The SSL annotations support that, so maybe you can look at their code to see how it does things. I wrote it, but it's more than two weeks since, so I don't remember anything. :-)

ghost · 2017-01-31T22:18:14Z

cc @kubernetes/sig-federation-misc

googlebot added the cla: yes label Apr 20, 2016

k8s-github-robot assigned justinsb Apr 20, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 20, 2016

justinsb mentioned this pull request Apr 24, 2016

AWS: SSL support for ELB listeners through annotations #23495

Merged

justinsb reviewed Apr 24, 2016
View reviewed changes

justinsb mentioned this pull request Apr 29, 2016

AWS: Proposed set of annotations for ELB "advanced features" #24978

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2016

ajwdev force-pushed the elb-proxy-protocol branch from 0335ee9 to 73135b2 Compare May 13, 2016 22:00

googlebot added cla: no and removed cla: yes labels May 13, 2016

ajwdev force-pushed the elb-proxy-protocol branch from 73135b2 to 6c7ec06 Compare May 13, 2016 22:04

googlebot added cla: yes and removed cla: no labels May 13, 2016

justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016

ajwdev force-pushed the elb-proxy-protocol branch from 57780d9 to d718f39 Compare May 26, 2016 02:40

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016

davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016

justinsb added area/platform/aws priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 27, 2016

wojtek-t added ok-to-merge and removed needs-ok-to-merge labels May 29, 2016

Add Amazon ELB proxy protocol support

01d9cdd

Add ELB proxy protocol support via the annotation "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol". This allows servers like Nginx and Haproxy to retrieve the real IP address of a remote client.

ajwdev force-pushed the elb-proxy-protocol branch from d718f39 to 01d9cdd Compare May 31, 2016 15:33

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2016

justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2016

k8s-github-robot merged commit 52cc96d into kubernetes:master May 31, 2016

ohaiwalt mentioned this pull request Jan 26, 2017

Documentation for ProxyProtocol with AWS ELB to get Client IP kubernetes/ingress-nginx#175

Closed

casidiablo mentioned this pull request Dec 31, 2018

Ingress not working on AWS. canonical/microk8s#142

Closed

asiegman mentioned this pull request Oct 31, 2019

[nginx-ingress] Add support for PROXY protocol cloudposse/helmfiles#199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569

AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569

ajwdev commented Apr 20, 2016 •

edited

Loading

k8s-bot commented Apr 20, 2016

k8s-bot commented Apr 20, 2016

k8s-bot commented Apr 20, 2016

justinsb commented Apr 24, 2016

justinsb Apr 24, 2016

justinsb commented Apr 24, 2016

k8s-bot commented May 6, 2016

justinsb commented May 10, 2016

googlebot commented May 13, 2016

googlebot commented May 13, 2016

ajwdev commented May 26, 2016 •

edited

Loading

justinsb commented May 26, 2016

ajwdev commented May 26, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

k8s-github-robot commented May 31, 2016

k8s-bot commented May 31, 2016

k8s-github-robot commented May 31, 2016

felixbuenemann commented Aug 11, 2016 •

edited

Loading

ajwdev commented Aug 11, 2016

therc commented Aug 11, 2016 •

edited

Loading

ghost commented Jan 31, 2017

AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569

AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569

Conversation

ajwdev commented Apr 20, 2016 • edited Loading

k8s-bot commented Apr 20, 2016

k8s-bot commented Apr 20, 2016

k8s-bot commented Apr 20, 2016

justinsb commented Apr 24, 2016

justinsb Apr 24, 2016

Choose a reason for hiding this comment

justinsb commented Apr 24, 2016

k8s-bot commented May 6, 2016

justinsb commented May 10, 2016

googlebot commented May 13, 2016

googlebot commented May 13, 2016

ajwdev commented May 26, 2016 • edited Loading

justinsb commented May 26, 2016

ajwdev commented May 26, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

justinsb commented May 31, 2016

k8s-github-robot commented May 31, 2016

k8s-bot commented May 31, 2016

k8s-github-robot commented May 31, 2016

felixbuenemann commented Aug 11, 2016 • edited Loading

ajwdev commented Aug 11, 2016

therc commented Aug 11, 2016 • edited Loading

ghost commented Jan 31, 2017

ajwdev commented Apr 20, 2016 •

edited

Loading

ajwdev commented May 26, 2016 •

edited

Loading

felixbuenemann commented Aug 11, 2016 •

edited

Loading

therc commented Aug 11, 2016 •

edited

Loading