From 3fd97bfb4a72ae2c86508d9f943cb2b25feb7459 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Fri, 19 Mar 2021 05:18:41 +0000 Subject: [PATCH 1/3] feat: create a GuLoggingStreamNameParameter This parameter is written as a singleton as we only ever want one per stack - if there are multiple apps defined in the stack, they'll all share the same parameter (as opposed to the AMI parameter which is app specific). --- .../core/parameters/log-shipping.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 src/constructs/core/parameters/log-shipping.ts diff --git a/src/constructs/core/parameters/log-shipping.ts b/src/constructs/core/parameters/log-shipping.ts new file mode 100644 index 000000000..0028339e7 --- /dev/null +++ b/src/constructs/core/parameters/log-shipping.ts @@ -0,0 +1,27 @@ +import type { GuStack } from "../stack"; +import { GuStringParameter } from "./base"; + +export class GuLoggingStreamNameParameter extends GuStringParameter { + private static instance: GuStringParameter | undefined; + + // eslint-disable-next-line custom-rules/valid-constructors -- TODO be better + private constructor(scope: GuStack) { + super(scope, "LoggingStreamName", { + description: "SSM parameter containing the Name (not ARN) on the kinesis stream", + default: "/account/services/logging.stream.name", + fromSSM: true, + }); + } + + public static getInstance(stack: GuStack): GuLoggingStreamNameParameter { + // Resources can only live in the same App so return a new `GuSSMRunCommandPolicy` where necessary. + // See https://github.com/aws/aws-cdk/blob/0ea4b19afd639541e5f1d7c1783032ee480c307e/packages/%40aws-cdk/core/lib/private/refs.ts#L47-L50 + const isSameStack = this.instance?.node.root === stack.node.root; + + if (!this.instance || !isSameStack) { + this.instance = new GuLoggingStreamNameParameter(stack); + } + + return this.instance; + } +} From de8cd5462f0e2edcf836b17d4e0b386fdb721d9e Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Fri, 19 Mar 2021 05:40:38 +0000 Subject: [PATCH 2/3] refactor(GuLogShippingPolicy): use the new `GuLoggingStreamNameParameter` parameter Also extend `GuAllowPolicy` to stay DRY. --- .../iam/policies/log-shipping.test.ts | 52 ++++--------------- src/constructs/iam/policies/log-shipping.ts | 32 +++++------- 2 files changed, 21 insertions(+), 63 deletions(-) diff --git a/src/constructs/iam/policies/log-shipping.test.ts b/src/constructs/iam/policies/log-shipping.test.ts index 97c4e17f4..590bdbeeb 100644 --- a/src/constructs/iam/policies/log-shipping.test.ts +++ b/src/constructs/iam/policies/log-shipping.test.ts @@ -1,59 +1,25 @@ import "@aws-cdk/assert/jest"; +import { SynthUtils } from "@aws-cdk/assert"; +import type { SynthedStack } from "../../../../test/utils"; import { attachPolicyToTestRole, simpleGuStackForTesting } from "../../../../test/utils"; import { GuLogShippingPolicy } from "./log-shipping"; describe("The GuLogShippingPolicy class", () => { - it("sets default props", () => { + it("creates a policy restricted to a kinesis stream defined in a parameter", () => { const stack = simpleGuStackForTesting(); const logShippingPolicy = new GuLogShippingPolicy(stack); - attachPolicyToTestRole(stack, logShippingPolicy); - expect(stack).toHaveResource("AWS::IAM::Policy", { - PolicyName: "GuLogShippingPolicy981BFE5A", - PolicyDocument: { - Version: "2012-10-17", - Statement: [ - { - Action: ["kinesis:Describe*", "kinesis:Put*"], - Effect: "Allow", - Resource: { - "Fn::Join": [ - "", - [ - "arn:aws:kinesis:", - { - Ref: "AWS::Region", - }, - ":", - { - Ref: "AWS::AccountId", - }, - ":stream/", - { - Ref: "LoggingStreamName", - }, - ], - ], - }, - }, - ], - }, + const json = SynthUtils.toCloudFormation(stack) as SynthedStack; + expect(json.Parameters.LoggingStreamName).toEqual({ + Type: "AWS::SSM::Parameter::Value", + Default: "/account/services/logging.stream.name", + Description: "SSM parameter containing the Name (not ARN) on the kinesis stream", }); - }); - - it("merges defaults and passed in props", () => { - const stack = simpleGuStackForTesting(); - - const logShippingPolicy = new GuLogShippingPolicy(stack, "LogShippingPolicy", { - policyName: "test", - }); - - attachPolicyToTestRole(stack, logShippingPolicy); expect(stack).toHaveResource("AWS::IAM::Policy", { - PolicyName: "test", + PolicyName: "GuLogShippingPolicy981BFE5A", PolicyDocument: { Version: "2012-10-17", Statement: [ diff --git a/src/constructs/iam/policies/log-shipping.ts b/src/constructs/iam/policies/log-shipping.ts index a3bcfa873..e22395354 100644 --- a/src/constructs/iam/policies/log-shipping.ts +++ b/src/constructs/iam/policies/log-shipping.ts @@ -1,25 +1,17 @@ -import { Effect, PolicyStatement } from "@aws-cdk/aws-iam"; import type { GuStack } from "../../core"; -import { GuStringParameter } from "../../core"; -import type { GuPolicyProps } from "./base-policy"; -import { GuPolicy } from "./base-policy"; +import { GuLoggingStreamNameParameter } from "../../core/parameters/log-shipping"; +import { GuAllowPolicy } from "./base-policy"; -export class GuLogShippingPolicy extends GuPolicy { - constructor(scope: GuStack, id: string = "GuLogShippingPolicy", props?: GuPolicyProps) { - super(scope, id, { ...props }); - - const loggingStreamNameParam = new GuStringParameter(scope, "LoggingStreamName", { - description: "SSM parameter containing the Name (not ARN) on the kinesis stream", - default: "/account/services/logging.stream.name", - fromSSM: true, +export class GuLogShippingPolicy extends GuAllowPolicy { + // eslint-disable-next-line custom-rules/valid-constructors -- TODO be better + constructor(scope: GuStack) { + super(scope, "GuLogShippingPolicy", { + actions: ["kinesis:Describe*", "kinesis:Put*"], + resources: [ + `arn:aws:kinesis:${scope.region}:${scope.account}:stream/${ + GuLoggingStreamNameParameter.getInstance(scope).valueAsString + }`, + ], }); - - this.addStatements( - new PolicyStatement({ - effect: Effect.ALLOW, - actions: ["kinesis:Describe*", "kinesis:Put*"], - resources: [`arn:aws:kinesis:${scope.region}:${scope.account}:stream/${loggingStreamNameParam.valueAsString}`], - }) - ); } } From 4ded3721b9f10a32aa46867c07ba4aabc10bffb6 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Fri, 19 Mar 2021 06:00:14 +0000 Subject: [PATCH 3/3] refactor(GuLogShippingPolicy): make `GuLogShippingPolicy` a singleton This policy is now a singleton as we only ever want one per stack. If there are multiple roles defined in this stack that require log shipping permissions, a single policy will be defined and attached to the roles. This DRYness helps keep within the file size limits of cloudformation. --- .../__snapshots__/log-shipping.test.ts.snap | 163 ++++++++++++++++++ .../iam/policies/log-shipping.test.ts | 33 +++- src/constructs/iam/policies/log-shipping.ts | 16 +- src/constructs/iam/roles/instance-role.ts | 2 +- test/utils/attach-policy-to-test-role.ts | 6 +- 5 files changed, 213 insertions(+), 7 deletions(-) create mode 100644 src/constructs/iam/policies/__snapshots__/log-shipping.test.ts.snap diff --git a/src/constructs/iam/policies/__snapshots__/log-shipping.test.ts.snap b/src/constructs/iam/policies/__snapshots__/log-shipping.test.ts.snap new file mode 100644 index 000000000..22e74a249 --- /dev/null +++ b/src/constructs/iam/policies/__snapshots__/log-shipping.test.ts.snap @@ -0,0 +1,163 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`The GuLogShippingPolicy singleton class will only be defined once in a stack, even when attached to multiple roles 1`] = ` +Object { + "Parameters": Object { + "LoggingStreamName": Object { + "Default": "/account/services/logging.stream.name", + "Description": "SSM parameter containing the Name (not ARN) on the kinesis stream", + "Type": "AWS::SSM::Parameter::Value", + }, + "Stage": Object { + "AllowedValues": Array [ + "CODE", + "PROD", + ], + "Default": "CODE", + "Description": "Stage name", + "Type": "String", + }, + }, + "Resources": Object { + "GuLogShippingPolicy981BFE5A": Object { + "Properties": Object { + "PolicyDocument": Object { + "Statement": Array [ + Object { + "Action": Array [ + "kinesis:Describe*", + "kinesis:Put*", + ], + "Effect": "Allow", + "Resource": Object { + "Fn::Join": Array [ + "", + Array [ + "arn:aws:kinesis:", + Object { + "Ref": "AWS::Region", + }, + ":", + Object { + "Ref": "AWS::AccountId", + }, + ":stream/", + Object { + "Ref": "LoggingStreamName", + }, + ], + ], + }, + }, + ], + "Version": "2012-10-17", + }, + "PolicyName": "GuLogShippingPolicy981BFE5A", + "Roles": Array [ + Object { + "Ref": "MyFirstRoleAF66ED75", + }, + Object { + "Ref": "MySecondRoleB9F57405", + }, + ], + }, + "Type": "AWS::IAM::Policy", + }, + "MyFirstRoleAF66ED75": Object { + "Properties": Object { + "AssumeRolePolicyDocument": Object { + "Statement": Array [ + Object { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": Object { + "Service": Object { + "Fn::Join": Array [ + "", + Array [ + "ec2.", + Object { + "Ref": "AWS::URLSuffix", + }, + ], + ], + }, + }, + }, + ], + "Version": "2012-10-17", + }, + "Tags": Array [ + Object { + "Key": "App", + "Value": "testing", + }, + Object { + "Key": "gu:cdk:version", + "Value": "TEST", + }, + Object { + "Key": "Stack", + "Value": "test-stack", + }, + Object { + "Key": "Stage", + "Value": Object { + "Ref": "Stage", + }, + }, + ], + }, + "Type": "AWS::IAM::Role", + }, + "MySecondRoleB9F57405": Object { + "Properties": Object { + "AssumeRolePolicyDocument": Object { + "Statement": Array [ + Object { + "Action": "sts:AssumeRole", + "Effect": "Allow", + "Principal": Object { + "Service": Object { + "Fn::Join": Array [ + "", + Array [ + "ec2.", + Object { + "Ref": "AWS::URLSuffix", + }, + ], + ], + }, + }, + }, + ], + "Version": "2012-10-17", + }, + "Tags": Array [ + Object { + "Key": "App", + "Value": "testing", + }, + Object { + "Key": "gu:cdk:version", + "Value": "TEST", + }, + Object { + "Key": "Stack", + "Value": "test-stack", + }, + Object { + "Key": "Stage", + "Value": Object { + "Ref": "Stage", + }, + }, + ], + }, + "Type": "AWS::IAM::Role", + }, + }, +} +`; diff --git a/src/constructs/iam/policies/log-shipping.test.ts b/src/constructs/iam/policies/log-shipping.test.ts index 590bdbeeb..e6c5ab5c0 100644 --- a/src/constructs/iam/policies/log-shipping.test.ts +++ b/src/constructs/iam/policies/log-shipping.test.ts @@ -4,11 +4,11 @@ import type { SynthedStack } from "../../../../test/utils"; import { attachPolicyToTestRole, simpleGuStackForTesting } from "../../../../test/utils"; import { GuLogShippingPolicy } from "./log-shipping"; -describe("The GuLogShippingPolicy class", () => { +describe("The GuLogShippingPolicy singleton class", () => { it("creates a policy restricted to a kinesis stream defined in a parameter", () => { const stack = simpleGuStackForTesting(); - const logShippingPolicy = new GuLogShippingPolicy(stack); + const logShippingPolicy = GuLogShippingPolicy.getInstance(stack); attachPolicyToTestRole(stack, logShippingPolicy); const json = SynthUtils.toCloudFormation(stack) as SynthedStack; @@ -50,4 +50,33 @@ describe("The GuLogShippingPolicy class", () => { }, }); }); + + it("will only be defined once in a stack, even when attached to multiple roles", () => { + const stack = simpleGuStackForTesting(); + + const logShippingPolicy = GuLogShippingPolicy.getInstance(stack); + attachPolicyToTestRole(stack, logShippingPolicy, "MyFirstRole"); + attachPolicyToTestRole(stack, logShippingPolicy, "MySecondRole"); + + expect(stack).toCountResources("AWS::IAM::Policy", 1); + expect(stack).toCountResources("AWS::IAM::Role", 2); + expect(SynthUtils.toCloudFormation(stack)).toMatchSnapshot(); + }); + + it("works across multiple stacks", () => { + const stack1 = simpleGuStackForTesting(); + const stack2 = simpleGuStackForTesting(); + + const logShippingPolicy1 = GuLogShippingPolicy.getInstance(stack1); + const logShippingPolicy2 = GuLogShippingPolicy.getInstance(stack2); + + attachPolicyToTestRole(stack1, logShippingPolicy1); + attachPolicyToTestRole(stack2, logShippingPolicy2); + + expect(stack1).toCountResources("AWS::IAM::Policy", 1); + expect(stack1).toCountResources("AWS::IAM::Role", 1); + + expect(stack2).toCountResources("AWS::IAM::Policy", 1); + expect(stack2).toCountResources("AWS::IAM::Role", 1); + }); }); diff --git a/src/constructs/iam/policies/log-shipping.ts b/src/constructs/iam/policies/log-shipping.ts index e22395354..89a68d08e 100644 --- a/src/constructs/iam/policies/log-shipping.ts +++ b/src/constructs/iam/policies/log-shipping.ts @@ -3,8 +3,10 @@ import { GuLoggingStreamNameParameter } from "../../core/parameters/log-shipping import { GuAllowPolicy } from "./base-policy"; export class GuLogShippingPolicy extends GuAllowPolicy { + private static instance: GuLogShippingPolicy | undefined; + // eslint-disable-next-line custom-rules/valid-constructors -- TODO be better - constructor(scope: GuStack) { + private constructor(scope: GuStack) { super(scope, "GuLogShippingPolicy", { actions: ["kinesis:Describe*", "kinesis:Put*"], resources: [ @@ -14,4 +16,16 @@ export class GuLogShippingPolicy extends GuAllowPolicy { ], }); } + + public static getInstance(stack: GuStack): GuLogShippingPolicy { + // Resources can only live in the same App so return a new `GuSSMRunCommandPolicy` where necessary. + // See https://github.com/aws/aws-cdk/blob/0ea4b19afd639541e5f1d7c1783032ee480c307e/packages/%40aws-cdk/core/lib/private/refs.ts#L47-L50 + const isSameStack = this.instance?.node.root === stack.node.root; + + if (!this.instance || !isSameStack) { + this.instance = new GuLogShippingPolicy(stack); + } + + return this.instance; + } } diff --git a/src/constructs/iam/roles/instance-role.ts b/src/constructs/iam/roles/instance-role.ts index 52412d90c..1eed3c853 100644 --- a/src/constructs/iam/roles/instance-role.ts +++ b/src/constructs/iam/roles/instance-role.ts @@ -32,7 +32,7 @@ export class GuInstanceRole extends GuRole { new GuGetDistributablePolicy(scope, props), new GuDescribeEC2Policy(scope), new GuParameterStoreReadPolicy(scope, props), - ...(props.withoutLogShipping ? [] : [new GuLogShippingPolicy(scope)]), + ...(props.withoutLogShipping ? [] : [GuLogShippingPolicy.getInstance(scope)]), ...(props.additionalPolicies ? props.additionalPolicies : []), ]; diff --git a/test/utils/attach-policy-to-test-role.ts b/test/utils/attach-policy-to-test-role.ts index 07aeeb9e3..60f49a533 100644 --- a/test/utils/attach-policy-to-test-role.ts +++ b/test/utils/attach-policy-to-test-role.ts @@ -4,10 +4,10 @@ import type { Stack } from "@aws-cdk/core"; import { Tags } from "@aws-cdk/core"; // IAM Policies need to be attached to a role, group or user to be created in a stack -export const attachPolicyToTestRole = (stack: Stack, policy: Policy, app: string = "testing"): void => { - const role = new Role(stack, "TestRole", { +export const attachPolicyToTestRole = (stack: Stack, policy: Policy, id: string = "TestRole"): void => { + const role = new Role(stack, id, { assumedBy: new ServicePrincipal("ec2.amazonaws.com"), }); - Tags.of(role).add("App", app); + Tags.of(role).add("App", "testing"); policy.attachToRole(role); };