Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

Change default for LAMBDA_DEV_PORT_EXPOSE on macOS in host mode - #11280

Merged
joe4dev merged 2 commits into
masterfrom
lambda-defaults-for-macos-hostmode
Jul 31, 2024
Merged

Change default for LAMBDA_DEV_PORT_EXPOSE on macOS in host mode#11280
joe4dev merged 2 commits into
masterfrom
lambda-defaults-for-macos-hostmode

Conversation

@joe4dev

@joe4dev joe4dev commented Jul 29, 2024

Copy link
Copy Markdown
Member

Motivation

Lambda won't work in host mode on macOS without setting LAMBDA_DEV_PORT_EXPOSE=1 (introduced in #7367), which is a bad developer experience.
So far, we opted for explicit configuration developer documentation here to unblock developers on macOS in host mode.
Several contributors hit this issue, and it was suggested that we set it by default if we detect LocalStack running in host mode on macOS. The Lambda service owners also discussed this in the last Lambda Service Owner Sync (2024-07-15).

Changes

Testing

Our test suite should capture any regressions. Anything else to consider? Is there a special environment where the assumption not is_in_docker == host mode does not apply (e.g., K8) @dfangl @simonrw?

Discussion

Options considered:

  • Option A: simply evaluate based on is_in_docker and the system platform, assuming not is_in_docker == host mode.
    • Mixing up config logic with production code makes config bugs more likely and leads to more complex config code
  • Option B: introduce some explicit host mode LS devs should set for sensible default config
    • Could separate special host mode defaults from generic production defaults

@joe4dev joe4dev added the semver: patch Non-breaking changes which can be included in patch releases label Jul 29, 2024
@joe4dev joe4dev added this to the 3.7 milestone Jul 29, 2024
@joe4dev joe4dev self-assigned this Jul 29, 2024
@joe4dev
joe4dev requested a review from thrau as a code owner July 29, 2024 19:32
@github-actions

github-actions Bot commented Jul 29, 2024

Copy link
Copy Markdown

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   3m 32s ⏱️ +2s
405 tests ±0  353 ✅ ±0   52 💤 ±0  0 ❌ ±0 
810 runs  ±0  706 ✅ ±0  104 💤 ±0  0 ❌ ±0 

Results for commit ae3a0c5. ± Comparison against base commit 482755a.

@github-actions

github-actions Bot commented Jul 29, 2024

Copy link
Copy Markdown

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 34m 40s ⏱️ - 1m 54s
3 303 tests ±0  2 909 ✅ ±0  394 💤 ±0  0 ❌ ±0 
3 305 runs  ±0  2 909 ✅ ±0  396 💤 ±0  0 ❌ ±0 

Results for commit ae3a0c5. ± Comparison against base commit 482755a.

@gregfurman

gregfurman commented Jul 30, 2024

Copy link
Copy Markdown
Contributor

Nice change! Tested locally running on host mode (without explicitly enabling the flag) on master and the lambda-defaults-for-macos-hostmode feature branch and the dev ports expose successfuly by default with these changes.

💻 I'm running Darwin 23.3.0 arm64

dfangl
dfangl approved these changes Jul 30, 2024
View reviewed changes

@dfangl dfangl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is_in_docker will also be True if in kubernetes, it is really just a variable whether we are running in a container or not.

I think the taken approach is fine, but there are now some duplicated methods with utils/platform.py. I understand it is not that easy to use them, but it might be worth consolidating them anyway in the future.

@joe4dev

joe4dev commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

is_in_docker will also be True if in kubernetes, it is really just a variable whether we are running in a container or not.

I think the taken approach is fine, but there are now some duplicated methods with utils/platform.py. I understand it is not that easy to use them, but it might be worth consolidating them anyway in the future.

Yeah, I know, and also small inconsistencies (e.g., lower(), is_mac_os()). I thought that was intentional because config should be self-contained for bootstrapping 🤷

simonrw
simonrw reviewed Jul 30, 2024
View reviewed changes

@simonrw simonrw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this change is sensible.

Regarding k8s behaviour: I think the same problem will be true for k8s as it is in Docker, but only when LS is being run in host mode. E.g. for kind the k8s "nodes" are Docker containers, therefore the same problem.

However we implicitly not really supporting that use case right now. We are focused on when LS is deployed in the k8s cluster as a pod, in which case there is no problem as all pods ports are accessible by other pods (with minor restrictions).

So in summary: I think we don't have to account for k8s behaviour specifically here.

@joe4dev
joe4dev merged commit 5d58c8e into master Jul 31, 2024
@joe4dev
joe4dev deleted the lambda-defaults-for-macos-hostmode branch July 31, 2024 08:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dominikschubert dominikschubert Awaiting requested review from dominikschubert
@thrau thrau Awaiting requested review from thrau
2 more reviewers
@simonrw simonrw simonrw left review comments
@dfangl dfangl dfangl approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@joe4dev joe4dev

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

3.7

Development

Successfully merging this pull request may close these issues.

4 participants

@joe4dev @gregfurman @simonrw @dfangl