Skip to content
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

Jump to bottom

feat(core): add volumes-from option to docker run command for bundling #21660

Closed
wants to merge 13 commits into from
Closed

feat(core): add volumes-from option to docker run command for bundling #21660

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9a18004
WIP add volumesFrom
webratz Aug 9, 2022
020b825
WIP add volumesFrom
webratz Aug 9, 2022
6bcb4ee
adding tests and usage instruction
webratz Aug 18, 2022
8473d45
disable failing test for now
webratz Sep 13, 2022
abdb9b5
add missing type
webratz Sep 13, 2022
658e1dd
Merge branch 'main' into master
mergify[bot] Oct 2, 2022
a7fc600
Merge branch 'aws:main' into master
webratz Oct 4, 2022
74aae68
cleanup
webratz Oct 4, 2022
2ee951b
re-add adjusted test
webratz Oct 5, 2022
9dbbd18
test variant to allow the call to fail
webratz Oct 5, 2022
1fb4ada
remove failing test
webratz Oct 5, 2022
96c5614
whitespace cleanup
webratz Oct 5, 2022
d6b0bd4
Merge branch 'main' into master
webratz Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/README.md
View file
Open in desktop
Expand Up @@ -145,6 +145,18 @@ new lambda.PythonFunction(this, 'function', {
});
```

You can also pass additional options to configure Docker for situations where the docker daemon is not running in the same system as you are bundling from.

```ts
const entry = '/path/to/function';

new lambda.PythonFunction(this, 'function', {
entry,
runtime: Runtime.PYTHON_3_8,
bundling: { volumesFrom: process.env.HOSTNAME },
});
```

## Custom Bundling with Code Artifact

To use a Code Artifact PyPI repo, the `PIP_INDEX_URL` for bundling the function can be customized (requires AWS CLI in the build environment):
Expand Down
9 changes: 9 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/lib/bundling.ts
View file
Open in desktop
Expand Up @@ -35,6 +35,13 @@ export interface BundlingProps extends BundlingOptions {
*/
readonly architecture?: Architecture;

/**
* Where to mount the specified volumes from
* Docker [volumes-from option](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from)
* @default - no volumes-from options
*/
readonly volumesFrom?: string;

/**
* Whether or not the bundling process should be skipped
*
Expand All @@ -59,6 +66,7 @@ export class Bundling implements CdkBundlingOptions {
public readonly image: DockerImage;
public readonly command: string[];
public readonly environment?: { [key: string]: string };
public readonly volumesFrom?: string | undefined;

constructor(props: BundlingProps) {
const {
Expand Down Expand Up @@ -86,6 +94,7 @@ export class Bundling implements CdkBundlingOptions {
});
this.command = ['bash', '-c', chain(bundlingCommands)];
this.environment = props.environment;
this.volumesFrom = props.volumesFrom;
}

private createBundlingCommand(options: BundlingCommandOptions): string[] {
Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/lib/types.ts
View file
Open in desktop
Expand Up @@ -76,4 +76,11 @@ export interface BundlingOptions {
* @default - Based on `assetHashType`
*/
readonly assetHash?: string;

/**
* Where to mount the specified volumes from
* Docker [volumes-from option](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from)
* @default - no volumes-from options
*/
readonly volumesFrom?: string;
}
15 changes: 15 additions & 0 deletions packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts
View file
Open in desktop
Expand Up @@ -250,6 +250,21 @@ test('Bundling with custom environment vars`', () => {
}));
});

test('Bundling with custom volumes', () => {
const entry = path.join(__dirname, 'lambda-handler');
Bundling.bundle({
entry: entry,
runtime: Runtime.PYTHON_3_7,
volumesFrom: process.env.HOSTNAME,
});

expect(Code.fromAsset).toHaveBeenCalledWith(entry, expect.objectContaining({
bundling: expect.objectContaining({
volumesFrom: process.env.HOSTNAME,
}),
}));
});

test('Do not build docker image when skipping bundling', () => {
const entry = path.join(__dirname, 'lambda-handler');
Bundling.bundle({
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/core/lib/asset-staging.ts
View file
Open in desktop
Expand Up @@ -461,6 +461,7 @@ export class AssetStaging extends Construct {
entrypoint: options.entrypoint,
workingDirectory: options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR,
securityOpt: options.securityOpt ?? '',
volumesFrom: options.volumesFrom,
});
}
} catch (err) {
Expand Down
17 changes: 17 additions & 0 deletions packages/@aws-cdk/core/lib/bundling.ts
View file
Open in desktop
Expand Up @@ -43,6 +43,13 @@ export interface BundlingOptions {
*/
readonly volumes?: DockerVolume[];

/**
* Where to mount the specified volumes from
* Docker [volumes-from option](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from)
* @default - no volumes-from options
*/
readonly volumesFrom?: string;

/**
* The environment variables to pass to the Docker container.
*
Expand Down Expand Up @@ -210,6 +217,9 @@ export class BundlingDockerImage {
...options.user
? ['-u', options.user]
: [],
...options.volumesFrom
? ['--volumes-from', options.volumesFrom]
: [],
...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
Expand Down Expand Up @@ -441,6 +451,13 @@ export interface DockerRunOptions {
*/
readonly volumes?: DockerVolume[];

/**
* Where to mount the specified volumes from
* Docker [volumes-from option](https://docs.docker.com/engine/reference/commandline/run/#mount-volumes-from-container---volumes-from)
* @default - no volumes-from options
*/
readonly volumesFrom?: string;

/**
* The environment variables to pass to the container.
*
Expand Down
33 changes: 33 additions & 0 deletions packages/@aws-cdk/core/test/bundling.test.ts
View file
Open in desktop
Expand Up @@ -438,6 +438,39 @@ describe('bundling', () => {
], { stdio: ['ignore', process.stderr, 'inherit'] })).toEqual(true);
});

// test('adding user provided volume options', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't have tests commented out. If the tests are failing, that generally indicates something isn't quite right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey,
the problem here is, that the fix solves a problem for specific docker in docker situations which require that specific environment to be there to be tested.
From my tries i was unable to mock this docker within docker situation in the CDK test suite, as this heavily depends on where the tests are run.
I would remove the test here, as it mostly would test basically the docker configuration, but not tell much about the functionality of the CDK code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some notes on how to reproduce the steps solely with docker in the issues description.
I'm still trying to find a way to make that testable, but as the CI environment is different than what is available local its very time consuming back and forth of seeing if a change works or not

// // GIVEN
// sinon.stub(process, 'platform').value('darwin');
// const spawnSyncStub = sinon.stub(child_process, 'spawnSync').returns({
// status: 0,
// stderr: Buffer.from('stderr'),
// stdout: Buffer.from('stdout'),
// pid: 123,
// output: ['stdout', 'stderr'],
// signal: null,
// });
// const image = DockerImage.fromRegistry('alpine');

// // GIVEN
// image.run({
// command: ['cool', 'command'],
// volumesFrom: process.env.HOSTNAME,
// volumes: [{ hostPath: '/host-path', containerPath: '/container-path' }],
// workingDirectory: '/working-directory',
// user: 'user:group',
// });

// expect(spawnSyncStub.calledWith('docker', [
// 'run', '--rm',
// '--volumes-from', process.env.HOSTNAME ?? 'foo',
// '-u', 'user:group',
// '-v', '/host-path:/container-path:delegated',
// '-w', '/working-directory',
// 'alpine',
// 'cool', 'command',
// ], { stdio: ['ignore', process.stderr, 'inherit'] })).toEqual(true);
// });

test('ensure selinux docker mount', () => {
// GIVEN
sinon.stub(process, 'platform').value('linux');
Expand Down