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

TarballImageAsset executable command in ...assets.json prevents running cdk deploy outside cloud assembly root directory #15721

Closed
chris-leach opened this issue Jul 22, 2021 · 15 comments · Fixed by #15750 or #16094
Assignees
Labels
@aws-cdk/assets Related to the @aws-cdk/assets package @aws-cdk/aws-ecr-assets Related to AWS CDK Docker Image Assets bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@chris-leach
Copy link

We were very excited to use the new TarballImageAsset class to deploy docker images built by Kaniko in our CI pipeline, but it seems to break deployments unless run in a specific and inconvenient way.

Reproduction Steps

Given the following CDK project, where hello-world.tar is produced with docker save hello-world -o hello-world.tar:

.
├── app.py
├── cdk.json
└── hello-world.tar

app.py:

from aws_cdk import core as cdk
from aws_cdk.aws_ecs import ContainerImage, FargateTaskDefinition

app = cdk.App()

stack = cdk.Stack(app, "TestStack")

task_def = FargateTaskDefinition(
    stack, "TestTaskDef",
)
task_def.add_container(
    "TestContainer",
    image=ContainerImage.from_tarball("hello-world.tar"),
)

app.synth()

cdk.json:

{
  "app": "python3 app.py",
  "context": {
    "@aws-cdk/aws-ecr-assets:dockerIgnoreSupport": true,
    "@aws-cdk/aws-ecs-patterns:removeDefaultDesiredCount": true,
    "@aws-cdk/aws-kms:defaultKeyPolicies": true,
    "@aws-cdk/aws-s3:grantWriteWithoutAcl": true,
    "@aws-cdk/aws-secretsmanager:parseOwnedSecretName": true,
    "@aws-cdk/core:enableStackNameDuplicates": "true",
    "@aws-cdk/core:stackRelativeExports": "true",
    "@aws-cdk/core:newStyleStackSynthesis": true,
    "aws-cdk:enableDiffNoFail": "true"
  }
}

Then, run cdk depoy -vvv from the project root.

What did you expect to happen?

The deployment should succeed, creating an ECS task definition from the docker image tarball.

What actually happened?

Deployment fails with the verbose output shown below.

The root cause appears to be that the executable command (in cdk.out/TestStack.assets.json, shown below) is run from the same working directory as the cdk deploy command - obviously the asset...tar file is not present when run from the project root.

...
      "source": {
        "executable": [
          "sh",
          "-c",
          "docker load -i asset.c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069.tar | sed \"s/Loaded image: //g\""
        ]
      },
...

This seems to be confirmed by the fact that when I first run cdk synth, and then cd cdk.out; cdk deploy --app ., the deployment succeeds.

Suggestions

It seems very strange to me that the deployment operation is so dependent on the current working directory - I would expect always to be able to run cdk deploy --app /path/to/cloud/assembly and have it work.

Ideally, asset executable commands would be run from the cloud assembly directory, regardless of where the cdk deploy command was run from. If changing this would break backwards compatibility, then a possible solution might be to provide the path to the cloud assembly root to the executable somehow, e.g. as an environment variable. The executable written by TarballImageAsset could then be something like:

"executable": ["sh", "-c", "docker load -i $CDK_CLOUD_ASM_ROOT/asset.c2a...069.tar | sed \"s/Loaded image: //g\""]

In any case there are a couple of other issues I can see that made it very hard to understand what is going wrong here:

  • Despite the docker load command failing, the exit code of the command overall is 0 because of the pipe to sed, causing execution to continue past where it should.
  • Similarly, the output from the command is the empty string, which the CLI then tries to pass as an argument to docker tag, causing the error seen below. Ideally the output of the executable should be validated as soon as it returns.

Thanks for reading, and if you would be willing to accept a PR along the lines of any of these suggestions then let me know.

Environment

  • CDK CLI Version : 1.114.0
  • Framework Version: 1.114.0
  • Node.js Version: 15.14.0
  • OS : Linux
  • Language (Version): Python (3.8.5)

Other

Verbose cdk deploy output:

...
TestStack: deploying...
Retrieved account ID XXXXXXXXXXXX from disk cache
Assuming role 'arn:aws:iam::XXXXXXXXXXXX:role/cdk-hnb659fds-deploy-role-XXXXXXXXXXXX-eu-west-2'.
Waiting for stack CDKToolkit to finish creating or updating...
[AWS cloudformation 200 0.142s 0 retries] describeStacks({ StackName: 'CDKToolkit' })
[AWS ssm 200 0.207s 0 retries] getParameter({ Name: '/cdk-bootstrap/hnb659fds/version' })
[0%] start: Publishing b04e11e72beb3cf24c3704c221e4f85f380262ec35ef875764f306f4eff0e644:current_account-current_region
Retrieved account ID XXXXXXXXXXXX from disk cache
Retrieved account ID XXXXXXXXXXXX from disk cache
Assuming role 'arn:aws:iam::XXXXXXXXXXXX:role/cdk-hnb659fds-file-publishing-role-XXXXXXXXXXXX-eu-west-2'.
[0%] check: Check s3://cdk-hnb659fds-assets-XXXXXXXXXXXX-eu-west-2/b04e11e72beb3cf24c3704c221e4f85f380262ec35ef875764f306f4eff0e644.json
[AWS s3 200 0.194s 0 retries] getBucketLocation({ Bucket: 'cdk-hnb659fds-assets-XXXXXXXXXXXX-eu-west-2' })
[AWS s3 200 0.036s 0 retries] listObjectsV2({
  Bucket: 'cdk-hnb659fds-assets-XXXXXXXXXXXX-eu-west-2',
  Prefix: 'b04e11e72beb3cf24c3704c221e4f85f380262ec35ef875764f306f4eff0e644.json',
  MaxKeys: 1
})
[0%] found: Found s3://cdk-hnb659fds-assets-XXXXXXXXXXXX-eu-west-2/b04e11e72beb3cf24c3704c221e4f85f380262ec35ef875764f306f4eff0e644.json
[50%] success: Published b04e11e72beb3cf24c3704c221e4f85f380262ec35ef875764f306f4eff0e644:current_account-current_region
[50%] start: Publishing c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069:current_account-current_region
Retrieved account ID XXXXXXXXXXXX from disk cache
Retrieved account ID XXXXXXXXXXXX from disk cache
Assuming role 'arn:aws:iam::XXXXXXXXXXXX:role/cdk-hnb659fds-image-publishing-role-XXXXXXXXXXXX-eu-west-2'.
[AWS ecr 200 0.163s 0 retries] describeRepositories({
  repositoryNames: [
    'cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2',
    [length]: 1
  ]
})
[50%] check: Check XXXXXXXXXXXX.dkr.ecr.eu-west-2.amazonaws.com/cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2:c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069
[AWS ecr 400 0.025s 0 retries] describeImages({
  repositoryName: 'cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2',
  imageIds: [
    {
      imageTag: 'c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069'
    },
    [length]: 1
  ]
})
Call failed: describeImages({"repositoryName":"cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2","imageIds":[{"imageTag":"c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069"}]}) => The image with imageId {imageDigest:'null', imageTag:'c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069'} does not exist within the repository with name 'cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2' in the registry with id 'XXXXXXXXXXXX' (code=ImageNotFoundException)
[AWS ecr 200 0.015s 0 retries] getAuthorizationToken({})
[50%] debug: docker login --username AWS --password-stdin https://XXXXXXXXXXXX.dkr.ecr.eu-west-2.amazonaws.com
[50%] build: Building Docker image using command 'sh,-c,docker load -i asset.c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069.tar | sed "s/Loaded image: //g"'
[50%] upload: Push XXXXXXXXXXXX.dkr.ecr.eu-west-2.amazonaws.com/cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2:c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069
[50%] debug: docker tag  XXXXXXXXXXXX.dkr.ecr.eu-west-2.amazonaws.com/cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2:c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069
Error parsing reference: "" is not a valid repository/tag: invalid reference format
[100%] fail: docker tag  XXXXXXXXXXXX.dkr.ecr.eu-west-2.amazonaws.com/cdk-hnb659fds-container-assets-XXXXXXXXXXXX-eu-west-2:c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069 exited with error code 1: Error parsing reference: "" is not a valid repository/tag: invalid reference format

 ❌  TestStack failed: Error: Failed to publish one or more assets. See the error messages above for more information.
    at Object.publishAssets (/home/name/.nvm/versions/node/v15.14.0/lib/node_modules/aws-cdk/lib/util/asset-publishing.ts:25:11)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)
    at CloudFormationDeployments.publishStackAssets (/home/name/.nvm/versions/node/v15.14.0/lib/node_modules/aws-cdk/lib/api/cloudformation-deployments.ts:278:7)
    at CloudFormationDeployments.deployStack (/home/name/.nvm/versions/node/v15.14.0/lib/node_modules/aws-cdk/lib/api/cloudformation-deployments.ts:179:5)
    at CdkToolkit.deploy (/home/name/.nvm/versions/node/v15.14.0/lib/node_modules/aws-cdk/lib/cdk-toolkit.ts:184:24)
    at initCommandLine (/home/name/.nvm/versions/node/v15.14.0/lib/node_modules/aws-cdk/bin/cdk.ts:213:9)
Failed to publish one or more assets. See the error messages above for more information.

This is 🐛 Bug Report

@chris-leach chris-leach added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 22, 2021
@peterwoodworth peterwoodworth added the @aws-cdk/aws-ecr-assets Related to AWS CDK Docker Image Assets label Jul 22, 2021
@eladb
Copy link
Contributor

eladb commented Jul 25, 2021

@chris-leach you will need to specify a path relative to current running script directory.

I believe you can do it like this in python:

scriptdir=os.path.dirname(os.path.abspath(__file__))

task_def.add_container(
    "TestContainer",
    image=ContainerImage.from_tarball(scriptdir + "/hello-world.tar"),
)

I've created #15750 to improve documentation.

@eladb eladb added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jul 25, 2021
@eladb eladb changed the title TarballImageAsset breaks cdk deploy TarballImageAsset must be absolute path (otherwise, breaks cdk deploy) Jul 25, 2021
@eladb eladb changed the title TarballImageAsset must be absolute path (otherwise, breaks cdk deploy) TarballImageAsset documentation is misleading since path must be absolute Jul 25, 2021
@eladb eladb removed their assignment Jul 25, 2021
@chris-leach
Copy link
Author

chris-leach commented Jul 25, 2021

Hi @eladb, thanks very much for your reply but I think you've misunderstood the issue.

The issue is not that the Python script cannot find the source tarball. I made the contrived example above for the purpose of the issue - apologies if it confused things - in my actual application I do of course use an absolute path to refer to the source tarball (as an aside, pathlib is preferred to os.path these days).

In the actual application cdk synth succeeds from any working directory. The source tarball is successfully copied into the cdk.out directory, and the executable command shown above refers to the copy, not the original file.

The cdk.out directory looks like this:

cdk.out/
├── TestStack.assets.json
├── TestStack.template.json
├── asset.c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069.tar
├── cdk.out
├── manifest.json
└── tree.json

The problem, as mentioned above, is that the executable in TestStack.assets.json - not the Python - refers to asset.c2a...069.tar in a relative way.

...
  "dockerImages": {
    "c2a72eb491044b4b079b0430e01e40d428a629b736f0e3c61f2f51170ed20069": {
      "source": {
        "executable": [
          "sh",
          "-c",
          "docker load -i asset.c2a...069.tar | sed \"s/Loaded image: //g\""
        ]
      },
      "destinations": {
        ...
      }
    }
  }
...

This is necessary to an extent, since the intention as I understand it is that people can move the cloud assembly directory around, tar it up to deploy from elsewhere, etc. As mentioned above, though, the way that TarballImageAsset writes the executable prevents people from running cdk deploy --app /path/to/cdk.out, instead forcing them to cd /path/to/cdk.out; cdk deploy --app .. This would not be the biggest issue in the world, if it didn't also break the degenerate case of just running cdk deploy from the main project directory.

I hope this clarifies the issue, and would very much appreciate it if you could consider whether any of the suggestions above might improve things.

Thanks again for your time.

@chris-leach chris-leach changed the title TarballImageAsset documentation is misleading since path must be absolute TarballImageAsset executable command in ...assets.json prevents running cdk deploy outside cloud assembly root directory Jul 25, 2021
@github-actions github-actions bot added the @aws-cdk/assets Related to the @aws-cdk/assets package label Jul 25, 2021
@eladb
Copy link
Contributor

eladb commented Jul 26, 2021

@chris-leach thanks for clarifying. I will look into it.

@eladb
Copy link
Contributor

eladb commented Jul 26, 2021

@pgarbe can you take a look?

@pgarbe
Copy link
Contributor

pgarbe commented Jul 26, 2021

@eladb sure, can do

@pgarbe
Copy link
Contributor

pgarbe commented Jul 26, 2021

I can verify that issue. When I created the feature, I used CdkPipeline and it that case it works very well as the workdir in publish-assets stage is within the cdk.out folder (because how the files is moved between the stages within CodePipeline).

@chris-leach do you have already an idea how to tackle that?

Regarding your other comment: The container image handling fails if the exec command does not return a name of a valid container image. But I agree, there could be a better error handling.

@chris-leach
Copy link
Author

@pgarbe My initial idea was to change the way dockerImages...executable works so that the subprocess is always run from the root of the cloud assembly directory, but this might have a fairly large impact on backwards compatibility, and possibly require changes to the cloud assmbly specification (I'm not totally sure how these things work). That said, I'm not sure how often dockerImages...executable is used in practice.

If making the cloud assembly root the CWD for all docker executables is indeed unacceptable, a less intrusive change would be to make it available to the executable as an environment variable, so existing executables could remain unchanged, but new ones written by TarballImageAsset could change from:

["sh", "-c", "docker load -i asset.c2a...069.tar | sed \"s/Loaded image: //g\""]

to e.g.:

["sh", "-c", "docker load -i $CDK_CLOUD_ASSEMBLY_ROOT/asset.c2a...069.tar | sed \"s/Loaded image: //g\""]

What do you think?

@pgarbe
Copy link
Contributor

pgarbe commented Jul 26, 2021

I like the idea of the environment variable. Solves the issue and is also compatible. I'm not much into the cloud assembly manifest so I'm not sure if it violates the spec. Maybe @eladb or @rix0rrr can guide here?

@eladb
Copy link
Contributor

eladb commented Jul 26, 2021

How is that different from the normal "docker build" we are doing and referencing a directory inside the cloud assembly? I don't see a reason for this to be any different or am I missing something.

@pgarbe
Copy link
Contributor

pgarbe commented Jul 26, 2021

The fromTarball implementation uses the execute source, and for a Dockerfile directory is used. See definition

If the asset is build from a Dockerfile, the directory contains the relative directory inside cdk.out folder. The buildImage implementation resolves it to the full path. In the case when only execute source is defined just the command gets executed.

@eladb
Copy link
Contributor

eladb commented Aug 1, 2021

@njlynch @rix0rrr @otaviomacedo it seems like buildExternalAsset() runs the executable without setting the cwd. Would it make sense that cwd will be set to the cloud assembly root?

private async buildExternalAsset(executable: string[]): Promise<string | undefined> {

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 17, 2021

I am okay changing the default cwd, as long as we also add a cwd?: string parameter to override back to a well-known location.

I won't have time to work on this myself but I would happily welcome a PR that addresses this.

@pgarbe
Copy link
Contributor

pgarbe commented Aug 17, 2021

Sounds good to me. I can have a look and send a PR.

@pgarbe
Copy link
Contributor

pgarbe commented Aug 17, 2021

@rix0rrr created the PR (#16094). What exactly do you have in mind with the optional parameter? Should it be added to the manifest schema?

@mergify mergify bot closed this as completed in #16094 Sep 7, 2021
mergify bot pushed a commit that referenced this issue Sep 7, 2021
…mbly root directory (#16094)

Runs the executable command of container assets in the cloud assembly root directory and not the current directory.

Could be a breaking change.

fixes #15721
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Sep 7, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

mergify bot pushed a commit that referenced this issue Sep 12, 2021
Improve docs to indicate that the path to a tarball should be absolute and not relative.

Fixes #15721 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/assets Related to the @aws-cdk/assets package @aws-cdk/aws-ecr-assets Related to AWS CDK Docker Image Assets bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
5 participants