From f951871430ba59a148b8cb88e0d1b9e517c0a52e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Harald=20Jens=C3=A5s?= Date: Mon, 4 May 2020 20:01:35 +0200 Subject: [PATCH] Use dhcp-host tag support when supported In dnsmasq 2.81 there is a regression (see [1] for details). Prior versions of dnsmasq would select a host record where: a) no address is present in the host record. b) an address matching address family of the client request is present in the host record. dnsmasq 2.81 will also use a host record where a only an address not matching the address family of the client request is present. The same issue is also backported to the dnsmasq-2.79-11.el8.x86_64 which is e.g. in RHEL 8.2 and Centos 8. dnsmasq version 2.81 also adds support for using tag's on host records. When a dhcpv6 request is received, dnsmasq automatically sets the tag 'dhcpv6'. This change adds a runtime check, testing for dnsmasq host entry tag support. And adds 'tag:dhcpv6' to all IPv6 host records when dnsmasq supports this. Adding the tag makes dnsmasq prefer the tagged host for dhcpv6 requests, i.e it's a workaround fix for the regression issue. [1] http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2020q2/014051.html Closes-Bug: #1876094 Change-Id: Ie654c84137914226bdc3e31e16219345c2efaac9 --- neutron/agent/linux/dhcp.py | 44 ++++--- neutron/cmd/runtime_checks.py | 11 ++ neutron/tests/unit/agent/linux/test_dhcp.py | 108 +++++++++++++++--- ...ue-with-dnsmasq-2.81-c95a46e4f4459bd1.yaml | 6 + 4 files changed, 136 insertions(+), 33 deletions(-) create mode 100644 releasenotes/notes/fix-dual-stack-issue-with-dnsmasq-2.81-c95a46e4f4459bd1.yaml diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 74118ee3abf..0f17aa4337e 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -60,6 +60,7 @@ NS_PREFIX = 'qdhcp-' DNSMASQ_SERVICE_NAME = 'dnsmasq' DHCP_RELEASE_TRIES = 3 DHCP_RELEASE_TRIES_SLEEP = 0.3 +HOST_DHCPV6_TAG = 'tag:dhcpv6,' # this variable will be removed when neutron-lib is updated with this value DHCP_OPT_CLIENT_ID_NUM = 61 @@ -358,6 +359,7 @@ class Dnsmasq(DhcpLocalProcess): _ID = 'id:' _IS_DHCP_RELEASE6_SUPPORTED = None + _IS_HOST_TAG_SUPPORTED = None @classmethod def check_version(cls): @@ -521,6 +523,12 @@ class Dnsmasq(DhcpLocalProcess): "will not call it again.") return self._IS_DHCP_RELEASE6_SUPPORTED + def _is_dnsmasq_host_tag_supported(self): + if self._IS_HOST_TAG_SUPPORTED is None: + self._IS_HOST_TAG_SUPPORTED = checks.dnsmasq_host_tag_support() + + return self._IS_HOST_TAG_SUPPORTED + def _release_lease(self, mac_address, ip, ip_version, client_id=None, server_id=None, iaid=None): """Release a DHCP lease.""" @@ -703,6 +711,7 @@ class Dnsmasq(DhcpLocalProcess): no_dhcp, # A flag indicating that the address doesn't need a DHCP # IP address. no_opts, # A flag indication that options shouldn't be written + tag, # A dhcp-host tag to add to the configuration if supported ) """ v6_nets = dict((subnet.id, subnet) for subnet in @@ -722,10 +731,13 @@ class Dnsmasq(DhcpLocalProcess): for alloc in fixed_ips: no_dhcp = False no_opts = False + tag = '' if alloc.subnet_id in v6_nets: addr_mode = v6_nets[alloc.subnet_id].ipv6_address_mode no_dhcp = addr_mode in (constants.IPV6_SLAAC, constants.DHCPV6_STATELESS) + if self._is_dnsmasq_host_tag_supported(): + tag = HOST_DHCPV6_TAG # we don't setup anything for SLAAC. It doesn't make sense # to provide options for a client that won't use DHCP no_opts = addr_mode == constants.IPV6_SLAAC @@ -733,7 +745,7 @@ class Dnsmasq(DhcpLocalProcess): hostname, fqdn = self._get_dns_assignment(alloc.ip_address, dns_assignment) - yield (port, alloc, hostname, fqdn, no_dhcp, no_opts) + yield (port, alloc, hostname, fqdn, no_dhcp, no_opts, tag) def _get_port_extra_dhcp_opts(self, port): return getattr(port, edo_ext.EXTRADHCPOPTS, False) @@ -767,7 +779,7 @@ class Dnsmasq(DhcpLocalProcess): s.id for s in self._get_all_subnets(self.network) if s.enable_dhcp and s.ip_version == constants.IP_VERSION_4] for host_tuple in self._iter_hosts(): - port, alloc, hostname, name, no_dhcp, no_opts = host_tuple + port, alloc, hostname, name, no_dhcp, no_opts, tag = host_tuple # don't write ip address which belongs to a dhcp disabled subnet # or an IPv6 subnet. if no_dhcp or alloc.subnet_id not in dhcpv4_enabled_subnet_ids: @@ -818,11 +830,11 @@ class Dnsmasq(DhcpLocalProcess): # NOTE(ihrachyshka): the loop should not log anything inside it, to # avoid potential performance drop when lots of hosts are dumped for host_tuple in self._iter_hosts(merge_addr6_list=True): - port, alloc, hostname, name, no_dhcp, no_opts = host_tuple + port, alloc, hostname, name, no_dhcp, no_opts, tag = host_tuple if no_dhcp: if not no_opts and self._get_port_extra_dhcp_opts(port): - buf.write('%s,%s%s\n' % ( - port.mac_address, + buf.write('%s,%s%s%s\n' % ( + port.mac_address, tag, 'set:', self._PORT_TAG_PREFIX % port.id)) continue @@ -835,21 +847,21 @@ class Dnsmasq(DhcpLocalProcess): if self._get_port_extra_dhcp_opts(port): client_id = self._get_client_id(port) if client_id and len(port.extra_dhcp_opts) > 1: - buf.write('%s,%s%s,%s,%s,%s%s\n' % - (port.mac_address, self._ID, client_id, name, - ip_address, 'set:', + buf.write('%s,%s%s%s,%s,%s,%s%s\n' % + (port.mac_address, tag, self._ID, client_id, + name, ip_address, 'set:', self._PORT_TAG_PREFIX % port.id)) elif client_id and len(port.extra_dhcp_opts) == 1: - buf.write('%s,%s%s,%s,%s\n' % - (port.mac_address, self._ID, client_id, name, - ip_address)) + buf.write('%s,%s%s%s,%s,%s\n' % + (port.mac_address, tag, self._ID, client_id, + name, ip_address)) else: - buf.write('%s,%s,%s,%s%s\n' % - (port.mac_address, name, ip_address, + buf.write('%s,%s%s,%s,%s%s\n' % + (port.mac_address, tag, name, ip_address, 'set:', self._PORT_TAG_PREFIX % port.id)) else: - buf.write('%s,%s,%s\n' % - (port.mac_address, name, ip_address)) + buf.write('%s,%s%s,%s\n' % + (port.mac_address, tag, name, ip_address)) file_utils.replace_file(filename, buf.getvalue()) LOG.debug('Done building host file %s', filename) @@ -1045,7 +1057,7 @@ class Dnsmasq(DhcpLocalProcess): """ buf = six.StringIO() for host_tuple in self._iter_hosts(): - port, alloc, hostname, fqdn, no_dhcp, no_opts = host_tuple + port, alloc, hostname, fqdn, no_dhcp, no_opts, tag = host_tuple # It is compulsory to write the `fqdn` before the `hostname` in # order to obtain it in PTR responses. if alloc: diff --git a/neutron/cmd/runtime_checks.py b/neutron/cmd/runtime_checks.py index 0fd33b6fc3f..a18e49b86a7 100644 --- a/neutron/cmd/runtime_checks.py +++ b/neutron/cmd/runtime_checks.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron_lib import exceptions from oslo_log import log as logging from neutron.agent.linux import utils as agent_utils @@ -35,3 +36,13 @@ def dhcp_release6_supported(): "Exception: %s", e) return False return True + + +def dnsmasq_host_tag_support(): + cmd = ['dnsmasq', '--test', '--dhcp-host=tag:foo'] + env = {'LC_ALL': 'C', 'PATH': '/sbin:/usr/sbin'} + try: + agent_utils.execute(cmd, addl_env=env, log_fail_as_error=False) + except exceptions.ProcessExecutionError: + return False + return True diff --git a/neutron/tests/unit/agent/linux/test_dhcp.py b/neutron/tests/unit/agent/linux/test_dhcp.py index cdbf437b57a..5ef2aa43bff 100644 --- a/neutron/tests/unit/agent/linux/test_dhcp.py +++ b/neutron/tests/unit/agent/linux/test_dhcp.py @@ -30,6 +30,7 @@ import testtools from neutron.agent.linux import dhcp from neutron.agent.linux import ip_lib +from neutron.cmd import runtime_checks as checks from neutron.conf.agent import common as config from neutron.conf.agent import dhcp as dhcp_config from neutron.conf import common as base_config @@ -1395,9 +1396,23 @@ class TestDnsmasq(TestBase): self.conf.set_override('dnsmasq_config_file', '/foo') self._test_spawn(['--conf-file=/foo', '--domain=openstacklocal']) - def test_spawn_no_dns_domain(self): + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) + def test_spawn_no_dns_domain(self, mock_tag_support): + mock_tag_support.return_value = False (exp_host_name, exp_host_data, - exp_addn_name, exp_addn_data) = self._test_no_dns_domain_alloc_data + exp_addn_name, exp_addn_data) = self._test_no_dns_domain_alloc_data() + self.conf.set_override('dns_domain', '') + network = FakeDualNetwork(domain=self.conf.dns_domain) + self._test_spawn(['--conf-file='], network=network) + self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), + mock.call(exp_addn_name, exp_addn_data)]) + + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) + def test_spawn_no_dns_domain_tag_support(self, mock_tag_support): + mock_tag_support.return_value = True + (exp_host_name, exp_host_data, exp_addn_name, + exp_addn_data) = self._test_no_dns_domain_alloc_data( + tag=dhcp.HOST_DHCPV6_TAG) self.conf.set_override('dns_domain', '') network = FakeDualNetwork(domain=self.conf.dns_domain) self._test_spawn(['--conf-file='], network=network) @@ -2028,19 +2043,18 @@ class TestDnsmasq(TestBase): self.conf.force_metadata = True self._test_output_opts_file(expected, FakeV6Network()) - @property - def _test_no_dns_domain_alloc_data(self): + def _test_no_dns_domain_alloc_data(self, tag=''): exp_host_name = '/dhcp/cccccccc-cccc-cccc-cccc-cccccccccccc/host' exp_host_data = ('00:00:80:aa:bb:cc,host-192-168-0-2,' '192.168.0.2\n' - '00:00:f3:aa:bb:cc,host-fdca-3ba5-a17a-4ba3--2,' + '00:00:f3:aa:bb:cc,{tag}host-fdca-3ba5-a17a-4ba3--2,' '[fdca:3ba5:a17a:4ba3::2]\n' '00:00:0f:aa:bb:cc,host-192-168-0-3,' '192.168.0.3\n' - '00:00:0f:aa:bb:cc,host-fdca-3ba5-a17a-4ba3--3,' + '00:00:0f:aa:bb:cc,{tag}host-fdca-3ba5-a17a-4ba3--3,' '[fdca:3ba5:a17a:4ba3::3]\n' '00:00:0f:rr:rr:rr,host-192-168-0-1,' - '192.168.0.1\n').lstrip() + '192.168.0.1\n').format(tag=tag).lstrip() exp_addn_name = '/dhcp/cccccccc-cccc-cccc-cccc-cccccccccccc/addn_hosts' exp_addn_data = ( '192.168.0.2\t' @@ -2060,19 +2074,18 @@ class TestDnsmasq(TestBase): return (exp_host_name, exp_host_data, exp_addn_name, exp_addn_data) - @property - def _test_reload_allocation_data(self): + def _test_reload_allocation_data(self, tag=''): exp_host_name = '/dhcp/cccccccc-cccc-cccc-cccc-cccccccccccc/host' exp_host_data = ('00:00:80:aa:bb:cc,host-192-168-0-2.openstacklocal.,' '192.168.0.2\n' - '00:00:f3:aa:bb:cc,host-fdca-3ba5-a17a-4ba3--2.' + '00:00:f3:aa:bb:cc,{tag}host-fdca-3ba5-a17a-4ba3--2.' 'openstacklocal.,[fdca:3ba5:a17a:4ba3::2]\n' '00:00:0f:aa:bb:cc,host-192-168-0-3.openstacklocal.,' '192.168.0.3\n' - '00:00:0f:aa:bb:cc,host-fdca-3ba5-a17a-4ba3--3.' + '00:00:0f:aa:bb:cc,{tag}host-fdca-3ba5-a17a-4ba3--3.' 'openstacklocal.,[fdca:3ba5:a17a:4ba3::3]\n' '00:00:0f:rr:rr:rr,host-192-168-0-1.openstacklocal.,' - '192.168.0.1\n').lstrip() + '192.168.0.1\n').format(tag=tag).lstrip() exp_addn_name = '/dhcp/cccccccc-cccc-cccc-cccc-cccccccccccc/addn_hosts' exp_addn_data = ( '192.168.0.2\t' @@ -2120,10 +2133,12 @@ class TestDnsmasq(TestBase): dm.reload_allocations() self.assertFalse(test_pm.register.called) - def test_reload_allocations(self): + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) + def test_reload_allocations(self, mock_tag_support): + mock_tag_support.return_value = False (exp_host_name, exp_host_data, exp_addn_name, exp_addn_data, - exp_opt_name, exp_opt_data,) = self._test_reload_allocation_data + exp_opt_name, exp_opt_data,) = self._test_reload_allocation_data() net = FakeDualNetwork() hpath = '/dhcp/%s/host' % net.id @@ -2143,6 +2158,22 @@ class TestDnsmasq(TestBase): mock.call(exp_opt_name, exp_opt_data), ]) + mock_tag_support.return_value = True + (exp_host_name, exp_host_data, + exp_addn_name, exp_addn_data, + exp_opt_name, exp_opt_data,) = self._test_reload_allocation_data( + tag=dhcp.HOST_DHCPV6_TAG) + test_pm.reset_mock() + dm = self._get_dnsmasq(net, test_pm) + dm.reload_allocations() + self.assertTrue(test_pm.register.called) + + self.safe.assert_has_calls([ + mock.call(exp_host_name, exp_host_data), + mock.call(exp_addn_name, exp_addn_data), + mock.call(exp_opt_name, exp_opt_data), + ]) + def test_release_unused_leases(self): dnsmasq = self._get_dnsmasq(FakeDualNetwork()) @@ -2776,7 +2807,10 @@ class TestDnsmasq(TestBase): self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data)]) - def test_host_and_opts_file_on_stateless_dhcpv6_network(self): + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) + def test_host_and_opts_file_on_stateless_dhcpv6_network( + self, mock_tag_support): + mock_tag_support.return_value = False exp_host_name = '/dhcp/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/host' exp_host_data = ( '00:16:3e:c2:77:1d,' @@ -2792,7 +2826,20 @@ class TestDnsmasq(TestBase): self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), mock.call(exp_opt_name, exp_opt_data)]) - def test_host_and_opts_file_on_stateful_dhcpv6_same_subnet_fixedips(self): + mock_tag_support.return_value = True + exp_host_data = ( + '00:16:3e:c2:77:1d,tag:dhcpv6,' + 'set:port-hhhhhhhh-hhhh-hhhh-hhhh-hhhhhhhhhhhh\n').lstrip() + dm = self._get_dnsmasq(FakeV6NetworkStatelessDHCP()) + dm._output_hosts_file() + dm._output_opts_file() + self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), + mock.call(exp_opt_name, exp_opt_data)]) + + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) + def test_host_and_opts_file_on_stateful_dhcpv6_same_subnet_fixedips( + self, mock_tag_support): + mock_tag_support.return_value = False self.conf.set_override('dnsmasq_enable_addr6_list', True) exp_host_name = '/dhcp/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/host' exp_host_data = ( @@ -2809,6 +2856,17 @@ class TestDnsmasq(TestBase): self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), mock.call(exp_opt_name, exp_opt_data)]) + mock_tag_support.return_value = True + exp_host_data = ( + '00:00:f3:aa:bb:cc,tag:dhcpv6,' + 'host-fdca-3ba5-a17a-4ba3--2.openstacklocal.,' + '[fdca:3ba5:a17a:4ba3::2],[fdca:3ba5:a17a:4ba3::4]\n'.lstrip()) + dm = self._get_dnsmasq(FakeV6NetworkStatefulDHCPSameSubnetFixedIps()) + dm._output_hosts_file() + dm._output_opts_file() + self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), + mock.call(exp_opt_name, exp_opt_data)]) + def test_host_and_opts_file_on_stateless_dhcpv6_network_no_dns(self): exp_host_name = '/dhcp/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/host' exp_opt_name = '/dhcp/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/opts' @@ -2835,8 +2893,10 @@ class TestDnsmasq(TestBase): dm._output_hosts_file() self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data)]) + @mock.patch.object(checks, 'dnsmasq_host_tag_support', autospec=True) def test_host_and_opts_file_on_net_with_V6_stateless_and_V4_subnets( - self): + self, mock_tag_support): + mock_tag_support.return_value = False exp_host_name = '/dhcp/bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb/host' exp_host_data = ( '00:16:3e:c2:77:1d,set:port-hhhhhhhh-hhhh-hhhh-hhhh-hhhhhhhhhhhh\n' @@ -2867,6 +2927,20 @@ class TestDnsmasq(TestBase): self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), mock.call(exp_opt_name, exp_opt_data)]) + mock_tag_support.return_value = True + exp_host_data = ( + '00:16:3e:c2:77:1d,tag:dhcpv6,' + 'set:port-hhhhhhhh-hhhh-hhhh-hhhh-hhhhhhhhhhhh\n' + '00:16:3e:c2:77:1d,host-192-168-0-3.openstacklocal.,' + '192.168.0.3,set:port-hhhhhhhh-hhhh-hhhh-hhhh-hhhhhhhhhhhh\n' + '00:00:0f:rr:rr:rr,' + 'host-192-168-0-1.openstacklocal.,192.168.0.1\n').lstrip() + dm = self._get_dnsmasq(FakeNetworkWithV6SatelessAndV4DHCPSubnets()) + dm._output_hosts_file() + dm._output_opts_file() + self.safe.assert_has_calls([mock.call(exp_host_name, exp_host_data), + mock.call(exp_opt_name, exp_opt_data)]) + def test_has_metadata_subnet_returns_true(self): self.assertTrue(dhcp.Dnsmasq.has_metadata_subnet( [FakeV4MetadataSubnet()])) diff --git a/releasenotes/notes/fix-dual-stack-issue-with-dnsmasq-2.81-c95a46e4f4459bd1.yaml b/releasenotes/notes/fix-dual-stack-issue-with-dnsmasq-2.81-c95a46e4f4459bd1.yaml new file mode 100644 index 00000000000..6fcd2b7c286 --- /dev/null +++ b/releasenotes/notes/fix-dual-stack-issue-with-dnsmasq-2.81-c95a46e4f4459bd1.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed an issue where the client on a dual-stack (IPv4 + IPv6) network failed + to get configuration from the dnsmasq DHCP server. See bug: `1876094 + `_.