Terraform-provider-aws: ALB SNI Support

[oarmstrong]

Announced today, is support for SNI with application load balancers. This allows for multiple certificates to be attached to the load balancer and SNI will automatically select the correct certificate.

Would it be possible to support this functionality? I imagine we'd want to have an additional option in the aws_alb_listener target to take a list of certificates to attach. There does still need to be a default certificate, it may be better to take this as the first item in the certificate list though and just have a single option.

I may take a look at this myself in a few days if nobody else gets around to it in the meantime.

[ebekker]

The API does treat the additional certs distinctly from the default cert, so it may be easier to implement if you mirror this relationship. The other thing is that it's possible to actually specify the same cert twice, once as the default and once as an additional cert -- in their UI, they actually recommend doing this if you're going to "swap out" one default for another in order to maintain continuity during the transition.

[ebekker]

Similar to the way SG rules are added to security groups, it may also be useful to support defining additional certs as either inline or as separate resources -- for example, in the former case you know all of your certs up front, or in the latter, they may be added on as part of separate configurations where each add-on cert is more closely tied to other resources.

[oarmstrong]

I can see keeping the certificate_arn argument then and adding an additional additional_certificate_arns argument which would be a list of ARNs for additional certificates then. Makes sense to keep it inline with how the API treats it and to be able to neatly follow the recommendation you found of adding the default certificate as an additional certificate too (although I wasn't able to see that in the UI anywhere).

Great idea regarding another resource for adding certificates. I'd definitely get use out of that.

[ebekker]

It's in the console UI for managing the additional certs, if you click on the default cert to expand this message:

[oarmstrong]

Cheers. I didn't actually notice it was expandable at first.

[nubbthedestroyer]

+1 this would be useful

[mindnuts]

:+1:

Is there any ETA on this, we are waiting for this to reduce the number of load balancers on production environment. I believe its possible to do it manually but i would prefer to do it from TF.

Cheers

[oarmstrong]

If nobody else has done anything I'll see if I can get on this tomorrow.

[orenmaor]

Updated ETA?

[oarmstrong]

I've started on this over at https://github.com/oarmstrong/terraform-provider-aws/tree/alb-sni. I'll submit a WIP PR as soon as there's something that actually runs.

[anbotero]

@oarmstrong awesome work, man. Thanks for the heads up and keep it up! We greatly appreciate it! 👍

[oarmstrong]

What I've got over there now _seems_ to do the job. Only manually tested it so need to whip up the acceptance tests.

But if anyone wants to give it a test I'd be very grateful. Still no documentation, but it's additional_certificate_arns in the aws_(a)lb_listener resource which is a list of strings.

_Insert disclaimer here about not using this in your production environment yet._

[oarmstrong]

FYI for those on this issue, that PR is done as far as I'm concerned. Just waiting for feedback so I can correct the inevitable mistakes before it gets merged in.

I used this on my production environment yesterday so it should be pretty safe to test.

[ozbillwang]

@oarmstrong Nice job.

Just waiting for this feature. By the way, if you can't wait, you can build it by yourself to start using this feature.

[tomaszkiewicz]

@ebekker I agree with you that it should be modeled in similar way to the security groups - I mean with separate resource for each additional certificate.
The sample case: I deploy ECS cluster with one ALB at day one, then - in the future - I deploy multiple microservices, each registers in ALB with its own certificate.

The sample resource:

In cluster module:

resource "aws_lb_listener" "https" {
  certificate_arn = "${var.base_certificate_arn}"
  .....
}

Then, in microservice module:

resource "aws_lb_listener_sni_certificate" "cert1" {
  listener_arn = "${var.cluster_https_listener_arn}"
  certificate_arn = "..............."
}

[oarmstrong]

I agree too. I haven't done that as part of the PR, but I believe the architecture should allow for a new resource in the future.

[mindnuts]

@radeksimko Can this be merged please :pray:

[Puneeth-n]

@oarmstrong What happened to the PR?

[oarmstrong]

Nothing yet, still open. https://github.com/terraform-providers/terraform-provider-aws/pull/2203

[Puneeth-n]

@oarmstrong Thanks :) Was searching for this PR.

[heartwilltell]

@radeksimko a lot of people waiting for this feature. Please review the #2203 PR

[oarmstrong]

We've been code reviewed! Will take a little while to get things corrected but progress is being made.

[oarmstrong]

Another update for you wonderful people waiting on this.

As noted by @ebekker (and seconded by myself) a new resource for adding certificates into a load balancer would be a useful feature. What I implemented was an extension to the existing resource. However, it was pointed out to me that supporting both methods would be difficult (see the security group rules mess).

So, the consensus on the PR was to ditch my first attempt and have a crack at implementing this immediately solely as a new resource. The majority of my code was pretty good so most of the hard work is already done.

I'm going to work on a new resource for this feature instead and get a new PR open. @radeksimko has stated that he'll be away until the new year after this week. I'll see if I can get it done before then but who knows if he'll even have time to give us another review before he leaves.

You can probably all expect this merged some time early in the new year. Sorry for the disappointment but it's definitely worth getting this done the right way first time.

[Puneeth-n]

Can't wait to have this feature. will save a couple of load balancers. 😅

[oarmstrong]

So I've sent in my new PR #2649 for a aws_lb_listener_certificate resource! :tada:

The only thing that's keeping it WIP is a failing test that I'm not too sure how to correct. If any of you here are contributors and can help out with that please jump into the PR and take a look.

We'll be merged soon... Optimistically hoping for the 1.6.0 release...

---

Edit: If you want to build this yourself and use it you should be pretty safe doing so. I've used this implementation in production myself. It's also pretty unlikely (IMO) that the API will change before release.

[Puneeth-n]

@oarmstrong Are the acceptance tests still failing?

[oarmstrong]

@Puneeth-n yes, just one of the tests that I wrote though - I haven't ran the full suite.

[Puneeth-n]

@oarmstrong will give it a spin later today.

[Puneeth-n]

@oarmstrong I had a quick look at your PR. I fixed the segmentation faults. There is one more issue now. After the tests run, deletion of resources fail. Even though actually al the resources are cleaned up. Would you prefer if I create a pr against your branch and you take it forward from there? I am on vacation next week and need to get things done at work today :)

[oarmstrong]

I didn't have a segfault, so it'll be interesting to see what I did wrong there. Please do PR against my branch and I'll merge it in.

Many thanks!

/cc @Puneeth-n

[Puneeth-n]

@oarmstrong I created a PR against master because I couldn't for some reason create it against your branch. More details in #2686

[egorchabala]

Can't wait to have this feature

[cemo]

Gentle ping @radeksimko, would you please review https://github.com/terraform-providers/terraform-provider-aws/pull/2686

[fractos]

1.11 seems ludicrously far in the future

[oarmstrong]

If there's anything I can do to the PR or talk about to get this merged quicker, please let me know!

[sudermanjr]

+1 This feature would make my life a lot easier.

[memoryleak]

+1

[mindnuts]

This feature is important to many people because of possible savings and reduction in complexity. Not sure why the PR is pushed to 1.11.

[Puneeth-n]

I know its frustrating that this feature is not yet merged despite the fact that it was released by AWS 6 months ago. I even missed my flight to Riga back in December while I was working on fixing the issues in this PR 😓 But please express by using 👍 or 👎 rather than spamming everyone. 🙏

[mlehner616]

This is, by far the most requested feature in the short term roadmap. What I'm not sure I understand is why features requested by 3 people were scheduled for 1.9 and 1.10 in front of this (and I'm not suggesting moving those)? Especially since it appears most if not all the work is done here, what else needs to be done or can be done to move this feature forward sooner?

[oliver1980]

+1 this would be super useful, and save me some headache.

[oarmstrong]

As per #2686 this can probably be closed. We're merged guys!!

https://github.com/terraform-providers/terraform-provider-aws/blob/d78d3d956cea87192fad3db165a7553cd53d4922/CHANGELOG.md

New Resource: aws_lb_listener_certificate [GH-2686]

We'll be in 1.10.0!! :beers:

Massive thanks to @Puneeth-n for help with the tests and a few bugs. Also thanks to the maintainers for getting this merged in!

[bflad]

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[hashibot[bot]]

I'm going to lock this issue because it has been closed for _30 days_ ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!