Merge pull request #2393 from vyos/mergify/bp/sagitta/pr-2277

bonding: T5254: Fixed changing ethernet when it is a bond member (backport #2277)

2 files changed, 239 insertions, 36 deletions

diff --git a/src/conf_mode/interfaces-bonding.py b/src/conf_mode/interfaces-bonding.py
index 0bd306ed0..1179e3e4f 100755
--- a/src/conf_mode/interfaces-bonding.py
+++ b/src/conf_mode/interfaces-bonding.py
@@ -18,7 +18,6 @@ import os
 
 from sys import exit
 from netifaces import interfaces
-
 from vyos.config import Config
 from vyos.configdict import get_interface_dict
 from vyos.configdict import is_node_changed
@@ -34,10 +33,13 @@ 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.ethernet import EthernetIf
 from vyos.ifconfig import Section
 from vyos.utils.dict import dict_search
+from vyos.utils.dict import dict_to_paths_values
 from vyos.configdict import has_address_configured
 from vyos.configdict import has_vrf_configured
+from vyos.configdep import set_dependents, call_dependents
 from vyos import ConfigError
 from vyos import airbag
 airbag.enable()
@@ -90,7 +92,6 @@ def get_config(config=None):
 
     # determine which members have been removed
     interfaces_removed = leaf_node_changed(conf, base + [ifname, 'member', 'interface'])
-
     # Reset config level to interfaces
     old_level = conf.get_level()
     conf.set_level(['interfaces'])
@@ -102,6 +103,10 @@ def get_config(config=None):
 
         tmp = {}
         for interface in interfaces_removed:
+            # if member is deleted from bond, add dependencies to call
+            # ethernet commit again in apply function
+            # to apply options under ethernet section
+            set_dependents('ethernet', conf, interface)
             section = Section.section(interface) # this will be 'ethernet' for 'eth0'
             if conf.exists([section, interface, 'disable']):
                 tmp[interface] = {'disable': ''}
@@ -116,9 +121,21 @@ def get_config(config=None):
 
     if dict_search('member.interface', bond):
         for interface, interface_config in bond['member']['interface'].items():
+
+            interface_ethernet_config = conf.get_config_dict(
+                ['interfaces', 'ethernet', interface],
+                key_mangling=('-', '_'),
+                get_first_key=True,
+                no_tag_node_value_mangle=True,
+                with_defaults=False,
+                with_recursive_defaults=False)
+
+            interface_config['config_paths'] = dict_to_paths_values(interface_ethernet_config)
+
             # Check if member interface is a new member
             if not conf.exists_effective(base + [ifname, 'member', 'interface', interface]):
                 bond['shutdown_required'] = {}
+                interface_config['new_added'] = {}
 
             # Check if member interface is disabled
             conf.set_level(['interfaces'])
@@ -151,7 +168,6 @@ def get_config(config=None):
             # bond members must not have a VRF attached
             tmp = has_vrf_configured(conf, interface)
             if tmp: interface_config['has_vrf'] = {}
-
     return bond
 
 
@@ -212,6 +228,14 @@ def verify(bond):
             if 'has_vrf' in interface_config:
                 raise ConfigError(error_msg + 'it has a VRF assigned!')
 
+            if 'new_added' in interface_config and 'config_paths' in interface_config:
+                for option_path, option_value in interface_config['config_paths'].items():
+                    if option_path in EthernetIf.get_bond_member_allowed_options() :
+                        continue
+                    if option_path in BondIf.get_inherit_bond_options():
+                        continue
+                    raise ConfigError(error_msg + f'it has a "{option_path.replace(".", " ")}" assigned!')
+
     if 'primary' in bond:
         if bond['primary'] not in bond['member']['interface']:
             raise ConfigError(f'Primary interface of bond "{bond_name}" must be a member interface')
@@ -227,13 +251,17 @@ def generate(bond):
 
 def apply(bond):
     b = BondIf(bond['ifname'])
-
     if 'deleted' in bond:
         # delete interface
         b.remove()
     else:
         b.update(bond)
-
+    if dict_search('member.interface_remove', bond):
+        try:
+            call_dependents()
+        except ConfigError:
+            raise ConfigError('Error in updating ethernet interface '
+                              'after deleting it from bond')
     return None
 
 if __name__ == '__main__':
diff --git a/src/conf_mode/interfaces-ethernet.py b/src/conf_mode/interfaces-ethernet.py
index f3e65ad5e..7374a29f7 100755
--- a/src/conf_mode/interfaces-ethernet.py
+++ b/src/conf_mode/interfaces-ethernet.py
@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import pprint
 
 from glob import glob
 from sys import exit
@@ -35,6 +36,7 @@ from vyos.configverify import verify_vrf
 from vyos.configverify import verify_bond_bridge_member
 from vyos.ethtool import Ethtool
 from vyos.ifconfig import EthernetIf
+from vyos.ifconfig import BondIf
 from vyos.pki import find_chain
 from vyos.pki import encode_certificate
 from vyos.pki import load_certificate
@@ -42,6 +44,9 @@ from vyos.pki import wrap_private_key
 from vyos.template import render
 from vyos.utils.process import call
 from vyos.utils.dict import dict_search
+from vyos.utils.dict import dict_to_paths_values
+from vyos.utils.dict import dict_set
+from vyos.utils.dict import dict_delete
 from vyos.utils.file import write_file
 from vyos import ConfigError
 from vyos import airbag
@@ -51,6 +56,90 @@ airbag.enable()
 cfg_dir = '/run/wpa_supplicant'
 wpa_suppl_conf = '/run/wpa_supplicant/{ifname}.conf'
 
+def update_bond_options(conf: Config, eth_conf: dict) -> list:
+    """
+    Return list of blocked options if interface is a bond member
+    :param conf: Config object
+    :type conf: Config
+    :param eth_conf: Ethernet config dictionary
+    :type eth_conf: dict
+    :return: List of blocked options
+    :rtype: list
+    """
+    blocked_list = []
+    bond_name = list(eth_conf['is_bond_member'].keys())[0]
+    config_without_defaults = conf.get_config_dict(
+        ['interfaces', 'ethernet', eth_conf['ifname']],
+        key_mangling=('-', '_'),
+        get_first_key=True,
+        no_tag_node_value_mangle=True,
+        with_defaults=False,
+        with_recursive_defaults=False)
+    config_with_defaults = conf.get_config_dict(
+        ['interfaces', 'ethernet', eth_conf['ifname']],
+        key_mangling=('-', '_'),
+        get_first_key=True,
+        no_tag_node_value_mangle=True,
+        with_defaults=True,
+        with_recursive_defaults=True)
+    bond_config_with_defaults = conf.get_config_dict(
+        ['interfaces', 'bonding', bond_name],
+        key_mangling=('-', '_'),
+        get_first_key=True,
+        no_tag_node_value_mangle=True,
+        with_defaults=True,
+        with_recursive_defaults=True)
+    eth_dict_paths = dict_to_paths_values(config_without_defaults)
+    eth_path_base = ['interfaces', 'ethernet', eth_conf['ifname']]
+
+    #if option is configured under ethernet section
+    for option_path, option_value in eth_dict_paths.items():
+        bond_option_value = dict_search(option_path, bond_config_with_defaults)
+
+        #If option is allowed for changing then continue
+        if option_path in EthernetIf.get_bond_member_allowed_options():
+            continue
+        # if option is inherited from bond then set valued from bond interface
+        if option_path in BondIf.get_inherit_bond_options():
+            # If option equals to bond option then do nothing
+            if option_value == bond_option_value:
+                continue
+            else:
+                # if ethernet has option and bond interface has
+                # then copy it from bond
+                if bond_option_value is not None:
+                    if is_node_changed(conf, eth_path_base + option_path.split('.')):
+                        Warning(
+                            f'Cannot apply "{option_path.replace(".", " ")}" to "{option_value}".' \
+                            f' Interface "{eth_conf["ifname"]}" is a bond member.' \
+                            f' Option is inherited from bond "{bond_name}"')
+                    dict_set(option_path, bond_option_value, eth_conf)
+                    continue
+                # if ethernet has option and bond interface does not have
+                # then delete it form dict and do not apply it
+                else:
+                    if is_node_changed(conf, eth_path_base + option_path.split('.')):
+                        Warning(
+                            f'Cannot apply "{option_path.replace(".", " ")}".' \
+                            f' Interface "{eth_conf["ifname"]}" is a bond member.' \
+                            f' Option is inherited from bond "{bond_name}"')
+                    dict_delete(option_path, eth_conf)
+        blocked_list.append(option_path)
+
+    # if inherited option is not configured under ethernet section but configured under bond section
+    for option_path in BondIf.get_inherit_bond_options():
+        bond_option_value = dict_search(option_path, bond_config_with_defaults)
+        if bond_option_value is not None:
+            if option_path not in eth_dict_paths:
+                if is_node_changed(conf, eth_path_base + option_path.split('.')):
+                    Warning(
+                        f'Cannot apply "{option_path.replace(".", " ")}" to "{dict_search(option_path, config_with_defaults)}".' \
+                        f' Interface "{eth_conf["ifname"]}" is a bond member. ' \
+                        f'Option is inherited from bond "{bond_name}"')
+                dict_set(option_path, bond_option_value, eth_conf)
+    eth_conf['bond_blocked_changes'] = blocked_list
+    return None
+
 def get_config(config=None):
     """
     Retrive CLI config as dictionary. Dictionary can never be empty, as at least the
@@ -68,6 +157,8 @@ def get_config(config=None):
 
     base = ['interfaces', 'ethernet']
     ifname, ethernet = get_interface_dict(conf, base)
+    if 'is_bond_member' in ethernet:
+        update_bond_options(conf, ethernet)
 
     if 'deleted' not in ethernet:
        if pki: ethernet['pki'] = pki
@@ -80,26 +171,20 @@ def get_config(config=None):
 
     return ethernet
 
-def verify(ethernet):
-    if 'deleted' in ethernet:
-        return None
 
-    ifname = ethernet['ifname']
-    verify_interface_exists(ifname)
-    verify_mtu(ethernet)
-    verify_mtu_ipv6(ethernet)
-    verify_dhcpv6(ethernet)
-    verify_address(ethernet)
-    verify_vrf(ethernet)
-    verify_bond_bridge_member(ethernet)
-    verify_eapol(ethernet)
-    verify_mirror_redirect(ethernet)
 
-    ethtool = Ethtool(ifname)
-    # No need to check speed and duplex keys as both have default values.
+def verify_speed_duplex(ethernet: dict, ethtool: Ethtool):
+    """
+     Verify speed and duplex
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    :param ethtool: Ethernet object
+    :type ethtool: Ethtool
+    """
     if ((ethernet['speed'] == 'auto' and ethernet['duplex'] != 'auto') or
-        (ethernet['speed'] != 'auto' and ethernet['duplex'] == 'auto')):
-            raise ConfigError('Speed/Duplex missmatch. Must be both auto or manually configured')
+            (ethernet['speed'] != 'auto' and ethernet['duplex'] == 'auto')):
+        raise ConfigError(
+            'Speed/Duplex missmatch. Must be both auto or manually configured')
 
     if ethernet['speed'] != 'auto' and ethernet['duplex'] != 'auto':
         # We need to verify if the requested speed and duplex setting is
@@ -107,37 +192,66 @@ def verify(ethernet):
         speed = ethernet['speed']
         duplex = ethernet['duplex']
         if not ethtool.check_speed_duplex(speed, duplex):
-            raise ConfigError(f'Adapter does not support changing speed and duplex '\
-                              f'settings to: {speed}/{duplex}!')
+            raise ConfigError(
+                f'Adapter does not support changing speed ' \
+                f'and duplex settings to: {speed}/{duplex}!')
 
+
+def verify_flow_control(ethernet: dict, ethtool: Ethtool):
+    """
+     Verify flow control
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    :param ethtool: Ethernet object
+    :type ethtool: Ethtool
+    """
     if 'disable_flow_control' in ethernet:
         if not ethtool.check_flow_control():
-            raise ConfigError('Adapter does not support changing flow-control settings!')
+            raise ConfigError(
+                'Adapter does not support changing flow-control settings!')
+
 
+def verify_ring_buffer(ethernet: dict, ethtool: Ethtool):
+    """
+     Verify ring buffer
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    :param ethtool: Ethernet object
+    :type ethtool: Ethtool
+    """
     if 'ring_buffer' in ethernet:
         max_rx = ethtool.get_ring_buffer_max('rx')
         if not max_rx:
-            raise ConfigError('Driver does not support RX ring-buffer configuration!')
+            raise ConfigError(
+                'Driver does not support RX ring-buffer configuration!')
 
         max_tx = ethtool.get_ring_buffer_max('tx')
         if not max_tx:
-            raise ConfigError('Driver does not support TX ring-buffer configuration!')
+            raise ConfigError(
+                'Driver does not support TX ring-buffer configuration!')
 
         rx = dict_search('ring_buffer.rx', ethernet)
         if rx and int(rx) > int(max_rx):
-            raise ConfigError(f'Driver only supports a maximum RX ring-buffer '\
+            raise ConfigError(f'Driver only supports a maximum RX ring-buffer ' \
                               f'size of "{max_rx}" bytes!')
 
         tx = dict_search('ring_buffer.tx', ethernet)
         if tx and int(tx) > int(max_tx):
-            raise ConfigError(f'Driver only supports a maximum TX ring-buffer '\
+            raise ConfigError(f'Driver only supports a maximum TX ring-buffer ' \
                               f'size of "{max_tx}" bytes!')
 
-    # verify offloading capabilities
+
+def verify_offload(ethernet: dict, ethtool: Ethtool):
+    """
+     Verify offloading capabilities
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    :param ethtool: Ethernet object
+    :type ethtool: Ethtool
+    """
     if dict_search('offload.rps', ethernet) != None:
-        if not os.path.exists(f'/sys/class/net/{ifname}/queues/rx-0/rps_cpus'):
+        if not os.path.exists(f'/sys/class/net/{ethernet["ifname"]}/queues/rx-0/rps_cpus'):
             raise ConfigError('Interface does not suport RPS!')
-
     driver = ethtool.get_driver_name()
     # T3342 - Xen driver requires special treatment
     if driver == 'vif':
@@ -145,14 +259,73 @@ def verify(ethernet):
             raise ConfigError('Xen netback drivers requires scatter-gatter offloading '\
                               'for MTU size larger then 1500 bytes')
 
-    if {'is_bond_member', 'mac'} <= set(ethernet):
-        Warning(f'changing mac address "{mac}" will be ignored as "{ifname}" ' \
-                f'is a member of bond "{is_bond_member}"'.format(**ethernet))
 
+def verify_allowedbond_changes(ethernet: dict):
+    """
+     Verify changed options if interface is in bonding
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    """
+    if 'bond_blocked_changes' in ethernet:
+        for option in ethernet['bond_blocked_changes']:
+            raise ConfigError(f'Cannot configure "{option.replace(".", " ")}"' \
+                              f' on interface "{ethernet["ifname"]}".' \
+                              f' Interface is a bond member')
+
+
+def verify(ethernet):
+    if 'deleted' in ethernet:
+        return None
+    if 'is_bond_member' in ethernet:
+        verify_bond_member(ethernet)
+    else:
+        verify_ethernet(ethernet)
+
+
+def verify_bond_member(ethernet):
+    """
+     Verification function for ethernet interface which is in bonding
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    """
+    ifname = ethernet['ifname']
+    verify_interface_exists(ifname)
+    verify_eapol(ethernet)
+    verify_mirror_redirect(ethernet)
+    ethtool = Ethtool(ifname)
+    verify_speed_duplex(ethernet, ethtool)
+    verify_flow_control(ethernet, ethtool)
+    verify_ring_buffer(ethernet, ethtool)
+    verify_offload(ethernet, ethtool)
+    verify_allowedbond_changes(ethernet)
+
+def verify_ethernet(ethernet):
+    """
+     Verification function for simple ethernet interface
+    :param ethernet: dictionary which is received from get_interface_dict
+    :type ethernet: dict
+    """
+    ifname = ethernet['ifname']
+    verify_interface_exists(ifname)
+    verify_mtu(ethernet)
+    verify_mtu_ipv6(ethernet)
+    verify_dhcpv6(ethernet)
+    verify_address(ethernet)
+    verify_vrf(ethernet)
+    verify_bond_bridge_member(ethernet)
+    verify_eapol(ethernet)
+    verify_mirror_redirect(ethernet)
+    ethtool = Ethtool(ifname)
+    # No need to check speed and duplex keys as both have default values.
+    verify_speed_duplex(ethernet, ethtool)
+    verify_flow_control(ethernet, ethtool)
+    verify_ring_buffer(ethernet, ethtool)
+    verify_offload(ethernet, ethtool)
     # use common function to verify VLAN configuration
     verify_vlan_config(ethernet)
     return None
 
+
 def generate(ethernet):
     # render real configuration file once
     wpa_supplicant_conf = wpa_suppl_conf.format(**ethernet)
@@ -192,7 +365,8 @@ def generate(ethernet):
                 pki_ca_cert = ethernet['pki']['ca'][ca_cert_name]
                 loaded_ca_cert = load_certificate(pki_ca_cert['certificate'])
                 ca_full_chain = find_chain(loaded_ca_cert, loaded_ca_certs)
-                ca_chains.append('\n'.join(encode_certificate(c) for c in ca_full_chain))
+                ca_chains.append(
+                    '\n'.join(encode_certificate(c) for c in ca_full_chain))
 
             write_file(ca_cert_file_path, '\n'.join(ca_chains))
 
@@ -219,6 +393,7 @@ if __name__ == '__main__':
         c = get_config()
         verify(c)
         generate(c)
+
         apply(c)
     except ConfigError as e:
         print(e)