Skip to content

Commit

Permalink
revert: "fix(elasticloadbalancerV2): logicalId supports switch from a…
Browse files Browse the repository at this point in the history
…ddTargetGroups (under feature flag)" (#29716)

Reverts #29513

We will want to use a property to achieve the desired behavior instead of a feature flag since we are not changing the default behavior.
  • Loading branch information
scanlonp committed Apr 4, 2024
1 parent 8e3848c commit b82320b
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 114 deletions.
10 changes: 5 additions & 5 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -1196,18 +1196,18 @@ Adding a new flag looks as follows:
with the name of the context key that enables this new feature (for
example, `ENABLE_STACK_NAME_DUPLICATES`). The context key should be in the
form `module.Type:feature` (e.g. `@aws-cdk/core:enableStackNameDuplicates`).
2. Add your feature flag to the `FLAGS` map in
[cx-api/lib/features.ts](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/lib/features.ts).
- Set `introducedIn.v2` to the literal string `'V2NEXT'`.
- Double negatives should be avoided. If you want to add a flag that disables something that was previously
enabled, set `default.v2` to `true` and the `recommendedValue` to `false`. You will need to update
a test in `features.test.ts` -- this is okay if you have a good reason.
In your description, be sure to cover the following:
2. Use `FeatureFlags.of(construct).isEnabled(cxapi.ENABLE_XXX)` to check if this feature is enabled
in your code. If it is not defined, revert to the legacy behavior.
3. Add your feature flag to the `FLAGS` map in
[cx-api/lib/features.ts](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/lib/features.ts). In
your description, be sure to cover the following:
- Consciously pick the type of feature flag. Can the flag be removed in a future major version, or not?
- Motivate why the feature flag exists. What is the change to existing infrastructure and why is it not safe?
- In case of a "default change flag", describe what the user needs to do to restore the old behavior.
3. Use `FeatureFlags.of(construct).isEnabled(cxapi.ENABLE_XXX)` to check if this feature is enabled
in your code. If it is not defined, revert to the legacy behavior.
4. Add an entry for your feature flag in the [README](https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/cx-api/README.md) file.
5. In your tests, ensure that you test your feature with and without the feature flag enabled. You can do this by passing the feature flag to the `context` property when instantiating an `App`.
```ts
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { ApplicationTargetGroup, IApplicationLoadBalancerTarget, IApplicationTar
import { ListenerCondition } from './conditions';
import * as ec2 from '../../../aws-ec2';
import * as cxschema from '../../../cloud-assembly-schema';
import { Duration, FeatureFlags, Lazy, Resource, Token } from '../../../core';
import { Duration, Lazy, Resource, Token } from '../../../core';
import * as cxapi from '../../../cx-api';
import { BaseListener, BaseListenerLookupOptions, IListener } from '../shared/base-listener';
import { HealthCheck } from '../shared/base-target-group';
Expand Down Expand Up @@ -664,22 +664,15 @@ abstract class ExternalApplicationListener extends Resource implements IApplicat
* It is not possible to add a default action to an imported IApplicationListener.
* In order to add actions to an imported IApplicationListener a `priority`
* must be provided.
*
* Warning, if you are attempting to migrate an existing `ListenerAction`
* which was declared by the {@link addTargetGroups} method, you will
* need to enable the
* `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction`
* feature flag.
*/
public addAction(id: string, props: AddApplicationActionProps): void {
checkAddRuleProps(props);

if (props.priority !== undefined) {
const idSuffix = FeatureFlags.of(this).isEnabled(cxapi.ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION) ? '' : 'Rule';
// New rule
//
// TargetGroup.registerListener is called inside ApplicationListenerRule.
new ApplicationListenerRule(this, id + idSuffix, {
new ApplicationListenerRule(this, id + 'Rule', {
listener: this,
priority: props.priority,
...props,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { Metric } from '../../../aws-cloudwatch';
import * as ec2 from '../../../aws-ec2';
import * as cdk from '../../../core';
import { SecretValue } from '../../../core';
import * as cxapi from '../../../cx-api';
import * as elbv2 from '../../lib';
import { FakeSelfRegisteringTarget } from '../helpers';

Expand Down Expand Up @@ -1682,40 +1681,6 @@ describe('tests', () => {
}).toThrow(/specify only one/);
});

describe('ExternalApplicationListener logicalId support', () => {

test('compatibility mode for addAction', () => {
// GIVEN
const context = { [cxapi.ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION]: true };
const app = new cdk.App({ context });
const stack = new cdk.Stack(app, 'stack', {
env: {
account: '123456789012',
region: 'us-west-2',
},
});
const vpc = new ec2.Vpc(stack, 'Stack');
const targetGroup = new elbv2.ApplicationTargetGroup(stack, 'TargetGroup', { vpc, port: 80 });
const listener = elbv2.ApplicationListener.fromLookup(stack, 'a', {
loadBalancerTags: {
some: 'tag',
},
});
// WHEN
const identifierToken = 'SuperMagicToken';
listener.addAction(identifierToken, {
action: elbv2.ListenerAction.weightedForward([{ targetGroup, weight: 1 }]),
conditions: [elbv2.ListenerCondition.pathPatterns(['/fake'])],
priority: 42,
});

// THEN
const applicationListenerRule = listener.node.children.find((v)=> v.hasOwnProperty('conditions'));
expect(applicationListenerRule).toBeDefined();
expect(applicationListenerRule!.node.id).toBe(identifierToken); // Should not have `Rule` suffix
});
});

test('not allowed to specify defaultTargetGroups and defaultAction together', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down
17 changes: 0 additions & 17 deletions packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ Flags come in three types:
| [@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse](#aws-cdkaws-codepipelinecrossaccountkeysdefaultvaluetofalse) | Enables Pipeline to set the default value for crossAccountKeys to false. | 2.127.0 | (default) |
| [@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2](#aws-cdkaws-codepipelinedefaultpipelinetypetov2) | Enables Pipeline to set the default pipeline type to V2. | 2.133.0 | (default) |
| [@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope](#aws-cdkaws-kmsreducecrossaccountregionpolicyscope) | When enabled, IAM Policy created from KMS key grant will reduce the resource scope to this key only. | 2.134.0 | (fix) |
| [@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction](#aws-cdkaws-elasticloadbalancingv2externalapplicationlistener-norulesuffixforaddaction) | When enabled, you can switch from the `addTargetGroups()` method of declaring a `ListenerRule` to the `addAction()` method,
without changing the logicalId and replacing your resource. | V2NEXT | (fix) |

<!-- END table -->

Expand Down Expand Up @@ -1267,19 +1265,4 @@ When this feature flag is enabled and calling KMS key grant method, the created
| 2.134.0 | `false` | `true` |


### @aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction

*When enabled, you can switch from the `addTargetGroups()` method of declaring a `ListenerRule` to the `addAction()` method,
without changing the logicalId and replacing your resource.* (fix)

Setting this feature flag will cause the `addAction()` method to not add the `Rule` suffix on the logicalId.
This allows you to switch from the `addTargetGroups()` method without having CloudFormation deadlock while attempting to replace the resource.


| Since | Default | Recommended |
| ----- | ----- | ----- |
| (not in v1) | | |
| V2NEXT | `false` | `false` |


<!-- END details -->
26 changes: 0 additions & 26 deletions packages/aws-cdk-lib/cx-api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -309,29 +309,3 @@ _cdk.json_
}
}
```

* `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction`

Enable this feature flag if you have deployed a `ListenerRule` using the `addTargetGroups()`
convenience method against an `ExternalApplicationListener` and you need to migrate to
using the `addAction()` method for more complex rule configurations.
This will prevent `Rule` from being added as a suffix to the logicalId so that the logicalId will remain the same.

Do not enable this if you have already deployed `ListenerRule` resources using the
`addAction()` method.
Instead consider the [cdk-logical-id-mapper](https://github.com/mbonig/cdk-logical-id-mapper),
possibly in conjunction with `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-addTargetGroupsConsistentLogicalId` (see below).

* `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-addTargetGroupsConsistentLogicalId`

Enable this feature flag to ensure that the logicalIds of `ListenerRule`s created
on a `ExternalApplicationListener` by the `addTargetGroups()` method are consistent
with logicalIds for `ListenerRules` generated by other methods.
This will allow you to migrate between the different methods
without causing a replacement of the `ListenerRule` resource.

You should enable this on new apps, before creating any resources.
If you have already created resources with the previous behavior,
you may still enable this flag, but will need to use something like the
[cdk-logical-id-mapper](https://github.com/mbonig/cdk-logical-id-mapper).
Alternatively, do not enable this feature flag and instead consider the `@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction` as necessary.
22 changes: 0 additions & 22 deletions packages/aws-cdk-lib/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ export const LAMBDA_PERMISSION_LOGICAL_ID_FOR_LAMBDA_ACTION = '@aws-cdk/aws-clou
export const CODEPIPELINE_CROSS_ACCOUNT_KEYS_DEFAULT_VALUE_TO_FALSE = '@aws-cdk/aws-codepipeline:crossAccountKeysDefaultValueToFalse';
export const CODEPIPELINE_DEFAULT_PIPELINE_TYPE_TO_V2 = '@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2';
export const KMS_REDUCE_CROSS_ACCOUNT_REGION_POLICY_SCOPE = '@aws-cdk/aws-kms:reduceCrossAccountRegionPolicyScope';
export const ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION = '@aws-cdk/aws-elasticloadbalancingv2:ExternalApplicationListener-noRuleSuffixForAddAction';

export const FLAGS: Record<string, FlagInfo> = {
//////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -1035,27 +1034,6 @@ export const FLAGS: Record<string, FlagInfo> = {
introducedIn: { v2: '2.134.0' },
recommendedValue: true,
},

//////////////////////////////////////////////////////////////////////
[ALBV2_EXTERNALAPPLICATIONLISTENER_SWITCH_FROM_ADDTARGETGROUP_TO_ADDACTION]: {
type: FlagType.VisibleContext,
summary: 'When enabled, you can switch from the \`addTargetGroups()\` method of declaring a \`ListenerRule\` to the \`addAction()\` method, without changing the logicalId and replacing your resource.',
detailsMd: `
When switching from a less complex to a more complex use of ALB,
you will eventually need features not available in the \`addTargetGroups()\` convenience method.
In this case you will want to use the \`addAction()\` method.
Before this feature is enabled, switching over to \`addAction()\` from using \`addTargetGroups()\`
will add a \`Rule\` suffix to the logicalId of your \`ListenerRule\`,
causing CloudFormation to attempt to replace the resource.
Since \`ListenerRule\`s have a unique priority,
CloudFormation will always fail when generating the new \`ListenerRule\`.
Setting this feature flag will cause the \`addAction()\` method to not add the \`Rule\` suffix on the logicalId.
This allows you to switch from the \`addTargetGroups()\` method without having CloudFormation deadlock while attempting to replace the resource.
`,
introducedIn: { v2: 'V2NEXT' },
recommendedValue: false,
},
};

const CURRENT_MV = 'v2';
Expand Down

0 comments on commit b82320b

Please sign in to comment.