Add support for HTTP -> HTTPS redirect.

[mikkeloscar]

Now supported directly on the ALB: https://aws.amazon.com/about-aws/whats-new/2018/07/elastic-load-balancing-announces-support-for-redirects-and-fixed-responses-for-application-load-balancer/

[hjacobs]

We don't really need that for Zalando as we are using Skipper (which provides HTTP->HTTPS redirect), but it would still be very nice to have 😄

[so0k]

Skipper has been great so far, but a new requirement to route traffic through a Firewall DaemonSet on the edge nodes no longer allows us to run Skipper with hostPort exposed (We use an NodePort svc to route to Skipper, which worked fine for a week but is now flaky and I believe not a supported use case of skipper).

As for the design of this feature, I see 2 options:

1. Just use custom CF Template or add flag for controller
2. Implement ingress.kubernetes.io/force-ssl-redirect: "true" Ingress annotation

Option 1: Custom template or controller flag

We could just use a custom CF template which sets port 80 default action to redirect https

To make it more user friendly, we could just add a -force-ssl-redirect controller flag and update the built-in CF template to take a boolean template variable.

Option 2: Implement ssl-redirect Ingress annotation

To avoid projectcontour/contour#250 the buildManagedModel would need to create a different ALB for Ingress objects without the annotation. The CloudFormation Template would need an additional parameter

My Use Case

For my use case, Option 2 wouldn't work because I do all TLS termination with ACM+ALB and projectcontour/contour#715 -> Contour and kube-ingress-aws-controller both act on the same ingress objects (they share ingress class) so Contour updates Envoy HTTP listeners to redirect to HTTPS. Envoy gets HTTP from ALB and goes into a redirect loop. Ideally Envoy recognises X-Forwarded-Proto to avoid the redirect loop.

Option 1 is also much easier to implement, would a PR to add a controller flag be useful?

People could run a 2nd kube-ingress-controller withouth the flag and a different ingress.class to avoid projectcontour/contour#250

[szuecs]

@so0k I am not against any change enabling your use case, but I am also interested in the problem in detail you had with the skipper deployment in detail. Would you mind creating an issue in skipper repository, too?
I really would like to understand if there’s a limitation or we need more documentation how to support your use case.

To add my opinion on your options, I think both options could be useful and can support different use cases.
I would suggest to add -enable-https-redirect , which would default to create the redirect in the alb setup and an annotation such that you can enable/disable the default decision. I am not sure how complicated this would be to implement but it should be the right way for doing it.

[so0k]

@szuecs after replacing skipper we were still seeing incorrect responses - right now we have 5 domains allocated to the ALB listener with SNI and when requesting a.x.domain.com we sometimes get x.domain.com response.

We assume it's an issue with ALB, we are now testing kube-ingress-aws-controller with a maximum of 1 cert per ALB to see if this solves the issue.

else we'd have to switch to services of type LoadBalancer with ARNs allocated to ELB listeners :(

wondering if anyone else saw this issue (seems purely an ALB issue)

[szuecs]

@so0k can you open another issue with more insights, please?

Controller logs and CF stack output might be valuable to understand the issue.

We had some issues in the beginning with SNI and >25 certs and another one was that ALBs created via CF can not change the default cert, such that we sort and persist now the certs, to make changes more safe. We did not see any issues after that anymore.

[mikkeloscar]

This was implemented in #276. thanks @jhohertz :)