bonding: T1614: T532: new commit validators

As in the past during the priority race of the bash script invalid configuration
could appear in the CLI and are de-synced from the kernle state, e.g. some
bonding modes do not support arp_interval.

This is no longer allowed and added to the migration script so that the config
again represents the truth.

2 files changed, 65 insertions, 7 deletions

diff --git a/src/conf_mode/interface-bonding.py b/src/conf_mode/interface-bonding.py
index b01018e5b..03d28954d 100755
--- a/src/conf_mode/interface-bonding.py
+++ b/src/conf_mode/interface-bonding.py
@@ -249,7 +249,7 @@ def get_config():
 
     # ARP link monitoring frequency in milliseconds
     if conf.exists('arp-monitor interval'):
-        bond['arp_mon_intvl'] = conf.return_value('arp-monitor interval')
+        bond['arp_mon_intvl'] = int(conf.return_value('arp-monitor interval'))
 
     # IP address to use for ARP monitoring
     if conf.exists('arp-monitor target'):
@@ -374,6 +374,43 @@ def verify(bond):
             if vif['id'] == vif_s['id']:
                 raise ConfigError('Can not use identical ID on vif and vif-s interface')
 
+
+    conf = Config()
+    for intf in bond['member']:
+        # we can not add disabled slave interfaces to our bond
+        if conf.exists('interfaces ethernet ' + intf + ' disable'):
+            raise ConfigError('can not add disabled interface {} to {}'.format(intf, bond['intf']))
+
+        # can not add interfaces with an assigned address to a bond
+        if conf.exists('interfaces ethernet ' + intf + ' address'):
+            raise ConfigError('can not add interface {} with an assigned address to {}'.format(intf, bond['intf']))
+
+        # bond members are not allowed to be bridge members, too
+        for bridge in conf.list_nodes('interfaces bridge'):
+            if conf.exists('interfaces bridge ' + bridge + ' member interface ' + intf):
+                raise ConfigError('can not add interface {} that is part of bridge {} to {}'.format(intf, bridge, bond['intf']))
+
+        # bond members are not allowed to be vrrp members, too
+        for vrrp in conf.list_nodes('high-availability vrrp group'):
+            if conf.exists('high-availability vrrp group ' + vrrp + ' interface ' + intf):
+                raise ConfigError('can not add interface {} which belongs to a VRRP group to {}'.format(intf, bond['intf']))
+
+        # bond members are not allowed to be underlaying psuedo-ethernet devices
+        for peth in conf.list_nodes('interfaces pseudo-ethernet'):
+            if conf.exists('interfaces pseudo-ethernet ' + peth + ' link ' + intf):
+                raise ConfigError('can not add interface {} used by pseudo-ethernet {} to {}'.format(intf, peth, bond['intf']))
+
+    if bond['primary']:
+        if bond['primary'] not in bond['member']:
+            raise ConfigError('primary interface must be a member interface of {}'.format(bond['intf']))
+
+        if bond['mode'] not in ['active-backup', 'balance-tlb', 'balance-alb']:
+            raise ConfigError('primary interface only works for mode active-backup, transmit-load-balance or adaptive-load-balance')
+
+    if bond['arp_mon_intvl'] > 0:
+        if bond['mode'] in ['802.3ad', 'balance-tlb', 'balance-alb']:
+            raise ConfigError('ARP link monitoring does not work for mode 802.3ad, transmit-load-balance or adaptive-load-balance')
+
     return None
 
 
@@ -392,6 +429,11 @@ def apply(bond):
         # Always disable the bond prior changing anything
         b.state = 'down'
 
+        # The bonding mode can not be changed when there are interfaces enslaved
+        # to this bond, thus we will free all interfaces from the bond first!
+        for intf in b.get_slaves():
+            b.del_port(intf)
+
         # Configure interface address(es)
         for addr in bond['address_remove']:
             b.del_addr(addr)
@@ -444,17 +486,14 @@ def apply(bond):
         if bond['mac']:
             b.mac = bond['mac']
 
-        # The bonding mode can not be changed when there are interfaces enslaved
-        # to this bond, thus we will free all interfaces from the bond first!
-        for intf in b.get_slaves():
-            b.del_port(intf)
-
         # Bonding policy
         b.mode = bond['mode']
         # Maximum Transmission Unit (MTU)
         b.mtu = bond['mtu']
+
         # Primary device interface
-        b.primary = bond['primary']
+        if bond['primary']:
+            b.primary = bond['primary']
 
         # Add (enslave) interfaces to bond
         for intf in bond['member']:
diff --git a/src/migration-scripts/interfaces/1-to-2 b/src/migration-scripts/interfaces/1-to-2
index 10d542d1d..050137318 100755
--- a/src/migration-scripts/interfaces/1-to-2
+++ b/src/migration-scripts/interfaces/1-to-2
@@ -36,6 +36,25 @@ else:
             # create new bond member interface
             config.set(base + [bond, 'member', 'interface'], value=intf, replace=False)
 
+    #
+    # some combinations were allowed in the past from a CLI perspective
+    # but the kernel overwrote them - remove from CLI to not confuse the users.
+    # In addition new consitency checks are in place so users can't repeat the
+    # mistake. One of those nice issues is https://phabricator.vyos.net/T532
+    for bond in config.list_nodes(base):
+        if config.exists(base + [bond, 'arp-monitor', 'interval']) and config.exists(base + [bond, 'mode']):
+            mode = config.return_value(base + [bond, 'mode'])
+            if mode in ['802.3ad', 'transmit-load-balance', 'adaptive-load-balance']:
+                intvl = int(config.return_value(base + [bond, 'arp-monitor', 'interval']))
+                if intvl > 0:
+                    # this is not allowed and the linux kernel replies with:
+                    # option arp_interval: mode dependency failed, not supported in mode 802.3ad(4)
+                    # option arp_interval: mode dependency failed, not supported in mode balance-alb(6)
+                    # option arp_interval: mode dependency failed, not supported in mode balance-tlb(5)
+                    #
+                    # so we simply disable arp_interval by setting it to 0 and miimon will take care about the link
+                    config.set(base + [bond, 'arp-monitor', 'interval'], value='0')
+
     try:
         with open(file_name, 'w') as f:
             f.write(config.to_string())