Skip to content

dhcp: add option to use NetworkManager for DHCP discovery #5563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lkundrak wants to merge 1 commit into from
Closed

dhcp: add option to use NetworkManager for DHCP discovery #5563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 226 additions & 1 deletion cloudinit/net/dhcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Copyright (C) 2017 Canonical Ltd.
#
# Author: Chad Smith <chad.smith@canonical.com>
# Author: Lubomir Rintel <lkundrak@v3.sk>
#
# This file is part of cloud-init. See LICENSE file for license information.

Expand Down Expand Up @@ -1011,4 +1012,228 @@ def parse_static_routes(routes: str) -> List[Tuple[str, str]]:
return []


ALL_DHCP_CLIENTS = [Dhcpcd, IscDhclient, Udhcpc]
class NetworkManagerDhcpClient(DhcpClient):
Copy link
Copy Markdown
Member

@holmanb holmanb Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at this code thoroughly, but it does appear to have the same shortcoming as the udhcpc implementation: missing azure support for option 245.

cc @cjp256

On azure I see this:

$ nmcli --terse --fields DHCP4 device show eth0
DHCP4.OPTION[1]:dhcp_client_identifier = 01:00:0d:3a:91:ab:22
DHCP4.OPTION[2]:dhcp_lease_time = 4294967295
DHCP4.OPTION[3]:dhcp_server_identifier = 168.63.129.16
DHCP4.OPTION[4]:domain_name = vd15dwc2m3fevjchgt4zmpijze.gx.internal.cloudapp.net
DHCP4.OPTION[5]:domain_name_servers = 168.63.129.16
DHCP4.OPTION[6]:ip_address = 10.0.0.6
DHCP4.OPTION[7]:next_server = 168.63.129.16
DHCP4.OPTION[8]:private_245 = a8:3f:81:10
DHCP4.OPTION[9]:requested_broadcast_address = 1
DHCP4.OPTION[10]:requested_domain_name = 1
DHCP4.OPTION[11]:requested_domain_name_servers = 1
DHCP4.OPTION[12]:requested_domain_search = 1
DHCP4.OPTION[13]:requested_host_name = 1
DHCP4.OPTION[14]:requested_interface_mtu = 1
DHCP4.OPTION[15]:requested_ms_classless_static_routes = 1
DHCP4.OPTION[16]:requested_nis_domain = 1
DHCP4.OPTION[17]:requested_nis_servers = 1
DHCP4.OPTION[18]:requested_ntp_servers = 1
DHCP4.OPTION[19]:requested_rfc3442_classless_static_routes = 1
DHCP4.OPTION[20]:requested_root_path = 1
DHCP4.OPTION[21]:requested_routers = 1
DHCP4.OPTION[22]:requested_static_routes = 1
DHCP4.OPTION[23]:requested_subnet_mask = 1
DHCP4.OPTION[24]:requested_time_offset = 1
DHCP4.OPTION[25]:requested_wpad = 1
DHCP4.OPTION[26]:rfc3442_classless_static_routes = 0.0.0.0/0 10.0.0.1 168.63.129.16/32 10.0.0.1 169.254.169.254/32 10.0.0.1
DHCP4.OPTION[27]:routers = 10.0.0.1
DHCP4.OPTION[28]:subnet_mask = 255.255.255.0

So we have access to the wireserver IP address, but this PR doesn't seem deal with this special option correctly.

client_name = "nmcli"

def __init__(self):
super().__init__()
self.lease_file = None

try:
running = subp.subp(
[
self.dhcp_client_path,
"--terse",
"--get-values",
"RUNNING",
"general",
"status",
]
).stdout.strip()
if not running == "running":
raise NoDHCPLeaseMissingDhclientError()
except subp.ProcessExecutionError as error:
LOG.debug(
"nmcli exited with code: %s stderr: %r stdout: %r",
error.exit_code,
error.stderr,
error.stdout,
)
raise NoDHCPLeaseMissingDhclientError() from error

def dhcp_discovery(
self,
interface: str,
dhcp_log_func: Optional[Callable] = None,
distro=None,
) -> Dict[str, Any]:
"""Configure an interface with DHCP using NetworkManager.

@param interface: Name of the network interface which to configure
with NetworkManager
@param dhcp_log_func: A callable accepting the client output and
error streams.
@return: dict of lease options representing the most recent lease
NetworkManager obtained via DHCP
"""
LOG.debug("Connecting interface %s", interface)

try:
ac = subp.subp(
[
self.dhcp_client_path,
"--get-values",
"GENERAL.CON-PATH",
"device",
"show",
interface,
]
).stdout.strip()

if not ac == "":
orig_uuid = subp.subp(
[
self.dhcp_client_path,
"--get-values",
"GENERAL.UUID",
"connection",
"show",
ac,
]
).stdout.strip()
if orig_uuid == "":
orig_uuid = None
else:
LOG.debug("connection %s is already active", orig_uuid)

conn_uuid = "cd4ac1c3-888b-433f-8cbd-2634df28c36d"
conn_type = (
"infiniband"
if is_ib_interface(interface)
else "802-3-ethernet"
)

LOG.debug("adding connection %s", conn_uuid)
out, err = subp.subp(
[
self.dhcp_client_path,
"connection",
"add",
"save",
"no",
"connection.uuid",
conn_uuid,
"connection.type",
conn_type,
"connection.interface-name",
interface,
"connection.autoconnect",
"no",
"ipv4.may-fail",
"no",
"ipv6.method",
"ignore",
]
)
if dhcp_log_func is not None:
dhcp_log_func(out, err)

LOG.debug("activating connection %s", conn_uuid)
out, err = subp.subp(
[self.dhcp_client_path, "connection", "up", conn_uuid]
)
if dhcp_log_func is not None:
dhcp_log_func(out, err)

except subp.ProcessExecutionError as error:
LOG.debug(
"nmcli exited with code: %s stderr: %r stdout: %r",
error.exit_code,
error.stderr,
error.stdout,
)
raise NoDHCPLeaseMissingDhclientError() from error

lease = self.get_newest_lease(interface)

if orig_uuid is not None:
LOG.debug("triggering reactivation of connection %s", orig_uuid)
out, err = subp.subp(
[
self.dhcp_client_path,
"--wait",
"0",
"connection",
"up",
orig_uuid,
],
rcs=[0, 1],
)
if dhcp_log_func is not None:
dhcp_log_func(out, err)

LOG.debug("removing connection %s", conn_uuid)
out, err = subp.subp(
[self.dhcp_client_path, "connection", "del", conn_uuid]
)
if dhcp_log_func is not None:
dhcp_log_func(out, err)

return lease

@staticmethod
def parse_network_manager_lease(lease_dump: str, interface: str) -> Dict:
"""parse the DHCP lease from nmcli

map names to the datastructure we create from nmcli via
"nmcli --terse --fields DHCP4 device show eth0":

example output:

DHCP4.OPTION[1]:dhcp_client_identifier = 01:fa:16:3e:db:dc:bf
DHCP4.OPTION[2]:dhcp_lease_time = 43200
DHCP4.OPTION[3]:dhcp_server_identifier = 10.0.215.254
DHCP4.OPTION[4]:domain_name_servers = 10.11.5.160 10.2.70.215
DHCP4.OPTION[5]:expiry = 1722039992
DHCP4.OPTION[6]:interface_mtu = 1500
DHCP4.OPTION[7]:ip_address = 10.0.215.164
DHCP4.OPTION[8]:requested_broadcast_address = 1
...
"""
LOG.debug("Parsing lease for interface %s: %r", interface, lease_dump)

lease = {"interface": interface}
for line in lease_dump.strip().splitlines():
line = line.split(":", maxsplit=1)[1]
key, value = line.split(" = ", maxsplit=1)
if not key.startswith("requested_"):
key = key.replace("_", "-")
lease[key] = value
lease["fixed-address"] = lease.pop("ip-address")
return lease

def get_newest_lease(self, interface: str) -> Dict[str, Any]:
"""Get the most recent lease from the ephemeral phase as a dict.

Return a dict of dhcp options. The dict contains key value
pairs from the most recent lease.

@param interface: an interface name
@raises: InvalidDHCPLeaseFileError on empty or unparseable lease
file content.
"""
try:
return self.parse_network_manager_lease(
subp.subp(
[
self.dhcp_client_path,
"--terse",
"--fields",
"DHCP4",
"device",
"show",
interface,
]
).stdout,
interface,
)

except subp.ProcessExecutionError as error:
LOG.debug(
"nmcli exited with code: %s stderr: %r stdout: %r",
error.exit_code,
error.stderr,
error.stdout,
)
raise NoDHCPLeaseError from error

@staticmethod
def parse_static_routes(routes: str) -> List[Tuple[str, str]]:
static_routes = routes.split()
if static_routes:
# format: dest1/mask gw1 ... destn/mask gwn
return [i for i in zip(static_routes[::2], static_routes[1::2])]
return []


ALL_DHCP_CLIENTS = [Dhcpcd, IscDhclient, Udhcpc, NetworkManagerDhcpClient]
2 changes: 1 addition & 1 deletion systemd/cloud-init-local.service.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ After=systemd-remount-fs.service
Requires=dbus.socket
After=dbus.socket
{% endif %}
Before=NetworkManager.service
After=NetworkManager.service
Copy link
Copy Markdown
Member

@holmanb holmanb Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break some cloud-init expectations, so I don't think that we can accept this as it is. We could always just patch it for downstreams using NetworkManager, but this would leave cloud-init differently on different distributions, and it really misses the point of what the local stage is supposed to be doing.

It might be that cloud-init doesn't really need to get a network configuration before the daemon is up for datasources that require network - if that is true then we could always use the "activator" codepath - but that would change things like what hostname is advertised to the DHCP server (iirc there is a bug related to this). Thoughts @cjp256?

Either way if we go down that route, this code proposal would have to make some changes and that would be a big architectural change - so let me think about this a bit.

{% if variant in ["almalinux", "cloudlinux", "rhel"] %}
Before=network.service
{% endif %}
Expand Down
47 changes: 46 additions & 1 deletion tests/unittests/net/test_dhcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
Dhcpcd,
InvalidDHCPLeaseFileError,
IscDhclient,
NetworkManagerDhcpClient,
NoDHCPLeaseError,
NoDHCPLeaseInterfaceError,
NoDHCPLeaseMissingDhclientError,
Expand Down Expand Up @@ -423,7 +424,7 @@ def test_dhcp_client_failover(
subp.ProcessExecutionError(exit_code=-5),
]

m_which.side_effect = [False, False, False, False]
m_which.side_effect = [False, False, False, False, False]
with pytest.raises(NoDHCPLeaseError):
maybe_perform_dhcp_discovery(Distro("somename", {}, None))

Expand Down Expand Up @@ -1414,6 +1415,50 @@ def test_dhcpcd_discovery_timeout(
)


class TestNetworkManagerDhcpClient:
def test_parse_lease_dump(self):
lease = dedent(
"""
DHCP4.OPTION[1]:dhcp_client_identifier = 01:fa:16:3e:db:dc:bf
DHCP4.OPTION[2]:dhcp_lease_time = 3600
DHCP4.OPTION[3]:dhcp_server_identifier = 192.168.0.1
DHCP4.OPTION[4]:domain_name_servers = 192.168.0.2
DHCP4.OPTION[5]:expiry = 1722039992
DHCP4.OPTION[6]:interface_mtu = 9001
DHCP4.OPTION[7]:ip_address = 192.168.0.212
DHCP4.OPTION[8]:requested_broadcast_address = 1
DHCP4.OPTION[9]:requested_domain_name = 1
DHCP4.OPTION[10]:requested_domain_name_servers = 1
DHCP4.OPTION[11]:requested_domain_search = 1
DHCP4.OPTION[12]:requested_host_name = 1
DHCP4.OPTION[13]:requested_interface_mtu = 1
DHCP4.OPTION[14]:requested_ms_classless_static_routes = 1
DHCP4.OPTION[15]:requested_nis_domain = 1
DHCP4.OPTION[16]:requested_nis_servers = 1
DHCP4.OPTION[17]:requested_ntp_servers = 1
DHCP4.OPTION[18]:requested_rfc3442_classless_static_routes = 1
DHCP4.OPTION[19]:requested_root_path = 1
DHCP4.OPTION[20]:requested_routers = 1
DHCP4.OPTION[21]:requested_static_routes = 1
DHCP4.OPTION[22]:requested_subnet_mask = 1
DHCP4.OPTION[23]:requested_time_offset = 1
DHCP4.OPTION[24]:requested_wpad = 1
DHCP4.OPTION[25]:routers = 192.168.0.1
DHCP4.OPTION[26]:subnet_mask = 255.255.240.0
"""
)
with mock.patch("cloudinit.net.dhcp.util.load_binary_file"):
parsed_lease = (
NetworkManagerDhcpClient.parse_network_manager_lease(
lease, "eth0"
)
)
assert "eth0" == parsed_lease["interface"]
assert "192.168.0.212" == parsed_lease["fixed-address"]
assert "255.255.240.0" == parsed_lease["subnet-mask"]
assert "192.168.0.1" == parsed_lease["routers"]


class TestMaybePerformDhcpDiscovery:
def test_none_and_missing_fallback(self):
with pytest.raises(NoDHCPLeaseInterfaceError):
Expand Down