Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GuHttpsClassicLoadBalancer #204

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Add GuHttpsClassicLoadBalancer #204

merged 1 commit into from
Jan 26, 2021

Conversation

jamie-lynch
Copy link
Contributor

What does this change?

This PR adds a new GuHttpsClassicLoadBalancer construct which sets defaults for the load balancer listener. It restricts the number of listeners to one with the following defaults:

InstancePort: "9000",
InstanceProtocol: "http",
LoadBalancerPort: "443",
Protocol: "https",

If a value for the sslCertificateId prop is not provided, it also creates a new parameter and sets the output of that as the prop value.

Does this change require changes to existing projects or CDK CLI?

No changes are required but stacks which implement the GuClassLoadBalancer and have an HTTPS listener can reduce the number of props they provide.

How to test

Migrate an existing stack to use the GuHttpsClassLoadBalancer and run the snapshot test to confirm that nothing has changed.

How can we measure success?

Stacks can define fewer props to get a standard load balancer which listens for HTTPS traffic

@jamie-lynch jamie-lynch requested a review from a team January 25, 2021 15:53
});
});

test("adds the CertificateARN parameter if no value provided", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@jamie-lynch jamie-lynch merged commit 9dc3b05 into main Jan 26, 2021
@jamie-lynch jamie-lynch deleted the jl/https-clb branch January 26, 2021 08:26
const listenerProps = { ...GuHttpsClassicLoadBalancer.DefaultListener, ...props.listener };

if (!listenerProps.sslCertificateId) {
const certificateId = new GuArnParameter(scope, "CertificateARN", {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACM certificates can be created via CloudFormation. Historically, it's been painful as it's a blocking resource - the stack will remain in "updating" until the certificate has been validated.

Validation takes two forms:

  • If email validated, an email will be sent with a link to be clicked. This email goes to a list of domain admins (around 5 people)
  • If DNS validated, a CNAME needs created in DNS. Again, admins for the domain service is around 5 people.

However, since the creation of https://github.com/guardian/dns-validation-lambda, it's become more viable to create ACM certs via CFN if domain validated.

(almost at the end of this story!)

Rather than this construct bringing a param for the user to fill in at runtime, should/could it bring an ACM resource with it? Providing the benefit of:

  • Less involvement for user (aka reduce human error)
  • Applies best practice of domain validated certificates (best practice because they auto-renew vs. email validated which needs manual renewal once a year)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be cool (and thanks for providing the context)! I guess if we find that not all cases can move to this for whatever reason then we can always provide a prop to add a param instead. I'll add this as an idea to the discussions section for now. 💡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants