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

Migrate DNS server to community #9049

Merged
merged 46 commits into from Sep 8, 2023
Merged

Migrate DNS server to community #9049

merged 46 commits into from Sep 8, 2023

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Sep 1, 2023

Motivation

We want to simplify network configuration. A point of confusion is that localhost.localstack.cloud is publically registered, and resolves to 127.0.0.1. When communicating between docker containers, this is an issue - trying to connect to e.g. an opensearch domain with url *.localhost.localstack.cloud returned by LocalStack will attempt to connect to the "application" container, rather than LocalStack.

Recently we have upgraded the DNS server provided by LocalStack Pro to dynamically resolve the IP address of the LocalStack container based on request subnet, but this functionality was only available in Pro.

With this PR we bring the DNS server from Pro to LocalStack community. This change brings along with it increased test coverage.

Changes

  • Move the DNS server to community
  • Add bootstrap tests covering the DNS server behaviour
  • Start the DNS server regardless of whether the host port can be bound
  • If port 53 (or DNS_PORT) can be bound on the host, publish the docker port using the CLI
  • Move the DNS server module from localstack.services.dns_server to localstack.dns.server to group core resources (e.g. http, dns, etc.) at the top of the tree
  • Remove broken attempts to configure the DNS of the host

Testing

  • Start LocalStack: DNS_ADDRESS=127.0.0.1 localstack start
  • If port 53/udp is available on the host, then run dig @127.0.0.1 localhost.localstack.cloud and a valid response is returned
  • Start a container in the same docker network/subnet as the LocalStack container, then run from that container dig @<ip of localstack container> localhost.localstack.cloud and the IP address of the container should be returned
  • Remove broken attempts to configure the host DNS

TODO

What's left to do:

  • Get -ext PR ready to merge

@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Sep 1, 2023
@simonrw simonrw self-assigned this Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 23m 18s ⏱️
2 171 tests 1 673 ✔️ 498 💤 0
2 172 runs  1 673 ✔️ 499 💤 0

Results for commit b7a961f.

♻️ This comment has been updated with latest results.

@simonrw
Copy link
Contributor Author

simonrw commented Sep 6, 2023

podman tests have been broken for a while

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.

Already looks good, just some points for discussion

localstack/config.py Show resolved Hide resolved
localstack/utils/bootstrap.py Outdated Show resolved Hide resolved
localstack/dns/server.py Outdated Show resolved Hide resolved
localstack/dns/server.py Show resolved Hide resolved
localstack/utils/docker_utils.py Show resolved Hide resolved
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.

Thanks for adding the CODEOWNERS file, forgot to mention that. LGTM, great work!

localstack/utils/bootstrap.py Outdated Show resolved Hide resolved
@simonrw simonrw changed the title Migrate DNS server to community Migrate DNS server to community (DO NOT MERGE) Sep 7, 2023
@simonrw simonrw marked this pull request as draft September 7, 2023 15:02
@simonrw simonrw changed the title Migrate DNS server to community (DO NOT MERGE) Migrate DNS server to community Sep 7, 2023
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.

🎉

@simonrw simonrw marked this pull request as ready for review September 8, 2023 14:27
@simonrw simonrw merged commit 1782c00 into master Sep 8, 2023
28 of 29 checks passed
@simonrw simonrw deleted the dns/move-to-community branch September 8, 2023 14:33
serialbandicoot pushed a commit to serialbandicoot/localstack that referenced this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants