-
Notifications
You must be signed in to change notification settings - Fork 6
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor: Encapsulate overriding of logicalId in a single place
A resource's logicalId maps to the name of the created resource. For example, take this CloudFormation YAML for an ASG: ```yaml DataNodeAutoScalingGroup: Type: AWS::AutoScaling::AutoScalingGroup Properties: AvailabilityZones: !GetAZs "" LaunchConfigurationName: !Ref DataNodeLaunchConfig MinSize: 3 MaxSize: 4 Cooldown: 180 HealthCheckType: EC2 HealthCheckGracePeriod: 300 VPCZoneIdentifier: !Ref PrivateVpcSubnets Tags: - { Key: Stack, Value: !Ref Stack, PropagateAtLaunch: true } - { Key: App, Value: "elk-es-data", PropagateAtLaunch: true } - { Key: Stage, Value: !Ref Stage, PropagateAtLaunch: true } - { Key: ElasticSearchCluster, Value: !Ref ClusterName, PropagateAtLaunch: true } ``` This creates an ASG called `DataNodeAutoScalingGroup`. This pattern is widely used in YAML definitions of CloudFormation as YAML files are typically hand-written. CDK changes this as it auto-generates the logicalIds, as there one will gain more understanding about the resource from looking at the CDK template. In order to enable a no-op migration between a YAML stack a CDK stack, we allow the logicalId to be overridden. Our constructs seem to follow different rules regarding when to override the logicalId of a CloudFormation resource. For example, we have [this](https://github.com/guardian/cdk/blob/60cbf21698b3ddbb8cfd23556fd2af886e9551c7/src/constructs/rds/instance.ts#L38-L40): ```typescript if (props.overrideId || (scope.migratedFromCloudFormation && props.overrideId !== false)) ``` And [this](https://github.com/guardian/cdk/blob/60cbf21698b3ddbb8cfd23556fd2af886e9551c7/src/constructs/iam/policies/base-policy.ts#L15-L18): ```typescript if (props.overrideId) { ``` This change aims to bring consistency and simplicity across our constructs by applying a universal rule that, `migratedFromCloudFormation` on the `GuStack` must be `true` and `existingLogicalId` to be set for the logicalId to be overridden. If these conditions are not met, we let CDK auto-generate the logicalId. It's worth noting that this doesn't implement the change on the constructs, it's simply to define the behaviour. This is mainly to keep the diff down - we'll migrate the constructs in follow-up PRs. Usage will roughly look like this: ```typescript const MyComponentProps extends GuMigratingResource { } class MyComponent extends SomeGuConstruct { constructor(scope: GuStack, id: string, props: MyComponentProps) { super(scope, id, props); GuMigratingResource.setLogicalId(this, scope, props); } } ```
- Loading branch information
Showing
3 changed files
with
185 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
import "@aws-cdk/assert/jest"; | ||
import { SynthUtils } from "@aws-cdk/assert"; | ||
import type { BucketProps } from "@aws-cdk/aws-s3"; | ||
import { Bucket } from "@aws-cdk/aws-s3"; | ||
import type { SynthedStack } from "../../utils/test"; | ||
import { simpleGuStackForTesting } from "../../utils/test"; | ||
import type { GuStatefulConstruct } from "./migrating"; | ||
import { GuMigratingResource } from "./migrating"; | ||
import type { GuStack } from "./stack"; | ||
|
||
class TestGuStatefulConstruct extends Bucket implements GuStatefulConstruct { | ||
isStatefulConstruct: true; | ||
|
||
constructor(scope: GuStack, id: string, props?: BucketProps) { | ||
super(scope, id, props); | ||
this.isStatefulConstruct = true; | ||
} | ||
} | ||
|
||
/* | ||
NOTE: In reality, we'd never directly call `GuMigratingResource.setLogicalId` as the GuConstruct will do that. | ||
We're calling it here to test the function in isolation. | ||
*/ | ||
|
||
describe("GuMigratingResource", () => { | ||
// eslint-disable-next-line @typescript-eslint/no-empty-function -- we are testing `console.info` being called, we don't need to see the message | ||
const info = jest.spyOn(console, "info").mockImplementation(() => {}); | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-empty-function -- we are testing `console.warn` being called, we don't need to see the message | ||
const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); | ||
|
||
afterEach(() => { | ||
warn.mockReset(); | ||
info.mockReset(); | ||
}); | ||
|
||
test("Creating a new resource in a new stack produces a new auto-generated ID", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: false }); | ||
const construct = new Bucket(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, {}); | ||
|
||
expect(warn).toHaveBeenCalledTimes(0); | ||
expect(info).toHaveBeenCalledTimes(0); | ||
|
||
const json = SynthUtils.toCloudFormation(stack) as SynthedStack; | ||
const resourceKeys = Object.keys(json.Resources); | ||
|
||
expect(resourceKeys).toHaveLength(1); | ||
expect(resourceKeys[0]).toMatch(/^MyBucket[A-Z0-9]+$/); | ||
}); | ||
|
||
test("Keeping a resource's logicalId when migrating a stack", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true }); | ||
const construct = new Bucket(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, { existingLogicalId: "my-pre-existing-bucket" }); | ||
|
||
expect(warn).toHaveBeenCalledTimes(0); | ||
expect(info).toHaveBeenCalledTimes(0); | ||
|
||
const json = SynthUtils.toCloudFormation(stack) as SynthedStack; | ||
expect(Object.keys(json.Resources)).toContain("my-pre-existing-bucket"); | ||
}); | ||
|
||
test("Creating a construct in a migrating stack, w/out setting existingLogicalId", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true }); | ||
const construct = new Bucket(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, {}); | ||
|
||
expect(info).toHaveBeenCalledTimes(0); | ||
expect(warn).toHaveBeenCalledTimes(0); | ||
|
||
const json = SynthUtils.toCloudFormation(stack) as SynthedStack; | ||
const resourceKeys = Object.keys(json.Resources); | ||
|
||
expect(resourceKeys).toHaveLength(1); | ||
expect(resourceKeys[0]).toMatch(/^MyBucket[A-Z0-9]+$/); | ||
}); | ||
|
||
test("Specifying a construct's existingLogicalId in a new stack", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: false }); | ||
const construct = new Bucket(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, { existingLogicalId: "my-pre-existing-bucket" }); | ||
|
||
expect(info).toHaveBeenCalledTimes(0); | ||
expect(warn).toHaveBeenCalledTimes(1); | ||
expect(warn).toHaveBeenCalledWith( | ||
"GuStack has 'migratedFromCloudFormation' set to false. MyBucket has an 'existingLogicalId' set to my-pre-existing-bucket. This will have no effect - the logicalId will be auto-generated. Set 'migratedFromCloudFormation' to true for 'existingLogicalId' to be observed." | ||
); | ||
|
||
const json = SynthUtils.toCloudFormation(stack) as SynthedStack; | ||
const resourceKeys = Object.keys(json.Resources); | ||
|
||
expect(resourceKeys).toHaveLength(1); | ||
expect(resourceKeys[0]).toMatch(/^MyBucket[A-Z0-9]+$/); | ||
}); | ||
|
||
test("Creating a stateful construct in a migrating stack, w/out setting existingLogicalId", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: true }); | ||
const construct = new TestGuStatefulConstruct(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, {}); | ||
expect(warn).toHaveBeenCalledTimes(1); | ||
expect(warn).toHaveBeenCalledWith( | ||
"GuStack has 'migratedFromCloudFormation' set to true. MyBucket is a stateful construct and 'existingLogicalId' has not been set. MyBucket's logicalId will be auto-generated and consequently AWS will create a new resource rather than inheriting an existing one. This is not advised as downstream services, such as DNS, will likely need updating." | ||
); | ||
}); | ||
|
||
test("Creating a stateful construct in a new stack, w/out setting existingLogicalId", () => { | ||
const stack = simpleGuStackForTesting({ migratedFromCloudFormation: false }); | ||
const construct = new TestGuStatefulConstruct(stack, "MyBucket"); | ||
|
||
GuMigratingResource.setLogicalId(construct, stack, {}); | ||
expect(info).toHaveBeenCalledTimes(1); | ||
expect(info).toHaveBeenCalledWith( | ||
"GuStack has 'migratedFromCloudFormation' set to false. MyBucket is a stateful construct, it's logicalId will be auto-generated and AWS will create a new resource." | ||
); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import type { CfnElement, IConstruct } from "@aws-cdk/core"; | ||
import type { GuStack } from "./stack"; | ||
|
||
export interface GuMigratingResource { | ||
existingLogicalId?: string; | ||
} | ||
|
||
export interface GuStatefulConstruct extends IConstruct { | ||
isStatefulConstruct: true; | ||
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- user defined type guard | ||
function isGuStatefulConstruct(construct: any): construct is GuStatefulConstruct { | ||
return "isStatefulConstruct" in construct; | ||
} | ||
|
||
export const GuMigratingResource = { | ||
setLogicalId<T extends GuStatefulConstruct | IConstruct>( | ||
construct: T, | ||
{ migratedFromCloudFormation }: GuStack, | ||
{ existingLogicalId }: GuMigratingResource | ||
): void { | ||
const overrideLogicalId = (logicalId: string) => { | ||
const defaultChild = construct.node.defaultChild as CfnElement; | ||
defaultChild.overrideLogicalId(logicalId); | ||
}; | ||
|
||
const id = construct.node.id; | ||
const isStateful = isGuStatefulConstruct(construct); | ||
|
||
if (migratedFromCloudFormation) { | ||
if (existingLogicalId) { | ||
return overrideLogicalId(existingLogicalId); | ||
} | ||
|
||
if (isStateful) { | ||
console.warn( | ||
`GuStack has 'migratedFromCloudFormation' set to true. ${id} is a stateful construct and 'existingLogicalId' has not been set. ${id}'s logicalId will be auto-generated and consequently AWS will create a new resource rather than inheriting an existing one. This is not advised as downstream services, such as DNS, will likely need updating.` | ||
); | ||
} | ||
} else { | ||
if (existingLogicalId) { | ||
console.warn( | ||
`GuStack has 'migratedFromCloudFormation' set to false. ${id} has an 'existingLogicalId' set to ${existingLogicalId}. This will have no effect - the logicalId will be auto-generated. Set 'migratedFromCloudFormation' to true for 'existingLogicalId' to be observed.` | ||
); | ||
} | ||
|
||
if (isStateful) { | ||
console.info( | ||
`GuStack has 'migratedFromCloudFormation' set to false. ${id} is a stateful construct, it's logicalId will be auto-generated and AWS will create a new resource.` | ||
); | ||
} | ||
} | ||
}, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters