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

Add dev config flag for fixing lambda invoke on macOS in host mode #7367

Merged
merged 4 commits into from Dec 23, 2022

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Dec 20, 2022

Co-authored by @dominikschubert

Problem

The new lambda provider requires bi-directional communication (host <=> lambda container), which breaks host mode in macOS because the lambda container is not reachable via IPv4 in bridge mode.
Hence, invoking any lambda fails while the lambda container waits for an invocation (i.e., during "blocking the credentials service") after a connection timeout with the error Connection to 172.17.0.2 timed out.

Enable new lambda provider: PROVIDER_OVERRIDE_LAMBDA=asf

Relevant test case: tests.integration.awslambda.test_lambda.TestLambdaFeatures.test_invocation_type_request_response

Background

Docker Desktop can’t route traffic to Linux containers. However, if you’re a Windows user, you can ping the Windows containers. (Source: Docker I cannot ping my containers)

There is also a long discussion thread about this issue in the moby project.

LocalStack Workarounds

  • Start LS in a container with bind-mounted code and run tests against it using TEST_SKIP_LOCALSTACK_START=1
    • ➖  Debugging doesn’t work
    • ➖ Tests that monkey-patch stuff don’t work
  • Execute test in container TEST_PATH="tests/integration/awslambda/test_lambda_common.py" make test-docker-mount
    • ➖  cannot install moto using pip install -e
      ⇒ delete -v $$PACKAGES_DIR/moto:/opt/code/localstack/.venv/lib/python3.10/site-packages/moto/ from test-docker-mount-code in the Makefile

Thanks, @dfangl for the hints 🙏

Potential macOS networking Workarounds (not tested)

Suggested Changes

Introduce a dev config flag LAMBDA_DEV_PORT_EXPOSE and conditionally expose a free TCP port in the lambda container.

@joe4dev joe4dev requested a review from dfangl December 20, 2022 14:06
@joe4dev joe4dev temporarily deployed to localstack-ext-tests December 20, 2022 14:06 — with GitHub Actions Inactive
@joe4dev joe4dev marked this pull request as draft December 20, 2022 14:06
@joe4dev joe4dev added aws:lambda AWS Lambda platform: arm64/aarch64 Issues related to Apple Silicon labels Dec 20, 2022
@joe4dev joe4dev changed the title Add dev config flag for fixing lambda invoke on macOS Add dev config flag for fixing lambda invoke on macOS in host mode Dec 20, 2022
@coveralls
Copy link

coveralls commented Dec 20, 2022

Coverage Status

Coverage decreased (-0.0008%) to 84.844% when pulling 2385ef2 on fix-lambda-invoke-dev-mode-macos into 9cd9c68 on master.

@github-actions
Copy link

github-actions bot commented Dec 20, 2022

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 23m 8s ⏱️ - 11m 16s
1 576 tests +2  1 302 ✔️ +1  274 💤 +1  0 ±0 
2 222 runs  +4  1 670 ✔️ +1  552 💤 +3  0 ±0 

Results for commit 2385ef2. ± Comparison against base commit 9cd9c68.

♻️ This comment has been updated with latest results.

@joe4dev joe4dev marked this pull request as ready for review December 21, 2022 08:11
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor nits!

@@ -208,6 +211,9 @@ def _build_executor_endpoint(self, service_endpoint: ServiceEndpoint) -> Executo
self.id,
)
executor_endpoint = ExecutorEndpoint(self.id, service_endpoint=service_endpoint)
if config.LAMBDA_ASF_DEV_PORT_EXPOSE:
executor_endpoint.container_address = "localhost"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to set self.ip as well, so the contract for get_address is not broken.
We are not currently using it, but still.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 👁️
I grouped the container_address assignment at the bottom for better readability.

@@ -208,6 +211,9 @@ def _build_executor_endpoint(self, service_endpoint: ServiceEndpoint) -> Executo
self.id,
)
executor_endpoint = ExecutorEndpoint(self.id, service_endpoint=service_endpoint)
if config.LAMBDA_ASF_DEV_PORT_EXPOSE:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge that whole if block with the first one in start? It seems unnecessary that this happens at object creation time. We set the executor endpoint IP in the not-port-expose path in start() as well.
Especially get_free_tcp_port can take some time, so I would rather have it in the start method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes a lot of sense and reduces clutter because the constructor can stay generic this way

@joe4dev joe4dev removed aws:lambda AWS Lambda platform: arm64/aarch64 Issues related to Apple Silicon labels Dec 21, 2022
@joe4dev joe4dev temporarily deployed to localstack-ext-tests December 22, 2022 10:22 — with GitHub Actions Inactive
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor thing again (to set self.ip correctly), then its good to go from my side!

Co-authored-by: Daniel Fangl <daniel.fangl@localstack.cloud>
@joe4dev joe4dev temporarily deployed to localstack-ext-tests December 22, 2022 10:41 — with GitHub Actions Inactive
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work tackling this!

@joe4dev joe4dev temporarily deployed to localstack-ext-tests December 22, 2022 13:53 — with GitHub Actions Inactive
@joe4dev
Copy link
Member Author

joe4dev commented Dec 22, 2022

Updated naming as discussed:

  • Omit ASF as the new lambda provider becomes default in the next major release
  • Keep DEV to emphasize that its a feature flag for LS developers only
  • Keep EXPOSE to clarify its meaning as a boolean flag (rather than indicate a manual port setting)

@joe4dev joe4dev merged commit 6c92dd6 into master Dec 23, 2022
@joe4dev joe4dev deleted the fix-lambda-invoke-dev-mode-macos branch December 23, 2022 08:22
@whummer
Copy link
Member

whummer commented Dec 27, 2022

Just gave this a try on my Mac in host mode - works like a charm, great job @joe4dev 🚀

@joe4dev
Copy link
Member Author

joe4dev commented Dec 27, 2022

Glad to hear it works 👍
Thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants