From 5b4105505c2e65d9f449c66c9d3211778067dc68 Mon Sep 17 00:00:00 2001 From: Akash Askoolum Date: Thu, 30 Dec 2021 14:01:02 +0000 Subject: [PATCH] fix: `GuEcsTask` uses `stack` and `stage` from parent `GuStack` for consistency We do not need to specify `stack` and `stage` at the component/pattern level as these properties are available on `GuStack`. Using the `GuStack` values also increases the type-safety as `stage` is _not_ a `string`. This change also refactors to use destructing of the `props` object, which yields simpler code for default values. --- .../ecs/__snapshots__/ecs-task.test.ts.snap | 69 +++++++++++++-- src/constructs/ecs/ecs-task.test.ts | 4 - src/constructs/ecs/ecs-task.ts | 83 ++++++++++--------- src/patterns/scheduled-ecs-task.test.ts | 2 - 4 files changed, 108 insertions(+), 50 deletions(-) diff --git a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap index 6dac21dcdc..08f7566436 100644 --- a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap +++ b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap @@ -126,7 +126,18 @@ Object { "AlarmActions": Array [ "arn:something:else:here:we:goalarm-topic", ], - "AlarmDescription": "ecs-test-TEST job failed ", + "AlarmDescription": Object { + "Fn::Join": Array [ + "", + Array [ + "ecs-test-", + Object { + "Ref": "Stage", + }, + " job failed ", + ], + ], + }, "ComparisonOperator": "GreaterThanOrEqualToThreshold", "Dimensions": Array [ Object { @@ -152,7 +163,18 @@ Object { "AlarmActions": Array [ "arn:something:else:here:we:goalarm-topic", ], - "AlarmDescription": "ecs-test-TEST job timed out ", + "AlarmDescription": Object { + "Fn::Join": Array [ + "", + Array [ + "ecs-test-", + Object { + "Ref": "Stage", + }, + " job timed out ", + ], + ], + }, "ComparisonOperator": "GreaterThanOrEqualToThreshold", "Dimensions": Array [ Object { @@ -187,7 +209,17 @@ Object { }, "testecstaskecstestClusterCBD4036C": Object { "Properties": Object { - "ClusterName": "ecs-test-cluster-TEST", + "ClusterName": Object { + "Fn::Join": Array [ + "", + Array [ + "ecs-test-cluster-", + Object { + "Ref": "Stage", + }, + ], + ], + }, "Tags": Array [ Object { "Key": "App", @@ -236,7 +268,11 @@ Object { "Arn", ], }, - "\\",\\"TaskDefinition\\":\\"test-TEST-ecs-test\\",\\"NetworkConfiguration\\":{\\"AwsvpcConfiguration\\":{\\"Subnets\\":[\\"abc-123\\"],\\"SecurityGroups\\":[\\"id-123\\"]}},\\"Overrides\\":{\\"ContainerOverrides\\":[{\\"Name\\":\\"test-ecs-task-ecs-test-TaskContainer\\"}]},\\"LaunchType\\":\\"FARGATE\\",\\"PlatformVersion\\":\\"LATEST\\"}}}}", + "\\",\\"TaskDefinition\\":\\"test-stack-", + Object { + "Ref": "Stage", + }, + "-ecs-test\\",\\"NetworkConfiguration\\":{\\"AwsvpcConfiguration\\":{\\"Subnets\\":[\\"abc-123\\"],\\"SecurityGroups\\":[\\"id-123\\"]}},\\"Overrides\\":{\\"ContainerOverrides\\":[{\\"Name\\":\\"test-ecs-task-ecs-test-TaskContainer\\"}]},\\"LaunchType\\":\\"FARGATE\\",\\"PlatformVersion\\":\\"LATEST\\"}}}}", ], ], }, @@ -246,7 +282,17 @@ Object { "Arn", ], }, - "StateMachineName": "ecs-test-TEST", + "StateMachineName": Object { + "Fn::Join": Array [ + "", + Array [ + "ecs-test-", + Object { + "Ref": "Stage", + }, + ], + ], + }, "Tags": Array [ Object { "Key": "App", @@ -545,7 +591,18 @@ Object { "Arn", ], }, - "Family": "test-TEST-ecs-test", + "Family": Object { + "Fn::Join": Array [ + "", + Array [ + "test-stack-", + Object { + "Ref": "Stage", + }, + "-ecs-test", + ], + ], + }, "Memory": "1024", "NetworkMode": "awsvpc", "RequiresCompatibilities": Array [ diff --git a/src/constructs/ecs/ecs-task.test.ts b/src/constructs/ecs/ecs-task.test.ts index 6a8f6fe087..22ea5b4eb2 100644 --- a/src/constructs/ecs/ecs-task.test.ts +++ b/src/constructs/ecs/ecs-task.test.ts @@ -32,8 +32,6 @@ describe("The GuEcsTask pattern", () => { containerConfiguration: { id: "node:10", type: "registry" }, monitoringConfiguration: { noMonitoring: true }, vpc: makeVpc(stack), - stack: "test", - stage: "TEST", app: "ecs-test", }); @@ -44,8 +42,6 @@ describe("The GuEcsTask pattern", () => { new GuEcsTask(stack, `test-ecs-task-${app}`, { containerConfiguration: { id: "node:10", type: "registry" }, vpc, - stack: "test", - stage: "TEST", app: app, taskTimeoutInMinutes: 60, cpu: 1024, diff --git a/src/constructs/ecs/ecs-task.ts b/src/constructs/ecs/ecs-task.ts index 30872290c5..aca8a278e4 100644 --- a/src/constructs/ecs/ecs-task.ts +++ b/src/constructs/ecs/ecs-task.ts @@ -18,7 +18,6 @@ import { EcsFargateLaunchTarget, EcsRunTask } from "@aws-cdk/aws-stepfunctions-t import { CfnOutput, Duration } from "@aws-cdk/core"; import type { NoMonitoring } from "../cloudwatch"; import type { GuStack } from "../core"; -import type { Identity } from "../core/identity"; import { AppIdentity } from "../core/identity"; import { GuGetDistributablePolicyStatement } from "../iam"; @@ -100,7 +99,7 @@ export type GuEcsTaskMonitoringProps = { snsTopicArn: string; noMonitoring: fals * See https://docs.aws.amazon.com/step-functions/latest/dg/connect-ecs.html for further detail and other override options - this construct currently * only supports environment variables. */ -export interface GuEcsTaskProps extends Identity { +export interface GuEcsTaskProps extends AppIdentity { vpc: IVpc; containerConfiguration: ContainerConfiguration; taskTimeoutInMinutes?: number; @@ -139,86 +138,96 @@ export class GuEcsTask { stateMachine: StateMachine; constructor(scope: GuStack, id: string, props: GuEcsTaskProps) { - const timeout = props.taskTimeoutInMinutes ?? 15; + const { + app, - // see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-taskdefinition.html#cfn-ecs-taskdefinition-cpu for details - const defaultCpu = 2048; // 2 cores and from 4-16GB memory - const defaultMemory = 4096; // 4GB + // see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ecs-taskdefinition.html#cfn-ecs-taskdefinition-cpu for details + cpu = 2048, // 2 cores and from 4-16GB memory + memory = 4096, // 4GB - const cpu = props.cpu ?? defaultCpu; - const memory = props.memory ?? defaultMemory; + containerConfiguration, + taskCommand, + taskTimeoutInMinutes = 15, + customTaskPolicies, + vpc, + monitoringConfiguration, + securityGroups = [], + environmentOverrides, + } = props; + + const { stack, stage } = scope; const cluster = new Cluster(scope, `${id}-Cluster`, { - clusterName: `${props.app}-cluster-${props.stage}`, + clusterName: `${app}-cluster-${stage}`, enableFargateCapacityProviders: true, - vpc: props.vpc, + vpc, }); const taskDefinition = new TaskDefinition(scope, `${id}-TaskDefinition`, { compatibility: Compatibility.FARGATE, cpu: cpu.toString(), memoryMiB: memory.toString(), - family: `${props.stack}-${props.stage}-${props.app}`, + family: `${stack}-${stage}-${app}`, }); const containerDefinition = taskDefinition.addContainer(`${id}-TaskContainer`, { - image: getContainer(props.containerConfiguration), - entryPoint: props.taskCommand ? ["/bin/sh"] : undefined, - command: props.taskCommand ? ["-c", `${props.taskCommand}`] : undefined, // if unset, falls back to CMD in docker file, or no command will be run + image: getContainer(containerConfiguration), + entryPoint: taskCommand ? ["/bin/sh"] : undefined, + command: taskCommand ? ["-c", taskCommand] : undefined, // if unset, falls back to CMD in docker file, or no command will be run cpu, - memoryLimitMiB: props.memory, + memoryLimitMiB: memory, logging: LogDrivers.awsLogs({ - streamPrefix: props.app, + streamPrefix: app, logRetention: 14, }), }); - const distPolicy = new GuGetDistributablePolicyStatement(scope, { app: props.app }); + const distPolicy = new GuGetDistributablePolicyStatement(scope, { app }); taskDefinition.addToTaskRolePolicy(distPolicy); - (props.customTaskPolicies ?? []).forEach((p) => taskDefinition.addToTaskRolePolicy(p)); + (customTaskPolicies ?? []).forEach((p) => taskDefinition.addToTaskRolePolicy(p)); const task = new EcsRunTask(scope, `${id}-task`, { - cluster: cluster, + cluster, launchTarget: new EcsFargateLaunchTarget({ platformVersion: FargatePlatformVersion.LATEST, }), - taskDefinition: taskDefinition, + taskDefinition, integrationPattern: IntegrationPattern.RUN_JOB, resultPath: "DISCARD", - timeout: Duration.minutes(timeout), - securityGroups: props.securityGroups ?? [], + timeout: Duration.minutes(taskTimeoutInMinutes), + securityGroups, containerOverrides: [ { containerDefinition: containerDefinition, - environment: props.environmentOverrides, + environment: environmentOverrides, }, ], }); this.stateMachine = new StateMachine(scope, `${id}-StateMachine`, { definition: task, - stateMachineName: `${props.app}-${props.stage}`, + stateMachineName: `${app}-${stage}`, }); - if (!props.monitoringConfiguration.noMonitoring) { + if (!monitoringConfiguration.noMonitoring) { const alarmTopic = Topic.fromTopicArn( scope, AppIdentity.suffixText(props, "AlarmTopic"), - props.monitoringConfiguration.snsTopicArn + monitoringConfiguration.snsTopicArn ); const alarms = [ { - name: `${props.app}-execution-failed`, - description: `${props.app}-${props.stage} job failed `, + name: `${app}-execution-failed`, + description: `${app}-${stage} job failed `, metric: this.stateMachine.metricFailed({ period: Duration.hours(1), statistic: "sum", }), }, { - name: `${props.app}-timeout`, - description: `${props.app}-${props.stage} job timed out `, + name: `${app}-timeout`, + description: `${app}-${stage} job timed out `, metric: this.stateMachine.metricTimedOut({ period: Duration.hours(1), statistic: "sum", @@ -226,25 +235,23 @@ export class GuEcsTask { }, ]; - alarms.forEach((a) => { - const alarm = new Alarm(scope, a.name, { - alarmDescription: a.description, + alarms.forEach(({ name, description, metric }) => { + const alarm = new Alarm(scope, name, { + alarmDescription: description, actionsEnabled: true, - metric: a.metric, + metric: metric, // default for comparisonOperator is GreaterThanOrEqualToThreshold threshold: 1, evaluationPeriods: 1, treatMissingData: TreatMissingData.NOT_BREACHING, }); alarm.addAlarmAction(new SnsAction(alarmTopic)); - AppIdentity.taggedConstruct({ app: props.app }, alarm); + AppIdentity.taggedConstruct({ app }, alarm); }); } // Tag all constructs with correct app tag - [cluster, task, taskDefinition, this.stateMachine].forEach((c) => - AppIdentity.taggedConstruct({ app: props.app }, c) - ); + [cluster, task, taskDefinition, this.stateMachine].forEach((c) => AppIdentity.taggedConstruct({ app }, c)); new CfnOutput(scope, `${id}-StateMachineArnOutput`, { value: this.stateMachine.stateMachineArn, diff --git a/src/patterns/scheduled-ecs-task.test.ts b/src/patterns/scheduled-ecs-task.test.ts index d013c999c4..710c4d9597 100644 --- a/src/patterns/scheduled-ecs-task.test.ts +++ b/src/patterns/scheduled-ecs-task.test.ts @@ -24,8 +24,6 @@ describe("The GuScheduledEcsTask pattern", () => { containerConfiguration: { id: "node:10", type: "registry" }, monitoringConfiguration: { noMonitoring: true }, vpc: makeVpc(stack), - stack: "test", - stage: "TEST", app: "ecs-test", });