Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(ecs): (Ecs Service now Depends on Task Definition forcing Update to Existing Services) #25777

Open
jsunico opened this issue May 30, 2023 · 10 comments
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@jsunico
Copy link

jsunico commented May 30, 2023

Describe the bug

I'm upgrading from CDK version 2.25.0 to 2.81.0. With no other changes on the stack aside from the upgrade, I'm getting an error in Cloudformation:

Resource handler returned message: "Invalid request provided: UpdateService error: Unable to update task definition on services with a CODE_DEPLOY deployment controller. Use AWS CodeDeploy to trigger a new deployment. (Service: AmazonECS; Status Code: 400; Error Code: InvalidParameterException; Request ID: xxxxxxxxx Proxy: null)" (RequestToken: XXXXXXXXXX, HandlerErrorCode: InvalidRequest)

I have compared the template from the Cloudformation and the local template built using the upgraded version and I can see the addition of the task definition in the DependsOn.

   "DependsOn": [
    "ApiLoadBalancerBlueTargetListenerTargetBlueRule1180D001",
    "ApiTaskDefinition51EA709E"
   ],

I have narrowed down which version this was introduced. It seems to occur since version 2.50.0. I believe this line in particular was causing the fargate service to update. However, as I'm using DeploymentControllerType.CODE_DEPLOY this causes the above error for already created services.

https://github.com/aws/aws-cdk/pull/22295/files#diff-becce6466790cd3cc81807ab64dd5f4ef85eed0285509d1ae43b381f24aefddaR469

Expected Behavior

Dependencies are not added if not required.

Current Behavior

Task definition is added as dependency on the fargate service forcing updates on existing resources.

Reproduction Steps

Build and deploy using v2.25.0

    const fargateService = new FargateService(this, 'ApiFargateService', {
      assignPublicIp: false,
      cluster: fargateCluster,
      deploymentController: {
        type: DeploymentControllerType.CODE_DEPLOY
      },
      capacityProviderStrategies: [{
        capacityProvider: 'FARGATE_SPOT',
        base: 1,
        weight: 100
      }],
      desiredCount: 0,
      healthCheckGracePeriod: Duration.seconds(10),
      maxHealthyPercent: 200,
      minHealthyPercent: 100,
      platformVersion: FargatePlatformVersion.LATEST,
      securityGroups: [fargateTaskSecurityGroup],
      serviceName: 'my-service',
      taskDefinition: ecsTaskDefinition,
    })

Build and deploy using 2.50.0

Possible Solution

If not required, can we remove the task definition from the list of the fargate service dependencies?

Additional Information/Context

No response

CDK CLI Version

2.50.0

Framework Version

No response

Node.js Version

v16.20.0

OS

Ubuntu 20.04.5 LTS (WSL)

Language

Typescript

Language Version

Typescript ~4.6.4

Other information

No response

@jsunico jsunico added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 30, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label May 30, 2023
@pahud
Copy link
Contributor

pahud commented May 30, 2023

From I can see in the aws-codedeploy README of #22295

When the CODE_DEPLOY deployment controller is used, the ECS service cannot be
deployed with a new task definition version through CloudFormation.

Looks like you are trying to update the ecs service with CDK with CODE_DEPLOY deployment controller? Can you share more details about that?

@pahud pahud added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels May 30, 2023
@jsunico
Copy link
Author

jsunico commented May 31, 2023

Hi @pahud, the update was an unintentional side effect of the change introduced in v2.50.0. From my end, I only upgraded my dependencies and tried to deploy.

Upon further inspection, the new CDK version adds a dependency into the fargate service triggering an update.

Here's my template in the Cloudformation dashboard:

"ApiFargateServiceXXXXXX": {
   "Type": "AWS::ECS::Service",
   "Properties": {
    "CapacityProviderStrategy": [
     {
      "Base": 1,
      "CapacityProvider": "FARGATE_SPOT",
      "Weight": 100
     }
    ],
    "Cluster": "fargate-cluster-XXXXXX",
    "DeploymentConfiguration": {
     "MaximumPercent": 200,
     "MinimumHealthyPercent": 100
    },
    "DeploymentController": {
     "Type": "CODE_DEPLOY"
    },
    "DesiredCount": 0,
    "EnableECSManagedTags": false,
    "HealthCheckGracePeriodSeconds": 10,
    "LoadBalancers": [
     {
      "ContainerName": "XXXXXX",
      "ContainerPort": 3333,
      "TargetGroupArn": {
       "Ref": "BlueTargetGroupXXXXXX"
      }
     }
    ],
    "NetworkConfiguration": {
     "AwsvpcConfiguration": {
      "AssignPublicIp": "DISABLED",
      "SecurityGroups": [
       {
        "Fn::GetAtt": [
         "ApiSecurityGroupXXXXXX",
         "GroupId"
        ]
       }
      ],
      "Subnets": [
       "subnet-XXXXXX",
       "subnet-XXXXXX",
       "subnet-XXXXXX"
      ]
     }
    },
    "PlatformVersion": "LATEST",
    "ServiceName": "XXXXXX",
    "TaskDefinition": {
     "Ref": "ApiTaskDefinitionXXXXXX"
    }
   },
   "DependsOn": [
    "ApiLoadBalancerBlueTargetListenerTargetBlueRuleXXXXXX"
   ],
   "Metadata": {
    "aws:cdk:path": "XXXXXX/XXXXXX/ApiFargateService/Service"
   }

This is the one compiled/built locally using version 2.50.0:

"ApiFargateServiceXXXXXX": {
   "Type": "AWS::ECS::Service",
   "Properties": {
    "CapacityProviderStrategy": [
     {
      "Base": 1,
      "CapacityProvider": "FARGATE_SPOT",
      "Weight": 100
     }
    ],
    "Cluster": "XXXXXX",
    "DeploymentConfiguration": {
     "MaximumPercent": 200,
     "MinimumHealthyPercent": 100
    },
    "DeploymentController": {
     "Type": "CODE_DEPLOY"
    },
    "DesiredCount": 0,
    "EnableECSManagedTags": false,
    "HealthCheckGracePeriodSeconds": 10,
    "LoadBalancers": [
     {
      "ContainerName": "XXXXXX",
      "ContainerPort": 3333,
      "TargetGroupArn": {
       "Ref": "BlueTargetGroupXXXXXX"
      }
     }
    ],
    "NetworkConfiguration": {
     "AwsvpcConfiguration": {
      "AssignPublicIp": "DISABLED",
      "SecurityGroups": [
       {
        "Fn::GetAtt": [
         "ApiSecurityGroupXXXXXX",
         "GroupId"
        ]
       }
      ],
      "Subnets": [
       "subnet-XXXXXX",
       "subnet-XXXXXX",
       "subnet-XXXXXX"
      ]
     }
    },
    "PlatformVersion": "LATEST",
    "ServiceName": "XXXXXX",
    "TaskDefinition": "XXXXXX"
   },
   "DependsOn": [
    "ApiLoadBalancerBlueTargetListenerTargetBlueRuleXXXXXX",
    "ApiTaskDefinitionXXXXXX"
   ],
   "Metadata": {
    "aws:cdk:path": "XXXXXX/XXXXXX/ApiFargateService/Service"
   }
  }

Notice the DependsOn property.

These are the lines of codes that was introduced in v2.50.0 that is now adding dependency in the fargate service.

if (props.deploymentController?.type === DeploymentControllerType.CODE_DEPLOY) {
// Strip the revision ID from the service's task definition property to
// prevent new task def revisions in the stack from triggering updates
// to the stack's ECS service resource
this.resource.taskDefinition = taskDefinition.family;
this.node.addDependency(taskDefinition);
}

Hope this helps. Thank you.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 31, 2023
@kichik
Copy link

kichik commented Jul 11, 2023

This happened again with #25840 and 2.87.0.

const l1service = service.node.defaultChild as ecs.CfnService;
l1service.addPropertyDeletionOverride('DeploymentConfiguration.Alarms');

@jorgemaroto-eb
Copy link

having the same issue when we've updated the version of aws-cdk from 2.86.0 to 2.87.0

@SinBirb
Copy link

SinBirb commented Aug 22, 2023

Hi, we also have this issue, because we are using DeploymentControllerType.CODE_DEPLOY. Did anyone find a work-around?

Can you possibly add a feature flag to disable this new feature? Otherwise, we can not upgrade our CDK version for our existing infrastructure.

@pahud
Copy link
Contributor

pahud commented Aug 23, 2023

@SinBirb

I am not sure if the dependency is necessary

https://github.com/aws/aws-cdk/blame/1d7a9a80b08d41ce8759bed9286adaa8259c2bc8/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L578

As a workaround, I guess you probably can remove it with removeDependency if you really have to?

@vishnus17
Copy link

I found a fix for this by doing
const cfnService = service.node.defaultChild as ecs.CfnService;
cfnService.addOverride('Properties.TaskDefinition', service.taskDefinition.taskDefinitionArn);

I was able to upgrade to CDK 2.95.0 by adding this below my ECS service construct.

@jsunico
Copy link
Author

jsunico commented Oct 8, 2024

@SinBirb

I am not sure if the dependency is necessary

https://github.com/aws/aws-cdk/blame/1d7a9a80b08d41ce8759bed9286adaa8259c2bc8/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L578

As a workaround, I guess you probably can remove it with removeDependency if you really have to?

Hi @pahud, I tried this workaround but it doesn't seem to work. Could you point out if I'm doing something wrong, please?

    const l1FargateService = this.fargateService.node.defaultChild as CfnService;
    l1FargateService.removeDependency(this.ecsTaskDefinition.node.defaultChild as CfnTaskDefinition)

@jsunico
Copy link
Author

jsunico commented Oct 8, 2024

It seems like the dependency is being added on the node level rather than the defaultChild level. Are the two technically the same? There is currently no removeDependency in the node object.

In the addOverride option, I can:

    const l1FargateService = this.fargateService.node.defaultChild as CfnService;
    l1FargateService.addOverride('DependsOn', [])

But the problem is this removes all the dependencies. My Cloudformation has already one declared dependency and doing this will still cause a force update on the fargate service.

@SinBirb
Copy link

SinBirb commented Oct 23, 2024

I found a fix for this by doing const cfnService = service.node.defaultChild as ecs.CfnService; cfnService.addOverride('Properties.TaskDefinition', service.taskDefinition.taskDefinitionArn);

I was able to upgrade to CDK 2.95.0 by adding this below my ECS service construct.

@vishnus17 Thank you, that worked for us for CDK 2.163.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

6 participants