Skip to content

Commit

Permalink
fix: Update GuRole with revised logicalId logic
Browse files Browse the repository at this point in the history 
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuRole` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!

As a by-product, `GuInstanceRole`'s tests need updating. The snapshot tests are updated to reflect the fact that `GuInstanceRole` doesn't set `existingLogicalId` when calling `GuRole`, therefore the logicalId will always be auto-generated.
  • Loading branch information
@akash1810
akash1810 committed Apr 12, 2021
1 parent d69bdbe commit ec6db46
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 47 deletions.
58 changes: 29 additions & 29 deletions src/constructs/iam/roles/__snapshots__/instance-role.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Object {
"PolicyName": "describe-ec2-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand All @@ -60,7 +60,7 @@ Object {
"PolicyName": "GetConfigPolicy6F934A1C",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -96,13 +96,13 @@ Object {
"PolicyName": "GetDistributablePolicyTestingF9D43A3E",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
"Type": "AWS::IAM::Policy",
},
"InstanceRoleTesting": Object {
"InstanceRoleTestingCB7BD146": Object {
"Properties": Object {
"AssumeRolePolicyDocument": Object {
"Statement": Array [
Expand Down Expand Up @@ -184,7 +184,7 @@ Object {
"PolicyName": "parameter-store-read-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -220,7 +220,7 @@ Object {
"PolicyName": "ssm-run-command-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -274,10 +274,10 @@ Object {
"PolicyName": "describe-ec2-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleMyfirstapp",
"Ref": "InstanceRoleMyfirstapp5C11A22B",
},
Object {
"Ref": "InstanceRoleMysecondapp",
"Ref": "InstanceRoleMysecondapp48DD15D7",
},
],
},
Expand Down Expand Up @@ -313,7 +313,7 @@ Object {
"PolicyName": "GetDistributablePolicyMyfirstappB56CBAB1",
"Roles": Array [
Object {
"Ref": "InstanceRoleMyfirstapp",
"Ref": "InstanceRoleMyfirstapp5C11A22B",
},
],
},
Expand Down Expand Up @@ -349,7 +349,7 @@ Object {
"PolicyName": "GetDistributablePolicyMysecondapp5096BFDB",
"Roles": Array [
Object {
"Ref": "InstanceRoleMysecondapp",
"Ref": "InstanceRoleMysecondapp48DD15D7",
},
],
},
Expand Down Expand Up @@ -391,16 +391,16 @@ Object {
"PolicyName": "GuLogShippingPolicy981BFE5A",
"Roles": Array [
Object {
"Ref": "InstanceRoleMyfirstapp",
"Ref": "InstanceRoleMyfirstapp5C11A22B",
},
Object {
"Ref": "InstanceRoleMysecondapp",
"Ref": "InstanceRoleMysecondapp48DD15D7",
},
],
},
"Type": "AWS::IAM::Policy",
},
"InstanceRoleMyfirstapp": Object {
"InstanceRoleMyfirstapp5C11A22B": Object {
"Properties": Object {
"AssumeRolePolicyDocument": Object {
"Statement": Array [
Expand Down Expand Up @@ -448,7 +448,7 @@ Object {
},
"Type": "AWS::IAM::Role",
},
"InstanceRoleMysecondapp": Object {
"InstanceRoleMysecondapp48DD15D7": Object {
"Properties": Object {
"AssumeRolePolicyDocument": Object {
"Statement": Array [
Expand Down Expand Up @@ -530,7 +530,7 @@ Object {
"PolicyName": "parameter-store-read-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleMyfirstapp",
"Ref": "InstanceRoleMyfirstapp5C11A22B",
},
],
},
Expand Down Expand Up @@ -570,7 +570,7 @@ Object {
"PolicyName": "parameter-store-read-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleMysecondapp",
"Ref": "InstanceRoleMysecondapp48DD15D7",
},
],
},
Expand Down Expand Up @@ -606,10 +606,10 @@ Object {
"PolicyName": "ssm-run-command-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleMyfirstapp",
"Ref": "InstanceRoleMyfirstapp5C11A22B",
},
Object {
"Ref": "InstanceRoleMysecondapp",
"Ref": "InstanceRoleMysecondapp48DD15D7",
},
],
},
Expand Down Expand Up @@ -663,7 +663,7 @@ Object {
"PolicyName": "describe-ec2-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -699,7 +699,7 @@ Object {
"PolicyName": "GetDistributablePolicyTestingF9D43A3E",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -741,13 +741,13 @@ Object {
"PolicyName": "GuLogShippingPolicy981BFE5A",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
"Type": "AWS::IAM::Policy",
},
"InstanceRoleTesting": Object {
"InstanceRoleTestingCB7BD146": Object {
"Properties": Object {
"AssumeRolePolicyDocument": Object {
"Statement": Array [
Expand Down Expand Up @@ -829,7 +829,7 @@ Object {
"PolicyName": "parameter-store-read-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -865,7 +865,7 @@ Object {
"PolicyName": "ssm-run-command-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -914,7 +914,7 @@ Object {
"PolicyName": "describe-ec2-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -950,13 +950,13 @@ Object {
"PolicyName": "GetDistributablePolicyTestingF9D43A3E",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
"Type": "AWS::IAM::Policy",
},
"InstanceRoleTesting": Object {
"InstanceRoleTestingCB7BD146": Object {
"Properties": Object {
"AssumeRolePolicyDocument": Object {
"Statement": Array [
Expand Down Expand Up @@ -1038,7 +1038,7 @@ Object {
"PolicyName": "parameter-store-read-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down Expand Up @@ -1074,7 +1074,7 @@ Object {
"PolicyName": "ssm-run-command-policy",
"Roles": Array [
Object {
"Ref": "InstanceRoleTesting",
"Ref": "InstanceRoleTestingCB7BD146",
},
],
},
Expand Down
2 changes: 1 addition & 1 deletion src/constructs/iam/roles/instance-role.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ interface GuInstanceRoleProps extends AppIdentity {
export class GuInstanceRole extends GuRole {
constructor(scope: GuStack, props: GuInstanceRoleProps) {
super(scope, AppIdentity.suffixText(props, "InstanceRole"), {
overrideId: true,
path: "/",
assumedBy: new ServicePrincipal("ec2.amazonaws.com"),
// not setting existingLogicalId results in the logicalId always being auto-generated
});

const sharedPolicies = [
Expand Down
17 changes: 7 additions & 10 deletions src/constructs/iam/roles/roles.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,32 +1,29 @@
import { SynthUtils } from "@aws-cdk/assert";
import "../../../utils/test/jest";
import "@aws-cdk/assert/jest";
import { ServicePrincipal } from "@aws-cdk/aws-iam";
import { simpleGuStackForTesting } from "../../../utils/test";
import type { SynthedStack } from "../../../utils/test";
import { GuRole } from "./roles";

describe("The GuRole class", () => {
it("overrides id if prop set to true", () => {
const stack = simpleGuStackForTesting();
it("overrides the logicalId when existingLogicalId is set in a migrating stack", () => {
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true });

new GuRole(stack, "TestRole", {
overrideId: true,
existingLogicalId: "MyRole",
assumedBy: new ServicePrincipal("ec2.amazonaws.com"),
});

const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
expect(Object.keys(json.Resources)).toContain("TestRole");
expect(stack).toHaveResourceOfTypeAndLogicalId("AWS::IAM::Role", "MyRole");
});

it("does not override id if prop set to false", () => {
test("auto-generates the logicalId by default", () => {
const stack = simpleGuStackForTesting();

new GuRole(stack, "TestRole", {
assumedBy: new ServicePrincipal("ec2.amazonaws.com"),
});

const json = SynthUtils.toCloudFormation(stack) as SynthedStack;
expect(Object.keys(json.Resources)).not.toContain("TestRole");
expect(stack).toHaveResourceOfTypeAndLogicalId("AWS::IAM::Role", /^TestRole.+$/);
});

it("returns a string value for the child ref", () => {
Expand Down
12 changes: 5 additions & 7 deletions src/constructs/iam/roles/roles.ts
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
import type { CfnRole, RoleProps } from "@aws-cdk/aws-iam";
import { Role } from "@aws-cdk/aws-iam";
import { GuMigratableConstruct } from "../../../utils/mixin";
import type { GuStack } from "../../core";
import type { GuMigratingResource } from "../../core/migrating";

export interface GuRoleProps extends RoleProps {
overrideId?: boolean;
}
export interface GuRoleProps extends RoleProps, GuMigratingResource {}

export class GuRole extends Role {
export class GuRole extends GuMigratableConstruct(Role) {
private child: CfnRole;

constructor(scope: GuStack, id: string, props: GuRoleProps) {
super(scope, id, props);

this.child = this.node.defaultChild as CfnRole;
if (props.overrideId) {
this.child.overrideLogicalId(id);
}
}

// TODO is this needed? We don't do this in other constructs...
get ref(): string {
return this.child.ref;
}
Expand Down

0 comments on commit ec6db46

Please sign in to comment.