From 48ac074e2b228ae51c2e3d4a4eaef52ff60030c4 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Mon, 12 Apr 2021 22:11:09 +0100 Subject: [PATCH] fix: Update `GuBaseSecurityGroup` with revised logicalId logic In #364 we placed the logic of overriding a construct's logicalId into a single place. In this change, we're updating the `GuBaseSecurityGroup` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin! --- src/constructs/autoscaling/asg.test.ts | 13 +++++++------ .../ec2/security-groups/base.test.ts | 19 +++++++------------ src/constructs/ec2/security-groups/base.ts | 13 +++++-------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/constructs/autoscaling/asg.test.ts b/src/constructs/autoscaling/asg.test.ts index d37fdd5b52..aaf37e45f3 100644 --- a/src/constructs/autoscaling/asg.test.ts +++ b/src/constructs/autoscaling/asg.test.ts @@ -173,9 +173,10 @@ describe("The GuAutoScalingGroup", () => { const app = "Testing"; const stack = simpleGuStackForTesting(); - const securityGroup = new GuSecurityGroup(stack, "SecurityGroup", { vpc, overrideId: true, app }); - const securityGroup1 = new GuSecurityGroup(stack, "SecurityGroup1", { vpc, overrideId: true, app }); - const securityGroup2 = new GuSecurityGroup(stack, "SecurityGroup2", { vpc, overrideId: true, app }); + // not passing `existingLogicalId`, so logicalId will be auto-generated + const securityGroup = new GuSecurityGroup(stack, "SecurityGroup", { vpc, app }); + const securityGroup1 = new GuSecurityGroup(stack, "SecurityGroup1", { vpc, app }); + const securityGroup2 = new GuSecurityGroup(stack, "SecurityGroup2", { vpc, app }); new GuAutoScalingGroup(stack, "AutoscalingGroup", { ...defaultProps, @@ -188,13 +189,13 @@ describe("The GuAutoScalingGroup", () => { "Fn::GetAtt": [`GuHttpsEgressSecurityGroup${app}89CDDA4B`, "GroupId"], }, { - "Fn::GetAtt": [`SecurityGroup${app}`, "GroupId"], + "Fn::GetAtt": ["SecurityGroupTestingA32D34F9", "GroupId"], // auto-generated logicalId }, { - "Fn::GetAtt": [`SecurityGroup1${app}`, "GroupId"], + "Fn::GetAtt": ["SecurityGroup1TestingCA3A17A4", "GroupId"], // auto-generated logicalId }, { - "Fn::GetAtt": [`SecurityGroup2${app}`, "GroupId"], + "Fn::GetAtt": ["SecurityGroup2Testing6436C75B", "GroupId"], // auto-generated logicalId }, ], }); diff --git a/src/constructs/ec2/security-groups/base.test.ts b/src/constructs/ec2/security-groups/base.test.ts index 130ffc7c07..1980e1c706 100644 --- a/src/constructs/ec2/security-groups/base.test.ts +++ b/src/constructs/ec2/security-groups/base.test.ts @@ -1,9 +1,8 @@ import "@aws-cdk/assert/jest"; -import { SynthUtils } from "@aws-cdk/assert"; +import "../../../utils/test/jest"; import { Peer, Port, Vpc } from "@aws-cdk/aws-ec2"; import { Stack } from "@aws-cdk/core"; import { simpleGuStackForTesting } from "../../../utils/test"; -import type { SynthedStack } from "../../../utils/test"; import { GuHttpsEgressSecurityGroup, GuPublicInternetAccessSecurityGroup, GuSecurityGroup } from "./base"; describe("The GuSecurityGroup class", () => { @@ -13,22 +12,18 @@ describe("The GuSecurityGroup class", () => { publicSubnetIds: [""], }); - it("overrides the id if the prop is set to true", () => { - const stack = simpleGuStackForTesting(); - - new GuSecurityGroup(stack, "TestSecurityGroup", { vpc, overrideId: true, app: "testing" }); + it("overrides the logicalId when existingLogicalId is set in a migrating stack", () => { + const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true }); + new GuSecurityGroup(stack, "TestSecurityGroup", { vpc, existingLogicalId: "TestSG", app: "testing" }); - const json = SynthUtils.toCloudFormation(stack) as SynthedStack; - expect(Object.keys(json.Resources)).toContain("TestSecurityGroupTesting"); + expect(stack).toHaveResourceOfTypeAndLogicalId("AWS::EC2::SecurityGroup", "TestSG"); }); - it("does not overrides the id if the prop is set to false", () => { + test("auto-generates the logicalId by default", () => { const stack = simpleGuStackForTesting(); - new GuSecurityGroup(stack, "TestSecurityGroup", { vpc, app: "testing" }); - const json = SynthUtils.toCloudFormation(stack) as SynthedStack; - expect(Object.keys(json.Resources)).not.toContain("TestSecurityGroup"); + expect(stack).toHaveResourceOfTypeAndLogicalId("AWS::EC2::SecurityGroup", /^TestSecurityGroup.+$/); }); it("adds the ingresses passed in through props", () => { diff --git a/src/constructs/ec2/security-groups/base.ts b/src/constructs/ec2/security-groups/base.ts index 0f3d402e43..298fbc3685 100644 --- a/src/constructs/ec2/security-groups/base.ts +++ b/src/constructs/ec2/security-groups/base.ts @@ -1,7 +1,9 @@ -import type { CfnSecurityGroup, IPeer, SecurityGroupProps } from "@aws-cdk/aws-ec2"; +import type { IPeer, SecurityGroupProps } from "@aws-cdk/aws-ec2"; import { Peer, Port, SecurityGroup } from "@aws-cdk/aws-ec2"; +import { GuMigratableConstruct } from "../../../utils/mixin"; import type { GuStack } from "../../core"; import { AppIdentity } from "../../core/identity"; +import type { GuMigratingResource } from "../../core/migrating"; /** * A way to describe an ingress or egress rule for a security group. @@ -28,8 +30,7 @@ export interface SecurityGroupAccessRule { description: string; } -export interface GuBaseSecurityGroupProps extends SecurityGroupProps { - overrideId?: boolean; +export interface GuBaseSecurityGroupProps extends SecurityGroupProps, GuMigratingResource { ingresses?: SecurityGroupAccessRule[]; egresses?: SecurityGroupAccessRule[]; } @@ -46,14 +47,10 @@ export interface GuSecurityGroupProps extends GuBaseSecurityGroupProps, AppIdent * - [[GuPublicInternetAccessSecurityGroup]] * - [[GuHttpsEgressSecurityGroup]] */ -export abstract class GuBaseSecurityGroup extends SecurityGroup { +export abstract class GuBaseSecurityGroup extends GuMigratableConstruct(SecurityGroup) { protected constructor(scope: GuStack, id: string, props: GuBaseSecurityGroupProps) { super(scope, id, props); - if (props.overrideId) { - (this.node.defaultChild as CfnSecurityGroup).overrideLogicalId(id); - } - props.ingresses?.forEach(({ range, port, description }) => { const connection: Port = typeof port === "number" ? Port.tcp(port) : port;