feat: add first draft of EC2 App Pattern #399

jacobwinch · 2021-04-08T12:10:40Z

What does this change?

This PR adds a first attempt at our EC2 App pattern. It's still a work in progress, but we have confirmed that this minimal version of the pattern can be deployed successfully so it feels like a good time to get something merged.

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

No.

How to test

We've added a basic unit test for this. More unit tests (and proper docs) will be added as the pattern develops.

We've also manually tested deploying a stack with the following code:

import type { App } from "@aws-cdk/core";
import type { GuStackProps } from "@guardian/cdk/lib/constructs/core";
import { GuStack } from "@guardian/cdk/lib/constructs/core";
import { GuEc2App } from "@guardian/cdk/lib/patterns/ec2-app";

export class GuCDKEC2Pattern extends GuStack {
  constructor(scope: App, id: string, props: GuStackProps) {
    super(scope, id, props);

    const port = 8000;

    new GuEc2App(this, {
      app: "TestApp",
      applicationPort: port,
      publicFacing: false,
      userData: `#!/usr/bin/env bash
        mkdir test
        cd test
        echo OK >> healthcheck
        python -m SimpleHTTPServer ${port}
      `,
    });
  }
}

This just allows us to serve a static file via /healthcheck, so that we can ensure that requests can be served correctly.

How can we measure success?

It should be much faster/easier to create a new EC2 app from scratch using this pattern.

Have we considered potential risks?

Although this pattern is now functional, we don't want 'real' users to start experimenting with it at this stage. To mitigate against this:

We've added a comment to warn people that this is still under development
The pattern is not mentioned in https://github.com/guardian/cdk/blob/main/src/patterns/index.ts yet.

src/patterns/ec2-app.ts

jacobwinch · 2021-04-08T12:50:08Z

src/patterns/ec2-app.ts

+} from "../constructs/loadbalancing";
+
+interface GuEc2AppProps extends AppIdentity {
+  userData: GuUserData | string;


If the user needs to download private config then we still need to wire up these permissions (for more details on that, see: #312)

akash1810 · 2021-04-08T13:00:19Z

src/patterns/ec2-app.ts

+      },
+      role: new GuInstanceRole(scope, { app: props.app }),
+      healthCheck: HealthCheck.elb({ grace: Duration.minutes(2) }), // should this be defaulted at pattern or construct level?
+      userData: props.userData instanceof GuUserData ? props.userData.userData : props.userData,


Might be nicer for userData in GuAutoScalingGroup's props to be of type GuUserData | string.

I think it is already, what would look better here do you think?

src/patterns/ec2-app.ts

akash1810

This looks fantastic, great work!

Should we add a test for a single GuStack with multiple GuEc2Apps too?

stephengeller · 2021-04-12T13:09:04Z

I made a change to how the IDs of VPC constructs are declared, as we ran into issues with declaring multiple GuEC2Apps in the same stack, mainly due to the IDs of VPCs and subnets constructs being hardcoed. Would be keen to get opinions on the workaround implemented here.

akash1810

Added a couple of comments, mainly on code reuse.

src/constructs/ec2/vpc.ts

akash1810 · 2021-04-12T21:40:54Z

src/patterns/ec2-app.test.ts

+        {
+          Key: "gu:cdk:version",
+          PropagateAtLaunch: true,
+          Value: "TEST",
+        },


Better to use the mock here.

This would become:

import { TrackingTag } from "../../constants/library-info"; ... Tags: alphabeticalTags([ TrackingTag, { Key: "App", ... }, ... ]);

That is, this can be replaced:

{ Key: "gu:cdk:version", PropagateAtLaunch: true, Value: "TEST", },

With:

TrackingTag

Using the constant means we can update the naming of the tag in one place (should we want to).

Good shout. This doesn't currently work due to the missing PropagateAtLaunch keypair, but I've created a new constant that handles this called TrackingTagWithPropagate.

src/patterns/ec2-app.test.ts

akash1810 · 2021-04-12T21:43:12Z

src/patterns/ec2-app.ts

+  constructor(scope: GuStack, props: GuEc2AppProps) {
+    const id = (id: string) => `${props.app}${id}`;
+
+    scope.tags.setTag(id("App"), props.app);


The AppIdentity.taggedConstruct helper should be used here for consistency.

Good shout, done.

akash1810 · 2021-04-12T21:43:35Z

src/patterns/ec2-app.ts

+ */
+export class GuEc2App {
+  constructor(scope: GuStack, props: GuEc2AppProps) {
+    const id = (id: string) => `${props.app}${id}`;


AppIdentity.suffixText in other places that are app aware, can this be reused here for consistency?

akash1810 · 2021-04-12T21:44:43Z

src/patterns/ec2-app.ts

+
+    const asg = new GuAutoScalingGroup(scope, id("AutoScalingGroup"), {
+      vpc,
+      app: props.app,


minor: we've destructured app above, so this can become:

Suggested change

app: props.app,

app,

Good catch, done.

akash1810 · 2021-04-12T21:46:48Z

src/patterns/ec2-app.ts

+ */
+export class GuPlayApp extends GuEc2App {
+  constructor(scope: GuStack, props: GuMaybePortProps) {
+    super(scope, { ...props, applicationPort: props.applicationPort ?? GuApplicationPorts.Play });


I'd favour being strongly opinionated here and say Play always runs on 9000. That is, don't accept applicationPort in the props.

Good shout, done.

akash1810 · 2021-04-12T21:47:16Z

src/patterns/ec2-app.ts

+ */
+export class GuNodeApp extends GuEc2App {
+  constructor(scope: GuStack, props: GuMaybePortProps) {
+    super(scope, { ...props, applicationPort: props.applicationPort ?? GuApplicationPorts.Node });


Similar to above, I'd favour being strongly opinionated here and say Node always runs on 3000. That is, don't accept applicationPort in the props.

Good shout, done.

src/constructs/ec2/vpc.ts

akash1810 · 2021-04-12T21:51:31Z

src/patterns/ec2-app.test.ts

+    expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot();
+  });
+
+  it("can handle multiple EC2 apps in a single stack", function () {


🎉 lovely!

akash1810 · 2021-04-12T21:55:55Z

The commit history on this PR is 👌🏽 .

stephengeller · 2021-04-13T14:15:56Z

This should now be ready for review again @akash1810 @jacobwinch !

akash1810 · 2021-04-14T05:12:14Z

src/constants/library-info.ts

@@ -10,3 +10,8 @@ export const TrackingTag = {
  Key: "gu:cdk:version",
  Value: LibraryInfo.VERSION,
 };
+
+export const TrackingTagWithPropagate = {


I don't think this is correct, as I'm not sure we want to publicly export this variant as it's specifically for the tests and not for "daily use"? In the past we've done something like this.

I think there's an opportunity to add a custom matcher here (similar to toHaveResourceOfTypeAndLogicalId) as it looks like we want to assert a construct has the identity tags. That is, a "toHaveAnIdentityTaggedResource" matcher that'll take a stack, a resource type string and assert that it's tags contain all the identity keys?

After discussing this via Chat, we decided to go with the simpler option for now, in order to get this merged. We'll look at implementing the custom matcher when everyone is around to work on it together.

akash1810

Looks great!

👍🏽 to this too.

github-actions · 2021-04-14T10:13:01Z

🎉 This PR is included in version 8.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jacobwinch commented Apr 8, 2021

View reviewed changes

src/patterns/ec2-app.ts Show resolved Hide resolved

jacobwinch commented Apr 8, 2021

View reviewed changes

jacobwinch marked this pull request as ready for review April 8, 2021 12:50

jacobwinch requested a review from a team April 8, 2021 12:50

akash1810 reviewed Apr 8, 2021

View reviewed changes

stephengeller mentioned this pull request Apr 9, 2021

chore: add sensible defaults to GuAutoScalingGroup and GuApplicationLoadBalancer #406

Merged

stephengeller force-pushed the sg-jw/ec2-pattern branch 3 times, most recently from fdb284f to 756cf9b Compare April 12, 2021 12:55

akash1810 reviewed Apr 12, 2021

View reviewed changes

src/constructs/ec2/vpc.ts Show resolved Hide resolved

akash1810 reviewed Apr 12, 2021

View reviewed changes

feat: add first draft of EC2 App Pattern

168acfe

stephengeller force-pushed the sg-jw/ec2-pattern branch 6 times, most recently from b06fcf3 to 7ab36ad Compare April 13, 2021 14:06

Add GuNodeApp and GuPlayApp with tests

78cf237

stephengeller force-pushed the sg-jw/ec2-pattern branch from 7ab36ad to 78cf237 Compare April 13, 2021 14:10

akash1810 reviewed Apr 14, 2021

View reviewed changes

jacobwinch added 2 commits April 14, 2021 10:03

Merge branch 'main' into sg-jw/ec2-pattern

0beb48b

chore: tidy up TrackingTagWithPropagate

1e9f5f0

akash1810 approved these changes Apr 14, 2021

View reviewed changes

jacobwinch merged commit e84cc0d into main Apr 14, 2021

jacobwinch deleted the sg-jw/ec2-pattern branch April 14, 2021 10:10

github-actions bot added the released label Apr 14, 2021

jacobwinch mentioned this pull request Apr 14, 2021

fix: improve support for GuUserData in EC2 App pattern #446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add first draft of EC2 App Pattern #399

feat: add first draft of EC2 App Pattern #399

jacobwinch commented Apr 8, 2021 •

edited

Loading

jacobwinch Apr 8, 2021

jacobwinch Apr 14, 2021

akash1810 Apr 8, 2021

stephengeller Apr 9, 2021

akash1810 left a comment

stephengeller commented Apr 12, 2021

akash1810 left a comment

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

stephengeller Apr 13, 2021

akash1810 Apr 12, 2021

akash1810 commented Apr 12, 2021

stephengeller commented Apr 13, 2021

akash1810 Apr 14, 2021

jacobwinch Apr 14, 2021

akash1810 left a comment

github-actions bot commented Apr 14, 2021

feat: add first draft of EC2 App Pattern #399

feat: add first draft of EC2 App Pattern #399

Conversation

jacobwinch commented Apr 8, 2021 • edited Loading

What does this change?

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

How to test

How can we measure success?

Have we considered potential risks?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 left a comment

Choose a reason for hiding this comment

stephengeller commented Apr 12, 2021

akash1810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 commented Apr 12, 2021

stephengeller commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash1810 left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2021

jacobwinch commented Apr 8, 2021 •

edited

Loading