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!
guardian · Apr 12, 2021 · 48ac074 · 48ac074
1 parent d69bdbe
commit 48ac074
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 26 deletions.
diff --git 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
@@ -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
@@ -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;