ecs: task definition validation container bug

[petderek]

Describe the bug

When I add a container definition to a fargate task definition, I am prompted to include memory or memoryReservation as a required field. This is not a container-level requirement if memory is already specified in the task.

Expected Behavior

I expect the CDK to let me create a container without specifying memory.

Current Behavior

Error: Validation failed with the following errors:
  [EcsWorkspaceStack/nginxTaskDef] ECS Container web must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified
    at validateTree (/Users/petderek/experiments/ecs-workspace/node_modules/aws-cdk-lib/core/lib/private/synthesis.ts:356:11)
    at synthesize (/Users/petderek/experiments/ecs-workspace/node_modules/aws-cdk-lib/core/lib/private/synthesis.ts:49:5)
    at App.synth (/Users/petderek/experiments/ecs-workspace/node_modules/aws-cdk-lib/core/lib/stage.ts:217:33)
    at process.<anonymous> (/Users/petderek/experiments/ecs-workspace/node_modules/aws-cdk-lib/core/lib/app.ts:195:45)

Reproduction Steps

Something like this should repro:

        const basicNginxTask = new aws_ecs.TaskDefinition(this, "nginxTaskDef", {
            compatibility: aws_ecs.Compatibility.EC2_AND_FARGATE,
            cpu: "256",
            executionRole: task_execution_role,
            family: "nginx",
            memoryMiB: "512",
            networkMode: aws_ecs.NetworkMode.AWS_VPC,
        });
        basicNginxTask.addContainer("nginxContainer", {
            containerName: "web",
            essential: true,
            image: aws_ecs.ContainerImage.fromRegistry("public.ecr.aws/nginx/nginx:stable"),
            portMappings: [{containerPort: 80}],
            // uncomment this to 'fix' and compile
            //memoryReservationMiB: 512,
        });

Possible Solution

One solution would be to check if there is memory set at in the parent before validating the existence of memory in the container.

Additional Information/Context

The correct behavior is mentioned in the register-task-definition documentation.

If using the Fargate launch type, this parameter is optional.
If using the EC2 launch type, you must specify either a task-level memory value or a container-level memory value. If you specify both a container-level memory and memoryReservation value, memory must be greater than memoryReservation . If you specify memoryReservation , then that value is subtracted from the available memory resources for the container instance where the container is placed. Otherwise, the value of memory is used.

CDK CLI Version

2.76.0 (build 78c411b)

Framework Version

No response

Node.js Version

v18.16.0

OS

mac

Language

Typescript

Language Version

No response

Other information

No response

[peterwoodworth]

Thanks for reporting,

We should only need to add another condition to the if statement to check if the memory of the task definition hasn't been set:

aws-cdk/packages/aws-cdk-lib/aws-ecs/lib/base/task-definition.ts

Lines 733 to 745 in 78c411b

	private validateTaskDefinition(): string[] {
	const ret = new Array<string>();
	if (isEc2Compatible(this.compatibility)) {
	// EC2 mode validations
	// Container sizes
	for (const container of this.containers) {
	if (!container.memoryLimitSpecified) {
	ret.push(`ECS Container ${container.containerName} must have at least one of 'memoryLimitMiB' or 'memoryReservationMiB' specified`);
	}
	}
	}

[jackhannan-hello]

Could I try to solve this. This would be my first open source contribution

[peterwoodworth]

It would be very welcomed @jackhannan-hello, be sure to check out our Contributing Guide, and feel free to ping me or @pahud if you have any questions

[jackhannan-hello]

Every time I try to clone my fork in VS Code it does not seem to work.

and everything is deleted. @peterwoodworth

[peterwoodworth]

Yeah, unfortunately we're aware of this issue on windows following the repo restructure #25164

Try using the gitpod solution?

[jackhannan-hello]

I think I may have fixed the issue but I am confused on how to test it. I understand I have to make a sample.ts file. I included what petderek said to reproduce but I do not know what to import to be able to test. What should I import @peterwoodworth @pahud ?

[peterwoodworth]

I haven't followed the steps exactly as described in the contributing guide since that's a new section, and we've also recently restructured the repository. But, all you should need to do is import whichever modules are necessary to test this use case.

[peterwoodworth]

We'll want a new integ test for this use case anyway, try looking at the existing integ tests and copy a simple one to build on top of that to test this use case?

[pahud]

@jackhannan-hello

I would not recommend to develop aws-cdk on Windows if this is your first time contributing to CDK. Instead, I will recommend gitpod as it prebuild everything for you and you can just open and start your hack.

Feel free to follow the verify your fix by deployment guide. In your case you would want to write a sample.ts in packages/@aws-cdk-testing/framework-integ/test/aws-ecs/test with yarn watch running in another separate terminal that compiles sample.ts to sample.js for you.

In your sample.ts just write a very basic CDK app with aws-ecs to verify your hack by real deployment:

import {
  App, Stack,
  aws_ecs as ecs,
  aws_ec2 as ec2,
} from 'aws-cdk-lib';

const app = new App();
const env = { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT };
const stack = new Stack(app, 'my-test-stack', { env });

const cluster = new ecs.Cluster(stack, 'Cluster', {
  vpc,
});

Feel free to reach out to me directly on cdk.dev slack and we welcome discussion around contributing experience in the slack #contributing channel as well.

[tam0ri]

Previous PR was automatically closed and has not been active for a month. So I newly submitted PR.

[peterwoodworth]

Sounds good 👍🏻

[github-actions]

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