Skip to content

Commit

Permalink
feat!: Tag support for a stack with multiple apps (#326)
Browse files Browse the repository at this point in the history
Currently, as noted by [this comment](https://github.com/guardian/cdk/blob/27543797cc5c2f0dbfcf5c262c5456c13e730a10/src/constructs/core/stack.ts#L11-L12), our constructs are tailored for single app stacks. We don't support a stack that has, for example, a frontend web app and an API app. If we were to create two ASGs in the same stack, they'd both get incorrectly tagged as the same "app". That is, we don't support micro-services very well.

This change moves the app string into an interface, this interface is then extended by any construct's props if they need to be app-aware.

As "app" is not in `GuStack` and we want to continue adding the "app" tag to resources, we're having to call `Tags.of(this).add("App", props.app);` a few more times, maybe there's a better solution here that keeps things DRY?

Lastly, this is quite a big diff - a lot of the changes are within test files. I've tried to make each commit standalone, so it might be easier to review commit by commit?

BREAKING CHANGE:
- `GuStack` no longer takes knows about an "app"
- Constructors for `GuAutoScalingGroup`, `GuUserData`, `GuParameterStoreReadPolicy`,  `GuGetDistributablePolicy` and `GuInstanceRole` now require `props`
- Props for `GuLambdaFunction`, `GuDatabaseInstance`, `GuInstanceTypeParameter` and `GuAmiParameter` now take an "app" prop
  • Loading branch information
akash1810 authored Mar 17, 2021
1 parent fcb00f0 commit 1bda3c1
Show file tree
Hide file tree
Showing 31 changed files with 276 additions and 188 deletions.
45 changes: 11 additions & 34 deletions src/constructs/autoscaling/asg.test.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import "@aws-cdk/assert/jest";
import { SynthUtils } from "@aws-cdk/assert/lib/synth-utils";
import { InstanceType, Vpc } from "@aws-cdk/aws-ec2";
import { InstanceType, UserData, Vpc } from "@aws-cdk/aws-ec2";
import { ApplicationProtocol } from "@aws-cdk/aws-elasticloadbalancingv2";
import { Stack } from "@aws-cdk/core";
import { simpleGuStackForTesting } from "../../../test/utils";
import type { SynthedStack } from "../../../test/utils";
import { Stage } from "../../constants";
import { GuAmiParameter } from "../core";
import { GuSecurityGroup } from "../ec2";
import { GuApplicationTargetGroup } from "../loadbalancing";
import type { GuAutoScalingGroupProps } from "./asg";
import { GuAutoScalingGroup, GuUserData } from "./";
import { GuAutoScalingGroup } from "./";

describe("The GuAutoScalingGroup", () => {
const vpc = Vpc.fromVpcAttributes(new Stack(), "VPC", {
Expand All @@ -19,13 +18,9 @@ describe("The GuAutoScalingGroup", () => {
publicSubnetIds: [""],
});

const { userData } = new GuUserData(simpleGuStackForTesting()).addCommands(
...["service some-dependency start", "service my-app start"]
);

const defaultProps: GuAutoScalingGroupProps = {
vpc,
userData,
userData: UserData.custom(["#!/bin/bash", "service some-dependency start", "service my-app start"].join("\n")),
stageDependentProps: {
[Stage.CODE]: {
minimumInstances: 1,
Expand All @@ -34,6 +29,7 @@ describe("The GuAutoScalingGroup", () => {
minimumInstances: 3,
},
},
app: "testing",
};

test("adds the AMI parameter if no imageId prop provided", () => {
Expand All @@ -43,34 +39,15 @@ describe("The GuAutoScalingGroup", () => {

const json = SynthUtils.toCloudFormation(stack) as SynthedStack;

expect(json.Parameters.AMI).toEqual({
Description: "AMI ID",
expect(json.Parameters["AMITesting"]).toEqual({
Description:
"Amazon Machine Image ID for the app testing. Use this in conjunction with AMIgo to keep AMIs up to date.",
Type: "AWS::EC2::Image::Id",
});

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
ImageId: {
Ref: "AMI",
},
});
});

test("does not add the AMI parameter if an imageId prop provided", () => {
const stack = simpleGuStackForTesting();

new GuAutoScalingGroup(stack, "AutoscalingGroup", {
...defaultProps,
imageId: new GuAmiParameter(stack, "CustomAMI", {}),
});

const json = SynthUtils.toCloudFormation(stack) as SynthedStack;

expect(Object.keys(json.Parameters)).not.toContain("AMI");
expect(Object.keys(json.Parameters)).toContain("CustomAMI");

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
ImageId: {
Ref: "CustomAMI",
Ref: "AMITesting",
},
});
});
Expand All @@ -82,15 +59,15 @@ describe("The GuAutoScalingGroup", () => {

const json = SynthUtils.toCloudFormation(stack) as SynthedStack;

expect(json.Parameters.InstanceType).toEqual({
expect(json.Parameters["InstanceTypeTesting"]).toEqual({
Type: "String",
Description: "EC2 Instance Type",
Description: "EC2 Instance Type for the app testing",
Default: "t3.small",
});

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
InstanceType: {
Ref: "InstanceType",
Ref: "InstanceTypeTesting",
},
});
});
Expand Down
32 changes: 15 additions & 17 deletions src/constructs/autoscaling/asg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,26 @@ import type { ApplicationTargetGroup } from "@aws-cdk/aws-elasticloadbalancingv2
import { Stage } from "../../constants";
import type { GuStack, GuStageDependentValue } from "../core";
import { GuAmiParameter, GuInstanceTypeParameter } from "../core";
import type { AppIdentity } from "../core/identity";
import { GuHttpsEgressSecurityGroup } from "../ec2";

// Since we want to override the types of what gets passed in for the below props,
// we need to use Omit<T, U> to remove them from the interface this extends
// https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys
export interface GuAutoScalingGroupProps
extends Omit<
AutoScalingGroupProps,
| "imageId"
| "osType"
| "machineImage"
| "instanceType"
| "userData"
| "minCapacity"
| "maxCapacity"
| "desiredCapacity"
| "securityGroup"
> {
AutoScalingGroupProps,
| "imageId"
| "osType"
| "machineImage"
| "instanceType"
| "userData"
| "minCapacity"
| "maxCapacity"
| "desiredCapacity"
| "securityGroup"
>,
AppIdentity {
stageDependentProps: GuStageDependentAsgProps;
instanceType?: InstanceType;
imageId?: GuAmiParameter;
Expand Down Expand Up @@ -92,19 +94,15 @@ export class GuAutoScalingGroup extends AutoScalingGroup {
return {
osType: OperatingSystemType.LINUX,
userData,
imageId:
props.imageId?.valueAsString ??
new GuAmiParameter(scope, "AMI", {
description: "AMI ID",
}).valueAsString,
imageId: props.imageId?.valueAsString ?? new GuAmiParameter(scope, props).valueAsString,
};
}

const mergedProps = {
...props,
...wireStageDependentProps(scope, props.stageDependentProps),
machineImage: { getImage: getImage },
instanceType: props.instanceType ?? new InstanceType(new GuInstanceTypeParameter(scope).valueAsString),
instanceType: props.instanceType ?? new InstanceType(new GuInstanceTypeParameter(scope, props).valueAsString),
userData,

// Do not use the default AWS security group which allows egress on any port.
Expand Down
10 changes: 8 additions & 2 deletions src/constructs/autoscaling/user-data.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ describe("GuUserData", () => {

test("Distributable should be downloaded from a standard path in S3 (bucket/stack/stage/app/filename)", () => {
const stack = simpleGuStackForTesting();
const app = "testing";

const props: GuUserDataProps = {
app,
distributable: {
bucket: new GuDistributionBucketParameter(stack),
fileName: "my-app.deb",
executionStatement: `dpkg -i /${stack.app}/my-app.deb`,
executionStatement: `dpkg -i /${app}/my-app.deb`,
},
};

Expand All @@ -39,6 +41,7 @@ describe("GuUserData", () => {
minimumInstances: 3,
},
},
app: "testing",
});

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
Expand All @@ -65,12 +68,14 @@ describe("GuUserData", () => {

test("Distributable should download configuration first", () => {
const stack = simpleGuStackForTesting();
const app = "testing";

const props: GuUserDataProps = {
app,
distributable: {
bucket: new GuDistributionBucketParameter(stack),
fileName: "my-app.deb",
executionStatement: `dpkg -i /${stack.app}/my-app.deb`,
executionStatement: `dpkg -i /${app}/my-app.deb`,
},
configuration: {
bucket: new GuPrivateConfigBucketParameter(stack),
Expand All @@ -91,6 +96,7 @@ describe("GuUserData", () => {
minimumInstances: 3,
},
},
app: "testing",
});

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
Expand Down
29 changes: 14 additions & 15 deletions src/constructs/autoscaling/user-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { UserData } from "@aws-cdk/aws-ec2";
import { Bucket } from "@aws-cdk/aws-s3";
import type { GuPrivateS3ConfigurationProps } from "../../utils/ec2";
import type { GuDistributionBucketParameter, GuStack } from "../core";
import type { AppIdentity } from "../core/identity";

/**
* Where to download a distributable from.
Expand All @@ -15,7 +16,7 @@ export interface GuUserDataS3DistributableProps {
executionStatement: string; // TODO can we detect this and auto generate it? Maybe from the file extension?
}

export interface GuUserDataProps {
export interface GuUserDataProps extends AppIdentity {
distributable: GuUserDataS3DistributableProps;
configuration?: GuPrivateS3ConfigurationProps;
}
Expand All @@ -34,9 +35,8 @@ export class GuUserData {
return this._userData;
}

private downloadDistributable(scope: GuStack, props: GuUserDataS3DistributableProps) {
const localDirectory = `/${scope.app}`;
const bucketKey = [scope.stack, scope.stage, scope.app, props.fileName].join("/");
private downloadDistributable(scope: GuStack, app: string, props: GuUserDataS3DistributableProps) {
const bucketKey = [scope.stack, scope.stage, app, props.fileName].join("/");

const bucket = Bucket.fromBucketAttributes(scope, "DistributionBucket", {
bucketName: props.bucket.valueAsString,
Expand All @@ -45,14 +45,12 @@ export class GuUserData {
this.addS3DownloadCommand({
bucket,
bucketKey,
localFile: `${localDirectory}/${props.fileName}`,
localFile: `/${app}/${props.fileName}`,
});
}

private downloadConfiguration(scope: GuStack, props: GuPrivateS3ConfigurationProps) {
const localDirectory = `/etc/${scope.app}`;

const bucket = Bucket.fromBucketAttributes(scope, `${scope.app}ConfigurationBucket`, {
private downloadConfiguration(scope: GuStack, app: string, props: GuPrivateS3ConfigurationProps) {
const bucket = Bucket.fromBucketAttributes(scope, `${app}ConfigurationBucket`, {
bucketName: props.bucket.valueAsString,
});

Expand All @@ -62,20 +60,21 @@ export class GuUserData {
this.addS3DownloadCommand({
bucket,
bucketKey,
localFile: `${localDirectory}/${fileName}`,
localFile: `/etc/${app}/${fileName}`,
});
});
}

// eslint-disable-next-line custom-rules/valid-constructors -- TODO only lint for things that extend IConstruct
constructor(scope: GuStack, props?: GuUserDataProps) {
constructor(scope: GuStack, props: GuUserDataProps) {
this._userData = UserData.forLinux();

if (props) {
props.configuration && this.downloadConfiguration(scope, props.configuration);
this.downloadDistributable(scope, props.distributable);
this.addCommands(props.distributable.executionStatement);
if (props.configuration) {
this.downloadConfiguration(scope, props.app, props.configuration);
}

this.downloadDistributable(scope, props.app, props.distributable);
this.addCommands(props.distributable.executionStatement);
}

addCommands(...commands: string[]): GuUserData {
Expand Down
2 changes: 2 additions & 0 deletions src/constructs/cloudwatch/alarm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe("The GuAlarm class", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
new GuAlarm(stack, "alarm", {
alarmName: `Alarm in ${stack.stage}`,
Expand All @@ -31,6 +32,7 @@ describe("The GuAlarm class", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
new GuAlarm(stack, "alarm", {
alarmName: `Alarm in ${stack.stage}`,
Expand Down
4 changes: 4 additions & 0 deletions src/constructs/cloudwatch/lambda-alarms.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ describe("The GuLambdaErrorPercentageAlarm pattern", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
const props = {
toleratedErrorPercentage: 80,
Expand All @@ -28,6 +29,7 @@ describe("The GuLambdaErrorPercentageAlarm pattern", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
const props = {
toleratedErrorPercentage: 65,
Expand All @@ -47,6 +49,7 @@ describe("The GuLambdaErrorPercentageAlarm pattern", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
const props = {
toleratedErrorPercentage: 65,
Expand All @@ -67,6 +70,7 @@ describe("The GuLambdaErrorPercentageAlarm pattern", () => {
code: { bucket: "bucket1", key: "folder/to/key" },
handler: "handler.ts",
runtime: Runtime.NODEJS_12_X,
app: "testing",
});
const props = {
toleratedErrorPercentage: 65,
Expand Down
52 changes: 52 additions & 0 deletions src/constructs/core/identity.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import "@aws-cdk/assert/jest";
import { Topic } from "@aws-cdk/aws-sns";
import { Stack } from "@aws-cdk/core";
import { alphabeticalTags } from "../../../test/utils";
import { AppIdentity } from "./identity";

describe("AppIdentity.suffixText", () => {
it("should title case the app before suffixing it", () => {
const actual = AppIdentity.suffixText({ app: "myapp" }, "InstanceType");
const expected = "InstanceTypeMyapp";
expect(actual).toEqual(expected);
});

it("should work with title case input", () => {
const actual = AppIdentity.suffixText({ app: "MyApp" }, "InstanceType");
const expected = "InstanceTypeMyApp";
expect(actual).toEqual(expected);
});

it("should work with uppercase input", () => {
const actual = AppIdentity.suffixText({ app: "MYAPP" }, "InstanceType");
const expected = "InstanceTypeMYAPP";
expect(actual).toEqual(expected);
});

it("should handle hyphens", () => {
const actual = AppIdentity.suffixText({ app: "my-app" }, "InstanceType");
const expected = "InstanceTypeMy-app";
expect(actual).toEqual(expected);
});
});

describe("AppIdentity.taggedConstruct", () => {
it("should apply a tag to a resource", () => {
// not using any Gu constructs to purely test the impact of AppIdentity.taggedConstruct
const stack = new Stack();

const snsTopic = new Topic(stack, "myTopic");
const appIdentity: AppIdentity = { app: "testing" };

AppIdentity.taggedConstruct(appIdentity, snsTopic);

expect(stack).toHaveResource("AWS::SNS::Topic", {
Tags: alphabeticalTags([
{
Key: "App",
Value: "testing",
},
]),
});
});
});
Loading

0 comments on commit 1bda3c1

Please sign in to comment.