Skip to content

Allow imageId and instanceType to be passed into ASG #213

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 29, 2021
Merged

Allow imageId and instanceType to be passed into ASG #213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6827b8
take an instanceType prop or default to new param
Jan 28, 2021
2005fa7
use imageId prop if passed in but default to creating parameter
Jan 28, 2021
10502b0
change imageId prop to GuAmiParameter
Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions src/constructs/autoscaling/asg.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import "@aws-cdk/assert/jest";
import { SynthUtils } from "@aws-cdk/assert/lib/synth-utils";
import { Vpc } from "@aws-cdk/aws-ec2";
import { InstanceType, Vpc } from "@aws-cdk/aws-ec2";
import { ApplicationProtocol } from "@aws-cdk/aws-elasticloadbalancingv2";
import { Stack } from "@aws-cdk/core";
import { simpleGuStackForTesting } from "../../../test/utils/simple-gu-stack";
Expand All @@ -21,7 +21,7 @@ describe("The GuAutoScalingGroup", () => {
userData: "user data",
};

test("adds the AMI parameter", () => {
test("adds the AMI parameter if no imageId prop provided", () => {
const stack = simpleGuStackForTesting();

new GuAutoScalingGroup(stack, "AutoscalingGroup", { ...defaultProps, osType: 1 });
Expand All @@ -40,7 +40,21 @@ describe("The GuAutoScalingGroup", () => {
});
});

test("adds the instanceType parameter", () => {
test("does not add the AMI parameter if an imageId prop provided", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that Riff-Raff currently relies on AMI being configured as a parameter in order to automatically update the underlying AMI during a deployment. That's a widely used feature in the department (and ideally we want every application to do this due to the security benefits it brings), so I think we might need to discourage passing it as a prop for now.

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's a good point. The reason this was added back as an optional prop was to allow for stacks that have multiple ASGs. Currently, that would fail as we'd try to create the same parameter more than once. This would allow a stack to define multiple parameters and pass them into each ASG as required. If we want to go as far as requiring AMI values to be a parameter, we could change the type of imageId to GuAMIParameter to allow for this overriding but prevent the value being hardcoded in the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason this was added back as an optional prop was to allow for stacks that have multiple ASGs.

Hmm, I hadn't thought about that other use-case! Maybe we should still allow this then and use patterns/docs to push 'typical' single-ASG stacks towards using an AMI parameter.

Copy link
Contributor Author

@jamie-lynch jamie-lynch Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've added a suggestion below which might be the best of both worlds. Agreed that the standard EC2 pattern should then just create the parameter for you!

const stack = simpleGuStackForTesting();

new GuAutoScalingGroup(stack, "AutoscalingGroup", { ...defaultProps, osType: 1, imageId: "123" });

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

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

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

test("adds the instanceType parameter if none provided", () => {
const stack = simpleGuStackForTesting();

new GuAutoScalingGroup(stack, "AutoscalingGroup", defaultProps);
Expand All @@ -60,6 +74,20 @@ describe("The GuAutoScalingGroup", () => {
});
});

test("does not create the instanceType parameter if value is provided", () => {
const stack = simpleGuStackForTesting();

new GuAutoScalingGroup(stack, "AutoscalingGroup", { ...defaultProps, instanceType: new InstanceType("t3.small") });

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

expect(Object.keys(json.Parameters)).not.toContain(InstanceType);

expect(stack).toHaveResource("AWS::AutoScaling::LaunchConfiguration", {
InstanceType: "t3.small",
});
});

test("correctly sets the user data using prop", () => {
const stack = simpleGuStackForTesting();

Expand Down
16 changes: 8 additions & 8 deletions src/constructs/autoscaling/asg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { GuAmiParameter, GuInstanceTypeParameter } from "../core";
// https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys
export interface GuAutoScalingGroupProps
extends Omit<AutoScalingGroupProps, "imageId" | "osType" | "machineImage" | "instanceType" | "userData"> {
instanceType?: InstanceType;
imageId?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require the prop to be a parameter so we can cater for stacks with multiple ASGs but enforce using parameters for AMIs.

Suggested change
imageId?: string;
imageId?: GuAMIParameter;

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a nice idea 👍

osType?: OperatingSystemType;
machineImage?: MachineImage;
userData: string;
Expand All @@ -21,27 +23,25 @@ export interface GuAutoScalingGroupProps

export class GuAutoScalingGroup extends AutoScalingGroup {
constructor(scope: GuStack, id: string, props: GuAutoScalingGroupProps) {
const imageId = new GuAmiParameter(scope, "AMI", {
description: "AMI ID",
});

const instanceType = new GuInstanceTypeParameter(scope);

// We need to override getImage() so that we can pass in the AMI as a parameter
// Otherwise, MachineImage.lookup({ name: 'some str' }) would work as long
// as the name is hard-coded
function getImage(): MachineImageConfig {
return {
osType: props.osType ?? OperatingSystemType.LINUX,
userData: UserData.custom(props.userData),
imageId: imageId.valueAsString,
imageId:
props.imageId ??
new GuAmiParameter(scope, "AMI", {
description: "AMI ID",
}).valueAsString,
};
}

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

Expand Down