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 and NetworkMultipleTargetGroupsServiceBase hardcode port 80 on target group #18073

Closed
LukvonStrom opened this issue Dec 17, 2021 · 1 comment · Fixed by #18157
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p2

Comments

@LukvonStrom
Copy link
Contributor

What is the problem?

As stated in the title, both base classes default to port 80 for the Loadbalancer TargetGroup.
The culprit for NetworkLoadBalancedServiceBase is located here: https://github.com/aws/aws-cdk/blob/v2-main/packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts#L345

it needs to be changed to something like this:

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

The culprit for NetworkMultipleTargetGroupsServiceBase is located here: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts#L377

it needs to be

        port: targetProps.containerPort ?? 80,

to result in

{
      "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
      "Properties": {
        "Port": [Custom Port],
        "Protocol": "TCP",
      },
    }

Reproduction Steps

I will provide a sample for the NetworkLoadBalancedServiceBase bug:

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, 'FarGate-Cluster', {
      clusterName: 'test',
      vpc: vpc,
      enableFargateCapacityProviders: true,
      containerInsights: true
    })

    const loadBalancedFargateService = new NetworkLoadBalancedFargateService(this, 'NLBService', {
      cluster: fargateCluster,
      memoryLimitMiB: 1024,
      cpu: 512,
      taskImageOptions: {
        image: ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
        containerPort: 81
      },
      listenerPort: 8181,
    });

  }
}

What did you expect to happen?

I expected the Target Group to look like this:

"NLBServiceLBPublicListenerECSGroup1257E89B": {
      "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
      "Properties": {
        "Port": 81,
(snip)
      },
      "Metadata": {
        "aws:cdk:path": "ElbTestStack/NLBService/LB/PublicListener/ECSGroup/Resource"
      }
    }

What actually happened?

The Target Group looked like this:

"NLBServiceLBPublicListenerECSGroup1257E89B": {
      "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
      "Properties": {
        "Port": 80,
(snip)
      },
      "Metadata": {
        "aws:cdk:path": "ElbTestStack/NLBService/LB/PublicListener/ECSGroup/Resource"
      }
    }

when implementing

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

the bug was fixed.

CDK CLI Version

2.2.0 (build 4f5c27c)

Framework Version

No response

Node.js Version

v16.13.0

OS

WSL 2 on Ubuntu

Language

Typescript

Language Version

No response

Other information

No response

@LukvonStrom LukvonStrom added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 17, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Dec 17, 2021
@LukvonStrom LukvonStrom changed the title (ecs-patterns): NetworkLoadBalancedServiceBase and NetworkMultipleTargetGroupsServiceBase default to port 80 on target group (ecs-patterns): NetworkLoadBalancedServiceBase and NetworkMultipleTargetGroupsServiceBase hardcode port 80 on target group Dec 17, 2021
@madeline-k madeline-k removed the needs-triage This issue or PR still needs to be triaged. label Jan 19, 2022
@madeline-k madeline-k removed their assignment Jan 19, 2022
@mergify mergify bot closed this as completed in #18157 Jan 21, 2022
mergify bot pushed a commit that referenced this issue Jan 21, 2022
…Patterns (#18157)

This PR introduces changeable ports as a regression fix for the hardcoded port 80 in both NLB constructs bases.

Closes #18073 

Additionally it seems like the regression reported in the linked issue was not spotted in the integration tests either. 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
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.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…Patterns (aws#18157)

This PR introduces changeable ports as a regression fix for the hardcoded port 80 in both NLB constructs bases.

Closes aws#18073 

Additionally it seems like the regression reported in the linked issue was not spotted in the integration tests either. 

----

*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 bug This issue is a bug. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants