Skip to content

Commit

Permalink
chore(ecs-patterns): revert "feature flag missing for breaking change…
Browse files Browse the repository at this point in the history 
… passing container port for target group port (#20284)" (#20430)

This reverts #20284 since its tests fail to pass in CDK v2, blocking the next CDK release. The root cause of failure looks as though it may be the same as #20427 - I've included the test logs below:

<details>

```
@aws-cdk/aws-ecs-patterns: FAIL test/fargate/load-balanced-fargate-service-v2.test.js (11.703 s)
@aws-cdk/aws-ecs-patterns:   � When Network Load Balancer › Fargate networkloadbalanced construct uses custom Port for target group when feature flag is enabled
@aws-cdk/aws-ecs-patterns:     Template has 1 resources with type AWS::ElasticLoadBalancingV2::TargetGroup, but none match as expected.
@aws-cdk/aws-ecs-patterns:     The closest result is:
@aws-cdk/aws-ecs-patterns:       {
@aws-cdk/aws-ecs-patterns:         "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
@aws-cdk/aws-ecs-patterns:         "Properties": {
@aws-cdk/aws-ecs-patterns:           "Port": 80,
@aws-cdk/aws-ecs-patterns:           "Protocol": "TCP",
@aws-cdk/aws-ecs-patterns:           "TargetType": "ip",
@aws-cdk/aws-ecs-patterns:           "VpcId": {
@aws-cdk/aws-ecs-patterns:             "Ref": "VPCB9E5F0B4"
@aws-cdk/aws-ecs-patterns:           }
@aws-cdk/aws-ecs-patterns:         }
@aws-cdk/aws-ecs-patterns:       }
@aws-cdk/aws-ecs-patterns:     with the following mismatches:
@aws-cdk/aws-ecs-patterns:     	Expected 81 but received 80 at /Properties/Port (using objectLike matcher)
@aws-cdk/aws-ecs-patterns:       83 |     const matchError = hasResourceProperties(this.template, type, props);
@aws-cdk/aws-ecs-patterns:       84 |     if (matchError) {
@aws-cdk/aws-ecs-patterns:     > 85 |       throw new Error(matchError);
@aws-cdk/aws-ecs-patterns:          |             ^
@aws-cdk/aws-ecs-patterns:       86 |     }
@aws-cdk/aws-ecs-patterns:       87 |   }
@aws-cdk/aws-ecs-patterns:       88 |
@aws-cdk/aws-ecs-patterns:       at Template.hasResourceProperties (../assertions/lib/template.ts:85:13)
@aws-cdk/aws-ecs-patterns:       at fn (test/fargate/load-balanced-fargate-service-v2.test.ts:709:31)
@aws-cdk/aws-ecs-patterns:       at Object.<anonymous> (../../../tools/@aws-cdk/cdk-build-tools/lib/feature-flag.ts:34:35)
@aws-cdk/aws-ecs-patterns:   � When Network Load Balancer › test Fargate multinetworkloadbalanced construct uses custom Port for target group when feature flag is enabled
@aws-cdk/aws-ecs-patterns:     Template has 2 resources with type AWS::ElasticLoadBalancingV2::TargetGroup, but none match as expected.
@aws-cdk/aws-ecs-patterns:     The closest result is:
@aws-cdk/aws-ecs-patterns:       {
@aws-cdk/aws-ecs-patterns:         "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
@aws-cdk/aws-ecs-patterns:         "Properties": {
@aws-cdk/aws-ecs-patterns:           "Port": 80,
@aws-cdk/aws-ecs-patterns:           "Protocol": "TCP",
@aws-cdk/aws-ecs-patterns:           "TargetType": "ip",
@aws-cdk/aws-ecs-patterns:           "VpcId": {
@aws-cdk/aws-ecs-patterns:             "Ref": "VPCB9E5F0B4"
@aws-cdk/aws-ecs-patterns:           }
@aws-cdk/aws-ecs-patterns:         }
@aws-cdk/aws-ecs-patterns:       }
@aws-cdk/aws-ecs-patterns:     with the following mismatches:
@aws-cdk/aws-ecs-patterns:     	Expected 81 but received 80 at /Properties/Port (using objectLike matcher)
@aws-cdk/aws-ecs-patterns:       83 |     const matchError = hasResourceProperties(this.template, type, props);
@aws-cdk/aws-ecs-patterns:       84 |     if (matchError) {
@aws-cdk/aws-ecs-patterns:     > 85 |       throw new Error(matchError);
@aws-cdk/aws-ecs-patterns:          |             ^
@aws-cdk/aws-ecs-patterns:       86 |     }
@aws-cdk/aws-ecs-patterns:       87 |   }
@aws-cdk/aws-ecs-patterns:       88 |
@aws-cdk/aws-ecs-patterns:       at Template.hasResourceProperties (../assertions/lib/template.ts:85:13)
@aws-cdk/aws-ecs-patterns:       at fn (test/fargate/load-balanced-fargate-service-v2.test.ts:823:31)
@aws-cdk/aws-ecs-patterns:       at Object.<anonymous> (../../../tools/@aws-cdk/cdk-build-tools/lib/feature-flag.ts:34:35)
```

</details>


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@Chriscbr
Chriscbr authored May 20, 2022
1 parent 484cfb2 commit 3a11317
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 164 deletions.
11 changes: 5 additions & 6 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import { INetworkLoadBalancer, NetworkListener, NetworkLoadBalancer, NetworkTarg
import { IRole } from '@aws-cdk/aws-iam';
import { ARecord, CnameRecord, IHostedZone, RecordTarget } from '@aws-cdk/aws-route53';
import { LoadBalancerTarget } from '@aws-cdk/aws-route53-targets';
import { CfnOutput, Duration, FeatureFlags, Stack } from '@aws-cdk/core';
import { ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT } from '@aws-cdk/cx-api';
import * as cdk from '@aws-cdk/core';
import { Construct } from 'constructs';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
Expand Down Expand Up @@ -104,7 +103,7 @@ export interface NetworkLoadBalancedServiceBaseProps {
*
* @default - defaults to 60 seconds if at least one load balancer is in-use and it is not already set
*/
readonly healthCheckGracePeriod?: Duration;
readonly healthCheckGracePeriod?: cdk.Duration;

/**
* The maximum number of tasks, specified as a percentage of the Amazon ECS
Expand Down Expand Up @@ -348,7 +347,7 @@ export abstract class NetworkLoadBalancedServiceBase extends CoreConstruct {
const loadBalancer = props.loadBalancer ?? new NetworkLoadBalancer(this, 'LB', lbProps);
const listenerPort = props.listenerPort ?? 80;
const targetProps = {
port: FeatureFlags.of(this).isEnabled(ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT) ? props.taskImageOptions?.containerPort ?? 80 : 80,
port: props.taskImageOptions?.containerPort ?? 80,
};

this.listener = loadBalancer.addListener('PublicListener', { port: listenerPort });
Expand Down Expand Up @@ -385,7 +384,7 @@ export abstract class NetworkLoadBalancedServiceBase extends CoreConstruct {
}

if (props.loadBalancer === undefined) {
new CfnOutput(this, 'LoadBalancerDNS', { value: this.loadBalancer.loadBalancerDnsName });
new cdk.CfnOutput(this, 'LoadBalancerDNS', { value: this.loadBalancer.loadBalancerDnsName });
}
}

Expand All @@ -395,7 +394,7 @@ export abstract class NetworkLoadBalancedServiceBase extends CoreConstruct {
protected getDefaultCluster(scope: CoreConstruct, vpc?: IVpc): Cluster {
// magic string to avoid collision with user-defined constructs
const DEFAULT_CLUSTER_ID = `EcsDefaultClusterMnL3mNNYN${vpc ? vpc.node.id : ''}`;
const stack = Stack.of(scope);
const stack = cdk.Stack.of(scope);
return stack.node.tryFindChild(DEFAULT_CLUSTER_ID) as Cluster || new Cluster(stack, DEFAULT_CLUSTER_ID, { vpc });
}

Expand Down
5 changes: 2 additions & 3 deletions packages/@aws-cdk/aws-ecs-patterns/lib/base/network-multiple-target-groups-service-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import { NetworkListener, NetworkLoadBalancer, NetworkTargetGroup } from '@aws-c
import { IRole } from '@aws-cdk/aws-iam';
import { ARecord, IHostedZone, RecordTarget } from '@aws-cdk/aws-route53';
import { LoadBalancerTarget } from '@aws-cdk/aws-route53-targets';
import { CfnOutput, Duration, FeatureFlags, Stack } from '@aws-cdk/core';
import { ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT } from '@aws-cdk/cx-api';
import { CfnOutput, Duration, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';

// v2 - keep this import as a separate section to reduce merge conflict when forward merging with the v2 branch.
Expand Down Expand Up @@ -375,7 +374,7 @@ export abstract class NetworkMultipleTargetGroupsServiceBase extends CoreConstru
protected registerECSTargets(service: BaseService, container: ContainerDefinition, targets: NetworkTargetProps[]): NetworkTargetGroup {
for (const targetProps of targets) {
const targetGroup = this.findListener(targetProps.listener).addTargets(`ECSTargetGroup${container.containerName}${targetProps.containerPort}`, {
port: FeatureFlags.of(this).isEnabled(ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT) ? targetProps.containerPort ?? 80 : 80,
port: targetProps.containerPort ?? 80,
targets: [
service.loadBalancerTarget({
containerName: container.containerName,
Expand Down
150 changes: 5 additions & 145 deletions packages/@aws-cdk/aws-ecs-patterns/test/fargate/load-balanced-fargate-service-v2.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ import { Vpc } from '@aws-cdk/aws-ec2';
import * as ecs from '@aws-cdk/aws-ecs';
import { ContainerImage } from '@aws-cdk/aws-ecs';
import { CompositePrincipal, Role, ServicePrincipal } from '@aws-cdk/aws-iam';
import { testFutureBehavior, testLegacyBehavior } from '@aws-cdk/cdk-build-tools';
import { App, Duration, Stack } from '@aws-cdk/core';
import { ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT } from '@aws-cdk/cx-api';
import { Duration, Stack } from '@aws-cdk/core';
import { ApplicationLoadBalancedFargateService, ApplicationMultipleTargetGroupsFargateService, NetworkLoadBalancedFargateService, NetworkMultipleTargetGroupsFargateService } from '../../lib';

describe('When Application Load Balancer', () => {
Expand Down Expand Up @@ -665,36 +663,9 @@ describe('When Network Load Balancer', () => {
}).toThrow(/You must specify one of: taskDefinition or image/);
});

testLegacyBehavior('Fargate neworkloadbalanced construct uses Port 80 for target group when feature flag is not enabled', App, (app) => {
test('test Fargate networkloadbalanced construct with custom Port', () => {
// GIVEN
const stack = new Stack(app);
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

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

Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::TargetGroup', {
Port: 80,
Protocol: 'TCP',
TargetType: 'ip',
VpcId: {
Ref: 'VPCB9E5F0B4',
},
});
});

testFutureBehavior('Fargate networkloadbalanced construct uses custom Port for target group when feature flag is enabled', { [ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT]: true }, App, (app) => {
// GIVEN
const stack = new Stack(app);
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

Expand All @@ -719,79 +690,9 @@ describe('When Network Load Balancer', () => {
});
});

testFutureBehavior('Fargate networkloadbalanced construct uses 80 for target group when feature flag is enabled but container port is not provided', { [ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT]: true }, App, (app) => {
// GIVEN
const stack = new Stack(app);
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

new NetworkLoadBalancedFargateService(stack, 'NLBService', {
cluster: cluster,
memoryLimitMiB: 1024,
cpu: 512,
taskImageOptions: {
image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
},
listenerPort: 8181,
});

Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::TargetGroup', {
Port: 80,
Protocol: 'TCP',
TargetType: 'ip',
VpcId: {
Ref: 'VPCB9E5F0B4',
},
});
});

testLegacyBehavior('Fargate multinetworkloadbalanced construct uses Port 80 for target group when feature flag is not enabled', App, (app) => {
// GIVEN
const stack = new Stack(app);
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

new NetworkMultipleTargetGroupsFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
},
});


new NetworkMultipleTargetGroupsFargateService(stack, 'NLBService', {
cluster: cluster,
memoryLimitMiB: 1024,
cpu: 512,
taskImageOptions: {
image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
},
loadBalancers: [
{
name: 'lb1',
listeners: [
{ name: 'listener1', port: 8181 },
],
},
],
targetGroups: [{
containerPort: 81,
}],
});

Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::TargetGroup', {
Port: 80,
Protocol: 'TCP',
TargetType: 'ip',
VpcId: {
Ref: 'VPCB9E5F0B4',
},
});
});

testFutureBehavior('test Fargate multinetworkloadbalanced construct uses custom Port for target group when feature flag is enabled', { [ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT]: true }, App, (app) => {
test('test Fargate multinetworkloadbalanced construct with custom Port', () => {
// GIVEN
const stack = new Stack(app);
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

Expand Down Expand Up @@ -832,45 +733,4 @@ describe('When Network Load Balancer', () => {
},
});
});

testFutureBehavior('test Fargate multinetworkloadbalanced construct uses 80 for target group when feature flag is enabled but container port is not provided', { [ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT]: true }, App, (app) => {
// GIVEN
const stack = new Stack(app);
const vpc = new Vpc(stack, 'VPC');
const cluster = new ecs.Cluster(stack, 'Cluster', { vpc });

new NetworkMultipleTargetGroupsFargateService(stack, 'Service', {
cluster,
taskImageOptions: {
image: ecs.ContainerImage.fromRegistry('test'),
},
});


new NetworkMultipleTargetGroupsFargateService(stack, 'NLBService', {
cluster: cluster,
memoryLimitMiB: 1024,
cpu: 512,
taskImageOptions: {
image: ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
},
loadBalancers: [
{
name: 'lb1',
listeners: [
{ name: 'listener1', port: 8181 },
],
},
],
});

Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::TargetGroup', {
Port: 80,
Protocol: 'TCP',
TargetType: 'ip',
VpcId: {
Ref: 'VPCB9E5F0B4',
},
});
});
});
10 changes: 0 additions & 10 deletions packages/@aws-cdk/cx-api/lib/features.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,6 @@ export const EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME = '@aws-cdk/aws-ec2:uniqueIm
*/
export const IAM_MINIMIZE_POLICIES = '@aws-cdk/aws-iam:minimizePolicies';

/**
* Enable this feature flag to pass through the `NetworkLoadBalanced<Ec2|Fargate>ServiceProps.taskImageOptions.containerPort`
* and the `NetworkMultipleTargetGroups<Ec2|Fargate>ServiceProps.targetGroups[X].containerPort` to the generated
* `ElasticLoadBalancingV2::TargetGroup`'s `Port` property.
*
* This is a feature flag because updating `Port` causes a replacement of the target groups, which is a breaking change.
*/
export const ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT = '@aws-cdk/aws-ecs-patterns:containerPortToTargetGroupPort';

/**
* Flag values that should apply for new projects
*
Expand Down Expand Up @@ -269,7 +260,6 @@ export const FUTURE_FLAGS: { [key: string]: boolean } = {
[EC2_UNIQUE_IMDSV2_LAUNCH_TEMPLATE_NAME]: true,
[CHECK_SECRET_USAGE]: true,
[IAM_MINIMIZE_POLICIES]: true,
[ECS_PATTERNS_TARGET_GROUP_PORT_FROM_CONTAINER_PORT]: true,
};

/**
Expand Down

0 comments on commit 3a11317

Please sign in to comment.