Skip to content

Commit

Permalink
refactor: Encapsulate overriding of logicalId in a single place
Browse files Browse the repository at this point in the history
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
akash1810 committed Apr 7, 2021
1 parent 486946b commit 5274685
Show file tree
Hide file tree
Showing 3 changed files with 197 additions and 0 deletions.
122 changes: 122 additions & 0 deletions src/constructs/core/migrating.test.ts
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."
);
});
});
67 changes: 67 additions & 0 deletions src/constructs/core/migrating.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import type { CfnElement, IConstruct } from "@aws-cdk/core";
import type { GuStack } from "./stack";

export interface GuMigratingResource {
/**
* A string to use to override the logicalId AWS CDK auto-generates for a resource.
* This is useful when migrating a pre-existing stack into guardian/cdk,
* as it ensures resources are kept rather than recreated.
* For example, imagine a YAML stack that creates a load balancer with logicalId `DotcomLoadbalancer`.
* We would want to set `existingLogicalId` for the GuLoadBalancer in guardian/cdk to ensure it is preserved when moving to guardian/cdk.
* Otherwise it will be created as something like `DotcomLoadbalancerABCDEF`,
* is a new resource, and require any DNS entries to be updated accordingly.
*
* @requires `migratedFromCloudFormation` to be true in [[ GuStack ]]
* @see GuStackProps
*/
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.`
);
}
}
},
};
8 changes: 8 additions & 0 deletions src/constructs/core/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ import { GuStageParameter } from "./parameters";

export interface GuStackProps extends StackProps {
stack: string;

/**
* A flag to symbolise if a stack is being migrated from a previous format (eg YAML) into guardian/cdk.
* A value of `true` means resources in the stack can have custom logicalIds set using the property `existingLogicalId` (where available).
* A value of `false` or `undefined` means the stack is brand new. Any resource that gets created will have an auto-generated logicalId.
*
* @see GuMigratingResource
*/
migratedFromCloudFormation?: boolean;
}

Expand Down

0 comments on commit 5274685

Please sign in to comment.