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

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

Closed
pwrmiller opened this issue Oct 16, 2023 · 5 comments · Fixed by #30382
Closed

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

pwrmiller opened this issue Oct 16, 2023 · 5 comments · Fixed by #30382
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. good first issue Related to contributions. See CONTRIBUTING.md p1

Comments

@pwrmiller
Copy link

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

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

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
Copy link
Contributor

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
Copy link
Contributor

stm29 commented May 29, 2024

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

@stm29
Copy link
Contributor

stm29 commented May 30, 2024

#30382 is Good to be reviewed.

@mergify mergify bot closed this as completed in #30382 May 30, 2024
mergify bot pushed a commit that referenced this issue May 30, 2024
…30382)

### Issue 

fixes #27555 
Closes Half Fix [i.e, Allows user to give Empty Placement Constraints ]  #27555 

This PR does not address supporting empty placement strategies because of the following reason : [27555 : comment](#27572 (comment))

This was raised with the guidance from - [pr / 28431 : Comment ](#28431 (comment))

### Reason for this change

Users unable to give empty placementConstraints

### Description of how you validated changes

- Added a UnitCase to cover with empty `[]` placementConstraints
- Integration Tests
```
$ yarn integ test/aws-ecs/test/ec2/integ.placement-constraint-default-empty.js --update-on-failed
```
After integ tests were completed, `npm test` to verify the snapshot.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Copy link

⚠️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.

This was referenced Jun 1, 2024
atanaspam pushed a commit to atanaspam/aws-cdk that referenced this issue Jun 3, 2024
…aws#30382)

### Issue 

fixes aws#27555 
Closes Half Fix [i.e, Allows user to give Empty Placement Constraints ]  aws#27555 

This PR does not address supporting empty placement strategies because of the following reason : [27555 : comment](aws#27572 (comment))

This was raised with the guidance from - [pr / 28431 : Comment ](aws#28431 (comment))

### Reason for this change

Users unable to give empty placementConstraints

### Description of how you validated changes

- Added a UnitCase to cover with empty `[]` placementConstraints
- Integration Tests
```
$ yarn integ test/aws-ecs/test/ec2/integ.placement-constraint-default-empty.js --update-on-failed
```
After integ tests were completed, `npm test` to verify the snapshot.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
vdahlberg pushed a commit to vdahlberg/aws-cdk that referenced this issue Jun 10, 2024
…aws#30382)

### Issue 

fixes aws#27555 
Closes Half Fix [i.e, Allows user to give Empty Placement Constraints ]  aws#27555 

This PR does not address supporting empty placement strategies because of the following reason : [27555 : comment](aws#27572 (comment))

This was raised with the guidance from - [pr / 28431 : Comment ](aws#28431 (comment))

### Reason for this change

Users unable to give empty placementConstraints

### Description of how you validated changes

- Added a UnitCase to cover with empty `[]` placementConstraints
- Integration Tests
```
$ yarn integ test/aws-ecs/test/ec2/integ.placement-constraint-default-empty.js --update-on-failed
```
After integ tests were completed, `npm test` to verify the snapshot.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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. good first issue Related to contributions. See CONTRIBUTING.md p1
Projects
None yet
5 participants