cc @justinsb

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

I'm working on tests, but I thought I'd submit the new code before the old one gets looked at. I've implemented the protocol=certARN syntax, added checks and errors, moved the listener creation to an earlier stage so that we don't leave security groups behind and added comments/pointers.

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

I added tests. I gave up on trying FakeELB-based tests: I simply added tests for the new getListener() function (another good reason to factor it out of EnsureLoadBalancer).

Nice. A few thoughts:

1. Is HTTP vs TCP orthogonal here? e.g. should it maybe be a different annotation. There is also another WIP PR (AWS: ELB proxy protocol support via annotation service.beta.kubernetes.io/aws-load-balancer-proxy-protocol #24569) to add PROXY protocol, so I'd like to figure out what our final set of annotations should be (once we have SSL, PROXY, HTTP etc).
2. Do we want to support http & https? Even if the http listener is only going to redirect to https, I think we still need it. But I'm also not sure exactly how we solve this cleanly within the framework we have.

Random thoughts (no action required):

1. I'm wondering if we should use the arn, or use tags to create something easier to manage. But if you're requiring a full arn e.g. arn:aws:iam::123456789012:server-certificate/company/servercerts/ProdServerCert then that works fine because we can always look at the arn: prefix.
2. A long-term goal of k8s (as I understand it) is to allow multiple untrusted tenants to share a k8s cluster. So being able to use any cert you know the name of is not ideal. But I imagine we would solve this with an ACL tag on the cert, or an ACL entry when we implement real ACLs.

I'm wary of needing multiple annotations (cert, http/tcp) just for the SSL certificate. I do agree we just keep all of the ELB annotations in sync.

As for http, maybe we can have a prefix or suffix indicating the creation of a plain text listener, too. I think our default use case should be HTTPS/SSL only. Redirects are usually a bad idea, see e.g. http://www.vinaysahni.com/best-practices-for-a-pragmatic-restful-api#ssl

I think that in the long run an Ingress with all the necessary checks is the way to go for multiple tenancy.

Hey all, just weighing in...

@justinb: Do we want to support http & https? Even if the http listener is only going to redirect to https, I think we still need it. But I'm also not sure exactly how we solve this cleanly within the framework we have.
This is a definite must IMO. Doesn't necessarily need to be there first cut, but I would say that a lot of people (including myself) would expect this ability and get burned without it.

Using tags or other annotations are totally fine IMO, even if it's a little messy in that you need to wire a few of them together to make it work.

I split the single annotation into aws-load-balancer-ssl-cert / aws-load-balancer-backend-protocol=(https|http|ssl|tcp) and updated the PR description. Mixed listeners will be implemented separately.

A few nits which you can fix or not as you see fit.

The default value for protocol though I think is more important... I think it would be surprising to set the ssl-cert annotation and have the protocol switch to HTTP.

(otherwise LGTM)

I fixed all nits and changed the default.

LGTM :-)

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

GCE e2e build/test passed for commit 5933440.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from submit-queue

@therc This looks great - thanks for your work on this. When testing this out, I tried to set up the following:

ELB HTTPS/:443 -> Service HTTP listener port
ELB HTTP/:80 -> Service HTTP listener port

by setting:

metadata:
    annotations:
        service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:us-east-1:xxxxx:certificate/xxxxx
        service.beta.kubernetes.io/aws-load-balancer-backend-protocol: http
spec:
  type: LoadBalancer
  ports:
    - port: 80
      targetPort: 8000
      protocol: TCP
      name: http
    - port: 443
      targetPort: 8000
      protocol: TCP
      name: https

From this setup, I got:

ELB HTTPS/:443 -> HTTP
ELB HTTPS/:80 -> HTTP

Is this expected behavior? If not, is there a way this can be set up to allow HTTP connections over 80 and not HTTPS?

@dkozma that's expected, but #26268 will take care of your case.

@therc Perfect, thanks!

@dkozma would you be able to test #26976?

@therc I can spin up a cluster tomorrow and let you know the results.

@therc Sorry for the delay on this. Just built your branch and spun up a new cluster and everything works great - I have ELB listeners for 80 and 443 working correctly.. Thank you for your efforts in adding this feature.

@therc
@dkozma

I have a suggestion to this feature. Currently the annotation is named aws-load-balancer-backend-protocol. However, look at the image below:

AWS uses the terminology instance and not backend. To avoid confusion, and to ensure consistency, I suggest that we change

aws-load-balancer-backend-protocol

to

aws-load-balancer-instance-protocol

Please let me know your thoughts. Thanks!

cc @kubernetes/sig-federation-misc