fix(core): assets are duplicated between nested Cloud Assemblies #11008

rix0rrr · 2020-10-21T12:18:20Z

We stage assets into the Cloud Assembly directory. If there are multiple
nested Cloud Assemblies, the same asset will be staged multiple times.
This leads to an N-fold increase in size of the Cloud Assembly when used
in combination with CDK Pipelines (where N is the number of stages
deployed), and may even lead the Cloud Assembly to exceed CodePipeline's
maximum artifact size of 250MB.

Add the concept of an assetOutdir next to a regular Cloud Assembly
outDir, so that multiple Cloud Assemblies can share an asset
directory. As an initial implementation, the assetOutdir of nested
Cloud Assemblies is just the regular outdir of the root Assembly.

We are playing a bit fast and loose with the semantics of file paths
across our code base; many properties just say "the path of X" without
making clear whether it's absolute or relative, and if it's relative
what it's relative to (cwd()? Or the Cloud Assembly directory?).

Turns out that especially in dealing with assets, the answer is
"can be anything" and things just happen to work out based on who is
providing the path and who is consuming it. In order to limit the
scope of the changes I needed to make I kept modifications to the
AssetStaging class:

stagedPath now consistently returns an absolute path.
relativeStagedPath() a path relative to the Cloud Assembly or an
absolute path, as appropriate.

Related changes in this PR:

Refactor the copying vs. bundling logic in AssetStaging. I found
the current maze of ifs and member variable changes too hard to
follow to convince myself the new code would be doing the right thing,
so I refactored it to reduce the branching factor.
Switch the tests of aws-ecr-assets over to Jest using
nodeunitShim.

Fixes #10877, fixes #9627, fixes #9917.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

We stage assets into the Cloud Assembly directory. If there are multiple nested Cloud Assemblies, the same asset will be staged multiple times. This leads to an N-fold increase in size of the Cloud Assembly when used in combination with CDK Pipelines (where N is the number of stages deployed), and may even lead the Cloud Assembly to exceed CodePipeline's maximum artifact size of 250MB. Add the concept of an `assetOutdir` next to a regular Cloud Assembly `outDir`, so that multiple Cloud Assemblies can share an asset directory. As an initial implementation, the `assetOutdir` of nested Cloud Assemblies is just the regular `outdir` of the root Assembly. We are playing a bit fast and loose with the semantics of file paths across our code base; many properties just say "the path of X" without making clear whether it's absolute or relative, and if it's relative what it's relative to (`cwd()`? Or the Cloud Assembly directory?). Turns out that especially in dealing with assets, the answer is "can be anything" and things just happen to work out based on who is providing the path and who is consuming it. In order to limit the scope of the changes I needed to make I kept modifications to the `AssetStaging` class: * `stagedPath` now consistently returns an absolute path. * `relativeStagedPath()` a path relative to the Cloud Assembly or an absolute path, as appropriate. Related changes in this PR: - Refactor the *copying* vs. *bundling* logic in `AssetStaging`. I found the current maze of `if`s and member variable changes too hard to follow to convince myself the new code would be doing the right thing, so I refactored it to reduce the branching factor. - Switch the tests of `aws-ecr-assets` over to Jest using `nodeunitShim`. Fixes #10877, fixes #9627, fixes #9917.

gitpod-io · 2020-10-21T12:18:23Z

packages/@aws-cdk/core/lib/asset-staging.ts

njlynch · 2020-10-21T13:13:02Z

packages/@aws-cdk/core/lib/asset-staging.ts

+    const old = this.cache.get(cacheKey);
+    if (old) { return old; }
+
+    const noo = calcFn();


Missing several oooooooooos. http://nooooooooooooooo.com/

Maybe to remove the pronunciation ambiguity:

Suggested change

const noo = calcFn();

const calcValue = calcFn();

And old can become cachedValue?

You are right, I was trying to be cute

njlynch · 2020-10-21T13:19:14Z

packages/@aws-cdk/core/lib/asset-staging.ts

+   * returns an absolute path if it was not.
+   */
+  public relativeStagedPath(stack: Stack) {
+    const thisAsmDir = Stage.of(stack)?.outdir;


Why outdir here and not assetOutdir (or just this.outdir)?

I'll try to make that more clear through naming.

packages/@aws-cdk/core/lib/asset-staging.ts

jogold · 2020-10-21T14:12:09Z

packages/@aws-cdk/core/lib/asset-staging.ts

+      // "Staging disabled" only applies to copying. If we've already done the work for bundling,
+      // it's hardly any work to rename a directory afterwards.
+      skip = this.node.tryGetContext(cxapi.DISABLE_ASSET_STAGING_CONTEXT);
+      stageThisAsset = () => this.stageByCopying(skip);


When staging is disabled and bundling is used, the staged path should be the (absolute) path to the bundling directory, is it still the case now?

See #10367, users using this expect the aws:asset:path property of Metadata to be an absolute path to the bundling directory.

Ah, good catch.

Staging disabled combined with bundling is not really a thing anymore.

It was not clear to me what the point of disabling staging was, because that wasn't explained in the docs or comments, so I assumed it was for performance reasons and concluded that if you've gone to the effort of bundling, one more or fewer mvs doesn't really make a difference.

If the point of "disabling staging" is to get an absolute path into aws:asset:path... well that seems a little silly. If aws:asset:path must always be an absolute path, why don't we then always stick an absolute path into it regardless? Why would it be relative if staging is enabled (thereby making it useless, apparently) and then have a flag to make it absolute?

It was not clear to me what the point of disabling staging was

Also not 100% clear to me, not using this feature. It was introduced to allow SAM to interact with CDK: https://docs.aws.amazon.com/cdk/latest/guide/sam.html

If the point of "disabling staging" is to get an absolute path into aws:asset:path... well that seems a little silly.

Agree, maybe the right solution as you say is to always output an absolute path. @eladb?

But maybe we don't want to have absolute paths in a deployed CF template.

That's probably the reason. We always insert aws:asset:path, even though we're only interested in it if people also supply --no-staging (which is intended to be interpreted as --integrate-with-sam-cli).

Oh well. I'll clarify some more and fix the behavior. Thanks for the catch Jonathan!

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

eladb · 2020-10-22T12:06:52Z

packages/@aws-cdk/core/lib/asset-staging.ts

+ * Must be declared at the top of the file because we're going to use it statically in the
+ * AssetStaging class.
+ */
+class Cache<A> {


Can you move to a separate private file?

Well I can, but I didn't because I was afraid people were going to insist I'd write unit tests for it :)

But sigh I guess I will then

…huijbers/shared-assets

mergify · 2020-10-26T11:45:06Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-10-26T12:13:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ae4cc94
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-10-26T12:14:03Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

If the asset uses OUTPUT or BUNDLE it's generally because its source is a very large directory. This is the case for the `NodejsFunction` which mounts the project root (along with the `node_modules` folder!). Use a custom hash in this case when skipping bundling. Otherwise running `cdk ls` can result in heavy fingerprinting operations (again this is the case for the `NodejsFunction`). Regression introduced in aws#11008 (https://github.com/aws/aws-cdk/pull/11008/files#diff-62eef996be8abeb157518522c3cbf84a33dd4751c103304df87b04eb6d7bbab6L160)

…ling is skipped (#11440) If the asset uses OUTPUT or BUNDLE it's generally because its source is a very large directory. This is the case for the `NodejsFunction` which mounts the project root (along with the `node_modules` folder!). Use a custom hash in this case when skipping bundling. Otherwise running `cdk ls` can result in heavy fingerprinting operations (again this is the case for the `NodejsFunction`) and can be much slower than running `cdk synth` or `cdk diff`, making it pointless to skip bundling. Regression introduced in #11008 (https://github.com/aws/aws-cdk/pull/11008/files#diff-62eef996be8abeb157518522c3cbf84a33dd4751c103304df87b04eb6d7bbab6L160) Before #11008: https://github.com/aws/aws-cdk/blob/c1453147e7c484c54f1380d2fa9ba0cdf2859002/packages/%40aws-cdk/core/lib/asset-staging.ts#L159-L160 Closes #11459 Closes #11460 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr requested a review from a team October 21, 2020 12:18

rix0rrr self-assigned this Oct 21, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 21, 2020

rix0rrr added 3 commits October 21, 2020 14:19

Update jest.config.js

f66bcac

Update foreach.sh

e14e862

Remove trailing import

4174a9f

njlynch reviewed Oct 21, 2020

View reviewed changes

jogold mentioned this pull request Oct 21, 2020

fix(core): asset used in multiple Stages is only bundled in the first Stage #10957

Closed

jogold reviewed Oct 21, 2020

View reviewed changes

rix0rrr and others added 4 commits October 22, 2020 13:49

Update packages/@aws-cdk/core/lib/asset-staging.ts

3af45ae

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Update packages/@aws-cdk/core/lib/asset-staging.ts

5ed58fc

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

Clarify relative path function

beed6dd

Fix test

499fc65

eladb reviewed Oct 22, 2020

View reviewed changes

rix0rrr added 4 commits October 22, 2020 14:17

Move cache

405f0f8

Merge branch 'master' into huijbers/shared-assets

7fa1d0b

Make sure bundled no-staged assets return absolute paths

0311ac9

Merge branch 'huijbers/shared-assets' of github.com:aws/aws-cdk into …

23eca04

…huijbers/shared-assets

rix0rrr requested review from eladb, njlynch and a team October 23, 2020 08:06

njlynch approved these changes Oct 26, 2020

View reviewed changes

Merge branch 'master' into huijbers/shared-assets

ae4cc94

mergify bot merged commit c84217f into master Oct 26, 2020

mergify bot deleted the huijbers/shared-assets branch October 26, 2020 12:14

jogold mentioned this pull request Nov 12, 2020

fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

Merged

maxdumas mentioned this pull request Nov 24, 2020

[pipelines] Add stage to strip assets out of cloud assembly before deploying to CloudFormation #9917

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): assets are duplicated between nested Cloud Assemblies #11008

fix(core): assets are duplicated between nested Cloud Assemblies #11008

rix0rrr commented Oct 21, 2020

gitpod-io bot commented Oct 21, 2020 •

edited

Loading

njlynch Oct 21, 2020

rix0rrr Oct 22, 2020

njlynch Oct 21, 2020

rix0rrr Oct 22, 2020

jogold Oct 21, 2020

jogold Oct 21, 2020 •

edited

Loading

rix0rrr Oct 22, 2020

jogold Oct 22, 2020 •

edited

Loading

jogold Oct 22, 2020 •

edited

Loading

rix0rrr Oct 22, 2020

eladb Oct 22, 2020

rix0rrr Oct 22, 2020

mergify bot commented Oct 26, 2020

aws-cdk-automation commented Oct 26, 2020

mergify bot commented Oct 26, 2020

fix(core): assets are duplicated between nested Cloud Assemblies #11008

fix(core): assets are duplicated between nested Cloud Assemblies #11008

Conversation

rix0rrr commented Oct 21, 2020

gitpod-io bot commented Oct 21, 2020 • 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

jogold Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

jogold Oct 22, 2020 • 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

mergify bot commented Oct 26, 2020

aws-cdk-automation commented Oct 26, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 26, 2020

gitpod-io bot commented Oct 21, 2020 •

edited

Loading

jogold Oct 21, 2020 •

edited

Loading

jogold Oct 22, 2020 •

edited

Loading

jogold Oct 22, 2020 •

edited

Loading