Skip to content

feat(core): allow mounting existing or volume copy volumes #32832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

feat(core): allow mounting existing or volume copy volumes #32832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4298466
feat(DockerImage): allow mounting existing or volume copy volumes
cgatt Jan 10, 2025
9387c9d
refactor(asset-staging): explicitly set bind mount volume type
cgatt Jan 10, 2025
eeeaaef
test(staging): use regex for user ID to pass in CI
cgatt Jan 10, 2025
c36ec02
update alpha package tests
cgatt Jan 13, 2025
93c9728
move user determination behaviour back to assetStaging
cgatt Jan 13, 2025
06dda4d
Merge branch 'aws:main' into feat/bundling-volume-types
cgatt Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-go-alpha/lib/bundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class Bundling implements cdk.BundlingOptions {
public readonly environment?: { [key: string]: string };
public readonly local?: cdk.ILocalBundling;
public readonly entrypoint?: string[];
public readonly volumes?: cdk.DockerVolume[];
public readonly volumes?: (cdk.DockerVolume | cdk.VolumeCopyDockerVolume | cdk.ExistingDockerVolume)[];
public readonly volumesFrom?: string[];
public readonly workingDirectory?: string;
public readonly user?: string;
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-lambda-python-alpha/lib/bundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as path from 'path';
import { Architecture, AssetCode, Code, Runtime } from 'aws-cdk-lib/aws-lambda';
import { AssetStaging, BundlingFileAccess, BundlingOptions as CdkBundlingOptions, DockerImage, DockerVolume } from 'aws-cdk-lib/core';
import { AssetStaging, BundlingFileAccess, BundlingOptions as CdkBundlingOptions, DockerImage, DockerVolume, type ExistingDockerVolume, type VolumeCopyDockerVolume } from 'aws-cdk-lib/core';
import { Packaging, DependenciesFile } from './packaging';
import { BundlingOptions, ICommandHooks } from './types';

Expand Down Expand Up @@ -65,7 +65,7 @@ export class Bundling implements CdkBundlingOptions {
public readonly image: DockerImage;
public readonly entrypoint?: string[];
public readonly command: string[];
public readonly volumes?: DockerVolume[];
public readonly volumes?: (DockerVolume | VolumeCopyDockerVolume | ExistingDockerVolume)[];
public readonly volumesFrom?: string[];
public readonly environment?: { [key: string]: string };
public readonly workingDirectory?: string;
Expand Down
6 changes: 4 additions & 2 deletions packages/aws-cdk-lib/aws-lambda-nodejs/lib/bundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import { IConstruct } from 'constructs';
import { PackageInstallation } from './package-installation';
import { LockFile, PackageManager } from './package-manager';
import { BundlingOptions, OutputFormat, SourceMapMode } from './types';
import { exec, extractDependencies, findUp, getTsconfigCompilerOptions, isSdkV2Runtime } from './util';
import {
exec, extractDependencies, findUp, getTsconfigCompilerOptions, isSdkV2Runtime,
} from './util';
import { Architecture, AssetCode, Code, Runtime } from '../../aws-lambda';
import * as cdk from '../../core';
import { LAMBDA_NODEJS_SDK_V3_EXCLUDE_SMITHY_PACKAGES } from '../../cx-api';
Expand Down Expand Up @@ -83,7 +85,7 @@ export class Bundling implements cdk.BundlingOptions {
public readonly image: cdk.DockerImage;
public readonly entrypoint?: string[];
public readonly command: string[];
public readonly volumes?: cdk.DockerVolume[];
public readonly volumes?: (cdk.DockerVolume | cdk.VolumeCopyDockerVolume | cdk.ExistingDockerVolume)[];
public readonly volumesFrom?: string[];
public readonly environment?: { [key: string]: string };
public readonly workingDirectory: string;
Expand Down
45 changes: 41 additions & 4 deletions packages/aws-cdk-lib/core/lib/asset-staging.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import * as crypto from 'crypto';
import * as os from 'os';
import * as path from 'path';
import { Construct } from 'constructs';
import * as fs from 'fs-extra';
import { AssetHashType, AssetOptions, FileAssetPackaging } from './assets';
import { BundlingFileAccess, BundlingOptions, BundlingOutput } from './bundling';
import { BundlingFileAccess, BundlingOptions, BundlingOutput, DockerVolumeType } from './bundling';
import { FileSystem, FingerprintOptions } from './fs';
import { clearLargeFileFingerprintCache } from './fs/fingerprint';
import { Names } from './names';
import { AssetBundlingVolumeCopy, AssetBundlingBindMount } from './private/asset-staging';
import { Cache } from './private/cache';
import { Stack } from './stack';
import { Stage } from './stage';
Expand Down Expand Up @@ -452,17 +452,43 @@ export class AssetStaging extends Construct {
sourcePath: this.sourcePath,
bundleDir,
...options,
securityOpt: options.securityOpt ?? '',
user: options.user ?? this.determineUser(),
volumes: [...(options.volumes ?? [])],
workingDirectory:
options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR,
};

// Add the asset input and output volumes based on BundlingFileAccess setting
switch (options.bundlingFileAccess) {
case BundlingFileAccess.VOLUME_COPY:
new AssetBundlingVolumeCopy(assetStagingOptions).run();
Copy link
Contributor

@iliapolo iliapolo May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I prefer these individual helper classes over the proposed DockerVolumeHelper - even at the expense of some duplication. Since volume commands differ so much between the two options, a common helper class makes it harder to reason through.

assetStagingOptions.volumes.push({
dockerVolumeType: DockerVolumeType.VOLUME_COPY,
hostInputPath: assetStagingOptions.sourcePath,
containerPath: AssetStaging.BUNDLING_INPUT_DIR,
},
{
dockerVolumeType: DockerVolumeType.VOLUME_COPY,
hostOutputPath: assetStagingOptions.bundleDir,
containerPath: AssetStaging.BUNDLING_OUTPUT_DIR,
});
break;
case BundlingFileAccess.BIND_MOUNT:
default:
new AssetBundlingBindMount(assetStagingOptions).run();
assetStagingOptions.volumes.push({
dockerVolumeType: DockerVolumeType.BIND_MOUNT,
hostPath: assetStagingOptions.sourcePath,
containerPath: AssetStaging.BUNDLING_INPUT_DIR,
},
{
dockerVolumeType: DockerVolumeType.BIND_MOUNT,
hostPath: assetStagingOptions.bundleDir,
containerPath: AssetStaging.BUNDLING_OUTPUT_DIR,
});
break;
}

assetStagingOptions.image.run(assetStagingOptions);
}
} catch (err) {
// When bundling fails, keep the bundle output for diagnosability, but
Expand Down Expand Up @@ -526,6 +552,17 @@ export class AssetStaging extends Construct {
}
return targetPath;
}

/**
* Determines a useful default user if not given otherwise
*/
private determineUser(): string {
// Default to current user
const userInfo = os.userInfo();
return userInfo.uid !== -1 // uid is -1 on Windows
? `${userInfo.uid}:${userInfo.gid}`
: '1000:1000';
}
}

function renderAssetFilename(assetHash: string, extension = '') {
Expand Down
189 changes: 125 additions & 64 deletions packages/aws-cdk-lib/core/lib/bundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { spawnSync } from 'child_process';
import * as crypto from 'crypto';
import { isAbsolute, join } from 'path';
import { DockerCacheOption } from './assets';
import { FileSystem } from './fs';
import { dockerExec } from './private/asset-staging';
import { dockerExec, DockerVolumeHelper } from './private/bundling';
import { quiet, reset } from './private/jsii-deprecated';

/**
Expand Down Expand Up @@ -61,7 +60,7 @@ export interface BundlingOptions {
*
* @default - no additional volumes are mounted
*/
readonly volumes?: DockerVolume[];
readonly volumes?: (DockerVolume | VolumeCopyDockerVolume | ExistingDockerVolume)[];
Copy link
Contributor

@iliapolo iliapolo May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you already surmised, this is a problem because it provides a poor user experience for static languages, where this translates into an generic Object type.

Before we consider alternatives, I have some questions.

VolumeCopyDockerVolume

Why do we need this new type? Both DockerVolume and VolumeCopyDockerVolume offer the same functionality: expose path X on the host machine as path Y inside the container.

So from a properties standpoint, they both should have the same properties. Looking at the code you described in the linked issue:

const codeAsset = new AssetStaging(this, 'CodeAsset', {
  sourcePath: path.resolve('mule'),
  exclude: ['target/*', 'bin/*', '*.class', '*.jar'],
  bundling: {
    image: DockerImage.fromRegistry('public.ecr.aws/docker/library/alpine'),
    bundlingFileAccess: BundlingFileAccess.VOLUME_COPY,
    volumes: [{
      hostPath: '~/.m2',
      containerPath: '/root/.m2',
    }],
  },
});

It seems like we can just apply the bundlingFileAccess for the additional volume configuration as well. I'm not sure I see a use-case for having each individual volume define its own "exposure" mechanism.

ExistingDockerVolume

This one seems to be addressing a different issue/use-case - i'm not opposed to it, but I would prefer for it to be a separate PR.

As for the implementation, lets drop the union type and instead change DockerVolume to support existing volumes, as proposed by @SamStephens in the issue:

  • Change hostPath to be optional.
  • Add a volumeName optional property to indicate we want an existing volume.
  • Validate mutual exclusivity of hostPath and volumeName.

Its not the best experience because it creates a runtime validation instead of compile time, but is still preferable over union types.


/**
* Where to mount the specified volumes from
Expand Down Expand Up @@ -255,7 +254,6 @@ export class BundlingDockerImage {
* Runs a Docker image
*/
public run(options: DockerRunOptions = {}) {
const volumes = options.volumes || [];
const environment = options.environment || {};
const entrypoint = options.entrypoint?.[0] || null;
const command = [
Expand All @@ -267,36 +265,39 @@ export class BundlingDockerImage {
: [],
];

const dockerArgs: string[] = [
'run', '--rm',
...options.securityOpt
? ['--security-opt', options.securityOpt]
: [],
...options.network
? ['--network', options.network]
: [],
...options.platform
? ['--platform', options.platform]
: [],
...options.user
? ['-u', options.user]
: [],
...options.volumesFrom
? flatten(options.volumesFrom.map(v => ['--volumes-from', v]))
: [],
...flatten(volumes.map(v => ['-v', `${v.hostPath}:${v.containerPath}:${isSeLinux() ? 'z,' : ''}${v.consistency ?? DockerVolumeConsistency.DELEGATED}`])),
...flatten(Object.entries(environment).map(([k, v]) => ['--env', `${k}=${v}`])),
...options.workingDirectory
? ['-w', options.workingDirectory]
: [],
...entrypoint
? ['--entrypoint', entrypoint]
: [],
this.image,
...command,
];
const volumeHelper = new DockerVolumeHelper(options);

dockerExec(dockerArgs);
try {
const dockerArgs: string[] = [
'run', '--rm',
...options.securityOpt
? ['--security-opt', options.securityOpt]
: [],
...options.network
? ['--network', options.network]
: [],
...options.platform
? ['--platform', options.platform]
: [],
...options.user
? ['-u', options.user]
: [],
...volumeHelper.volumeCommands,
...flatten(Object.entries(environment).map(([k, v]) => ['--env', `${k}=${v}`])),
...options.workingDirectory
? ['-w', options.workingDirectory]
: [],
...entrypoint
? ['--entrypoint', entrypoint]
: [],
this.image,
...command,
];

dockerExec(dockerArgs);
} finally {
volumeHelper.cleanup();
}
}

/**
Expand Down Expand Up @@ -461,19 +462,62 @@ export class DockerImage extends BundlingDockerImage {
}

/**
* A Docker volume
* The access mechanism used to make this volume available to the bundling container
*/
export interface DockerVolume {
export enum DockerVolumeType {
/**
* The path to the file or directory on the host machine
* Creates temporary volumes and containers to copy files from the host to the bundling container and back.
* This is slower, but works also in more complex situations with remote or shared docker sockets.
*/
readonly hostPath: string;
VOLUME_COPY = 'VOLUME_COPY',

/**
* The source and output folders will be mounted as bind mount from the host system
* This is faster and simpler, but less portable than `VOLUME_COPY`.
*/
BIND_MOUNT = 'BIND_MOUNT',

/**
* The volume already exists and will not be created or destroyed by this class
*/
EXISTING = 'EXISTING',
}

/**
* Common properties for all Docker volume types.
*/
export interface DockerVolumeBase {
/**
* The path where the file or directory is mounted in the container
* The path inside the container where the volume is mounted.
* This property is required for all volume types.
*/
readonly containerPath: string;

/**
* `--volume` options to use when mounting this volume to the docker container.
*
* @default - 'z' option is used for selinux bind mounts
*/
readonly opts?: string[];
}

/**
* Configuration for a `BIND_MOUNT` type Docker volume.
*/
export interface DockerVolume extends DockerVolumeBase {
/**
* The type of the Docker volume.
*
* @default DockerVolumeType.BIND_MOUNT
*/
readonly dockerVolumeType?: DockerVolumeType.BIND_MOUNT;

/**
* The path on the host machine to be mounted as a bind mount.
* This property is required for `BIND_MOUNT` volumes.
*/
readonly hostPath: string;

/**
* Mount consistency. Only applicable for macOS
*
Expand All @@ -483,6 +527,48 @@ export interface DockerVolume {
readonly consistency?: DockerVolumeConsistency;
}

/**
* Configuration for a `VOLUME_COPY` type Docker volume.
*/
export interface VolumeCopyDockerVolume extends DockerVolumeBase {
/**
* The type of the Docker volume.
*
* @default DockerVolumeType.BIND_MOUNT
*/
readonly dockerVolumeType: DockerVolumeType.VOLUME_COPY;

/**
* The path on the host machine to be used as the input for the volume.
* @default - Does not copy from the host machine
Copy link
Contributor

@iliapolo iliapolo May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why would we define a VolumeCopy volume if we aren't copying anything?

Copy link
Author

@cgatt cgatt Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is optional to support the cases where we only want to copy things out of the container, such as the asset staging output. I can make the doc more explicit and throw an exception if both this and hostOutputPath are undefined if that would be preferable?

Copy link
Author

@cgatt cgatt Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative is to have a required hostPath matching DockerVolume, then an enum to allow selecting copy in, copy out, or both. The downside of this is that it loss the functionality to copy the modified files to a different output path and leave the original files unmodified, which I thought was a nice to have.

*/
readonly hostInputPath?: string;

/**
* The path on the host machine where the output from the volume will be written.
* @default - Does not copy to the host machine
*/
readonly hostOutputPath?: string;
Copy link
Contributor

@iliapolo iliapolo May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this capability is really related to what this PR is trying to address. There is also no test for it.

In general I don't think we need this, it poses a slight shift in how we think about bundling. Currently, the bundling mechanism takes N inputs and produces 1 output - thats clear and easy to reason about. Allowing for M outputs makes it more complex and I don't see a reason for it.

If you have a clear use case in mind - please elaborate on it, preferably in a different issue/PR.

Copy link
Author

@cgatt cgatt Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case for this (and the original incentive for this PR) is to allow mounting dependency cache folders (e.g. .m2/repository) when bundling in CICD. Any updates to this cache from updated/new packages being installed, etc, need to be copied out of the bundling container so that the CICD cache can be updated in turn (in our case, a github actions dependency cache).
This gives a similar final experience to a bind mount, but works with Docker in Docker and avoids permission issues from files being written as the root user in a bundling container.

Copy link
Author

@cgatt cgatt Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On further thought it's also required for the existing asset staging behaviour - even for 1 output scenarios, if the user opts for volume copy then asset staging needs the hostOutputPath functionality for the asset output.

}

/**
* Configuration for an `EXISTING` type Docker volume.
*/
export interface ExistingDockerVolume extends DockerVolumeBase {
/**
* The type of the Docker volume.
*
* @default DockerVolumeType.BIND_MOUNT
*/
readonly dockerVolumeType: DockerVolumeType.EXISTING;

/**
* The name of the existing volume.
* This property is required for `EXISTING` volumes.
*/
readonly volumeName: string;
}

/**
* Supported Docker volume consistency types. Only valid on macOS due to the way file storage works on Mac
*/
Expand Down Expand Up @@ -524,7 +610,7 @@ export interface DockerRunOptions {
*
* @default - no volumes are mounted
*/
readonly volumes?: DockerVolume[];
readonly volumes?: (DockerVolume | VolumeCopyDockerVolume | ExistingDockerVolume)[];

/**
* Where to mount the specified volumes from
Expand Down Expand Up @@ -640,28 +726,3 @@ export interface DockerBuildOptions {
function flatten(x: string[][]) {
return Array.prototype.concat([], ...x);
}

function isSeLinux(): boolean {
if (process.platform != 'linux') {
return false;
}
const prog = 'selinuxenabled';
const proc = spawnSync(prog, [], {
stdio: [ // show selinux status output
'pipe', // get value of stdio
process.stderr, // redirect stdout to stderr
'inherit', // inherit stderr
],
});
if (proc.error) {
// selinuxenabled not a valid command, therefore not enabled
return false;
}
if (proc.status == 0) {
// selinux enabled
return true;
} else {
// selinux not enabled
return false;
}
}
Loading
Loading