Improve DNS server handling in DHCP handlers

* Set router address as DNS server instead of loopback addresses
* Enable dnsmasq DNS service if possible and add DNS servers otherwise

CHANGELOG.md

@@ -5,6 +5,8 @@
 ### New features
 
 * Audio profile switcher in applet menu (@abhijeetviswa)
+* Set router address as DNS server instead of loopback addresses
+* Enable dnsmasq DNS service if possible and add DNS servers otherwise
 
 ### Changes
 

blueman/main/NetConf.py

@@ -1,6 +1,7 @@
 import errno
 import os
 import ipaddress
+import socket
 from tempfile import mkstemp
 from time import sleep
 import logging
@@ -62,7 +63,9 @@ def _pid_path(self) -> str:
         return f"/var/run/{self._key}.pan1.pid"
 
     def apply(self, ip4_address: str, ip4_mask: str) -> None:
-        error = self._start(_get_binary(*self._BINARIES), ip4_address, ip4_mask)
+        error = self._start(_get_binary(*self._BINARIES), ip4_address, ip4_mask,
+                            [ip4_address if addr.is_loopback else str(addr)
+                             for addr in DNSServerProvider.get_servers()])
         if error is None:
             logging.info(f"{self._key} started correctly")
             with open(self._pid_path) as f:
@@ -74,7 +77,7 @@ def apply(self, ip4_address: str, ip4_mask: str) -> None:
             logging.info(error_msg)
             raise NetworkSetupError(f"{self._key} failed to start: {error_msg}")
 
-    def _start(self, binary: str, ip4_address: str, ip4_mask: str) -> Optional[bytes]:
+    def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> Optional[bytes]:
         ...
 
     def clean_up(self) -> None:
@@ -106,13 +109,17 @@ def _clean_up_configuration(self) -> None:
 class DnsMasqHandler(DHCPHandler):
     _BINARIES = ["dnsmasq"]
 
-    def _start(self, binary: str, ip4_address: str, ip4_mask: str) -> Optional[bytes]:
+    def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> Optional[bytes]:
         ipiface = ipaddress.ip_interface('/'.join((ip4_address, ip4_mask)))
-        cmd = [binary, "--port=0", f"--pid-file={self._pid_path}", "--except-interface=lo",
+        cmd = [binary, f"--pid-file={self._pid_path}", "--except-interface=lo",
                "--interface=pan1", "--bind-interfaces",
                f"--dhcp-range={ipiface.network[2]},{ipiface.network[-2]},60m",
                f"--dhcp-option=option:router,{ip4_address}"]
 
+        with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
+            if s.connect_ex(("localhost", 53)) == 0:
+                cmd += ["--port=0", f"--dhcp-option=option:dns-server,{', '.join(dns_servers)}"]
+
         logging.info(cmd)
         p = Popen(cmd, stderr=PIPE)
 
@@ -162,20 +169,20 @@ def _read_dhcp_config() -> Tuple[str, str]:
         return dhcp_config, existing_subnet
 
     @staticmethod
-    def _generate_subnet_config(ip4_address: str, ip4_mask: str) -> str:
+    def _generate_subnet_config(ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> str:
         ipiface = ipaddress.ip_interface('/'.join((ip4_address, ip4_mask)))
 
         return DHCPDSUBNET % {"ip_mask": ipiface.network.network_address,
                               "netmask": ipiface.netmask,
-                              "dns": ', '.join(str(addr) for addr in DNSServerProvider.get_servers()),
+                              "dns": ', '.join(dns_servers),
                               "rtr": ipiface.ip,
                               "start": ipiface.network[2],
                               "end": ipiface.network[-2]}
 
-    def _start(self, binary: str, ip4_address: str, ip4_mask: str) -> Optional[bytes]:
+    def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> Optional[bytes]:
         dhcp_config, existing_subnet = self._read_dhcp_config()
 
-        subnet = self._generate_subnet_config(ip4_address, ip4_mask)
+        subnet = self._generate_subnet_config(ip4_address, ip4_mask, dns_servers)
 
         with open(DHCP_CONFIG_FILE, "w") as f:
             f.write(dhcp_config)
@@ -207,20 +214,20 @@ def _clean_up_configuration(self) -> None:
 class UdhcpdHandler(DHCPHandler):
     _BINARIES = ["udhcpd"]
 
-    def _generate_config(self, ip4_address: str, ip4_mask: str) -> str:
+    def _generate_config(self, ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> str:
         ipiface = ipaddress.ip_interface('/'.join((ip4_address, ip4_mask)))
 
         return UDHCP_CONF_TEMPLATE % {"ip_mask": ipiface.network.network_address,
-                                      "dns": ', '.join(str(addr) for addr in DNSServerProvider.get_servers()),
+                                      "dns": ', '.join(dns_servers),
                                       "rtr": ipiface.ip,
                                       "start": ipiface.network[2],
                                       "end": ipiface.network[-2],
                                       "pid_path": self._pid_path}
 
-    def _start(self, binary: str, ip4_address: str, ip4_mask: str) -> Optional[bytes]:
+    def _start(self, binary: str, ip4_address: str, ip4_mask: str, dns_servers: List[str]) -> Optional[bytes]:
         config_file, self._config_path = mkstemp(prefix="udhcpd-")
-        os.write(config_file, self._generate_config(ip4_address, ip4_mask).encode('UTF-8'))
-        os.close(config_file)
+        with open(config_file, "w", encoding="utf8") as f:
+            f.write(self._generate_config(ip4_address, ip4_mask, dns_servers))
 
         logging.info(f"Running udhcpd with config file {self._config_path}")
         cmd = [binary, "-S", self._config_path]

test/main/test_netconf.py

@@ -1,38 +1,67 @@
 import os.path
 import shutil
 import subprocess
+from ipaddress import IPv4Address
 from subprocess import Popen
+from typing import Optional, List
 from unittest import TestCase
 from unittest.mock import patch, Mock, PropertyMock
 
 from blueman.main.NetConf import DnsMasqHandler, NetworkSetupError, DhcpdHandler, UdhcpdHandler, NetConf, DHCPHandler
 
 
+class FakeSocket:
+    def __init__(self, connect_return_value: int) -> None:
+        self._ret = connect_return_value
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        pass
+
+    def connect_ex(self, *_args: object) -> int:
+        return self._ret
+
+
 @patch("blueman.main.NetConf.have", return_value="/usr/bin/mydnsmasq")
 @patch("blueman.main.NetConf.DnsMasqHandler._pid_path", PropertyMock(return_value="/tmp/pid"))
 class TestDnsmasqHandler(TestCase):
     @patch("blueman.main.NetConf.Popen", return_value=Popen("true"))
     @patch("blueman.main.NetConf.NetConf.lock")
-    def test_success(self, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None:
+    @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(1))
+    def test_success_with_dns(self, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None:
         with open("/tmp/pid", "w") as f:
             f.write("123")
         DnsMasqHandler().apply("203.0.113.1", "255.255.255.0")
         self._check_invocation(have_mock, popen_mock)
         lock_mock.assert_called_with("dhcp")
 
+    @patch("blueman.main.NetConf.Popen", return_value=Popen("true"))
+    @patch("blueman.main.NetConf.NetConf.lock")
+    @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(0))
+    @patch("blueman.main.NetConf.DNSServerProvider.get_servers", lambda: [IPv4Address("203.0.113.10")])
+    def test_success_without_dns(self, lock_mock: Mock, popen_mock: Mock, have_mock: Mock) -> None:
+        with open("/tmp/pid", "w") as f:
+            f.write("123")
+        DnsMasqHandler().apply("203.0.113.1", "255.255.255.0")
+        self._check_invocation(have_mock, popen_mock, ["--port=0", "--dhcp-option=option:dns-server,203.0.113.10"])
+        lock_mock.assert_called_with("dhcp")
+
     @patch("blueman.main.NetConf.Popen", return_value=Popen(["sh", "-c", "echo errormsg >&2"], stderr=subprocess.PIPE))
+    @patch("blueman.main.NetConf.socket.socket", lambda *args: FakeSocket(1))
     def test_failure(self, popen_mock: Mock, have_mock: Mock) -> None:
         with self.assertRaises(NetworkSetupError) as cm:
             DnsMasqHandler().apply("203.0.113.1", "255.255.255.0")
         self._check_invocation(have_mock, popen_mock)
         self.assertEqual(cm.exception.args, ("dnsmasq failed to start: errormsg",))
 
-    def _check_invocation(self, have_mock: Mock, popen_mock: Mock) -> None:
+    def _check_invocation(self, have_mock: Mock, popen_mock: Mock, additional_args: Optional[List[str]] = None) -> None:
         have_mock.assert_called_with("dnsmasq")
         popen_mock.assert_called_with(
-            ["/usr/bin/mydnsmasq", "--port=0", "--pid-file=/tmp/pid", "--except-interface=lo",
+            ["/usr/bin/mydnsmasq", "--pid-file=/tmp/pid", "--except-interface=lo",
              "--interface=pan1", "--bind-interfaces", "--dhcp-range=203.0.113.2,203.0.113.254,60m",
-             "--dhcp-option=option:router,203.0.113.1"],
+             "--dhcp-option=option:router,203.0.113.1"] + ([] if additional_args is None else additional_args),
             stderr=subprocess.PIPE
         )