fix(elbv2): allow multiple certificates on ALB listener

[MrArnoldPalmer]

Fixes a cloudformation error when attaching multiple certificates to an alb listener. Cloudformation allows only a single certificate attached to the LoadBalancer resource. If multiple certificates are passed to the construct on initialization or through the .addCertificateArns method, separate ListenerCertificate resources are created for all after the first.

Closes #3757

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[MrArnoldPalmer]

This potentially creates multiple ListenerCertificate constructs where technically only one would be necessary.

const listener = lb.addListener(this, "x", {
  certificateArns: ["cert0"]
});
listener.addCertificateArns("ListenerCert1", ["cert1", "cert2"]);
listener.addCertificateArns("ListenerCert2", ["cert3", "cert4"]);

Attaches cert0 to the listener resource, and creates two separate ListenerCertificate resources, one with certs 1 and 2, the second with certs 3 and 4. If we built the list of cert arns and created the resources within prepare we could limit the number of these constructs needed.

This was done to avoid the usage of the prepare method. I was warned that prepare can cause breakage in automatic dependency resolution.

We may want to add a section on attaching certificates to load balancers to the documentation to detail what resources get created under different conditions. If that is something that the team believes should be included with this fix, let me know.

[rix0rrr]

This does not preclude it from being an empty list, just means it's not undefined.

[MrArnoldPalmer]

Added a check for an empty list as well.

[rix0rrr]

Neat!

[rix0rrr]

I'd rather you explicitly test length > 0 than rely on truthiness of 0.

[rix0rrr]

Is this to deal with the case where an empty list was passed? I'd rather you test for that explicitly than rely on the (to me somewhat obscure) behavior that:

const [x, ...xs] = [];
// x === undefined

I would have expected that to throw. Obviously not, because JavaScript :), but the typeof check confused me before I tried this out in the REPL.

I'd much rather see a

if (arns.length === 0) { return; }

At the top.

[MrArnoldPalmer]

This was to defend against an empty array being passed, I agree that explicitly testing that condition is better.

[rix0rrr]

I agree that the whole logic here is a little hairy to handle different cases. What if we embraced mutability, and do something like this:

addCertificateArns(id, arns: string[]) {
  if (arns.length > 0 && this.certificateArns.length === 0) {
    // Pop off the first certificate and put it on the main resource
    const first = arns.splice(0, 1)[0];
    this.certificateArns.push(first);
  } 

  if (arns.length > 0) { 
    new ApplicationListenerCertificate(this, id, { ... });
  }
}

If you feel bad about mutating the input argument (reasonable! :) then we can always copy it into a local variable.

[MrArnoldPalmer]

using .shift feels better but because of the type definition, requires a non-null assertion operator like so:

const first = additionalCertArns.shift()!;

Otherwise typescript sees the type of first being string | undefined even though its guarded against by checking that additionalCertArns.length is gt 0.

[rix0rrr]

Oh yeah, shift is totally better, I forgot that existed :)

[rix0rrr]

But I'm happy to accept the PR as-is

[rix0rrr]

Thanks!

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request is now being automatically merged.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository