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

ecs.ContainerImage.fromAsset broken in 1.30 #6814

Closed
doomsower opened this issue Mar 19, 2020 · 5 comments
Closed

ecs.ContainerImage.fromAsset broken in 1.30 #6814

doomsower opened this issue Mar 19, 2020 · 5 comments
Assignees
@MrArnoldPalmer
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. documentation This is a problem with documentation. p0

Comments

@doomsower
Copy link

I've upgraded aws-cdk and @aws-cdk/* packages from 1.28.0 to 1.30.0 and ecs.ContainerImage.fromAsset got broken.

Docker asset is defined like this:

ecs.ContainerImage.fromAsset(path.resolve(__dirname, '../../../..'), {
    file: 'services/backend/Dockerfile',
    buildArgs: {
      //
    },
  });

CLI output says that 1.30.0 builds docker images with command that looks like:

docker build  --tag cdkasset-xxx \
              --file services/backend/Dockerfile \
              /Volumes/Projects/whatever/build/cdk/cdk.out/asset.xxx

And it breaks, because path services/backend/Dockerfile is relative to cdk project dir and it doesn't exist.

1.28.0 uses:

docker build --tag 942768376061.dkr.ecr.eu-central-1.amazonaws.com/aws-cdk/assets:xxx 
             --target production 
             --file cdk.out/asset.xxx/services/admin/Dockerfile 
             cdk.out/asset.xxx exited with error code 1

So 1.30.0 builds and tags images differently, but changelog doesn't mention it.

Downgrading back to 1.28.0 solves the problem.

Reproduction Steps

Error Log

Environment

  • CLI Version : 1.30.0
  • Framework Version: 1.30.0
  • OS : MacOS
  • Language : Typescript

Other

This is 🐛 Bug Report
@doomsower doomsower added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 19, 2020
@SomayaB SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Mar 19, 2020
@fmcmac
Copy link

fmcmac commented Mar 23, 2020

This issue also appears to exist for "DockerImageAsset"

@SoManyHs SoManyHs added documentation This is a problem with documentation. and removed needs-triage This issue or PR still needs to be triaged. labels Mar 23, 2020
MrArnoldPalmer added a commit to MrArnoldPalmer/aws-cdk that referenced this issue Mar 23, 2020
@MrArnoldPalmer
fix(ecr-assets): Docker asset path regression
ce1a68f 
Previously, users were allowed to pass a the `file` argument in
DockerImageAssetProps with a directory included in the string, IE:
'my-dir/Dockerfile'. This worked because when calling `docker build`,
the --file argument was formatted by prepending the asset's staging
directory. However, this was removed in v1.30.0 with the assumption that
`file` could contain only a filename and `directory` included all
other relative path information. This resulted in the docker build
command receiving a --file argument that was relative to the ${cwd} and
possibly wrong.

This is resolved by using `path.resolve` to get the file's full path.
Changes to how we are passing directory information to compute the
asset's hash were required to not break existing asset hashes. Otherwise
there is a possibility that a directory other than the one where the
Dockerfile resides was being referenced when hashing the asset's
contents.

fixes: aws#6814
@MrArnoldPalmer MrArnoldPalmer mentioned this issue Mar 23, 2020
Closed
@iliapolo
Copy link
Contributor

Looking into it.

@iliapolo iliapolo mentioned this issue Mar 23, 2020
Closed
@iliapolo
Copy link
Contributor

We have identified the problem and working on a fix.

You can follow the progress in this tracking issue: #6954

MrArnoldPalmer added a commit to MrArnoldPalmer/aws-cdk that referenced this issue Mar 24, 2020
@MrArnoldPalmer
fix(cdk-assets): Docker Build Context Path
0283d4b 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as '.'. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: aws#6954 aws#6814
@MrArnoldPalmer MrArnoldPalmer mentioned this issue Mar 24, 2020
Merged
MrArnoldPalmer added a commit to MrArnoldPalmer/aws-cdk that referenced this issue Mar 24, 2020
@MrArnoldPalmer
fix(cdk-assets): Docker Build Context Path
ca318e3 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as '.'. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: aws#6954 aws#6814
MrArnoldPalmer added a commit to MrArnoldPalmer/aws-cdk that referenced this issue Mar 24, 2020
@MrArnoldPalmer
fix(cdk-assets): Docker Build Bad Context Path
3d0247b 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as '.'. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: aws#6954 aws#6814
MrArnoldPalmer added a commit to MrArnoldPalmer/aws-cdk that referenced this issue Mar 24, 2020
@MrArnoldPalmer
fix(cdk-assets): Docker Build Bad Context Path
70f984e 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as '.'. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: aws#6954 aws#6814
@RomainMuller
Copy link
Contributor

Internal tracking ref: V188269601

mergify bot pushed a commit that referenced this issue Mar 24, 2020
@MrArnoldPalmer
fix(cdk-assets): context path not honored by Docker asset build (#6957)
1edd507 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as `.`. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: #6954 #6814
shivlaks pushed a commit that referenced this issue Mar 24, 2020
@MrArnoldPalmer @shivlaks
fix(cdk-assets): context path not honored by Docker asset build (#6957)
ea703d0 
Prior v1.29.0, the context passed to the docker build command was always
the current working directory. In v1.29.0, this was changed to be the
directory the user passed when instantiating the asset. However, docker
doesn't respect the context passed to the build command when the
`--file` argument is also passed. This broke assets for users passing
the `file` prop when constructing a `DockerImageAsset`.

This is fixed by changing the current working directory of the `docker build`
command and passing that context as `.`. Then docker uses the current working
directory as the context and correctly finds the image definition file from the
`file` prop. An existing integration test in the framework covers this case,
but does not fail unless the stack is deployed fresh since the asset isn't
rebuilt when the hash is not changed.

Fix: #6954 #6814
@iliapolo
Copy link
Contributor

Resolved in v1.31.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MrArnoldPalmer MrArnoldPalmer

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. documentation This is a problem with documentation. p0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ecr-assets): Docker asset path regression MrArnoldPalmer/aws-cdk
8 participants
@RomainMuller @iliapolo @uttarasridhar @fmcmac @MrArnoldPalmer @doomsower @SomayaB @SoManyHs