aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.63k stars 3.91k forks source link

aws_cdk.aws_ecs: empty placement constraints / placement strategies not allowed #27555

Closed pwrmiller closed 5 months ago

pwrmiller commented 1 year ago

Describe the bug

Altering PlacementConstraints and PlacementStrategies in CDK to remove them does not remove PlacementConstraints and Placement Strategies

Findings: the ECS UpdateService API used by CloudFormation allows for the use of an empty list passed to PlacementConstraints to remove existing placement constraints. The same applies to PlacementStrategies (though the documentation is less clear, and indicates "to remove an existing placement strategy, specify an empty object"), which implies CDK should emit a null here if strategies are empty/ should be removed.

Source: ECS API Reference

Status quo: CDK does not allow the use of an empty array for these properties as it evaluates PlacementConstraints using:

Lazy.any({ produce: () => this.constraints }, { omitEmptyArray: true })

Reference: Implementation

Therefore, setting PlacementConstraints to an empty array results in no property being generated. Furthermore, the use of an escape hatch (setting PlacementConstraints via addPropertyOverride does not seem to update this property.

Expected Behavior

I expect to be able to update/ remove placement strategies and placement constraints. Removing the constraints triggers an update call without the parameter set, which means that the properties are not updated.

Current Behavior

Placement constraints cannot be removed easily. Placement strategies cannot be removed once set.

Reproduction Steps

Python code to reproduce the issue:

#!/usr/bin/env python3

import aws_cdk as cdk
import aws_cdk.aws_ec2 as EC2
import aws_cdk.aws_ecs as ECS

app = cdk.App()
stack = cdk.Stack(app, "Stack")
vpc = EC2.Vpc(stack, "Vpc")
cluster = ECS.Cluster(stack, "Cluster", vpc=vpc)
asg = cluster.add_capacity("DefaultAutoScalingGroup", instance_type=EC2.InstanceType("t2.micro"))

task_definition = ECS.Ec2TaskDefinition(stack, "TaskDefinition")
container = task_definition.add_container("web", image=ECS.ContainerImage.from_registry("amazon/amazon-ecs-sample"), memory_limit_mib=256)

# the constraints and strategies are not set
service = ECS.Ec2Service(stack, "Service", cluster=cluster, task_definition=task_definition, placement_constraints=[], placement_strategies=[])

# this does allow us to set the placement constraints to an empty array
cfn_service: ECS.CfnService = service.node.default_child
cfn_service.placement_constraints = []

# we have no way to set the placement strategies to None - this doesn't work
cfn_service.placement_strategies = None

# the escape hatch for placement strategies also leads to some surprise - this doesn't work
cfn_service.add_property_override("PlacementStrategies", None)

app.synth()

Possible Solution

CDK should always provide empty values for these parameters if they are not set, rather than not providing the parameters to avoid surprise on stack update when removing placement constraints.

Additional Information/Context

No response

CDK CLI Version

2.99.1

Framework Version

2.98.0

Node.js Version

18.16.0

OS

Mac OS

Language

Python

Language Version

Python (3.11.3)

Other information

No response

peterwoodworth commented 1 year ago

Before we look at the rest of the issue, I'd like to point out that values we lazily calculate cannot be modified with escape hatches, because that takes place after escape hatches go through. However, you can use escape hatches within aspects to modify those properties

peterwoodworth commented 1 year ago

Ok, this issue makes sense. I'm not sure why omitEmptyArray was enabled in the first place, maybe there was a reason for this. If that reason doesn't exist or doesn't apply anymore, this should be a pretty easy fix.

stm29 commented 5 months ago

Submitted Pull Request. Note: This is my First Pull Request, Happy to Correct if any Newbie mistakes.

stm29 commented 5 months ago

https://github.com/aws/aws-cdk/pull/30382 is Good to be reviewed.

github-actions[bot] commented 5 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.