Python/ifconfig: T1557: {add,del}_addr() now supports dhcp/dhcpv6

Instead of manually starting DHCP/DHCPv6 for every interface and have an
identical if/elif/else statement checking for dhcp/dhcpv6 rather move this
repeating stement into add_addr()/del_addr().

Single source is always preferred.

3 files changed, 112 insertions, 90 deletions

diff --git a/python/vyos/ifconfig.py b/python/vyos/ifconfig.py
index c5d3e447b..1886addfc 100644
--- a/python/vyos/ifconfig.py
+++ b/python/vyos/ifconfig.py
@@ -61,6 +61,7 @@ class Interface:
         >>> i = Interface('eth0')
         """
         self._ifname = str(ifname)
+        self._state = 'down'
 
         if not os.path.exists('/sys/class/net/{}'.format(ifname)) and not type:
             raise Exception('interface "{}" not found'.format(self._ifname))
@@ -111,8 +112,8 @@ class Interface:
         # All subinterfaces are now removed, continue on the physical interface
 
         # stop DHCP(v6) if running
-        self.del_dhcp()
-        self.del_dhcpv6()
+        self._del_dhcp()
+        self._del_dhcpv6()
 
         # NOTE (Improvement):
         # after interface removal no other commands should be allowed
@@ -359,6 +360,8 @@ class Interface:
         if state not in ['up', 'down']:
             raise ValueError('state must be "up" or "down"')
 
+        self._state = state
+
         # Assemble command executed on system. Unfortunately there is no way
         # to up/down an interface via sysfs
         cmd = 'ip link set dev {} {}'.format(self._ifname, state)
@@ -495,8 +498,14 @@ class Interface:
 
     def add_addr(self, addr):
         """
-        Add IP address to interface. Address is only added if it yet not added
-        to that interface.
+        Add IP(v6) address to interface. Address is only added if it is not
+        already assigned to that interface.
+
+        addr: can be an IPv4 address, IPv6 address, dhcp or dhcpv6!
+              IPv4: add IPv4 address to interface
+              IPv6: add IPv6 address to interface
+              dhcp: start dhclient (IPv4) on interface
+              dhcpv6: start dhclient (IPv6) on interface
 
         Example:
         >>> from vyos.ifconfig import Interface
@@ -506,13 +515,25 @@ class Interface:
         >>> j.get_addr()
         ['192.0.2.1/24', '2001:db8::ffff/64']
         """
-        if not is_intf_addr_assigned(self._ifname, addr):
-            cmd = 'ip addr add "{}" dev "{}"'.format(addr, self._ifname)
-            self._cmd(cmd)
+        if addr == 'dhcp':
+            self._set_dhcp()
+        elif addr == 'dhcpv6':
+            self._set_dhcpv6()
+        else:
+            if not is_intf_addr_assigned(self._ifname, addr):
+                cmd = 'ip addr add "{}" dev "{}"'.format(addr, self._ifname)
+                self._cmd(cmd)
 
     def del_addr(self, addr):
         """
-        Remove IP address from interface.
+        Delete IP(v6) address to interface. Address is only added if it is
+        assigned to that interface.
+
+        addr: can be an IPv4 address, IPv6 address, dhcp or dhcpv6!
+              IPv4: delete IPv4 address from interface
+              IPv6: delete IPv6 address from interface
+              dhcp: stop dhclient (IPv4) on interface
+              dhcpv6: stop dhclient (IPv6) on interface
 
         Example:
         >>> from vyos.ifconfig import Interface
@@ -525,12 +546,17 @@ class Interface:
         >>> j.get_addr()
         ['2001:db8::ffff/64']
         """
-        if is_intf_addr_assigned(self._ifname, addr):
-            cmd = 'ip addr del "{}" dev "{}"'.format(addr, self._ifname)
-            self._cmd(cmd)
+        if addr == 'dhcp':
+            self._del_dhcp()
+        elif addr == 'dhcpv6':
+            self._del_dhcpv6()
+        else:
+            if is_intf_addr_assigned(self._ifname, addr):
+                cmd = 'ip addr del "{}" dev "{}"'.format(addr, self._ifname)
+                self._cmd(cmd)
 
     # replace dhcpv4/v6 with systemd.networkd?
-    def set_dhcp(self):
+    def _set_dhcp(self):
         """
         Configure interface as DHCP client. The dhclient binary is automatically
         started in background!
@@ -557,15 +583,17 @@ class Interface:
         with open(self._dhcp_cfg_file, 'w') as f:
             f.write(dhcp_text)
 
-        cmd  = 'start-stop-daemon --start --quiet --pidfile ' + \
-            self._dhcp_pid_file
-        cmd += ' --exec /sbin/dhclient --'
-        # now pass arguments to dhclient binary
-        cmd += ' -4 -nw -cf {} -pf {} -lf {} {}'.format(
-            self._dhcp_cfg_file, self._dhcp_pid_file, self._dhcp_lease_file, self._ifname)
-        self._cmd(cmd)
+        if self._state == 'up':
+            cmd  = 'start-stop-daemon --start --quiet --pidfile ' + \
+                self._dhcp_pid_file
+            cmd += ' --exec /sbin/dhclient --'
+            # now pass arguments to dhclient binary
+            cmd += ' -4 -nw -cf {} -pf {} -lf {} {}'.format(
+                self._dhcp_cfg_file, self._dhcp_pid_file, self._dhcp_lease_file, self._ifname)
+            self._cmd(cmd)
 
-    def del_dhcp(self):
+
+    def _del_dhcp(self):
         """
         De-configure interface as DHCP clinet. All auto generated files like
         pid, config and lease will be removed.
@@ -601,7 +629,8 @@ class Interface:
         if os.path.isfile(self._dhcp_lease_file):
             os.remove(self._dhcp_lease_file)
 
-    def set_dhcpv6(self):
+
+    def _set_dhcpv6(self):
         """
         Configure interface as DHCPv6 client. The dhclient binary is automatically
         started in background!
@@ -622,25 +651,28 @@ class Interface:
         with open(self._dhcpv6_cfg_file, 'w') as f:
             f.write(dhcpv6_text)
 
-        # https://bugs.launchpad.net/ubuntu/+source/ifupdown/+bug/1447715
-        #
-        # wee need to wait for IPv6 DAD to finish once and interface is added
-        # this suxx :-(
-        sleep(5)
+        if self._state == 'up':
+            # https://bugs.launchpad.net/ubuntu/+source/ifupdown/+bug/1447715
+            #
+            # wee need to wait for IPv6 DAD to finish once and interface is added
+            # this suxx :-(
+            sleep(5)
 
-        # no longer accept router announcements on this interface
-        cmd = 'sysctl -q -w net.ipv6.conf.{}.accept_ra=0'.format(self._ifname)
-        self._cmd(cmd)
+            # no longer accept router announcements on this interface
+            cmd = 'sysctl -q -w net.ipv6.conf.{}.accept_ra=0'.format(self._ifname)
+            self._cmd(cmd)
 
-        cmd  = 'start-stop-daemon --start --quiet --pidfile ' + \
-            self._dhcpv6_pid_file
-        cmd += ' --exec /sbin/dhclient --'
-        # now pass arguments to dhclient binary
-        cmd += ' -6 -nw -cf {} -pf {} -lf {} {}'.format(
-            self._dhcpv6_cfg_file, self._dhcpv6_pid_file, self._dhcpv6_lease_file, self._ifname)
-        self._cmd(cmd)
+            # assemble command-line to start DHCPv6 client (dhclient)
+            cmd  = 'start-stop-daemon --start --quiet --pidfile ' + \
+                self._dhcpv6_pid_file
+            cmd += ' --exec /sbin/dhclient --'
+            # now pass arguments to dhclient binary
+            cmd += ' -6 -nw -cf {} -pf {} -lf {} {}'.format(
+                self._dhcpv6_cfg_file, self._dhcpv6_pid_file, self._dhcpv6_lease_file, self._ifname)
+            self._cmd(cmd)
 
-    def del_dhcpv6(self):
+
+    def _del_dhcpv6(self):
         """
         De-configure interface as DHCPv6 clinet. All auto generated files like
         pid, config and lease will be removed.
@@ -1281,7 +1313,6 @@ class BondIf(EthernetIf):
 
 
 class WireGuardIf(Interface):
-
     """
     Wireguard interface class, contains a comnfig dictionary since
     wireguard VPN is being comnfigured via the wg command rather than
@@ -1293,11 +1324,11 @@ class WireGuardIf(Interface):
     >>> from vyos.ifconfig import WireGuardIf as wg_if
     >>> wg_intfc = wg_if("wg01")
     >>> print (wg_intfc.wg_config)
-    {'private-key': None, 'keepalive': 0, 'endpoint': None, 'port': 0, 
+    {'private-key': None, 'keepalive': 0, 'endpoint': None, 'port': 0,
     'allowed-ips': [], 'pubkey': None, 'fwmark': 0, 'psk': '/dev/null'}
     >>> wg_intfc.wg_config['keepalive'] = 100
     >>> print (wg_intfc.wg_config)
-    {'private-key': None, 'keepalive': 100, 'endpoint': None, 'port': 0, 
+    {'private-key': None, 'keepalive': 100, 'endpoint': None, 'port': 0,
     'allowed-ips': [], 'pubkey': None, 'fwmark': 0, 'psk': '/dev/null'}
     """
 
diff --git a/src/conf_mode/interface-bonding.py b/src/conf_mode/interface-bonding.py
index 2ec764965..81667289d 100755
--- a/src/conf_mode/interface-bonding.py
+++ b/src/conf_mode/interface-bonding.py
@@ -85,12 +85,6 @@ def apply_vlan_config(vlan, config):
     if type(vlan) != type(EthernetIf("lo")):
         raise TypeError()
 
-    # Configure interface address(es)
-    for addr in config['address_remove']:
-        vlan.del_addr(addr)
-    for addr in config['address']:
-        vlan.add_addr(addr)
-
     # update interface description used e.g. within SNMP
     vlan.ifalias = config['description']
     # ignore link state changes
@@ -107,6 +101,14 @@ def apply_vlan_config(vlan, config):
     else:
         vlan.state = 'up'
 
+    # Configure interface address(es)
+    # - not longer required addresses get removed first
+    # - newly addresses will be added second
+    for addr in config['address_remove']:
+        vlan.del_addr(addr)
+    for addr in config['address']:
+        vlan.add_addr(addr)
+
 
 def get_config():
     # initialize kernel module if not loaded
@@ -139,6 +141,11 @@ def get_config():
     if conf.exists('address'):
         bond['address'] = conf.return_values('address')
 
+    # get interface addresses (currently effective) - to determine which
+    # address is no longer valid and needs to be removed from the bond
+    eff_addr = conf.return_effective_values('address')
+    bond['address_remove'] = list_diff(eff_addr, bond['address'])
+
     # ARP link monitoring frequency in milliseconds
     if conf.exists('arp-monitor interval'):
         bond['arp_mon_intvl'] = int(conf.return_value('arp-monitor interval'))
@@ -209,12 +216,6 @@ def get_config():
     if conf.exists('member interface'):
         bond['member'] = conf.return_values('member interface')
 
-    # get interface addresses (currently effective) - to determine which
-    # address is no longer valid and needs to be removed from the bond
-    eff_addr = conf.return_effective_values('address')
-    act_addr = conf.return_values('address')
-    bond['address_remove'] = list_diff(eff_addr, act_addr)
-
     # Primary device interface
     if conf.exists('primary'):
         bond['primary'] = conf.return_value('primary')
@@ -314,6 +315,7 @@ def apply(bond):
     b = BondIf(bond['intf'])
 
     if bond['deleted']:
+        #
         # delete bonding interface
         b.remove()
     else:
@@ -326,12 +328,6 @@ def apply(bond):
         for intf in b.get_slaves():
             b.del_port(intf)
 
-        # Configure interface address(es)
-        for addr in bond['address_remove']:
-            b.del_addr(addr)
-        for addr in bond['address']:
-            b.add_addr(addr)
-
         # ARP link monitoring frequency
         b.arp_interval = bond['arp_mon_intvl']
         # reset miimon on arp-montior deletion
@@ -348,8 +344,8 @@ def apply(bond):
         # from the kernel side this looks valid to me. We won't run into an error
         # when a user added manual adresses which would result in having more
         # then 16 adresses in total.
-        cur_addr = list(map(str, b.arp_ip_target.split()))
-        for addr in cur_addr:
+        arp_tgt_addr = list(map(str, b.arp_ip_target.split()))
+        for addr in arp_tgt_addr:
             b.arp_ip_target = '-' + addr
 
         # Add configured ARP target addresses
@@ -391,6 +387,20 @@ def apply(bond):
         for intf in bond['member']:
             b.add_port(intf)
 
+        # As the bond interface is always disabled first when changing
+        # parameters we will only re-enable the interface if it is not
+        # administratively disabled
+        if not bond['disable']:
+            b.state = 'up'
+
+        # Configure interface address(es)
+        # - not longer required addresses get removed first
+        # - newly addresses will be added second
+        for addr in bond['address_remove']:
+            b.del_addr(addr)
+        for addr in bond['address']:
+            b.add_addr(addr)
+
         # remove no longer required service VLAN interfaces (vif-s)
         for vif_s in bond['vif_s_remove']:
             b.del_vlan(vif_s)
@@ -420,12 +430,6 @@ def apply(bond):
             vlan = b.add_vlan(vif['id'])
             apply_vlan_config(vlan, vif)
 
-        # As the bond interface is always disabled first when changing
-        # parameters we will only re-enable the interface if it is not
-        # administratively disabled
-        if not bond['disable']:
-            b.state = 'up'
-
     return None
 
 if __name__ == '__main__':
diff --git a/src/conf_mode/interface-bridge.py b/src/conf_mode/interface-bridge.py
index b165428ee..8996fceec 100755
--- a/src/conf_mode/interface-bridge.py
+++ b/src/conf_mode/interface-bridge.py
@@ -61,9 +61,7 @@ def get_config():
     # Check if bridge has been removed
     if not conf.exists('interfaces bridge ' + bridge['intf']):
         bridge['deleted'] = True
-        # we should not bail out early here b/c we should
-        # find possible DHCP interfaces later on.
-        # DHCP interfaces invoke dhclient which should be stopped, too
+        return bridge
 
     # set new configuration level
     conf.set_level('interfaces bridge ' + bridge['intf'])
@@ -72,6 +70,11 @@ def get_config():
     if conf.exists('address'):
         bridge['address'] = conf.return_values('address')
 
+    # Determine interface addresses (currently effective) - to determine which
+    # address is no longer valid and needs to be removed from the bridge
+    eff_addr = conf.return_effective_values('address')
+    bridge['address_remove'] = list_diff(eff_addr, bridge['address'])
+
     # retrieve aging - how long addresses are retained
     if conf.exists('aging'):
         bridge['aging'] = int(conf.return_value('aging'))
@@ -137,12 +140,6 @@ def get_config():
     act_intf = conf.list_nodes('member interface')
     bridge['member_remove'] = list_diff(eff_intf, act_intf)
 
-    # Determine interface addresses (currently effective) - to determine which
-    # address is no longer valid and needs to be removed from the bridge
-    eff_addr = conf.return_effective_values('address')
-    act_addr = conf.return_values('address')
-    bridge['address_remove'] = list_diff(eff_addr, act_addr)
-
     # Priority for this bridge
     if conf.exists('priority'):
         bridge['priority'] = int(conf.return_value('priority'))
@@ -225,23 +222,13 @@ def apply(bridge):
         if bridge['disable']:
             br.state = 'down'
 
-        # remove configured network interface addresses/DHCP(v6) configuration
+        # Configure interface address(es)
+        # - not longer required addresses get removed first
+        # - newly addresses will be added second
         for addr in bridge['address_remove']:
-            if addr == 'dhcp':
-                br.del_dhcp()
-            elif addr == 'dhcpv6':
-                br.del_dhcpv6()
-            else:
-                br.del_addr(addr)
-
-        # add configured network interface addresses/DHCP(v6) configuration
+            br.del_addr(addr)
         for addr in bridge['address']:
-            if addr == 'dhcp':
-                br.set_dhcp()
-            elif addr == 'dhcpv6':
-                br.set_dhcpv6()
-            else:
-                br.add_addr(addr)
+            br.add_addr(addr)
 
         # configure additional bridge member options
         for member in bridge['member']: