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

[ajwdev]

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:

1. 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.

2. 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"
}

3. 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]

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]

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]

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]

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

1. 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.
2. 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"
3. 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]

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

[justinsb]

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]

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]

ok to test

[googlebot]

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]

CLAs look good, thanks!

[ajwdev]

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]

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

[ajwdev]

Squashed and re-pushed

[justinsb]

@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?

[justinsb]

Re-adding LGTM after rebase

[justinsb]

@k8s-bot unit test this flake #26345

[justinsb]

@k8s-bot unit test this flake #26345

[k8s-github-robot]

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

[k8s-bot]

GCE e2e build/test passed for commit 01d9cdd.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

Automatic merge from submit-queue

[felixbuenemann]

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]

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]

@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]

cc @kubernetes/sig-federation-misc