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.5k stars 3.84k forks source link

(ecs-patterns): NetworkLoadBalancedServiceBase does not support container port mapping using taskDefinition #29041

Open jackdreinhardt opened 7 months ago

jackdreinhardt commented 7 months ago

Describe the bug

When initializing the NetworkBalancedServiceBase with a taskDefinition, there is no way to set the target group port to a value other than 80.

Expected Behavior

I expect the target group port of the load balancer to be set to the taskDefinition's default container container port.

Current Behavior

The target group port is 80, different from the task definition's default container port mapping configuration.

Reproduction Steps

import { Stack, StackProps } from 'aws-cdk-lib';
import { Vpc } from 'aws-cdk-lib/aws-ec2';
import { Cluster, ContainerImage } from 'aws-cdk-lib/aws-ecs';
import { NetworkLoadBalancedFargateService } from 'aws-cdk-lib/aws-ecs-patterns';
import { Construct } from 'constructs';

export class ElbTestStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);
    const vpc = new Vpc(this, 'vpc', {});

    const fargateCluster = new Cluster(this, 'Cluster', {
      clusterName: 'test',
      vpc: vpc,
      enableFargateCapacityProviders: true,
      containerInsights: true
    })

    this.taskDefinition = new FargateTaskDefinition(this, 'TaskDefinition', {
            cpu: 0.5,
            memoryLimitMiB: 1,
    });
    this.taskDefinition.addContainer('service', {
        image: ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
        portMappings: [{containerPort: 81}]
    })

    const loadBalancedFargateService = new NetworkLoadBalancedFargateService(this, 'NLBService', {
      cluster: fargateCluster,
      memoryLimitMiB: 1024,
      cpu: 512,
      taskDefinition: this.taskDefinition,
      listenerPort: 8181,
    });
  }
}

Possible Solution

Currently, the logic to set the target port is as follows

const targetProps = {
  port: props.taskImageOptions?.containerPort ?? 80,
};

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts#L361

It needs to be changed to something like this:

const targetProps = {
  port: props.taskImageOptions?.containerPort ?? props.taskDefinition.defaultContainer.portMappings[0].containerPort ?? 80
}

Additional Information/Context

No response

CDK CLI Version

2.100.0

Framework Version

No response

Node.js Version

18

OS

AL2

Language

TypeScript

Language Version

No response

Other information

No response

pahud commented 7 months ago

Thank you. I am making it a feature request and we welcome PRs. Before that, I believe we can do escape hatches with addPropertyOverride() as a workaround.