feat: add option --container-host to commands local start-api, local start-lambda and local invoke by xazhao · Pull Request #2700 · aws/aws-sam-cli

xazhao · 2021-03-10T23:42:52Z

Add an option --container-host to commands local start-api, local start-lambda and local invoke. Customers will be able to specify the host of local emulated containers to support more use cases.

Which issue(s) does this change fix?

#2492 #2436

Why is this change necessary?

Some customers are having issues to run sam local commands within containers.

How does it address the issue?

After some investigation, this line is the root cause that SAM CLI can not be run within containers. The localhost can not be recognized by the system when running in a container. With the new option --container-host, customers will be able to specify host of the local emulated container (e.g. host.docker.internal in Docker).
If the new option is not set, localhost will be used as default so it won't have impact on current use cases.

What side effects does this change have?

Checklist

Add input/output type hints to new functions/methods
Write design document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
I didn't write integration tests for this feature. Basically the new option only make a small change to the URL of the container and unit tests show the correct URL is being called. It's very complicated to write integration test to simulate real use case since the feature will only be used when running in a container. Therefore I believe unit tests are sufficient for this change.
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.

samcli/local/docker/container.py

samcli/commands/local/start_api/cli.py

aahung · 2021-03-11T01:50:59Z

I have a hard time coming up with a reasonable cli option name for this.

I wonder if this is the approach we should go. What are the possible values customers might specify using --localhost? If there are only 1 possibility other than localhost (host.docker.internal), maybe we can get away with a boolean flag instead of a string cli argument, something like --listen-to-docker-internal-localhost-hostname (way too verbose of course).

Or even further, we can detect whether host.docker.internal is resolvable and use it by default (we need to know what's the side effect).

xazhao · 2021-03-11T18:47:21Z

As far as I know, docker.for.mac.localhost is another possible value. I think host.docker.internal would cover most cases though. It's a trade off between ease of use vs universal. I'm just thinking if there is another value that we don't know currently, we need to make this change again. So letting user specify the container host seems better to me. Let me know your opinion.

hoffa · 2021-03-11T19:07:35Z

All communication with the Docker daemon happens through socket, and technically the listening end (hence the containers started by the daemon) could be anywhere, so the address the containers are listening on could also be anywhere.

vitorfec · 2021-03-11T21:58:44Z

All communication with the Docker daemon happens through socket, and technically the listening end (hence the containers started by the daemon) could be anywhere, so the address the containers are listening on could also be anywhere.

This is exactly my problem. I have the daemon on a different machine and need to specify that as the host

tests/unit/local/lambdafn/test_runtime.py

sriram-mv · 2021-03-17T19:59:28Z

samcli/commands/local/cli_common/options.py

+        click.option(
+            "--container-host",
+            default="localhost",
+            help="Host of locally emulated Lambda container",


This help text should be more verbose and explain what are potential options, and when it needs to be specified. Looking at this from the user perspective, we want this to be clear/

Sure! I'm thinking Host of locally emulated Lambda container. This option can be used for running SAM CLI in containers e.g. If you want to run SAM CLI in a Docker container, use this option with host.docker.internal. For SAM CLI running on your local machine, this option is not needed. This should be clear for customers who want to use this option. Let me know your thought.

host.docker.internal works in some situations such as for macOS, but doesn't unconditionally resolve to the actual machine running Docker out of the box. See docker/for-linux#264

Even if SAM CLI is running locally (not inside a container), the Docker daemon could be running remotely (see e.g. #2700 (comment)), requiring this argument.

Mentioning host.docker.internal is probably fine, but the main point is that this option is useful when the container ports aren't exposed on SAM CLI's localhost, regardless of where SAM CLI may run.

Yes you're right. The above message could be a little misleading for some use cases. I will try to make the help text more general with a detailed example like host.docker.internal.

Host of locally emulated Lambda container. This option can be used when the container ports aren't exposed on SAM CLI's localhost. e.g. If you want to run SAM CLI in a Docker container on Mac OS, use this option with host.docker.internal.
How is this help text?

I like it! I'd maybe amend a little bit to something like this:

Host of locally emulated Lambda container. This option is useful when the container ports aren't exposed on the same host SAM CLI runs on. For example, if you want to run SAM CLI in a Docker container on macOS, use this option with host.docker.internal.

Talked to Chris and we feel this looks better for customers to understand and it's pretty clear too:

Host of locally emulated Lambda container. This option is useful when the containers run on a different host than SAM CLI. For example, if you want to run SAM CLI in a Docker container on macOS, use this option with host.docker.internal

samcli/commands/local/cli_common/options.py

tests/unit/local/docker/test_container.py

aahung

It seems container_host can never be None because it has a default value in click. Therefore, the Optional for this might be misleading.

samcli/commands/local/cli_common/options.py

aahung · 2021-03-19T00:13:12Z

samcli/local/docker/container.py

        :param docker_client: Optional, a docker client to replace the default one loaded from env
        :param container_opts: Optional, a dictionary containing the container options
        :param additional_volumes: Optional list of additional volumes
+        :param string container_host: Optional. Host of locally emulated Lambda container


this cannot be optional anymore, because if customers do not specify a value, click will use the default value localhost.

Same reason as my another comment below.

aahung · 2021-03-19T00:13:36Z

samcli/local/docker/container.py

        docker_client=None,
        container_opts=None,
        additional_volumes=None,
+        container_host=None,


same here, this is not optional

_init_ here is also used in another its child class LambdaBuildContainer. The new option is not used in that class so I make it as optional.

if this is used by other classes (i'm assuming those other classes are instantiated outside of sam local suite, is it sam build?) , should we set a default value? this can get overridden when the actual option is used.

As far as I know, container_host is only used in SAM LOCAL suite when one of its child class is instantiated. So whether or not setting the default value here makes no difference. If we could potentially expand the use of new option outside of sam local suite, then yes we should set default to localhost here. Considering there is no negative impact setting the default value, I will add the default value to the option here.

sriram-mv · 2021-03-19T19:03:47Z

samcli/local/docker/container.py

        docker_client=None,
        container_opts=None,
        additional_volumes=None,
+        container_host=None,


if this is used by other classes (i'm assuming those other classes are instantiated outside of sam local suite, is it sam build?) , should we set a default value? this can get overridden when the actual option is used.

aahung

Although we store localhost (the default value) in two different places, it is acceptable to me due to it is very unlikely to change. Thanks for addressing the comments.

samcli/commands/local/cli_common/options.py

Co-authored-by: _sam <3804518+aahung@users.noreply.github.com>

tests/unit/commands/local/lib/test_local_lambda.py

hoffa

LGTM! 👍

mndeveci

Great feature, thanks for the implementation 🎉

Can we also add some integration test cases to validate this new parameter as well?

mndeveci · 2021-03-23T00:41:45Z

samcli/commands/local/lib/local_lambda.py

        aws_region: Optional[str] = None,
        env_vars_values: Optional[Dict[Any, Any]] = None,
        debug_context: Optional[DebugContext] = None,
+        container_host: Optional[str] = None,


Since there is a default value for the parameter, would this param be None anytime?

This param won't be None in the current use case. The reasons I set it to None:

I'd like to have minimum impact on the current use case. If this class is initialized somewhere else, we shouldn't assume it's localhost unless it's passed from cli.

Match to the overall style.

mndeveci · 2021-03-23T00:44:39Z

samcli/commands/local/cli_common/options.py

-        )
+        ),
+        click.option(
+            "--container-host",


For any security reasons, do we need to validate if this option valid?

This is a good point. Let me check on this and get back to you.

For security reasons, there is nothing we can validate. There is a use case that docker daemon is running on a different host and the customer wants to use that host. We won't be able to understand if it's a good host provided by customer or just a bad actor.

xazhao · 2021-03-23T01:16:20Z

For the integration test, please see my explanation in the summary.

I didn't write integration tests for this feature. Basically the new option only make a small change to the URL of the container and unit tests show the correct URL is being called. It's very complicated to write integration test to simulate real use case since the feature will only be used when running in a container. Therefore I believe unit tests are sufficient for this change.

…start-lambda and local invoke (aws#2700)

…, local start-lambda and local invoke (#2700)" (#2794) This reverts commit e653fe2. Co-authored-by: Xia Zhao <xazhao@amazon.com>

hanneswidrig · 2021-04-12T17:36:06Z

Why did this get reverted in master?

xazhao · 2021-04-12T18:18:28Z

@hanneswidrig We found a bug when using --container-host with warm containers. We're preparing the PR fixing the bug. Also --container-host doesn't work on remote Docker host, we want to support that use case as well.

…, local start-lambda and local invoke (aws#2700)" (aws#2794) This reverts commit e653fe2. Co-authored-by: Xia Zhao <xazhao@amazon.com>

Add local start-api option --localhost

3b302a2

xazhao marked this pull request as draft March 11, 2021 00:02

hoffa suggested changes Mar 11, 2021

View reviewed changes

samcli/local/docker/container.py Outdated Show resolved Hide resolved

samcli/commands/local/start_api/cli.py Outdated Show resolved Hide resolved

hoffa changed the title ~~WIP Feat:Add local start-api option --localhost to make it run in Docker~~ WIP feat: add local start-api option --localhost to make it run in Docker Mar 11, 2021

xazhao and others added 3 commits March 15, 2021 17:16

Resolve comments and expand the option to start-lambda and invoke

8b862e4

Merge branch 'develop' into develop

2364efe

Merge branch 'develop' into develop

5238725

xazhao changed the title ~~WIP feat: add local start-api option --localhost to make it run in Docker~~ WIP feat: Add option --container-host to commands local start-api, local start-lambda and local invoke Mar 17, 2021

xazhao added 2 commits March 17, 2021 10:24

Update unit tests and change option help

48b3cf9

Update some args description

f8d9217

hoffa reviewed Mar 17, 2021

View reviewed changes

tests/unit/local/lambdafn/test_runtime.py Show resolved Hide resolved

sriram-mv suggested changes Mar 17, 2021

View reviewed changes

xazhao marked this pull request as ready for review March 17, 2021 21:25

hoffa changed the title ~~WIP feat: Add option --container-host to commands local start-api, local start-lambda and local invoke~~ feat: add option --container-host to commands local start-api, local start-lambda and local invoke Mar 17, 2021

hoffa reviewed Mar 17, 2021

View reviewed changes

samcli/commands/local/cli_common/options.py Show resolved Hide resolved

update help text and add showing default

28778bd

hoffa reviewed Mar 18, 2021

View reviewed changes

samcli/commands/local/cli_common/options.py Show resolved Hide resolved

hoffa reviewed Mar 18, 2021

View reviewed changes

tests/unit/local/docker/test_container.py Outdated Show resolved Hide resolved

xazhao and others added 2 commits March 18, 2021 15:05

remove redundant unit test

b98bf73

Merge branch 'develop' into develop

21b3b6b

aahung reviewed Mar 19, 2021

View reviewed changes

sriram-mv suggested changes Mar 19, 2021

View reviewed changes

change default value of the option and fix typo

33e0aa1

aahung approved these changes Mar 20, 2021

View reviewed changes

samcli/commands/local/cli_common/options.py Outdated Show resolved Hide resolved

xazhao and others added 2 commits March 19, 2021 17:18

Fix typo in samcli/commands/local/cli_common/options.py

3814f39

Co-authored-by: _sam <3804518+aahung@users.noreply.github.com>

Merge branch 'develop' into develop

f039f2d

sriram-mv approved these changes Mar 22, 2021

View reviewed changes

hoffa reviewed Mar 22, 2021

View reviewed changes

tests/unit/commands/local/lib/test_local_lambda.py Show resolved Hide resolved

hoffa approved these changes Mar 22, 2021

View reviewed changes

CoshUS added the pr/internal label Mar 22, 2021

mndeveci reviewed Mar 23, 2021

View reviewed changes

mndeveci approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'develop' into develop

a236b09

hoffa merged commit e653fe2 into aws:develop Mar 23, 2021

elbayaaa pushed a commit to elbayaaa/aws-sam-cli that referenced this pull request Mar 25, 2021

feat: add option --container-host to commands local start-api, local …

af6528f

…start-lambda and local invoke (aws#2700)

kstro21 mentioned this pull request Mar 31, 2021

Testing Api Gateway endpoint locally returning No response from invoke container and Invalid lambda response received in Dockerised sam app #2492

Closed

xazhao mentioned this pull request Apr 6, 2021

Revert "feat: add option --container-host to commands local start-api, local start-lambda and local invoke (#2700)" #2794

Merged

8 tasks

qingchm pushed a commit that referenced this pull request Apr 6, 2021

Revert "feat: add option --container-host to commands local start-api…

8c1cbfc

…, local start-lambda and local invoke (#2700)" (#2794) This reverts commit e653fe2. Co-authored-by: Xia Zhao <xazhao@amazon.com>

xazhao mentioned this pull request Apr 13, 2021

feat: add option --container-host and --container-host-interface to sam local commands #2806

Merged

8 tasks

Conversation

xazhao commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Checklist

Uh oh!

Uh oh!

Uh oh!

aahung commented Mar 11, 2021

Uh oh!

xazhao commented Mar 11, 2021

Uh oh!

hoffa commented Mar 11, 2021

Uh oh!

vitorfec commented Mar 11, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aahung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aahung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hoffa left a comment

Choose a reason for hiding this comment

Uh oh!

mndeveci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xazhao commented Mar 23, 2021

Uh oh!

hanneswidrig commented Apr 12, 2021

Uh oh!

xazhao commented Mar 10, 2021 •

edited

Loading