Skip to content

Commit

Permalink
fix(s3-assets): cannot publish a file without extension (#30597)
Browse files Browse the repository at this point in the history
### Issue # (if applicable)

Closes #30471 

###

### Reason for this change
Publishing a file with no extension using the `Asset` class with `BundlingOutput.SINGLE_FILE` and `AssetHashType.SOURCE`(default), as shown below, will result in an error `fail: EISDIR: illegal operation on a directory, read`, and publishing will fail.
```ts
export class AssetTestStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
      super(scope, id, props);

      const asset = new s3assets.Asset(this, 'asset', {
          path: path.join(__dirname, '../assets/main'),
          bundling: {
              image: DockerImage.fromRegistry('golang:1.21'),
              entrypoint: ["bash", "-c"],
              command: ["echo 123 > /asset-output/main"], // a file without extension
              outputType: BundlingOutput.SINGLE_FILE,
          },
      });
      new CfnOutput(this, 'AssetHash', { value: asset.assetHash });
  }
}
```

This is because the path in `*.asset.json` is different from the actual file path.
The `*.asset.json` expects the file to be in `asset.bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0`, but when I check the `cdk.out` directory, I see that `asset.bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0` is a directory, not a file.
```json
{
  "version": "36.0.0",
  "files": {
    "bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0": {
      "source": {
        "path": "asset.bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0",
        "packaging": "file"
      },
      "destinations": {
        "current_account-us-east-1": {
          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-us-east-1",
          "objectKey": "bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0.bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0",
          "region": "us-east-1",
          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-us-east-1"
        }
      }
    }
}
```
```bash
cdk.out
├── SampleStack.assets.json
├── SampleStack.template.json
├── asset.bead5b2c0d128650228f146d2326d5f3cbfb36738a9383fc6a09b1e9278803f0
│   └── main
├── cdk.out
├── manifest.json
└── tree.json
```

If I change it to a file with an extension, as shown below, I see that the file with the extension is staged under `cdk.out` dir, and the asset is published successfully.
```ts
export class AssetTestStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
      super(scope, id, props);

      const asset = new s3assets.Asset(this, 'asset', {
          path: path.join(__dirname, '../assets/main'),
          bundling: {
              image: DockerImage.fromRegistry('golang:1.21'),
              entrypoint: ["bash", "-c"],
              command: ["echo 123 > /asset-output/main.bin"], // a file with an extension
              outputType: BundlingOutput.SINGLE_FILE,
          },
      });
      new CfnOutput(this, 'AssetHash', { value: asset.assetHash });
  }
}
```
```bash
cdk.out
├── SampleStack.assets.json
├── SampleStack.template.json
├── asset.dc5ce447844d7490834e46df016edc7f671b4fae19ab55b6c78973dcb5af98f8
│   └── main.bin
├── asset.dc5ce447844d7490834e46df016edc7f671b4fae19ab55b6c78973dcb5af98f8.bin # !! staged file here !!
├── cdk.out
├── manifest.json
└── tree.json
```
```json
{
  "version": "36.0.0",
  "files": {
    "dc5ce447844d7490834e46df016edc7f671b4fae19ab55b6c78973dcb5af98f8": {
      "source": {
        "path": "asset.dc5ce447844d7490834e46df016edc7f671b4fae19ab55b6c78973dcb5af98f8.bin",
        "packaging": "file"
      },
      "destinations": {
        "current_account-us-east-1": {
          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-us-east-1",
          "objectKey": "dc5ce447844d7490834e46df016edc7f671b4fae19ab55b6c78973dcb5af98f8.bin",
          "region": "us-east-1",
          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-us-east-1"
        }
      }
    }
}
```

Files without extensions must be staged correctly in order to be published correctly.

### Description of changes
The directory to write the bundling output is usually `cdk.out/asset.{asset hash}`.
If the extension exists, it will be renamed from `cdk.out/asset.{asset hash}/{asset file name}` to `cdk.out/{asset hash}.{asset file extension}`.
https://github.com/aws/aws-cdk/blob/c826d8faaeb310623eb9a1a1c82930b679768007/packages/aws-cdk-lib/core/lib/asset-staging.ts#L392

If the extension does not exist, the file name `cdk.out/asset.{asset hash}` (without extension) will be the same as the directory where bundling output is written.
Therefore, the file is already considered staged and will not be staged correctly.
https://github.com/aws/aws-cdk/blob/c826d8faaeb310623eb9a1a1c82930b679768007/packages/aws-cdk-lib/core/lib/asset-staging.ts#L383

Therefore, in such cases, I fix to change the file name by adding a suffix such as `noext` after the file name so that the file is correctly renamed.

### Description of how you validated changes
unit tests and integ test.


### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
sakurai-ryo authored Oct 29, 2024
1 parent a279b98 commit ccab485
Show file tree
Hide file tree
Showing 13 changed files with 224 additions and 14 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,17 @@ class TestStack extends Stack {

const user = new iam.User(this, 'MyUser');
asset.grantRead(user);

new assets.Asset(this, 'BundledAssetWithoutExtension', {
path: path.join(__dirname, 'markdown-asset'),
bundling: {
image: DockerImage.fromBuild(path.join(__dirname, 'alpine-markdown')),
outputType: BundlingOutput.SINGLE_FILE,
command: [
'sh', '-c', 'echo 123 > /asset-output/main',
],
},
});
}
}

Expand Down
21 changes: 18 additions & 3 deletions packages/aws-cdk-lib/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,10 @@ export class AssetStaging extends Construct {
*/
private stageByCopying(): StagedAsset {
const assetHash = this.calculateHash(this.hashType);
const stagedPath = this.stagingDisabled
const targetPath = this.stagingDisabled
? this.sourcePath
: path.resolve(this.assetOutdir, renderAssetFilename(assetHash, getExtension(this.sourcePath)));
const stagedPath = this.renderStagedPath(this.sourcePath, targetPath);

if (!this.sourceStats.isDirectory() && !this.sourceStats.isFile()) {
throw new Error(`Asset ${this.sourcePath} is expected to be either a directory or a regular file`);
Expand Down Expand Up @@ -338,7 +339,10 @@ export class AssetStaging extends Construct {
// Calculate assetHash afterwards if we still must
assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundledAsset.path);

const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension));
const stagedPath = this.renderStagedPath(
bundledAsset.path,
path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension)),
);

this.stageAsset(bundledAsset.path, stagedPath, 'move');

Expand Down Expand Up @@ -388,7 +392,7 @@ export class AssetStaging extends Construct {
}

// Moving can be done quickly
if (style == 'move') {
if (style === 'move') {
fs.renameSync(sourcePath, targetPath);
return;
}
Expand Down Expand Up @@ -511,6 +515,17 @@ export class AssetStaging extends Construct {
throw new Error('Unknown asset hash type.');
}
}

private renderStagedPath(sourcePath: string, targetPath: string): string {
// Add a suffix to the asset file name
// because when a file without extension is specified, the source directory name is the same as the staged asset file name.
// But when the hashType is `AssetHashType.OUTPUT`, the source directory name begins with `bundling-temp-` and the staged asset file name is different.
// We only need to add a suffix when the hashType is not `AssetHashType.OUTPUT`.
if (this.hashType !== AssetHashType.OUTPUT && path.dirname(sourcePath) === targetPath) {
targetPath = targetPath + '_noext';
}
return targetPath;
}
}

function renderAssetFilename(assetHash: string, extension = '') {
Expand Down
11 changes: 11 additions & 0 deletions packages/aws-cdk-lib/core/test/docker-stub-cp.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ set -euo pipefail
echo "$@" >> /tmp/docker-stub-cp.input.concat
echo "$@" > /tmp/docker-stub-cp.input

# create a file without extension to emulate created files, fetch the target path from the "docker cp" command
if cat /tmp/docker-stub-cp.input.concat | grep "DOCKER_STUB_SINGLE_FILE_WITHOUT_EXT"; then
if echo "$@" | grep "cp"| grep "/asset-output"; then
outdir=$(echo "$@" | grep cp | grep "/asset-output" | xargs -n1 | grep "cdk.out" | head -n1 | cut -d":" -f1)
if [ -n "$outdir" ]; then
touch "${outdir}/test" # create a file witout extension
exit 0
fi
fi
fi

# create a fake zip to emulate created files, fetch the target path from the "docker cp" command
if echo "$@" | grep "cp"| grep "/asset-output"; then
outdir=$(echo "$@" | grep cp | grep "/asset-output" | xargs -n1 | grep "cdk.out" | head -n1 | cut -d":" -f1)
Expand Down
6 changes: 6 additions & 0 deletions packages/aws-cdk-lib/core/test/docker-stub.sh
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ if echo "$@" | grep "DOCKER_STUB_SINGLE_ARCHIVE"; then
exit 0
fi

if echo "$@" | grep "DOCKER_STUB_SINGLE_FILE_WITHOUT_EXT"; then
outdir=$(echo "$@" | xargs -n1 | grep "/asset-output" | head -n1 | cut -d":" -f1)
touch ${outdir}/test # create a file witout extension
exit 0
fi

if echo "$@" | grep "DOCKER_STUB_SINGLE_FILE"; then
outdir=$(echo "$@" | xargs -n1 | grep "/asset-output" | head -n1 | cut -d":" -f1)
touch ${outdir}/test.txt
Expand Down
128 changes: 128 additions & 0 deletions packages/aws-cdk-lib/core/test/staging.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ enum DockerStubCommand {
MULTIPLE_FILES = 'DOCKER_STUB_MULTIPLE_FILES',
SINGLE_ARCHIVE = 'DOCKER_STUB_SINGLE_ARCHIVE',
SINGLE_FILE = 'DOCKER_STUB_SINGLE_FILE',
SINGLE_FILE_WITHOUT_EXT = 'DOCKER_STUB_SINGLE_FILE_WITHOUT_EXT',
VOLUME_SINGLE_ARCHIVE = 'DOCKER_STUB_VOLUME_SINGLE_ARCHIVE',
}

Expand Down Expand Up @@ -1450,6 +1451,68 @@ describe('staging', () => {
expect(staging.isArchive).toEqual(false);
});

test('bundling that produces a single file with SINGLE_FILE_WITHOUT_EXT and hash type SOURCE', () => {
// GIVEN
const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new Stack(app, 'stack');
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const staging = new AssetStaging(stack, 'Asset', {
sourcePath: directory,
bundling: {
image: DockerImage.fromRegistry('alpine'),
command: [DockerStubCommand.SINGLE_FILE_WITHOUT_EXT],
outputType: BundlingOutput.SINGLE_FILE,
},
assetHashType: AssetHashType.SOURCE, // default
});

// THEN
const assembly = app.synth();
expect(fs.readdirSync(assembly.directory)).toEqual([
'asset.ef734136dc22840a94140575a2f98cbc061074e09535589d1cd2c11a4ac2fd75',
'asset.ef734136dc22840a94140575a2f98cbc061074e09535589d1cd2c11a4ac2fd75_noext',
'cdk.out',
'manifest.json',
'stack.template.json',
'tree.json',
]);
expect(staging.packaging).toEqual(FileAssetPackaging.FILE);
expect(staging.isArchive).toEqual(false);
});

test('bundling that produces a single file with SINGLE_FILE_WITHOUT_EXT and hash type CUSTOM', () => {
// GIVEN
const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new Stack(app, 'stack');
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const staging = new AssetStaging(stack, 'Asset', {
sourcePath: directory,
bundling: {
image: DockerImage.fromRegistry('alpine'),
command: [DockerStubCommand.SINGLE_FILE_WITHOUT_EXT],
outputType: BundlingOutput.SINGLE_FILE,
},
assetHashType: AssetHashType.CUSTOM,
assetHash: 'custom',
});

// THEN
const assembly = app.synth();
expect(fs.readdirSync(assembly.directory)).toEqual([
'asset.f81c5ba9e81eebb202881a8e61a83ab4b69f6bee261989eb93625c9cf5d35335',
'asset.f81c5ba9e81eebb202881a8e61a83ab4b69f6bee261989eb93625c9cf5d35335_noext',
'cdk.out',
'manifest.json',
'stack.template.json',
'tree.json',
]);
expect(staging.packaging).toEqual(FileAssetPackaging.FILE);
expect(staging.isArchive).toEqual(false);
});
});

describe('staging with docker cp', () => {
Expand Down Expand Up @@ -1517,6 +1580,71 @@ describe('staging with docker cp', () => {
expect.stringContaining('volume rm assetOutput'),
]));
});

test('bundling that produces a single file with docker image copy variant and hash type SOURCE', () => {
// GIVEN
const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new Stack(app, 'stack');
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const staging = new AssetStaging(stack, 'Asset', {
sourcePath: directory,
bundling: {
image: DockerImage.fromRegistry('alpine'),
command: [DockerStubCommand.SINGLE_FILE_WITHOUT_EXT],
outputType: BundlingOutput.SINGLE_FILE,
bundlingFileAccess: BundlingFileAccess.VOLUME_COPY,
},
assetHashType: AssetHashType.SOURCE, // default
});

// THEN
const assembly = app.synth();
expect(fs.readdirSync(assembly.directory)).toEqual([
'asset.93bd4079bff7440a725991ecf249416ae9ad73cb639f4a8d9e8f3ad8d491e89f',
'asset.93bd4079bff7440a725991ecf249416ae9ad73cb639f4a8d9e8f3ad8d491e89f_noext',
'cdk.out',
'manifest.json',
'stack.template.json',
'tree.json',
]);
expect(staging.packaging).toEqual(FileAssetPackaging.FILE);
expect(staging.isArchive).toEqual(false);
});

test('bundling that produces a single file with docker image copy variant and hash type CUSTOM', () => {
// GIVEN
const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });
const stack = new Stack(app, 'stack');
const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');

// WHEN
const staging = new AssetStaging(stack, 'Asset', {
sourcePath: directory,
bundling: {
image: DockerImage.fromRegistry('alpine'),
command: [DockerStubCommand.SINGLE_FILE_WITHOUT_EXT],
outputType: BundlingOutput.SINGLE_FILE,
bundlingFileAccess: BundlingFileAccess.VOLUME_COPY,
},
assetHashType: AssetHashType.CUSTOM,
assetHash: 'custom',
});

// THEN
const assembly = app.synth();
expect(fs.readdirSync(assembly.directory)).toEqual([
'asset.53a51b4c68874a8e831e24e8982120be2a608f50b2e05edb8501143b3305baa8',
'asset.53a51b4c68874a8e831e24e8982120be2a608f50b2e05edb8501143b3305baa8_noext',
'cdk.out',
'manifest.json',
'stack.template.json',
'tree.json',
]);
expect(staging.packaging).toEqual(FileAssetPackaging.FILE);
expect(staging.isArchive).toEqual(false);
});
});

// Reads a docker stub and cleans the volume paths out of the stub.
Expand Down

0 comments on commit ccab485

Please sign in to comment.