Support Ephemeral Networking for BSD

[holmanb]

Support ephemeral network for BSDs

Currently ephemeral.py uses hardcoded iproute2
calls to setup ephemeral networking, which is
incompatible with BSDs. Refactor to support
pluggable network interface operations.

Additional Context

Blocked by #2130 and #2122 (context).

[holmanb]

Perhaps netinfo.py (netdev_info(), _netdev_route_info_iproute(), route_info(), _netdev_route_info_netstat()) should also get support of this interface.

[holmanb]

Perhaps netinfo.py (netdev_info(), _netdev_route_info_iproute(), route_info(), _netdev_route_info_netstat()) should also get support of this interface.

This will be left for a separate pull request.

[blackboxsw]

Overall looks pretty good. Take the rest of my nits with a grain of salt, but we need minimally to fix the freebsd.py:build_dhclient_cmd.

Here's a consolidated diff with review comments:

• dropping unneeded parens
• adding capture=True to network config cmds
• adding path in bsd dhclient cmd

diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index a4c8097e4..4ed1356e9 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -203,6 +203,6 @@ class Distro(cloudinit.distros.bsd.BSD):
         interface: str,
         config_file: str,
     ) -> list:
-        return ["-l", lease_file, "-p", pid_file] + (
+        return [path, "-l", lease_file, "-p", pid_file] + (
             ["-c", config_file, interface] if config_file else [interface]
         )
diff --git a/cloudinit/net/netops/iproute2.py b/cloudinit/net/netops/iproute2.py
index 08d79b187..be2fa1055 100644
--- a/cloudinit/net/netops/iproute2.py
+++ b/cloudinit/net/netops/iproute2.py
@@ -10,7 +10,8 @@ class Iproute2(netops.NetOps):
         subp.subp(
             ["ip"]
             + (["-family", family] if family else [])
-            + ["link", "set", "dev", interface, "up"]
+            + ["link", "set", "dev", interface, "up"],
+            capture=True,
         )
 
     @staticmethod
@@ -18,7 +19,8 @@ class Iproute2(netops.NetOps):
         subp.subp(
             ["ip"]
             + (["-family", family] if family else [])
-            + ["link", "set", "dev", interface, "down"]
+            + ["link", "set", "dev", interface, "down"],
+            capture=True,
         )
 
     @staticmethod
@@ -31,20 +33,19 @@ class Iproute2(netops.NetOps):
     ):
         subp.subp(
             ["ip", "-4", "route", "add", route]
-            + (["via", gateway] if gateway and gateway != "0.0.0.0" else [])
-            + [
-                "dev",
-                interface,
-            ]
-            + (["src", source_address] if source_address else []),
+            + ["via", gateway] if gateway and gateway != "0.0.0.0" else []
+            + ["dev", interface]
+            + ["src", source_address] if source_address else [],
+            capture=True,
         )
 
     @staticmethod
     def append_route(interface: str, address: str, gateway: str):
         subp.subp(
             ["ip", "-4", "route", "append", address]
-            + (["via", gateway] if gateway and gateway != "0.0.0.0" else [])
-            + ["dev", interface]
+            + ["via", gateway] if gateway and gateway != "0.0.0.0" else []
+            + ["dev", interface],
+            capture=True,
         )
 
     @staticmethod
@@ -57,9 +58,10 @@ class Iproute2(netops.NetOps):
     ):
         subp.subp(
             ["ip", "-4", "route", "del", address]
-            + (["via", gateway] if gateway and gateway != "0.0.0.0" else [])
+            + ["via", gateway] if gateway and gateway != "0.0.0.0" else []
             + ["dev", interface]
-            + (["src", source_address] if source_address else [])
+            + ["src", source_address] if source_address else [],
+            capture=True,
         )
 
     @staticmethod
@@ -83,11 +85,13 @@ class Iproute2(netops.NetOps):
                 "dev",
                 interface,
             ],
+            capture=True,
             update_env={"LANG": "C"},
         )
 
     @staticmethod
     def del_addr(interface: str, address: str):
         subp.subp(
-            ["ip", "-family", "inet", "addr", "del", address, "dev", interface]
+            ["ip", "-family", "inet", "addr", "del", address, "dev", interface],
+            capture=True,
         )

[igalic]

thanks for the review, @blackboxsw
i have started working on bsd side of this change

[igalic]

❓

[igalic]

what exactly does this do?
how is this route different from one without src self.ip?

[holmanb]

I think adding src self.ip allows the route to force outgoing traffic to use the specified source address, which is otherwise unopinionated by a route that only specifies an interface.

[holmanb]

Thanks for the review @blackboxsw! I fixed the issue with the freebsd dhclient command. I think that the rest of the issues have also been addressed.

I did get an external review via email by someone who tested this out. It seems that this breaks something that worked before. I'm going to set this PR to WIP while I dig into the issue they found while testing.

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[blackboxsw]

This changeset looks good to me, it fixes the missing path issue on freebsd.py.'s build_dhclient_cmd. Are we are waiting on a potential failure path seen on BSD outside of this?

[holmanb]

This changeset looks good to me, it fixes the missing path issue on freebsd.py.'s build_dhclient_cmd. Are we are waiting on a potential failure path seen on BSD outside of this?

I made some changes to this PR after that report and haven't heard an update on that. I'd like to merge this so that we can unblock @igalic from working on the bsd implementation. I'll reach out to the reported to see if they have an update on the latest version of this, but in the meantime I'll merge.