feat(appmesh): add listener TLS certificates for VirtualNodes and VirtualGateways

[alexbrjo]

This change allows customers to include an ACM certificiate or specify a file certificate for their listeners to use to terminate TLS. #10051

const cert = new Certificate(stack, 'cert', {
  domainName: '',
});

new appmesh.VirtualNode(stack, 'test-node', {
  mesh,
  dnsHostName: 'test',
  listeners: [appmesh.VirtualNodeListener.grpc({
    port: 80,
    tlsCertificate: appmesh.TlsCertificate.acm({
      acmCertificate: cert,
      tlsMode: TlsMode.STRICT,
    }),
  },
  )],
});

---

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

[gitpod-io]

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[alexbrjo]

rebased this on the updated master branch. I must've forgot to re-build on local because there are compilation errors :) ... will fix

[skinny85]

Looking good @alexbrjo ! Some minor comments/questions.

[skinny85]

Don't expose this, I don't think there's any reason to.

[alexbrjo]

tlsMode is referenced in listeners renderTls() function. I prefer setting it here because we might add fields to the tls shape that will depend on fields not in tls-certificate. However if you have a reason for not exposing this I think I can refactor it in a future proof way.

function renderTls(scope: cdk.Construct, tlsCertificate: TlsCertificate): CfnVirtualGateway.VirtualGatewayListenerTlsProperty {
  return {
    certificate: tlsCertificate.bind(scope).tlsCertificate,
    mode: tlsCertificate.tlsMode.toString(),
  };
}

Pull request has been modified.

[alexbrjo]

There's one outstanding thread about access control for TlsMode, but I believe I've addressed all the other comments.

[skinny85]

Looks good @alexbrjo ! I'd like to take this opportunity to get rid of some duplication in VirtualGatewayListener that creeped in before your change. I think we need to clean it up!

[alexbrjo]

@skinny85 Adopted all the changes you suggested from this last review. Thanks for explaining the redundancy part more, I assumed you were talking about code duplicated between Virtual Node and Virtual Gateway.

Pull request has been modified.

[skinny85]

Looks great @alexbrjo ! The only thing I'm surprised about is why is the tlsMode property now required...?

[skinny85]

I think this should be optional, and should be defaulted by the classes that use this interface.

[skinny85]

For the sake of making the diff read better, let's leave this as a function (and let's also make renderTls a function instead of a method).

[skinny85]

Putting in "Request changes" to take it off my ToDo list - @alexbrjo , you ned to resolve the conflicts with master, once you do, please re-request my review, and I'll take a look!

Pull request has been modified.

[dfezzie]

General implementation looks good from my perspective

[skinny85]

Looks perfect @alexbrjo !

[skinny85]

You can safely call this scope, the "unused parameter" validation does not apply to abstract methods 🙂.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: d83838e
• 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 will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).