Add new option to manually expose the DNS port to the host, remove binding by default #11011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…xposure of the DNS port
dfangl
added
the
semver: minor
dfangl
commented
Jun 12, 2024
tests/cli/test_cli.py
Outdated
Comment on lines
266
to
296
| @pytest.mark.skip(reason="For this change, the tests run on the previous image") | ||
| def test_dns_server_starts_if_host_port_bound( | ||
| self, runner, container_client, monkeypatch, dns_query_from_container | ||
| ): | ||
| port = get_free_udp_port() | ||
| monkeypatch.setenv("DEBUG", "1") | ||
| monkeypatch.setenv("DNS_PORT", str(port)) | ||
| monkeypatch.setattr(config, "DNS_PORT", port) | ||
|
|
||
| # bind the port | ||
| s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) | ||
| s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| s.bind(("", port)) | ||
|
|
||
| with closing(s): | ||
| runner.invoke(cli, ["start", "-d"]) | ||
| runner.invoke(cli, ["wait", "-t", "60"]) | ||
|
|
||
| inspect = container_client.inspect_container(config.MAIN_CONTAINER_NAME) | ||
| assert f"{port}/udp" not in inspect["HostConfig"]["PortBindings"] | ||
| ip_address = list(inspect["NetworkSettings"]["Networks"].values())[0]["IPAddress"] | ||
|
|
||
| with pytest.raises(TimeoutError): | ||
| send_dns_query(name=LOCALHOST_HOSTNAME, port=port) | ||
|
|
||
| # use a docker container to test the DNS | ||
| stdout, _ = dns_query_from_container( | ||
| name=LOCALHOST_HOSTNAME, ip_address=ip_address, port=port | ||
| ) | ||
| assert ip_address in stdout.decode().splitlines() | ||
|
|
There was a problem hiding this comment.
Removed this test as it does not make sense anymore to test the "fallback if DNS port is not available" case.
alexrashed
approved these changes
Jun 12, 2024
There was a problem hiding this comment.
The changes are looking good to me! I only found a small nitpick concerning the tests.
Since we kind of have to react to these changes in Docker Desktop, I think the change is reasonable, and given the (presumably 😉) small usage, it's fair to declare this as a minor change.
Concerning your two discussion points:
- No separate bind address configuration: I would say this is something we can introduce if there's the need for it.
- Falling back to the Docker bind exception due to the deleted check: I think this is fair, since this feature now has to be explicitly enabled.
Thanks for jumping on this! 🦸🏽 It's always DNS...
…er test to make it more explicit
1 task
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
With recent Docker Desktop changes, port 53 is not exposable on the host from docker any more, as the port is already taken. Due to the specific error circumstances, our check whether or not we can expose it does not work any more.
As a fix, this PR will remove the automatic binding of the DNS port to the host, as this a rarely used feature (presumably), since it requires additional configuration to make proper use of it (like setting
DNS_SERVERto avoid loops in the resolution).This PR introduces a new flag
--host-dnsinstead, which will expose the port to the host.The flag has two shortcomings we should discuss:
GATEWAY_LISTENsets, so per default 127.0.0.1. It will not be seperately configureable, you can only change it for both the gateway and dns together.Changes
--host-dnsto force binding port 53 (or whatever theDNS_PORTconfig variable is set to) on the host