From 053b6b0a682392289fd70f5adbce4fd8da5db4a6 Mon Sep 17 00:00:00 2001 From: Mario Savarese Date: Fri, 16 Jun 2023 21:01:54 +0100 Subject: [PATCH 1/2] revert: revert #1891 Passing subnets to the construct causes CDK to generate custom resources Reverting this while the issue is being investigated. --- .../ecs/__snapshots__/ecs-task.test.ts.snap | 218 +----------------- src/constructs/ecs/ecs-task.test.ts | 35 +-- src/constructs/ecs/ecs-task.ts | 6 +- 3 files changed, 5 insertions(+), 254 deletions(-) diff --git a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap index 978be0b430..e3ce72c87d 100644 --- a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap +++ b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap @@ -3,107 +3,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of config 1`] = ` { "Mappings": { - "DefaultCrNodeVersionMap": { - "af-south-1": { - "value": "nodejs16.x", - }, - "ap-east-1": { - "value": "nodejs16.x", - }, - "ap-northeast-1": { - "value": "nodejs16.x", - }, - "ap-northeast-2": { - "value": "nodejs16.x", - }, - "ap-northeast-3": { - "value": "nodejs16.x", - }, - "ap-south-1": { - "value": "nodejs16.x", - }, - "ap-south-2": { - "value": "nodejs16.x", - }, - "ap-southeast-1": { - "value": "nodejs16.x", - }, - "ap-southeast-2": { - "value": "nodejs16.x", - }, - "ap-southeast-3": { - "value": "nodejs16.x", - }, - "ca-central-1": { - "value": "nodejs16.x", - }, - "cn-north-1": { - "value": "nodejs16.x", - }, - "cn-northwest-1": { - "value": "nodejs16.x", - }, - "eu-central-1": { - "value": "nodejs16.x", - }, - "eu-central-2": { - "value": "nodejs16.x", - }, - "eu-north-1": { - "value": "nodejs16.x", - }, - "eu-south-1": { - "value": "nodejs16.x", - }, - "eu-south-2": { - "value": "nodejs16.x", - }, - "eu-west-1": { - "value": "nodejs16.x", - }, - "eu-west-2": { - "value": "nodejs16.x", - }, - "eu-west-3": { - "value": "nodejs16.x", - }, - "me-central-1": { - "value": "nodejs16.x", - }, - "me-south-1": { - "value": "nodejs16.x", - }, - "sa-east-1": { - "value": "nodejs16.x", - }, - "us-east-1": { - "value": "nodejs16.x", - }, - "us-east-2": { - "value": "nodejs16.x", - }, - "us-gov-east-1": { - "value": "nodejs16.x", - }, - "us-gov-west-1": { - "value": "nodejs16.x", - }, - "us-iso-east-1": { - "value": "nodejs14.x", - }, - "us-iso-west-1": { - "value": "nodejs14.x", - }, - "us-isob-east-1": { - "value": "nodejs14.x", - }, - "us-west-1": { - "value": "nodejs16.x", - }, - "us-west-2": { - "value": "nodejs16.x", - }, - }, "ServiceprincipalMap": { "af-south-1": { "states": "states.af-south-1.amazonaws.com", @@ -210,8 +109,8 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "gu:cdk:constructs": [ "GuStack", "GuEcsTask", - "GuSubnetListParameter", "GuDistributionBucketParameter", + "GuSubnetListParameter", ], "gu:cdk:version": "TEST", }, @@ -223,18 +122,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of }, }, "Parameters": { - "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0ArtifactHashAF1370F8": { - "Description": "Artifact hash for asset "28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0"", - "Type": "String", - }, - "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0S3BucketCD1790E7": { - "Description": "S3 bucket for asset "28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0"", - "Type": "String", - }, - "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0S3VersionKeyCE63AE8F": { - "Description": "S3 key for asset version "28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0"", - "Type": "String", - }, "DistributionBucketName": { "Default": "/account/services/artifact.bucket", "Description": "SSM parameter containing the S3 bucket name holding distribution artifacts", @@ -247,100 +134,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of }, }, "Resources": { - "AWSCDKCfnUtilsProviderCustomResourceProviderHandlerCF82AA57": { - "DependsOn": [ - "AWSCDKCfnUtilsProviderCustomResourceProviderRoleFE0EE867", - ], - "Properties": { - "Code": { - "S3Bucket": { - "Ref": "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0S3BucketCD1790E7", - }, - "S3Key": { - "Fn::Join": [ - "", - [ - { - "Fn::Select": [ - 0, - { - "Fn::Split": [ - "||", - { - "Ref": "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0S3VersionKeyCE63AE8F", - }, - ], - }, - ], - }, - { - "Fn::Select": [ - 1, - { - "Fn::Split": [ - "||", - { - "Ref": "AssetParameters28739348edff6f1084f6a50d8d934e2d3fc2a3bb77442d8a9a1361d51ccd03c0S3VersionKeyCE63AE8F", - }, - ], - }, - ], - }, - ], - ], - }, - }, - "Handler": "__entrypoint__.handler", - "MemorySize": 128, - "Role": { - "Fn::GetAtt": [ - "AWSCDKCfnUtilsProviderCustomResourceProviderRoleFE0EE867", - "Arn", - ], - }, - "Runtime": "nodejs16.x", - "Timeout": 900, - }, - "Type": "AWS::Lambda::Function", - }, - "AWSCDKCfnUtilsProviderCustomResourceProviderRoleFE0EE867": { - "Properties": { - "AssumeRolePolicyDocument": { - "Statement": [ - { - "Action": "sts:AssumeRole", - "Effect": "Allow", - "Principal": { - "Service": "lambda.amazonaws.com", - }, - }, - ], - "Version": "2012-10-17", - }, - "ManagedPolicyArns": [ - { - "Fn::Sub": "arn:\${AWS::Partition}:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole", - }, - ], - }, - "Type": "AWS::IAM::Role", - }, - "CdkJsonStringify2": { - "DeletionPolicy": "Delete", - "Properties": { - "ServiceToken": { - "Fn::GetAtt": [ - "AWSCDKCfnUtilsProviderCustomResourceProviderHandlerCF82AA57", - "Arn", - ], - }, - "Value": { - "Ref": "ecstestPrivateSubnets", - }, - }, - "Type": "Custom::AWSCDKCfnJsonStringify", - "UpdateReplacePolicy": "Delete", - }, "ecstestexecutionfailedC93F511B": { "Properties": { "ActionsEnabled": true, @@ -456,14 +249,7 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "Arn", ], }, - "","TaskDefinition":"test-stack-TEST-ecs-test","NetworkConfiguration":{"AwsvpcConfiguration":{"Subnets":", - { - "Fn::GetAtt": [ - "CdkJsonStringify2", - "Value", - ], - }, - ","SecurityGroups":["id-123"]}},"Overrides":{"ContainerOverrides":[{"Name":"test-ecs-task-ecs-test-TaskContainer"}]},"LaunchType":"FARGATE","PlatformVersion":"LATEST"}}}}", + "","TaskDefinition":"test-stack-TEST-ecs-test","NetworkConfiguration":{"AwsvpcConfiguration":{"Subnets":["abc-123"],"SecurityGroups":["id-123"]}},"Overrides":{"ContainerOverrides":[{"Name":"test-ecs-task-ecs-test-TaskContainer"}]},"LaunchType":"FARGATE","PlatformVersion":"LATEST"}}}}", ], ], }, diff --git a/src/constructs/ecs/ecs-task.test.ts b/src/constructs/ecs/ecs-task.test.ts index 1bb607f6df..88a18961b2 100644 --- a/src/constructs/ecs/ecs-task.test.ts +++ b/src/constructs/ecs/ecs-task.test.ts @@ -1,10 +1,9 @@ import { Template } from "aws-cdk-lib/assertions"; -import type { ISubnet, IVpc } from "aws-cdk-lib/aws-ec2"; +import type { IVpc } from "aws-cdk-lib/aws-ec2"; import { SecurityGroup, Vpc } from "aws-cdk-lib/aws-ec2"; import { Effect, PolicyStatement } from "aws-cdk-lib/aws-iam"; import { GuTemplate, simpleGuStackForTesting } from "../../utils/test"; import type { GuStack } from "../core"; -import { GuVpc, SubnetType } from "../ec2/vpc"; import { GuEcsTask } from "./ecs-task"; const makeVpc = (stack: GuStack) => @@ -39,11 +38,10 @@ describe("The GuEcsTask pattern", () => { }); }); - const generateComplexStack = (stack: GuStack, app: string, vpc: IVpc, subnets?: ISubnet[]) => { + const generateComplexStack = (stack: GuStack, app: string, vpc: IVpc) => { new GuEcsTask(stack, `test-ecs-task-${app}`, { containerConfiguration: { id: "node:10", type: "registry" }, vpc, - subnets, app: app, taskTimeoutInMinutes: 60, cpu: 1024, @@ -77,33 +75,4 @@ describe("The GuEcsTask pattern", () => { template.hasGuTaggedResource("AWS::ECS::TaskDefinition", { appIdentity: { app: "ecs-test" } }); template.hasGuTaggedResource("AWS::ECS::TaskDefinition", { appIdentity: { app: "ecs-test2" } }); }); - - it("should default to private subnets when no subnet prop is specified", () => { - const stack = simpleGuStackForTesting(); - - generateComplexStack(stack, "ecs-private-subnet-test", makeVpc(stack)); - - const template = Template.fromStack(stack); - - template.hasParameter("ecsprivatesubnettestPrivateSubnets", { - Default: "/account/vpc/primary/subnets/private", - }); - }); - - it("should override private subnets when the `subnets` prop is specified", () => { - const stack = simpleGuStackForTesting(); - const app = "ecs-public-subnet-test"; - - generateComplexStack( - stack, - app, - makeVpc(stack), - GuVpc.subnetsFromParameter(stack, { type: SubnetType.PUBLIC, app }) - ); - - const template = Template.fromStack(stack); - template.hasParameter("ecspublicsubnettestPublicSubnets", { - Default: "/account/vpc/primary/subnets/public", - }); - }); }); diff --git a/src/constructs/ecs/ecs-task.ts b/src/constructs/ecs/ecs-task.ts index 7cd2ebd8a5..749f99eaf3 100644 --- a/src/constructs/ecs/ecs-task.ts +++ b/src/constructs/ecs/ecs-task.ts @@ -1,7 +1,7 @@ import { CfnOutput, Duration } from "aws-cdk-lib"; import { Alarm, TreatMissingData } from "aws-cdk-lib/aws-cloudwatch"; import { SnsAction } from "aws-cdk-lib/aws-cloudwatch-actions"; -import type { ISecurityGroup, ISubnet, IVpc } from "aws-cdk-lib/aws-ec2"; +import type { ISecurityGroup, IVpc } from "aws-cdk-lib/aws-ec2"; import type { IRepository } from "aws-cdk-lib/aws-ecr"; import { Cluster, @@ -20,7 +20,6 @@ import { Construct } from "constructs"; import type { NoMonitoring } from "../cloudwatch"; import type { GuStack } from "../core"; import { AppIdentity } from "../core"; -import { GuVpc, SubnetType } from "../ec2"; import { GuGetDistributablePolicyStatement } from "../iam"; /** @@ -113,7 +112,6 @@ export interface GuEcsTaskProps extends AppIdentity { customTaskPolicies?: PolicyStatement[]; environmentOverrides?: TaskEnvironmentVariable[]; storage?: number; - subnets?: ISubnet[]; } /** @@ -158,7 +156,6 @@ export class GuEcsTask extends Construct { taskTimeoutInMinutes = 15, customTaskPolicies, vpc, - subnets = GuVpc.subnetsFromParameter(scope, { type: SubnetType.PRIVATE, app }), monitoringConfiguration, securityGroups = [], environmentOverrides, @@ -205,7 +202,6 @@ export class GuEcsTask extends Construct { const task = new EcsRunTask(scope, `${id}-task`, { cluster, - subnets: { subnets }, launchTarget: new EcsFargateLaunchTarget({ platformVersion: FargatePlatformVersion.LATEST, }), From e804e802c2e37c396fbcd726e14bcd596320b527 Mon Sep 17 00:00:00 2001 From: Mario Savarese Date: Tue, 20 Jun 2023 13:01:14 +0100 Subject: [PATCH 2/2] revert: Remove outdate JSdoc --- src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap | 6 ------ src/constructs/ecs/ecs-task.ts | 1 - src/patterns/scheduled-ecs-task.ts | 1 - 3 files changed, 8 deletions(-) diff --git a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap index e3ce72c87d..058b979031 100644 --- a/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap +++ b/src/constructs/ecs/__snapshots__/ecs-task.test.ts.snap @@ -110,7 +110,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "GuStack", "GuEcsTask", "GuDistributionBucketParameter", - "GuSubnetListParameter", ], "gu:cdk:version": "TEST", }, @@ -127,11 +126,6 @@ exports[`The GuEcsTask pattern should create the correct resources with lots of "Description": "SSM parameter containing the S3 bucket name holding distribution artifacts", "Type": "AWS::SSM::Parameter::Value", }, - "ecstestPrivateSubnets": { - "Default": "/account/vpc/primary/subnets/private", - "Description": "A list of private subnets", - "Type": "AWS::SSM::Parameter::Value>", - }, }, "Resources": { "ecstestexecutionfailedC93F511B": { diff --git a/src/constructs/ecs/ecs-task.ts b/src/constructs/ecs/ecs-task.ts index 749f99eaf3..97ec8692c2 100644 --- a/src/constructs/ecs/ecs-task.ts +++ b/src/constructs/ecs/ecs-task.ts @@ -136,7 +136,6 @@ const getContainer = (config: ContainerConfiguration) => { * Note that if your task reliably completes in less than 15 minutes then you should probably use a [[`GuLambda`]] instead. This * pattern was mainly created to work around the 15 minute lambda timeout. * - * If the `subnets` prop is not defined, the task will run in a private subnet by default. */ export class GuEcsTask extends Construct { stateMachine: StateMachine; diff --git a/src/patterns/scheduled-ecs-task.ts b/src/patterns/scheduled-ecs-task.ts index 46d4e735e8..f20195317e 100644 --- a/src/patterns/scheduled-ecs-task.ts +++ b/src/patterns/scheduled-ecs-task.ts @@ -28,7 +28,6 @@ export interface GuScheduledEcsTaskProps extends GuEcsTaskProps { * Note that if your task reliably completes in less than 15 minutes then you should probably use a [[`GuScheduledLambda`]] instead. This * pattern was mainly created to work around the 15 minute lambda timeout. * - * If the `subnets` prop is not defined, the task will run in a private subnet by default. */ export class GuScheduledEcsTask extends GuAppAwareConstruct(GuEcsTask) { constructor(scope: GuStack, id: string, props: GuScheduledEcsTaskProps) {