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.

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

feat(core): support ssh build arg in DockerImageAsset #26356

Merged
merged 30 commits into from
Aug 4, 2023

Conversation

JackWBoynton
Copy link
Contributor

@JackWBoynton JackWBoynton commented Jul 13, 2023

Adds support for the docker build --ssh flag for specifying ssh agent socket or ssh keys for ecr DockerImageAsset


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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jul 13, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team July 13, 2023 21:33
@JackWBoynton JackWBoynton marked this pull request as draft July 13, 2023 21:34
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jul 13, 2023
@JackWBoynton JackWBoynton changed the title Docker build ssh feat(ecr): Add ssh build arg to DockerImageAsset Jul 13, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted.

@mrgrain mrgrain temporarily deployed to test-pipeline August 1, 2023 14:31 — with GitHub Actions Inactive
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@mrgrain mrgrain changed the title feat(ecr): Add ssh build arg to DockerImageAsset feat(core): support ssh build arg in DockerImageAsset Aug 1, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Aug 1, 2023

e7327cde-b87f-46d6-a42b-f23b5870cf0e

@mrgrain mrgrain added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Aug 2, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review August 2, 2023 07:57

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mrgrain
Copy link
Contributor

mrgrain commented Aug 2, 2023

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2023

update

✅ Branch has been successfully updated

@mergify mergify bot dismissed mrgrain’s stale review August 4, 2023 14:25

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: bd07d78
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 4, 2023
@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2023

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

@mergify mergify bot merged commit 7b3d381 into aws:main Aug 4, 2023
11 of 12 checks passed
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 4, 2023
@pepito-j
Copy link

pepito-j commented Aug 22, 2023

Hello. Just wanted to chime in as I noticed this was pushed, but it looks like the --ssh flag still isn't getting propagated in. The results are below but I believe this section still needs to be changed:

  • await this.docker.build({
    directory: fullPath,
    tag: localTagName,
    buildArgs: source.dockerBuildArgs,
    buildSecrets: source.dockerBuildSecrets,
    target: source.dockerBuildTarget,
    file: source.dockerFile,
    networkMode: source.networkMode,
    platform: source.platform,
    outputs: source.dockerOutputs,
    cacheFrom: source.cacheFrom,
    cacheTo: source.cacheTo,
    quiet: this.options.quiet,
    });

Doesn't seem like we're in buildSsh options and instead are still just leaving it empty from the async build function. It does reference it, but on the debug log output it never appears hence this suspicion:

  • public async build(options: BuildOptions) {
    const buildCommand = [
    'build',
    ...flatten(Object.entries(options.buildArgs || {}).map(([k, v]) => ['--build-arg', `${k}=${v}`])),
    ...flatten(Object.entries(options.buildSecrets || {}).map(([k, v]) => ['--secret', `id=${k},${v}`])),
    ...options.buildSsh ? ['--ssh', options.buildSsh] : [],
    '--tag', options.tag,
    ...options.target ? ['--target', options.target] : [],
    ...options.file ? ['--file', options.file] : [],
    ...options.networkMode ? ['--network', options.networkMode] : [],
    ...options.platform ? ['--platform', options.platform] : [],
    ...options.outputs ? options.outputs.map(output => [`--output=${output}`]) : [],
    ...options.cacheFrom ? [...options.cacheFrom.map(cacheFrom => ['--cache-from', this.cacheOptionToFlag(cacheFrom)]).flat()] : [],
    ...options.cacheTo ? ['--cache-to', this.cacheOptionToFlag(options.cacheTo)] : [],
    '.',
    ];

Local Tests:

CDK Library Version 2.91.0
CDK CLI Version 2.91.0
NodeJs v16
Environment: Cloud9 / Amazon Linux 2

Test App:

const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack', { env: { region: 'us-east-1'  }})
new cdk.aws_ecr_assets.DockerImageAsset(stack, 'Image', {
  directory: path.join(__dirname, '../', 'images/test/'), # Just a Dockerfile in here
  buildSsh: 'default',
  buildSecrets: { 
    SECRET: cdk.DockerBuildSecret.fromSrc(path.join(path.join(__dirname, '../README.md'),)) 
  }
})

Dockerfile

# syntax=docker/dockerfile:1
FROM alpine
RUN apk add --no-cache openssh-client git
RUN mkdir -p -m 0700 ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts
RUN --mount=type=ssh ssh-add -l > ~/test.txt

Partial Debug Output:

[21:10:17] 2 total assets, 1 still need to be published
[21:10:17] Retrieved account ID ********** from disk cache
[21:10:17] Waiting for stack CDKToolkit to finish creating or updating...
Stack:  start: Building ca115e1fc5a469db02217d34c4e6c89902a04ad19846806664444b229db52144:current_account-us-east-1
[21:10:17] Stack:  debug: docker login --username AWS --password-stdin https://**********.dkr.ecr.us-east-1.amazonaws.com
[21:10:17] Stack:  debug: docker inspect cdkasset-ca115e1fc5a469db02217d34c4e6c89902a04ad19846806664444b229db52144
[21:10:17] Stack:  build: Building Docker image at /home/ec2-user/environment/dockerTest/cdk.out/asset.ca115e1fc5a469db02217d34c4e6c89902a04ad19846806664444b229db52144
[21:10:17] Stack:  debug: docker build --secret id=SECRET,src=/home/ec2-user/environment/dockerTest/README.md --tag cdkasset-ca115e1fc5a469db02217d34c4e6c89902a04ad19846806664444b229db52144 .
........
#10 [stage-0 3/4] RUN mkdir -p -m 0700 ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts
#10 sha256:72821f6cf384c419894b47fc9a2f8eb07ca007d93d39da5b3f8912e46257564c
#10 CACHED

#11 [stage-0 4/4] RUN --mount=type=ssh ssh-add -l > ~/test.txt
#11 sha256:e81ab27a7f76dd7e206dc88e36e4c938c860ab988b3e315001516b45f176899b
#11 0.437 Error connecting to agent: No such file or directory
#11 ERROR: executor failed running [/bin/sh -c ssh-add -l > ~/test.txt]: exit code: 2
------
 > [stage-0 4/4] RUN --mount=type=ssh ssh-add -l > ~/test.txt:
------
executor failed running [/bin/sh -c ssh-add -l > ~/test.txt]: exit code: 2

 ❌ Deployment failed: Error: Failed to build asset 5f16471d2d39322f4e45553c2c2bb34e775e7926016766181ad7ce0fedb5f8b7:current_account-us-east-1
    at Deployments.buildSingleAsset (/home/ec2-user/environment/dockerTest/node_modules/aws-cdk/lib/api/deployments.ts:593:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at Object.buildAsset (/home/ec2-user/environment/dockerTest/node_modules/aws-cdk/lib/cdk-toolkit.ts:201:7)
    at /home/ec2-user/environment/dockerTest/node_modules/aws-cdk/lib/util/work-graph.ts:108:11

Notice that "docker build --secret id=SECRET,src=/home/ec2-user/environment/dockerTest/README.md --tag cdkasset-ca115e1fc5a469db02217d34c4e6c89902a04ad19846806664444b229db52144 ." doesn't have the "--ssh" flag still.

Manually running "docker build . --ssh default" does go through just fine however as Docker is properly picking up the ssh-agent running locally:

Admin:~/environment/dockerTest/images/test (master) $ docker build . --ssh default
[+] Building 1.4s (12/12) FINISHED                                                                                                                                                       
 => [internal] load build definition from Dockerfile                                                                                                                                0.0s
 => => transferring dockerfile: 248B                                                                                                                                                0.0s
 => [internal] load .dockerignore                                                                                                                                                   0.0s
 => => transferring context: 2B                                                                                                                                                     0.0s
 => resolve image config for docker.io/docker/dockerfile:1                                                                                                                          0.1s
 => CACHED docker-image://docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021                                                     0.0s
 => [internal] load build definition from Dockerfile                                                                                                                                0.0s
 => [internal] load metadata for docker.io/library/alpine:latest                                                                                                                    0.1s
 => [internal] load .dockerignore                                                                                                                                                   0.0s
 => [stage-0 1/4] FROM docker.io/library/alpine:latest@sha256:7144f7bab3d4c2648d7e59409f15ec52a18006a128c733fcff20d3a4a54ba44a                                                      0.0s
 => => resolve docker.io/library/alpine:latest@sha256:7144f7bab3d4c2648d7e59409f15ec52a18006a128c733fcff20d3a4a54ba44a                                                              0.0s
 => CACHED [stage-0 2/4] RUN apk add --no-cache openssh-client git                                                                                                                  0.0s
 => CACHED [stage-0 3/4] RUN mkdir -p -m 0700 ~/.ssh && ssh-keyscan github.com >> ~/.ssh/known_hosts                                                                                0.0s
 => [stage-0 4/4] RUN --mount=type=ssh ssh-add -l > ~/test.txt                                                                                                                      0.5s
 => exporting to image                                                                                                                                                              0.0s
 => => exporting layers                                                                                                                                                             0.0s
 => => writing image sha256:023077abc3547b11f60ce7680cffbc5da9f695f04b5089f3f9580ca4d2b68824                                                                                        0.0s

@JackWBoynton
Copy link
Contributor Author

Thanks for the catch! I have a PR for the fix here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants