bonding: T2515: preserve interface admin state when removing from bond

Removing a member from a bond/LACP will turn the physical interface always in
admin-down state. This is invalid, the interface should be placed into the state
configured on the VyOS CLI.

Smoketest on bond interfaces is extended to check this behavior.

3 files changed, 59 insertions, 29 deletions

diff --git a/python/vyos/ifconfig/bond.py b/python/vyos/ifconfig/bond.py
index 67dcd2b69..c33cf30bf 100644
--- a/python/vyos/ifconfig/bond.py
+++ b/python/vyos/ifconfig/bond.py
@@ -1,4 +1,4 @@
-# Copyright 2019 VyOS maintainers and contributors <maintainers@vyos.io>
+# Copyright 2019-2020 VyOS maintainers and contributors <maintainers@vyos.io>
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -381,9 +381,14 @@ class BondIf(Interface):
         # Some interface options can only be changed if the interface is
         # administratively down
         if self.get_admin_state() == 'down':
-            # Delete bond member port(s)
+            # Remove ALL bond member interfaces
             for interface in self.get_slaves():
                 self.del_port(interface)
+                # Removing an interface from a bond will always place the underlaying
+                # physical interface in admin-down state! If physical interface is
+                # not disabled, re-enable it.
+                if not vyos_dict_search(f'member.interface_remove.{interface}.disable', config):
+                    Interface(interface).set_admin_state('up')
 
             # Bonding policy/mode
             value = config.get('mode')
@@ -391,13 +396,12 @@ class BondIf(Interface):
 
             # Add (enslave) interfaces to bond
             value = vyos_dict_search('member.interface', config)
-            if value:
-                for interface in value:
-                    # if we've come here we already verified the interface
-                    # does not have an addresses configured so just flush
-                    # any remaining ones
-                    Interface(interface).flush_addrs()
-                    self.add_port(interface)
+            for interface in (value or []):
+                # if we've come here we already verified the interface
+                # does not have an addresses configured so just flush
+                # any remaining ones
+                Interface(interface).flush_addrs()
+                self.add_port(interface)
 
         # Primary device interface - must be set after 'mode'
         value = config.get('primary')
diff --git a/smoketest/scripts/cli/test_interfaces_bonding.py b/smoketest/scripts/cli/test_interfaces_bonding.py
index e3d3b25ee..b165883b9 100755
--- a/smoketest/scripts/cli/test_interfaces_bonding.py
+++ b/smoketest/scripts/cli/test_interfaces_bonding.py
@@ -20,6 +20,7 @@ import unittest
 from base_interfaces_test import BasicInterfaceTest
 
 from vyos.ifconfig import Section
+from vyos.ifconfig.interface import Interface
 from vyos.configsession import ConfigSessionError
 from vyos.util import read_file
 
@@ -57,5 +58,28 @@ class BondingInterfaceTest(BasicInterfaceTest.BaseTest):
             slaves = read_file(f'/sys/class/net/{interface}/bonding/slaves').split()
             self.assertListEqual(slaves, self._members)
 
+    def test_remove_member(self):
+        """ T2515: when removing a bond member the interface must be admin-up again """
+
+        # configure member interfaces
+        for interface in self._interfaces:
+            for option in self._options.get(interface, []):
+                self.session.set(self._base_path + [interface] + option.split())
+
+        self.session.commit()
+
+        # remove single bond member port
+        for interface in self._interfaces:
+            remove_member = self._members[0]
+            self.session.delete(self._base_path + [interface, 'member', 'interface', remove_member])
+
+        self.session.commit()
+
+        # removed member port must be admin-up
+        for interface in self._interfaces:
+            remove_member = self._members[0]
+            state = Interface(remove_member).get_admin_state()
+            self.assertEqual('up', state)
+
 if __name__ == '__main__':
     unittest.main()
diff --git a/src/conf_mode/interfaces-bonding.py b/src/conf_mode/interfaces-bonding.py
index 16e6e4f6e..a9679b47c 100755
--- a/src/conf_mode/interfaces-bonding.py
+++ b/src/conf_mode/interfaces-bonding.py
@@ -29,6 +29,7 @@ from vyos.configverify import verify_source_interface
 from vyos.configverify import verify_vlan_config
 from vyos.configverify import verify_vrf
 from vyos.ifconfig import BondIf
+from vyos.ifconfig import Section
 from vyos.validate import is_member
 from vyos.validate import has_address_configured
 from vyos import ConfigError
@@ -69,31 +70,33 @@ def get_config(config=None):
     # into a dictionary - we will use this to add additional information
     # later on for wach member
     if 'member' in bond and 'interface' in bond['member']:
-        # first convert it to a list if only one member is given
-        if isinstance(bond['member']['interface'], str):
-            bond['member']['interface'] = [bond['member']['interface']]
-
-        tmp={}
-        for interface in bond['member']['interface']:
-            tmp.update({interface: {}})
-
-        bond['member']['interface'] = tmp
+        # convert list if member interfaces to a dictionary
+        bond['member']['interface'] = dict.fromkeys(
+            bond['member']['interface'], {})
 
     if 'mode' in bond:
         bond['mode'] = get_bond_mode(bond['mode'])
 
     tmp = leaf_node_changed(conf, ['mode'])
-    if tmp:
-        bond.update({'shutdown_required': ''})
+    if tmp: bond.update({'shutdown_required': {}})
 
     # determine which members have been removed
-    tmp = leaf_node_changed(conf, ['member', 'interface'])
-    if tmp:
-        bond.update({'shutdown_required': ''})
-        if 'member' in bond:
-            bond['member'].update({'interface_remove': tmp })
-        else:
-            bond.update({'member': {'interface_remove': tmp }})
+    interfaces_removed = leaf_node_changed(conf, ['member', 'interface'])
+    if interfaces_removed:
+        bond.update({'shutdown_required': {}})
+        if 'member' not in bond:
+            bond.update({'member': {}})
+
+        tmp = {}
+        for interface in interfaces_removed:
+            section = Section.section(interface) # this will be 'ethernet' for 'eth0'
+            if conf.exists(['insterfaces', section, interface, 'disable']):
+                tmp.update({interface : {'disable': ''}})
+            else:
+                tmp.update({interface : {}})
+
+        # also present the interfaces to be removed from the bond as dictionary
+        bond['member'].update({'interface_remove': tmp})
 
     if 'member' in bond and 'interface' in bond['member']:
         for interface, interface_config in bond['member']['interface'].items():
@@ -109,8 +112,7 @@ def get_config(config=None):
 
             # bond members must not have an assigned address
             tmp = has_address_configured(conf, interface)
-            if tmp:
-                interface_config.update({'has_address' : ''})
+            if tmp: interface_config.update({'has_address' : ''})
 
     return bond