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.

Sign up for GitHub

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

Jump to bottom

Resolve LOCALSTACK_HOST to the LocalStack container #9178

Merged
merged 6 commits into from Oct 4, 2023
Merged

Resolve LOCALSTACK_HOST to the LocalStack container #9178

merged 6 commits into from Oct 4, 2023

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Sep 19, 2023

Motivation

The user may specify a custom value of LOCALSTACK_HOST which is the domain name of the LocalStack container from the outside. This will not work however in created compute environments, or external docker containers since we only resolve localhost.localstack.cloud to the LocalStack container.

Changes

  • Bind LOCALSTACK_HOST to LocalStack IP address
  • Update bootstrap tests to use stream_container_logs and ContainerConfigurators

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Sep 19, 2023
@simonrw simonrw self-assigned this Sep 19, 2023
@simonrw simonrw changed the title Resolve LOCALSTACK_HOST to the container Resolve LOCALSTACK_HOST to the LocalStack container Sep 19, 2023
simonrw
simonrw commented Sep 19, 2023
View reviewed changes
localstack/dns/server.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 19, 2023 •
edited

Coverage Status

coverage: 83.083% (+0.08%) from 83.007% when pulling 39ead70 on dns/localstack-host into 9b62617 on master.

@github-actions
Copy link

github-actions bot commented Sep 19, 2023 •
edited

LocalStack Community integration with Pro

       2 files         2 suites   1h 11m 41s ⏱️
2 217 tests 1 722 ✔️ 495 💤 0 ❌
2 218 runs  1 722 ✔️ 496 💤 0 ❌

Results for commit 39ead70.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review September 19, 2023 10:06
@simonrw simonrw added this to the Playground milestone Sep 25, 2023
dfangl
dfangl approved these changes Sep 26, 2023
View reviewed changes
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.

I think this is a sensible addition!

@simonrw simonrw marked this pull request as draft September 28, 2023 09:44
@simonrw simonrw marked this pull request as ready for review October 3, 2023 15:50
@simonrw simonrw merged commit 5bdde1f into master Oct 4, 2023
26 checks passed
@simonrw simonrw deleted the dns/localstack-host branch October 4, 2023 08:46
@joe4dev
Copy link
Member

joe4dev commented Oct 4, 2023

@simonrw Does this resolve the following networking issue with SNS/Lambda #8931?
It looks like a networking issue with external Docker containers trying to access LocalStack 🤔

@LordAndreasOtten LordAndreasOtten mentioned this pull request Oct 4, 2023
Closed
1 task
@LordAndreasOtten
Copy link

What seems to be happening withing sns is -> subscribe request will be send correct (interal and remote) but it gets. a response url back with 127.0.0.1:4556 instead of host:4566 and because of this the external/other container cant subscribe :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfangl dfangl dfangl approved these changes

Assignees

@simonrw simonrw

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
Playground
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@simonrw @coveralls @joe4dev @LordAndreasOtten @dfangl