From 3ff90cd08b2f08512b54ad9829f51dc9f11dcd48 Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Thu, 17 Feb 2022 11:10:54 -0500 Subject: [PATCH] net/dhcp: surface type of DHCP lease failure to caller When performing DHCP, it is useful for the caller to have context on the type of failure. This can be done with some new exceptions types, subclassing NoDHCPLeaseError so the caller's current contract remains. - Add the following errors: - NoDHCPLeaseInterfaceError if there are problems finding the (possibly specified) interface. - NoDHCPLeaseMissingDhclientError for missing dhclient. - Update InvalidDHCPLeaseFileError to subclass NoDHCPLeaseError. - Pass through these errors rather than catching it in obtain_lease(). Tests: - Add missing mock for test_provided_nic_does_not_exist(). - Add new test coverage for EphemeralDHCPv4 errors. - Update existing tests for maybe_perform_dhcp_discovery() to match new behavior. Signed-off-by: Chris Patterson --- cloudinit/net/dhcp.py | 27 +++++++------ tests/unittests/net/test_dhcp.py | 67 ++++++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py index f9af18cff39..cf7517a7cde 100644 --- a/cloudinit/net/dhcp.py +++ b/cloudinit/net/dhcp.py @@ -28,7 +28,11 @@ NETWORKD_LEASES_DIR = "/run/systemd/netif/leases" -class InvalidDHCPLeaseFileError(Exception): +class NoDHCPLeaseError(Exception): + """Raised when unable to get a DHCP lease.""" + + +class InvalidDHCPLeaseFileError(NoDHCPLeaseError): """Raised when parsing an empty or invalid dhcp.leases file. Current uses are DataSourceAzure and DataSourceEc2 during ephemeral @@ -36,8 +40,12 @@ class InvalidDHCPLeaseFileError(Exception): """ -class NoDHCPLeaseError(Exception): - """Raised when unable to get a DHCP lease.""" +class NoDHCPLeaseInterfaceError(NoDHCPLeaseError): + """Raised when unable to find a viable interface for DHCP.""" + + +class NoDHCPLeaseMissingDhclientError(NoDHCPLeaseError): + """Raised when unable to find dhclient.""" class EphemeralDHCPv4(object): @@ -89,12 +97,7 @@ def obtain_lease(self): """ if self.lease: return self.lease - try: - leases = maybe_perform_dhcp_discovery( - self.iface, self.dhcp_log_func - ) - except InvalidDHCPLeaseFileError as e: - raise NoDHCPLeaseError() from e + leases = maybe_perform_dhcp_discovery(self.iface, self.dhcp_log_func) if not leases: raise NoDHCPLeaseError() self.lease = leases[-1] @@ -165,16 +168,16 @@ def maybe_perform_dhcp_discovery(nic=None, dhcp_log_func=None): nic = find_fallback_nic() if nic is None: LOG.debug("Skip dhcp_discovery: Unable to find fallback nic.") - return [] + raise NoDHCPLeaseInterfaceError() elif nic not in get_devicelist(): LOG.debug( "Skip dhcp_discovery: nic %s not found in get_devicelist.", nic ) - return [] + raise NoDHCPLeaseInterfaceError() dhclient_path = subp.which("dhclient") if not dhclient_path: LOG.debug("Skip dhclient configuration: No dhclient command found.") - return [] + raise NoDHCPLeaseMissingDhclientError() with temp_utils.tempdir( rmtree_ignore_errors=True, prefix="cloud-init-dhcp-", needs_exe=True ) as tdir: diff --git a/tests/unittests/net/test_dhcp.py b/tests/unittests/net/test_dhcp.py index 876873d533e..08ca001a5f1 100644 --- a/tests/unittests/net/test_dhcp.py +++ b/tests/unittests/net/test_dhcp.py @@ -5,10 +5,14 @@ from textwrap import dedent import httpretty +import pytest import cloudinit.net as net from cloudinit.net.dhcp import ( InvalidDHCPLeaseFileError, + NoDHCPLeaseError, + NoDHCPLeaseInterfaceError, + NoDHCPLeaseMissingDhclientError, dhcp_discovery, maybe_perform_dhcp_discovery, networkd_load_leases, @@ -334,15 +338,21 @@ class TestDHCPDiscoveryClean(CiTestCase): def test_no_fallback_nic_found(self, m_fallback_nic): """Log and do nothing when nic is absent and no fallback is found.""" m_fallback_nic.return_value = None # No fallback nic found - self.assertEqual([], maybe_perform_dhcp_discovery()) + + with pytest.raises(NoDHCPLeaseInterfaceError): + maybe_perform_dhcp_discovery() + self.assertIn( "Skip dhcp_discovery: Unable to find fallback nic.", self.logs.getvalue(), ) - def test_provided_nic_does_not_exist(self): + @mock.patch("cloudinit.net.dhcp.find_fallback_nic", return_value=None) + def test_provided_nic_does_not_exist(self, m_fallback_nic): """When the provided nic doesn't exist, log a message and no-op.""" - self.assertEqual([], maybe_perform_dhcp_discovery("idontexist")) + with pytest.raises(NoDHCPLeaseInterfaceError): + maybe_perform_dhcp_discovery("idontexist") + self.assertIn( "Skip dhcp_discovery: nic idontexist not found in get_devicelist.", self.logs.getvalue(), @@ -354,7 +364,10 @@ def test_absent_dhclient_command(self, m_fallback, m_which): """When dhclient doesn't exist in the OS, log the issue and no-op.""" m_fallback.return_value = "eth9" m_which.return_value = None # dhclient isn't found - self.assertEqual([], maybe_perform_dhcp_discovery()) + + with pytest.raises(NoDHCPLeaseMissingDhclientError): + maybe_perform_dhcp_discovery() + self.assertIn( "Skip dhclient configuration: No dhclient command found.", self.logs.getvalue(), @@ -794,4 +807,50 @@ def test_ephemeral_dhcp_setup_network_if_url_connectivity( m_dhcp.called_once_with() +@pytest.mark.parametrize( + "error_class", + [ + NoDHCPLeaseInterfaceError, + NoDHCPLeaseInterfaceError, + NoDHCPLeaseMissingDhclientError, + ], +) +class TestEphemeralDhcpLeaseErrors: + @mock.patch("cloudinit.net.dhcp.maybe_perform_dhcp_discovery") + def test_obtain_lease_raises_error(self, m_dhcp, error_class): + m_dhcp.side_effect = [error_class()] + + with pytest.raises(error_class): + net.dhcp.EphemeralDHCPv4().obtain_lease() + + assert len(m_dhcp.mock_calls) == 1 + + @mock.patch("cloudinit.net.dhcp.maybe_perform_dhcp_discovery") + def test_obtain_lease_umbrella_error(self, m_dhcp, error_class): + m_dhcp.side_effect = [error_class()] + with pytest.raises(NoDHCPLeaseError): + net.dhcp.EphemeralDHCPv4().obtain_lease() + + assert len(m_dhcp.mock_calls) == 1 + + @mock.patch("cloudinit.net.dhcp.maybe_perform_dhcp_discovery") + def test_ctx_mgr_raises_error(self, m_dhcp, error_class): + m_dhcp.side_effect = [error_class()] + + with pytest.raises(error_class): + with net.dhcp.EphemeralDHCPv4(): + pass + + assert len(m_dhcp.mock_calls) == 1 + + @mock.patch("cloudinit.net.dhcp.maybe_perform_dhcp_discovery") + def test_ctx_mgr_umbrella_error(self, m_dhcp, error_class): + m_dhcp.side_effect = [error_class()] + with pytest.raises(NoDHCPLeaseError): + with net.dhcp.EphemeralDHCPv4(): + pass + + assert len(m_dhcp.mock_calls) == 1 + + # vi: ts=4 expandtab