Refactor to use cloud assembly #167

corymhall · 2024-10-01T18:30:29Z

Previously this library walked the construct tree in-memory and called the internal _toCloudFormation() method each Cfn* construct has to get the CloudFormation resource fragment. This is both inefficient (because we call synth which internally does the same thing, and then we do it again), and potentially inaccurate (synth internally does a lot more than we do when we walk the construct tree).

This PR is a refactor (maybe closer to a rewrite) that switches to taking the output of app.synth() and processing the resulting CloudAssembly. The CloudAssembly has everything necessary to do the conversion and contains the fully resolved CloudFormation template (we don't have to worry about unresolved tokens!).

This also sets us up for future work like nested stacks, multiple stacks, and #153.

Note to reviewers. This is a pretty big refactor so I would recommend reviewing this instead as a restart rather than trying to figure out what the old code is doing.

Closes #18

Previously this library walked the construct tree in-memory and called the internal `_toCloudFormation()` method each `Cfn*` construct has to get the CloudFormation resource fragment. This is both inefficient (because we call `synth` which internally does the same thing, and then we do it again), and potentially inaccurate (`synth` internally does a lot more than we do when we walk the construct tree). This PR is a refactor (maybe closer to a rewrite) that switches to taking the output of `app.synth()` and processing the resulting CloudAssembly. The CloudAssembly has everything necessary to do the conversion and contains the fully resolved CloudFormation template (we don't have to worry about unresolved tokens!). This also sets us up for future work like nested stacks, multiple stacks, and #153. Note to reviewers. This is a pretty big refactor so I would recommend reviewing this instead as a restart rather than trying to figure out what the old code is doing. Closes #18

src/assembly/manifest.ts

t0yv0 · 2024-10-03T19:41:27Z

src/converters/app-converter.ts

+        this.manifestReader = AssemblyManifestReader.fromPath(host.assemblyDir);
+    }
+
+    convert() {


If I've been following right convert() methods actually issue Pulumi resource calls e.g. new Bucket(). So the protocol here is SomeConverter is a partial result, but SomeConverter.convert() "runs" the Pulumi program. Just checking.

Yes I think that is correct.

t0yv0 · 2024-10-03T19:51:18Z

src/converters/app-converter.ts

+        return this.processIntrinsics(value) as T;
+    }
+
+    private processIntrinsics(obj: any): any {


I've been staring at this and it's a bit fast and furious for me, do we know what the domain and range are? What can these things be? Not entirely any right? Is incremental typing not worth it here? Perhaps can at least document the domain and range better.

Looking one thing this is applied on is this:

export interface CloudFormationResource { readonly Type: string; readonly Properties: any; readonly Condition?: string; readonly DependsOn?: string | string[]; }

Already model is Properties: any so perhaps it's a necessity.

readonly Properties: any;

We might be able to do better, worth looking into.

Could be a follow up for later productization phase though. If it works it works.

t0yv0 · 2024-10-03T19:51:42Z

src/converters/app-converter.ts

+    }
+
+    private isNoValue(obj: any): boolean {
+        return obj?.Ref === 'AWS::NoValue';


Eh, what fun is this?

Fun CloudFormation! This is how you specify undefined in CloudFormation 🙃

t0yv0 · 2024-10-03T19:52:27Z

src/converters/app-converter.ts

+            }
+
+            case 'Fn::Join':
+                return lift(([delim, strings]) => strings.join(delim), this.processIntrinsics(params));


Powerful meta-programming here. Again I'd like to discuss a bit other representations then any if they add any value for us here.

Yes, I would like to refactor this whole lift thing. It is very hard to track and I already fixed one bug where Fn::Base64 wasn't even working.

t0yv0 · 2024-10-03T19:53:17Z

src/converters/app-converter.ts

+        }
+    }
+
+    private resolveRef(target: any): any {


This looks like it's emitting Pulumi data source calls?

Yes it does. Some builtin CloudFormation intrinsics are akin to data source calls. I should better document this.

t0yv0 · 2024-10-03T19:59:00Z

src/graph.ts

+     * This will only be set if this node represents a CloudFormation resource.
+     * It will not be set for wrapper constructs
+     */
+    logicalId?: string;


Worth commenting what incoming and outgoing edges are? Something with dependency information? I was not super sure. What does it mean to have an outgoingEdge?

Also, Set<GraphNode> compares on hash equality? SO this is what we want here, one node = one in-mem object?

src/types.ts

t0yv0 · 2024-10-03T20:01:43Z

src/types.ts

+    attributes?: { [name: string]: pulumi.Input<any> };
+};
+
+export function containsEventuals(v: any): boolean {


Should this code live elsewhere?

t0yv0 · 2024-10-03T20:03:13Z

Can you highlight the app.synth() call in the code? Appreciated. I'll do another review round tomorrow, familiarizing with this codebase.

corymhall · 2024-10-04T12:30:13Z

src/stack.ts

        super('cdk:index:Stack', stack.node.id, {}, stack.options);
+        this.options = stack.options;

        this.name = stack.node.id;

        const assembly = stack.app.synth();


@t0yv0 this is where synth is called.

src/assembly/manifest.ts

src/converters/app-converter.ts

flostadler · 2024-10-04T12:49:35Z

src/graph.ts

-
-    private addEdgesForFragment(obj: any, source: GraphNode): void {
+    private addEdgesForCfnResource(obj: any, source: GraphNode): void {
+        // Since we are processing the final CloudFormation template, strings will always


flostadler · 2024-10-04T12:51:16Z

src/types.ts

+    return Object.values(v).some((e) => containsEventuals(e));
+}
+
+export function lift(f: (args: any) => any, args: any): any {


We should be able to type this method, right?

t0yv0 · 2024-10-07T15:02:17Z

src/assembly/types.ts

+ * ConstructTree is a tree of the current CDK construct
+ * It represents the structure in the `tree.json` file and is based
+ * off the implementation here:
+ * https://github.com/aws/aws-cdk/blob/4bce941fc680ebd396569383f6cf07527541dcc2/packages/aws-cdk-lib/core/lib/private/tree-metadata.ts?plain=1#L177


Presumably no aws-cdk code can be reused instead, since it's private?

For some reason I thought it wasn't exported, but it is. Updated to use the cdk import.

src/assembly/manifest.ts

t0yv0 · 2024-10-07T15:12:50Z

src/converters/app-converter.ts

+    readonly parameters = new Map<string, any>();
+    readonly resources = new Map<string, Mapping<pulumi.Resource>>();
+    readonly constructs = new Map<ConstructInfo, pulumi.Resource>();
+    stackResource!: CdkConstruct;


I'm always a little wary of mutable state. This starts out as undefined, right? Then it's set in convert. Is it ever accessed before it's set? Would it be easier to read and guarantee sequencing works out if this wasn't in the state at all?

Also curious what does "!" do in TypeScript, reading up on this.

GPT-4o:

In TypeScript, the =!= after a variable name (e.g., =stackResource!=) is called the definite assignment assertion. It tells the TypeScript compiler that you are confident the property will be assigned a value before it is accessed, even if the compiler cannot determine this based on its normal flow analysis.

Sounds like a possible footgun later on, but perhaps not worth rewriting now if we're confident.

Looks like this isn't actually used so I just removed it.

t0yv0 · 2024-10-07T15:15:16Z

src/graph.ts

+                node.logicalId = logicalId;
+                this.cfnElementNodes.set(logicalId, node);
+            } else {
+                console.error('SOMETHING WENT WRONG: ', tree);


I understand this is temporary, but do we want console.error or do we want throw new Error (fail fast)?

t0yv0 · 2024-10-07T15:16:38Z

src/types.ts

+/**
+ * Options specific to the Stack component.
+ */
+export interface StackOptions extends pulumi.ComponentResourceOptions {


This is part of public API, right?

Yep, I just updated the exports to only include this since it is the only thing we want exported from types.

t0yv0 · 2024-10-07T15:18:20Z

src/converters/artifact-converter.ts

+/**
+ * ArtifactConverter
+ */
+export abstract class ArtifactConverter {


Nit: does it have go be an abstract class? resolvePlaceholders could take stackComponent (or pulumi.Resource even) as an extra parameter and this would be just a function. Does it need state in the future?

It's an abstract class because we have multiple types of things that will extend it (FileAssetConverter, DockerAssetConverter, StackConverter).

t0yv0 · 2024-10-07T15:21:14Z

tests/converters/app-converter.test.ts

+    beforeEach(() => {
+        mockfs({
+            // Recursively loads all node_modules
+            node_modules: mockfs.load(path.resolve(__dirname, '../../node_modules')),


This can get very big, no? It's needed?

I may be able to filter it down some to only the cdk libraries (which are needed).

t0yv0 · 2024-10-07T15:26:54Z

tests/converters/app-converter.test.ts

+    registerOutput(outputId: string, output: any): void {}
+}
+
+describe('App Converter', () => {


Reading this out loud, if I understand this right, this builds out some manifests that CDK would build, and feeds them into AppConverter to assert that the right Pulumi resources are created against the mocks. There is an implied CDK program that would build these manifests. WDYT about having that CDK program kept in the test suite and the manifests recomputed? Perhaps wouldn't work or is too high level?

We could do that, but I prefer having the lower level tests rather than super high level tests. We had some very high level tests already and it made it very hard to know what areas were under test and which were not. I also think it helps with understanding what is being converted, otherwise you don't have anything to reference.

t0yv0 · 2024-10-07T15:29:31Z

This has been super helpful to deep dive in the codebase, thanks @corymhall ! I would like to complete a small tasks over the codebase to get even more familiar. Left you some non-binding low-level comments.

flostadler

Looks great!
Looking forward to diving deeper into the codebase

pulumi-bot · 2024-11-27T16:05:33Z

This PR has been shipped in release v1.0.0.

corymhall added 5 commits October 1, 2024 14:19

Setting resource name to fqn

b38d713

adding more tests

c8d74c3

adding artifact converter test

4d32eba

adding more tests

7f1afa5

corymhall marked this pull request as ready for review October 3, 2024 16:52

corymhall requested review from t0yv0 and flostadler October 3, 2024 16:52

t0yv0 reviewed Oct 3, 2024

View reviewed changes

src/assembly/manifest.ts Outdated Show resolved Hide resolved

t0yv0 reviewed Oct 3, 2024

View reviewed changes

src/assembly/manifest.ts Show resolved Hide resolved

t0yv0 reviewed Oct 3, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

t0yv0 reviewed Oct 3, 2024

View reviewed changes

corymhall commented Oct 4, 2024

View reviewed changes

flostadler reviewed Oct 4, 2024

View reviewed changes

corymhall added 2 commits October 4, 2024 15:18

Updates based on feedback

f7b1b5b

updating comment

b0875a8

mjeffryes assigned corymhall Oct 4, 2024

t0yv0 reviewed Oct 7, 2024

View reviewed changes

src/assembly/manifest.ts Outdated Show resolved Hide resolved

t0yv0 reviewed Oct 7, 2024

View reviewed changes

t0yv0 approved these changes Oct 7, 2024

View reviewed changes

flostadler approved these changes Oct 7, 2024

View reviewed changes

updates based on review

4cec1d3

corymhall merged commit 5bb37e9 into main Oct 7, 2024
8 checks passed

corymhall deleted the corymhall/from-assembly branch October 7, 2024 17:59

t0yv0 mentioned this pull request Oct 17, 2024

Examples do not compile against latest master #175

Closed

pulumi-bot mentioned this pull request Nov 27, 2024

Convert the output of App.synth rather than the construct tree. #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use cloud assembly #167

Refactor to use cloud assembly #167

corymhall commented Oct 1, 2024

t0yv0 Oct 3, 2024 •

edited

Loading

corymhall Oct 4, 2024

t0yv0 Oct 3, 2024

corymhall Oct 4, 2024

t0yv0 Oct 4, 2024

t0yv0 Oct 3, 2024

corymhall Oct 4, 2024

t0yv0 Oct 3, 2024

corymhall Oct 4, 2024

t0yv0 Oct 3, 2024

corymhall Oct 4, 2024

t0yv0 Oct 3, 2024

t0yv0 Oct 3, 2024

t0yv0 commented Oct 3, 2024

corymhall Oct 4, 2024

flostadler Oct 4, 2024

flostadler Oct 4, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 Oct 7, 2024

corymhall Oct 7, 2024

t0yv0 commented Oct 7, 2024

flostadler left a comment

pulumi-bot commented Nov 27, 2024

Refactor to use cloud assembly #167

Refactor to use cloud assembly #167

Conversation

corymhall commented Oct 1, 2024

t0yv0 Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Oct 7, 2024

flostadler left a comment

Choose a reason for hiding this comment

pulumi-bot commented Nov 27, 2024

t0yv0 Oct 3, 2024 •

edited

Loading