From 54dfbd94a6b753af2d0b5babe8d927c1fb901ab5 Mon Sep 17 00:00:00 2001 From: Arjun Baindur Date: Wed, 5 Dec 2018 12:43:05 -0800 Subject: [PATCH] Do not release DHCP lease when no client ID is set on port The DHCP agent has a really strict enforcement of client ID, which is part of the DHCP extra options. If a VM advertises a client ID, DHCP agent will automatically release it's lease whenever *any* other port is updated/deleted, even if no client ID is set on the port, because it thinks the client ID has changed. When reload_allocations() is called, the DHCP agent parses the leases and hosts files, and gets the list of all the ports in the network from the DB, computing 3 different sets. The set from the leases file (v4_leases) could have a client ID, but the set from the port DB and hosts file will have None. As a result, the set subtraction does not filter out the entry, and all ports that have an active lease with a client ID are released. The Client ID should only be enforced and leases released if it's actually set in the port DB's DHCP extra Opts. In that case it means someone knows what they are doing, and we want to check for a mismatch. If the client ID on a port is empty, it should not be treated like an unused lease. We can't expect end users that just create VMs with auto created ports to know/care about DHCP client IDs, then manually update ports or change app templates. In some cases, like Windows VMs, the client ID is advertised as the MAC by default. In fact, there is a Windows bug which prevents you from even turning this off: https://support.microsoft.com/en-us/help/3004537/dhcp-client-always-includes-option-61-in-the-dhcp-request-in-windows-8 Linux VMs don't have this on by default, but it may be enabled in some templates unknown to users. Change-Id: I8021f740bd78e654915337bd3287b45b2c422e95 Closes-Bug: #1806770 (cherry picked from commit f2111e035424bf714099966ad724e9a4bd604c18) --- neutron/agent/linux/dhcp.py | 13 +++++++ neutron/tests/unit/agent/linux/test_dhcp.py | 43 +++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 2faf9b8472a..8a0764a0d7b 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -859,6 +859,19 @@ class Dnsmasq(DhcpLocalProcess): if not entries_to_release: return + # If the VM advertises a client ID in its lease, but its not set in + # the port's Extra DHCP Opts, the lease will not be filtered above. + # Release the lease only if client ID is set in port DB and a mismatch + # Otherwise the lease is released when other ports are deleted/updated + entries_with_no_client_id = set() + for ip, mac, client_id in entries_to_release: + if client_id: + entry_no_client_id = (ip, mac, None) + if (entry_no_client_id in old_leases and + entry_no_client_id in new_leases): + entries_with_no_client_id.add((ip, mac, client_id)) + entries_to_release -= entries_with_no_client_id + # Try DHCP_RELEASE_TRIES times to release a lease, re-reading the # file each time to see if it's still there. We loop +1 times to # check the lease file one last time before logging any remaining diff --git a/neutron/tests/unit/agent/linux/test_dhcp.py b/neutron/tests/unit/agent/linux/test_dhcp.py index 1c495c13a8f..49e7f4b2f0e 100644 --- a/neutron/tests/unit/agent/linux/test_dhcp.py +++ b/neutron/tests/unit/agent/linux/test_dhcp.py @@ -2185,6 +2185,49 @@ class TestDnsmasq(TestBase): dnsmasq._release_lease.assert_called_once_with( mac1, ip1, constants.IP_VERSION_4, client_id1, 'server_id', mac1) + def test_release_unused_leases_one_lease_with_client_id_none(self): + dnsmasq = self._get_dnsmasq(FakeDualNetwork()) + + ip1 = '192.168.0.2' + mac1 = '00:00:80:aa:bb:cc' + client_id1 = 'client1' + ip2 = '192.168.0.4' + mac2 = '00:16:3E:C2:77:1D' + client_id2 = 'test4' + ip6 = '2001:0db8:11a3:09d7:1f34:8a2e:07a0:765d' + + old_leases = set([(ip1, mac1, client_id1), (ip2, mac2, None)]) + dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases) + dnsmasq._output_hosts_file = mock.Mock() + # Because the lease release code could fire multiple times, the + # second read of the lease file must not have the entries that + # would have been released. + dnsmasq._read_leases_file_leases = mock.Mock( + side_effect=[{ip6: {'iaid': 0xff, + 'client_id': 'client_id', + 'server_id': 'server_id'}, + ip1: {'iaid': mac1, + 'client_id': client_id1, + 'server_id': 'server_id'}, + ip2: {'iaid': mac2, + 'client_id': client_id2, + 'server_id': 'server_id'} + }, + {ip6: {'iaid': 0xff, + 'client_id': 'client_id', + 'server_id': 'server_id'}, + ip2: {'iaid': mac2, + 'client_id': client_id2, + 'server_id': 'server_id'} + }]) + dnsmasq._release_lease = mock.Mock() + dnsmasq.network.ports = [FakePort4()] + + dnsmasq._release_unused_leases() + + dnsmasq._release_lease.assert_called_once_with( + mac1, ip1, constants.IP_VERSION_4, client_id1, 'server_id', mac1) + def test_release_unused_leases_one_lease_from_leases_file(self): # leases file has a stale entry that is not in the host file dnsmasq = self._get_dnsmasq(FakeDualNetwork())