aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.6k stars 3.9k forks source link

aws-s3-deployment: Source.asset bundling ignores props changes between stack instances #24436

Open LosD opened 1 year ago

LosD commented 1 year ago

Describe the bug

If an app with multiple stack instances compiles uses Source.asset() to compile as part of the deployment, and the result is dependent on the stacks props, only first build will be correct, as the asset is reused for the next build. AssetHashType.OUTPUT or AssetHashType.SOURCE does not seem to make a difference, neither does putting the props in the environment variables.

This is for a local build, but I expect a remove build to do the same.

Expected Behavior

The build should change if props change (or at least it should be possible to communicate which props), so that props can be used to adjust the build.

Current Behavior

The build is correct for the first stack, the second stack never rebuilds.

Reproduction Steps

app.ts:

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import {S3DeploymentTsStack} from './stack';

const app = new cdk.App();

new S3DeploymentTsStack(app, 'S3DeploymentTsStackA', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  test: 'a'
});

new S3DeploymentTsStack(app, 'S3DeploymentTsStackB', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  test: 'b'
});

new S3DeploymentTsStack(app, 'S3DeploymentTsStackC', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  test: 'c'
});

stack.ts:

import {BundlingOptions, DockerImage} from 'aws-cdk-lib';
import * as cdk from 'aws-cdk-lib';
import {Source} from 'aws-cdk-lib/aws-s3-deployment';
import {execSync} from 'child_process';
import { Construct } from 'constructs';
import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment';

export interface S3DeploymentTsStackProps extends cdk.StackProps {
  readonly test: string;
}

export class S3DeploymentTsStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props: S3DeploymentTsStackProps) {
    super(scope, id, props);

    const bucket = new cdk.aws_s3.Bucket(this, 'Bucket');

    new s3deploy.BucketDeployment(this, 'DeployWebsite', {
      sources: [Source.asset('../../assets/test-asset', {
        bundling: {
          command: ['sh', '-c', 'echo "Docker build not supported"'],
          image: DockerImage.fromRegistry('alpine'),
          local: {
            tryBundle(outputDir: string, options: BundlingOptions): boolean {
              execSync(`echo "hello mr ${props.test}" > ${outputDir}/output.txt`);
              return true;
            }
          }
        }
      })],
      destinationBucket: bucket,
    });

    new cdk.CfnOutput(this, 'BucketName', { value: bucket.bucketName });
  }
}

Please note that the reproduction is not really dependent on the asset folder, as it's just doing a simple echo. In the actual issue, I was building an application, where the props was used to set the API endpoint used, to choose the API endpoint, depending on stage

Possible Solution

A possibility could be to require all props to go through the environment, and then make the environment part of the default hash (and state that requirement clearly).

Another could be to simply document the behaviour with a clear example of how to incorporate the props into the hash. After looking at it how it works internally, it's kind of obvious that it cannot be determined which props were used, but there is nothing in the documentation on how the default hash works as far as I can see, and it is very surprising when you're using it for the first time.

Additional Information/Context

No response

CDK CLI Version

2.63.0 (build 7f4e35e)

Framework Version

No response

Node.js Version

v18.14.0

OS

Linux (Ubuntu)

Language

Typescript

Language Version

4.9.3

Other information

No response

LosD commented 1 year ago

A workaround: Using ~assetHash: FileSystem.fingerprint(path)+props.usedPropX+props.usedPropY~ assetHash: FileSystem.fingerprint(path, {extraHash: props.usedPropX+props.usedPropY}), but that really shouldn't be necessary.

pahud commented 1 year ago

Hi

Are you referring to the Source.asset() from aws-s3-deployment?

LosD commented 1 year ago

Yep

(I'm using tryBundle instead of a docker build, but I'm fairly sure that doesn't affect hashing)

LosD commented 1 year ago

Oh. Forgot that it then wasn't part of core anymore (even though I believe the hashing is still from core/Asset itself?). My bad.

pahud commented 1 year ago

Hi @LosD

I can try reproduce this in my account with Source.asset() for 3 bundles but it would be great if you can provide the CDK code you used to help me better reproduce the error. That would be awesome!

pahud commented 1 year ago

Hi @LosD

I can't reproduce your case. I used the code below:

my stack

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment';

export interface S3DeploymentTsStackProps extends cdk.StackProps {
  readonly source: s3deploy.ISource;
}

export class S3DeploymentTsStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props: S3DeploymentTsStackProps) {
    super(scope, id, props);

    const bucket = new cdk.aws_s3.Bucket(this, 'Bucket');

    new s3deploy.BucketDeployment(this, 'DeployWebsite', {
      sources: [props.source],
      destinationBucket: bucket,
    });

    new cdk.CfnOutput(this, 'BucketName', { value: bucket.bucketName });

  }
}

My app that creates 3 stacks

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment';
import { S3DeploymentTsStack } from '../lib/s3-deployment-ts-stack';
import * as path from 'path';

const app = new cdk.App();

new S3DeploymentTsStack(app, 'S3DeploymentTsStackA', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  source: s3deploy.Source.asset(path.join(__dirname, '../a')),
});

new S3DeploymentTsStack(app, 'S3DeploymentTsStackB', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  source: s3deploy.Source.asset(path.join(__dirname, '../b')),
});

new S3DeploymentTsStack(app, 'S3DeploymentTsStackC', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
  source: s3deploy.Source.asset(path.join(__dirname, '../c')),
});

When I cdk synth, I got different hash folder under cdk.out:

image

And when I deploy to S3, they are deployed correctly with different contents:

image

Please provide your code so we can better help you look at this issue.

LosD commented 1 year ago

I'll try to work something out, but it might be a few days. 

LosD commented 1 year ago

@pahud I've modified your reproduction attempt with what I meant and put it in the issue.

Only asset output I get is "hello mr a"

As I stated earlier, it's using the local build, but I'm pretty sure that makes no difference. It's the hashing that doesn't take props into account, only looking at the source. From the name, I expected AssetHashType.OUTPUT to change that, but it doesn't.

I also added a possible solution, which after I looked further at it, is mostly a case of documenting the behaviour, or (maybe favourably) take the environment into account in the default hash, and then make it clear that it is the only correct way to use any kind of variable.

pahud commented 1 year ago

@LosD OK I got your point now. I'm making it a p2 and any pull request would be highly appreciated!