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-patterns): NetworkLoadBalancedServiceBase does not support container port mapping using taskDefinition #29041

Open
jackdreinhardt opened this issue Feb 9, 2024 · 1 comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@jackdreinhardt
Copy link

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

@jackdreinhardt jackdreinhardt added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 9, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Feb 9, 2024
@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 9, 2024
@pahud
Copy link
Contributor

pahud commented Feb 9, 2024

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.

@pahud pahud added feature-request A feature should be added or improved. and removed bug This issue is a bug. labels Feb 9, 2024
mergify bot pushed a commit that referenced this issue Feb 19, 2024
### Issue # (if applicable)

part of #29041, #29039

Closes #<issue number here>.

### Reason for this change


The testing hierarchy was disorganized.

### Description of changes

Organized test as below.
No logical change.

```ts
describe('Application Load Balancer', () => {

  describe('ApplicationLoadBalancedFargateService', () => {
  ...
  })

  describe('ApplicationMultipleTargetGroupsFargateService', () => {
  ...
  })
})

describe('Network Load Balancer', () => {
  describe('NetworkLoadBalancedFargateService', () => {
  ...
  })

  describe('NetworkMultipleTargetGroupsFargateService', () => {
  ...
  })
})
```





### Description of how you validated changes


pass unit tests

### 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*
mergify bot pushed a commit that referenced this issue Feb 19, 2024
### Issue # (if applicable)

part of #29041, #29039
continuation of #29153

Closes #<issue number here>.

### Reason for this change

tests in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` was disorganized.


### Description of changes

- No logical change.
- Organized tests in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` as below.

```ts
describe('ApplicationLoadBalancedEc2Service', () => {
...
})

describe('NetworkLoadBalancedEc2Service', () => {
...
})
```

- move tests of `ApplicationLoadBalancedFargateService`, `NetworkLoadBalancedFargateService` in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` to `packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts`.



### Description of how you validated changes

Pass unit tests


### 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*
GavinZZ pushed a commit that referenced this issue Feb 22, 2024
### Issue # (if applicable)

part of #29041, #29039

Closes #<issue number here>.

### Reason for this change


The testing hierarchy was disorganized.

### Description of changes

Organized test as below.
No logical change.

```ts
describe('Application Load Balancer', () => {

  describe('ApplicationLoadBalancedFargateService', () => {
  ...
  })

  describe('ApplicationMultipleTargetGroupsFargateService', () => {
  ...
  })
})

describe('Network Load Balancer', () => {
  describe('NetworkLoadBalancedFargateService', () => {
  ...
  })

  describe('NetworkMultipleTargetGroupsFargateService', () => {
  ...
  })
})
```





### Description of how you validated changes


pass unit tests

### 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*
GavinZZ pushed a commit that referenced this issue Feb 22, 2024
### Issue # (if applicable)

part of #29041, #29039
continuation of #29153

Closes #<issue number here>.

### Reason for this change

tests in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` was disorganized.


### Description of changes

- No logical change.
- Organized tests in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` as below.

```ts
describe('ApplicationLoadBalancedEc2Service', () => {
...
})

describe('NetworkLoadBalancedEc2Service', () => {
...
})
```

- move tests of `ApplicationLoadBalancedFargateService`, `NetworkLoadBalancedFargateService` in `packages/aws-cdk-lib/aws-ecs-patterns/test/ec2/l3s.test.ts` to `packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/load-balanced-fargate-service.test.ts`.



### Description of how you validated changes

Pass unit tests


### 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-patterns Related to ecs-patterns library effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

2 participants