-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(assets): support platform flag for DockerImageAsset #16770
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
|
||
new DockerImageFunction(this, 'MyLambda', { | ||
code: DockerImageCode.fromImageAsset(path.join(__dirname, 'docker-arm64-handler'), { | ||
platform: DockerPlatform.ARM_64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a way to automatically set the platform
based on the value of architectures
.
Maybe pass props.architectures
to _bind()
here?
aws-cdk/packages/@aws-cdk/aws-lambda/lib/image-function.ts
Lines 60 to 69 in 1deea90
export class DockerImageFunction extends Function { | |
constructor(scope: Construct, id: string, props: DockerImageFunctionProps) { | |
super(scope, id, { | |
...props, | |
handler: Handler.FROM_IMAGE, | |
runtime: Runtime.FROM_IMAGE, | |
code: props.code._bind(), | |
}); | |
} | |
} |
then you can set the platform
in fromImageAsset
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jogold,
This should have been addressed. Can you take a look again?
I think we still need an unit test to spyOn some method being called with correct platform
. I've checked your packages/@aws-cdk/aws-lambda-go/test/bundling.test.ts
and have been trying to write a similar test but ended up with no luck. Could you advise how to write the unit test for this PR? Any code snippets would be appreciated.
…b.com/pahud/aws-cdk into pahud/assets-allow-user-to-pass-12472
Hi @jogold can you take a look again? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Hi @pahud this PR seems to be a bit out of date, is this still being worked on? |
* Use this if the platform name is not yet supported by the CDK. | ||
* @param [platform=linux/amd64] the platform to use for docker build. | ||
*/ | ||
public static custom(platform?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to provide a default value here for platform
. DockerPlatform.custom()
doesn't make semantic sense...
public static custom(platform?: string) { | |
public static custom(platform: string) { |
* Specify this property to build images for a specific platform. Support docker API 1.38+. | ||
* | ||
* @default - no specific platform | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* |
@@ -81,6 +81,16 @@ new lambda.DockerImageFunction(this, 'ECRFunction', { | |||
The props for these docker image resources allow overriding the image's `CMD`, `ENTRYPOINT`, and `WORKDIR` | |||
configurations. See their docs for more information. | |||
|
|||
To deploy a `DockerImageFunction` on Lambda `arm64` architecture, make sure you specify `Architecture.ARM_64` in `architecture`. | |||
This will bundle docker image assets for `arm64` architecture with `--platform linux/arm64` implicitly even you build it from a `x86_64` architecture. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will bundle docker image assets for `arm64` architecture with `--platform linux/arm64` implicitly even you build it from a `x86_64` architecture. | |
This will bundle docker image assets for `arm64` architecture with `--platform linux/arm64` implicitly even you build it within an `x86_64` host. |
/** | ||
* determine the platform from `architecture`. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a docstring:
/** | |
* determine the platform from `architecture`. | |
*/ | |
// determine the platform from `architecture`. |
@pahud I wouldn't mind if you could address the changes requested by Elad – I want to see this get merged 😄 |
Sorry for the delay. Yes I will address this shortly and continue this PR. Thanks!! |
/** | ||
* determine the platform from `architecture`. | ||
*/ | ||
platform: arch?.dockerPlatform ? DockerPlatform.custom(arch.dockerPlatform) : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: align the style like this
...options.platform ? ['--platform', options.platform] : [], |
platform: arch?.dockerPlatform ? DockerPlatform.custom(arch.dockerPlatform) : undefined, | |
...arch?.dockerPlatform ? { platform: DockerPlatform.custom(arch.dockerPlatform) } : {}, |
@pahud what's the status here? |
This PR has been in CHANGES REQUESTED for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Will this PR be revived? |
@pahud @ericzbeard - Curious if this one is going to be revived? It looks to be close and I was hoping to use this feature in a demo I was building. |
…ker images (#20439) This PR adds support for specifying the desired build platform when building docker images (ie: build an arm64 image on an amd64/x86_64 host). Closes #12472 This PR does NOT touch Lambda builders, only ECR assets. #16770 attempted to implement support for ECR and Lambda but was abandoned. Meanwhile #16858 implemented lambda platform support. This implements the ECR side I have run `yarn integ` ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Based on [this](#16770) PR Add the missing part to add platform support when using lambda `fromImageAsset` As we are not allowed to specify `platform` flag for `DockerImageAsset`, users deploying cdk on x86_64 platform will not be able to bundle lambda.DockerImageFunction for the new arm64 architecture. Similarly, users deploying cdk on arm64 architecture like Mac M1 will not be able to bundle images for AWS Fargate, which is x86_64 only. # builder experience with aws-lambda For x86_64 users deploying Lambda functions with container runtime on Lambda Graviton2(arm64) from local container image assets with multi-architecture docker base images. Specify the platform to ensure the image would be built for ARM64 architecture. ``` new DockerImageFunction(stack, 'Lambda', { code: DockerImageCode.fromImageAsset(path.join(__dirname, 'docker-arm64-handler')), architecture: Architecture.ARM_64, }); ``` Fixes: #12472, #20907 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Based on [this](aws#16770) PR Add the missing part to add platform support when using lambda `fromImageAsset` As we are not allowed to specify `platform` flag for `DockerImageAsset`, users deploying cdk on x86_64 platform will not be able to bundle lambda.DockerImageFunction for the new arm64 architecture. Similarly, users deploying cdk on arm64 architecture like Mac M1 will not be able to bundle images for AWS Fargate, which is x86_64 only. # builder experience with aws-lambda For x86_64 users deploying Lambda functions with container runtime on Lambda Graviton2(arm64) from local container image assets with multi-architecture docker base images. Specify the platform to ensure the image would be built for ARM64 architecture. ``` new DockerImageFunction(stack, 'Lambda', { code: DockerImageCode.fromImageAsset(path.join(__dirname, 'docker-arm64-handler')), architecture: Architecture.ARM_64, }); ``` Fixes: aws#12472, aws#20907 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
As we are not allowed to specify
platform
flag forDockerImageAsset
, users deploying cdk on x86_64 platform will not be able to bundlelambda.DockerImageFunction
for the new arm64 architecture. Similarly, users deploying cdk on arm64 architecture like Mac M1 will not be able to bundle images for AWS Fargate, which is x86_64 only.With this support, we are allowed to:
platform
property ofDockerImageAsset
fromaws-ecr-assets
.DockerImageFunction
fromaws-lambda
on x86_64.aws-ecs
on arm64.Fixes: #12472
Background
When we bundle and publish the docker container image assets with
cdk deploy
, it's important we must build images of correct architectures for different services. For AWS Fargate, as it only supports x86_64 at this moment, we must build x86_64(amd64) architecture docker images, while for Lambda Graviton2, thearm64
would be required.Architecture
of the imagesRun
docker inspect
to check theArchitecture
for local images:For example:
(The image was built for
arm64
platform)How to build images with correct
Architecture
?Many docker image registries come with multi-architecture support. Read Introducing multi-architecture container images for Amazon ECR for more details on Amazon ECR. Let's take
public.ecr.aws/lambda/python:latest
for example. Rundocker manifest inspect
to check its manifest for architectures:This image supports both
linux/arm64
andlinux/amd64
from the manifest.Similarly, if we check
python:3.9
from docker hub, we can list all platforms/architectures supported from its manifest data.So how can we ensure to build correct architecture from different desktops like Apple Mac M1 or any x86_64 desktops? Let's take the following Dockerfile for example:
FROM public.ecr.aws/lambda/python:latest
For Apple Mac M1 users, if we
docker build
with this Dockerfile, under the covers the image layers forarm64
will be pulled because the Apple Mac M1 isarm64
architecture unless:DOCKER_DEFAULT_PLATFORM
env var(see (assets): allow user to pass platform option to Docker builds #12472 (comment))--platform
in the Dockerfile likedocker build
with--platform
flagLet's take M1 for example, if we
docker build
without passing--platform
, we'll build anarm64
image.However, if we pass
--platform='linux/amd64'
we'll be able to buildamd64
image from exactly the same Dockerfile.Similarly, for
x86_64
desktop users, we'll buildamd64
images by default andarm64
images by passing--platform='linux/arm64'
.When we bundle container image assets for AWS Fargate, we must build
amd64
images. For Lambda arm64 architecture, we must buildarm64
images for Lambda container runtime, no matter which platform you synthesize and deploy the assets. The key is always to pass correct--platform
flag with Dockerfile from a base image with multiple-platform support.However, what if we use
python:latest-arm64
as the base image for ourDockerfile
? Well, we will lose the advantages of multi-architecture support as this will only build arm64 images.builder experience with
aws-ecr-assets
Specify
platform
withARM_64
orAMD_64
from docker image assets with multi-architecture docker base imagesbuilder experience with
aws-ecs
For Mac M1 users deploying Amazon ECS services on AWS Fargate from local container image assets with multi-architecture docker base images. Specify the
platform
to ensure the image would be built forAMD_64
architecture.builder experience with
aws-lambda
For
x86_64
users deploying Lambda functions with container runtime on Lambda Graviton2(arm64) from local container image assets with multi-architecture docker base images. Specify theplatform
to ensure the image would be built forARM64
architecture.builder experience with
aws-apprunner
AWS App Runner supports
x86_64
architecture only at this moment. For Mac M1 users, useplatform
to ensure theAMD_64
architecture for theDockerImageAsset
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license