Use the same DockerContext and Dockerfile paths as the CDK is using

[i18n-tribe]

Which issue(s) does this change fix?

#4059

Why is this change necessary?

When defining an aws_lambda.DockerImageFunction construct using the CDK, the docker image is configured with code like this:

code = aws_lambda.DockerImageCode.from_image_asset(
    directory="/Users/john/PycharmProjects/project-root",
    file="./application/runtime/Dockerfile",
)

docker_lambda = aws_lambda.DockerImageFunction(code=code)

When the CDK project is built, the CDK uses directory as the DockerContext and file as the Dockerfile.

See the documentation:

directory (str) – the directory from which the asset must be created.
file (Optional[str]) – Path to the Dockerfile (relative to the directory). Default: ‘Dockerfile’

This means /Users/john/PycharmProjects/project-root is the DockerContext

and the Dockerfile is a path to a docker file from within the DockerContext - i.e. ./application/runtime/Dockerfile

This capability of the CDK is useful because...

Say you have a folder structure like:

project-root
└───application
│   └───runtime
│       │   Dockerfile
│       │   app.py
│       │   requirements.txt
└───another_application
│   └───runtime
│       │   Dockerfile
│       │   app.py
│       │   requirements.txt
└───shared_code
    │   shared.py

There are 2 apps in the repo - in folders application and another_application

Each app contains a lambda.

The 2 lambdas share some common code, which is in a folder called shared_code

In each lambda's Dockerfile, we need to copy the shared_code module into the container.

Therefore, when we are building each lambda's container, we need to set the DockerContext to be the root folder of the repo /project-root, because this means /project-root/shared_code will be accessible when we build the container.

The DockerContext cannot be the folder that contains Dockerfile. I.e. it cannot be project-root/application/runtime - because the /project-root/shared_code folder would not be readable while the container is being built.

This example works fine in the CDK.

However - when we wish to test locally using the sam-cli - it does not work.

The reason it does not work with the sam-cli is - the sam-cli code is not using the same DockerContext and Dockerfile paths as the CDK. The sam-cli code is currently ensuring that DockerContext is always the folder that contains Dockerfile, irrespective of how DockerContext has been configured in the aws_lambda.DockerImageFunction CDK construct.

So in this example - DockerContext is /Users/john/PycharmProjects/project-root/application/runtime

Side-by-side comparison:

	CDK	SAM
DockerContext	/Users/john/PycharmProjects/project-root	/Users/john/PycharmProjects/project-root/application/runtime
Dockerfile	./application/runtime/Dockerfile	Dockerfile

Side Note:

This example would be the same with a single app in the repo.

This example is the same in any case where there are 2 lambdas that share some code, but don't share a Dockerfile.

How does it address the issue?

The SAM CLI will now use the same DockerContext and Dockerfile paths as the CDK is using.

What side effects does this change have?

No effect at all on production code.

It could in theory effect local testing in some cases.

However – we cannot think of a scenario where local testing is currently working, but will stop working because of this change.

If there is any code that is currently working that will stop working because of this change – the currently working code must be relying on in some kind of hack to work around the issue this PR is fixing.

We wrote our own hack - it looks like this:

   code = create_docker_image_code_from_asset(
        dockerfile_path=dockerfile_path,
        directory=docker_context,
        cmd=cmd,
    )

    docker_lambda = aws_lambda.DockerImageFunction(code=code)

    # Required for local development
    if docker_lambda.node.default_child:
        docker_lambda.node.default_child.add_metadata("DockerContext", docker_context)
        docker_lambda.node.default_child.add_metadata("Dockerfile", dockerfile_path)
        docker_lambda.node.default_child.add_metadata("SamNormalized", True)

So we are manually setting DockerContext and Dockerfile to be the same as the CDK.

And by setting SamNormalized=True metadata, we are able to bypass the current sam-cli code that is setting DockerContext and Dockerfile metadata to be different to the CDK - which would overwrite what we had manually set.

sam build works fine with our hack.

Our code (that includes the hack) will still work after the change in this PR is merged. So if a team has done a similar hack to us, their ability to test locally will not break when this PR is merged.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

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

[qingchm]

A bit concerned about this change since this will break the current code behaviour, might need to investigate on the current usage volume to decide if we can change this part.

[i18n-tribe]

might need to investigate on the current usage volume to decide if we can change this part.

Sounds like a good move 👍

this change... will break the current code behaviour

As noted, I think it's good to investigate this.

From our own investigation...

I would not describe this as breaking any current behavior.

It does change the current behavior.

But a change is only a breaking if it causes something to stop working.

So - would this change cause any currently working code to break?

We do not think it will.

To illustrate...

Say your CDK code looks like this:

code = aws_lambda.DockerImageCode.from_image_asset(
    directory="/Users/john/PycharmProjects/project-root/application/runtime/",
    file="Dockerfile",
)

docker_lambda = aws_lambda.DockerImageFunction(code=code)

This works with the current code.

And this will still work after this PR is merged.

Currently - the SAM CLI only works with CDK code where file is just the Dockerfile and directory is the full path to the Dockerfile.

In contrast, CDK code like this...

code = aws_lambda.DockerImageCode.from_image_asset(
    directory="/Users/john/PycharmProjects/project-root",
    file="./application/runtime/Dockerfile",
)

docker_lambda = aws_lambda.DockerImageFunction(code=code)

...does not currently work with the SAM CLI. But it will work when this PR is merged.

An important question would be –

Why was the code written like this to begin with?

In the PR where this code was introduced – this does not seem to have been discussed in the comments.

What this code behaviour specified in your internal tools e.g. Jira? If so, what were the reasons given?

The goal of this feature is to be compatible with the CDK.

But currently, the code is not supporting an important capability of the CDK - the ability to control the directory that assets are built from.

Both cdk deploy and sam build work fine with metadata like:

DockerContext ="/Users/john/PycharmProjects/project-root",
Dockerfile ="./application/runtime/Dockerfile",

So it is not clear to us why the code in the PR was been written to ensure sam cli always works with metadata like:

DockerContext ="/Users/john/PycharmProjects/project-root/application/runtime",
Dockerfile ="Dockerfile",

[jfuss]

So it is not clear to us why the code in the PR was been written to ensure sam cli always works with metadata like

This is how SAM CLI handles it. So it was written to match SAM CLI expectations.

[i18n-tribe]

@jfuss apologies for the slow reply, I was on vacation.

So I had a look at the SAM CLI code to see how DockerContext and Dockerfile metadata are used by the SAM CLI.

I found 4 other usages.

Just one usage is relevant to this PR:

In this code block in samcli/lib/build/app_builder.py

We read the metadata properties:

dockerfile = cast(str, metadata.get("Dockerfile"))
docker_context = cast(str, metadata.get("DockerContext"))

Then we make docker_context into an absolute path:

docker_context_dir = pathlib.Path(self._base_dir, docker_context).resolve()

Then we call docker build using the python docker client, passing the properties as arguments:

build_args = {
    "path": str(docker_context_dir),
    "dockerfile": dockerfile,
}
build_logs = self._docker_client.api.build(**build_args)

So in summary -

• Dockerfile is passed without modification to _docker_client.api.build as the dockerfile argument
• DockerContext is converted to an absolute path, and passed to _docker_client.api.build as the path argument

Here is the documentation for '_docker_client.api.build'

And those arguments are:

path (str) – Path to the directory containing the Dockerfile
dockerfile (str) – path within the build context to the Dockerfile

In my opinion, the documentation for path is a little bit ambiguous.

Does this mean that path is:

1. the immediate parent directory containing the Dockerfile?
2. a directory that ultimately contains the dockerfile, which might be nested in child folder(s)?

I would completely understand if something thinks it means 1. - this is what I thought at first.

But if we look at the documentation for dockerfile - it says path within the build context to the Dockerfile

Because dockerfile can be a path within the build context - it can be a path (potentially via subfolders) to the Dockerfile.

So in fact, 2. is the correct interpretation.

This interpretation is reinforced by the docker cli documentation for docker build

Usage is:

docker build [OPTIONS] PATH 

Where PATH will be DockerContext in the SAM CLI.

And the option --file will be Dockerfile in the SAM CLI.

In the docker cli documentation, they refer to something called the build context.

The build context is very similar to PATH (AKA DockerContext).

The build context is a copy of the folder specified in PATH (DockerContext), excluding files specified in .dockerignore.

So --file (AKA Dockerfile) is a path to a docker file within the build context (usually a relative path is used).

There is no requirement for the docker file to be in the root of the build context.

I.E. --file (Dockerfile) can be a path via subfolders to the docker file.

Here's the documentation for --file.

The documentation has examples where --file is a path via subfolders to the docker file like:

docker build --file dockerfiles/Dockerfile.debug .
docker build --file dockerfiles/Dockerfile.prod .

In the example above, PATH is . - the current directory.

And there's a quote in this section of the docs.

By default the docker build command will look for a Dockerfile at the root of the build context. 
The -f, --file, option lets you specify the path to an alternative file to use instead. 
This is useful in cases where the same set of files are used for multiple builds. 

The use case where the same set of files are used for multiple builds is exactly what this PR is trying to solve.

This is the use case described in the PR description.

This use-case is supported by the AWS CDK.

This use-case is also - in fact - already supported by the SAM CLI.

The AWS CDK docs support this use-case explicitly:

directory (str) – the directory from which the asset must be created.
file (Optional[str]) – Path to the Dockerfile (relative to the directory). Default: ‘Dockerfile’

The SAM CLI developer guide docs do not support this use-case explicitly:

Dockerfile - The name of the Dockerfile associated with the Lambda function.
DockerContext - The location of the Dockerfile.

The SAM CLI developer guide sound like interpretation 1. of the documentation for '_docker_client.api.build'.

But in fact, the SAM CLI code works perfectly well if Dockerfile is a path via subfolders to the docker file.

And I've personally used this 'undocumented feature' in multiple SAM projects I have worked on.

In other words -

What seems to have happened is...

• Someone thought that in SAM projects, Dockerfile must be a direct child of DockerContext
• SAM developer guide documented this limitation
• When SAM was integrated with CDK, somebody wrote code to enforce this limitation

The code to enforce this supposed limitation means SAM is not fully compatible with CDK.

So this PR is removing that code, to improve support for the CDK.

Also - it seems likely the supposed limitation never in fact existed, and it's always been possible in SAM for Dockerfile to be a path via subfolders to the docker file.

So I've created a PR to update the SAM CLI developer guide, so people will be aware of this capability of the SAM CLI.

I've also created a PR to update the documentation for the Docker Python Client to be more clear.

It may be possible that unclear documentation in docker-py is the original source of the belief that SAM CLI requires Dockerfile to be a direct child of DockerContext.

---

A further note -

Above there is a link do documentation for the docker cli.

The python docker SDK doesn't use the docker cli, it uses the docker api.

So I checked the docker API documentation for build

It takes an argument:

dockerfile - Path within the build context to the Dockerfile.

It also takes a tar archive which contains the build context - documented as:

Build an image from a tar archive with a Dockerfile in it.

The Dockerfile specifies how the image is built from the tar archive.
It is typically in the archive's root, but can be at a different path or have a different name by specifying the dockerfile parameter.

I.e. dockerfile 'is typically in the archive's root' - but it can be a path to a dockerfile in a subfolder.

And I checked the source code of the docker python sdk, to see how it's interacting with the docker API.

Here's the code where it creates the build context tar archive.

This is a copy of the folder specified in path (AKA DockerContext), excluding files specified in .dockerignore.

And on this line it passes Dockerfile to the docker API as dockerfile.

---

For reference, here are the 3 other usages of DockerContext metadata, which are not relevant to this PR:

• In samcli/commands/build/build_context.py - If it's a docker lambda, confirm these metadata properties are defined.
• In samcli/lib/utils/resource_trigger.py - trigger a code refresh if this metadata is changed the the developer
• In samcli/lib/providers/sam_function_provider.py - converting DockerContext to be an absolute path if necessary

[moelasmar]

Thanks @i18n-tribe for your contribution and your very detailed explanation. I think you have a point regarding this change, I think we miss understood the DockerContext and DockerFile properties and we assumed that the DockerFile should be a file in the DockerContext directory path. I did some investigations to make sure that this change will not break any customer. I think it will work as expected, and fix this bug.

my only request is to add some integration testing to cover this case

[jfuss]

@i18n-tribe I am still trying to grasp what is being changed and how that impacts the CLI. At the very least, we need integration tests added to ensure this behaves as expected. I would have to check to see if we have integ tests that cover the current integrations, but if we don't have those, we need them added. This ensures we are not breaking current customers and allows us to better regression test going further.

[mndeveci]

Thanks for your contribution @i18n-tribe. Until you will find sometime and get back to this PR, I am marking it as draft.

[i18n-tribe]

@i18n-tribe I am still trying to grasp what is being changed and how that impacts the CLI.

@jfuss perhaps it could be productive if we arrange a quick video call to discuss?

I can completely understand you not yet finding this clear.

It's quite a subtle issue, and it takes quite a lot of words to explain in writing.

But I have a feeling if we discuss it briefly in realtime, we will quickly be able to align on a common understanding.

If you have Calendly or a similar app I could use to book a meeting at a convenient time for you, I'd be very happy to do so.

We could potentially also include @sriram-mv who originally wrote the code to support docker lambdas using SAM.

In the meantime, I have written a systematic analysis of how the SAM CLI uses Dockerfile and DockerContext.

From this analysis, and from my personal experience working on multiple SAM projects, I believe the SAM CLI code does in fact support the use-case where Dockerfile is inside a nested subfolder within DockerContext.

[i18n-tribe]

Thanks for your contribution @i18n-tribe. Until you will find sometime and get back to this PR, I am marking it as draft.

@mndeveci sorry for slow reply to @jfuss 's comments, I was on vacation.

I'd be more than happy to help move this forward when you have some time - by discussing on a brief call, and if you like by helping to write the integration tests.

I think a call would be really helpful as a next step, because we do not yet have a shared understanding of what this PR is doing.

Once we have a shared understanding of the PR, it will be easier to prioritise this PR accurately - at an appropriate level of priority.

[moelasmar]

Looks good to me. Could you please add some integration testing to cover this case.

[i18n-tribe]

Looks good to me. Could you please add some integration testing to cover this case.

Thanks @moelasmar , I have added a new commit with integration testing for the case where Dockerfile is a path to a docker file via subfolders, which is useful in the case where some images use some shared code.

All tests are passing in CI. I'm not sure if this new test class is running in CI, but I have manually run it locally with the following command, and it's passing 👍

SAM_CLI_DEV=1 APPVEYOR_CI_OVERRIDE=1 BY_CANARY=1 pytest tests/integration/buildcmd/test_build_cmd.py::TestBuildCommand_PythonFunctions_ImagesWithSharedCode

[moelasmar]

Thanks @i18n-tribe for your update. Can I ask you to update the integration test case to be a CDK application and add it here or at least use a synthesized template, as the sample test case you added not really testing the metadata normalizer change you introduce.

[i18n-tribe]

@moelasmar sorry for the slow reply, work has been busy.

I agree the first tests I added do not test the CDK integration, and it would be good to test CDK integration.

Thanks for linking to the location of the CDK integration tests. I have now added some new tests which hopefully cover this.

I think the first tests I added also add value, because they demonstrate the more general point that SAM does in fact support Dockerfile being a path to a docker file via subfolders.

Thus - the first tests I added mean that this PR to update the SAM developer guide can be merged.

[i18n-tribe]

@moelasmar - all the integration tests are passing on my machine 👍

Ran with command SAM_CLI_DEV=1 pytest tests/iac_integration/cdk/test_sam_cdk_integration.py

[moelasmar]

Thanks @i18n-tribe for your contribution

[leoGalani]

Hello Folks :)

Any estimative on when this gonna be merged?

I'm having problems with my current setup where the dockerfile resides outside the root folder and the build fails because it cannot reach shared code that resides in another folder and I need to specify the docker context.

[hawflau]

LGTM and thanks for your contribution!

[hawflau]

Hey @i18n-tribe, can you please checking the failing tests? - https://ci.appveyor.com/project/AWSSAMCLI/aws-sam-cli/builds/45273404

[i18n-tribe]

Hey @i18n-tribe, can you please checking the failing tests? - https://ci.appveyor.com/project/AWSSAMCLI/aws-sam-cli/builds/45273404

Fixed in this commit @hawflau 👍