Refactor: stop trying to derive implicit LCP names. Make it mandatory and explicitly configured

example.yaml

@@ -6,18 +6,18 @@ bondethernets:
 interfaces:
   GigabitEthernet1/0/0:
     description: "Infra: nikhef-core-1.nl.switch.coloclue.net e1/34"
-    lcp: e0-0
+    lcp: "e0-0"
     addresses: [ 94.142.244.85/24, 2A02:898::146:1/64 ]
     sub-interfaces:
       100:
         description: "Cust: hvn0.nlams0.ipng.ch"
+        lcp: "e0-0.100"
         addresses: [ 94.142.241.185/29, 2a02:898:146::1/64 ]
       101:
         description: "Infra: L2 for FrysIX AS112"
 
   GigabitEthernet1/0/1:
-    description: "Broken - has same LCP as above"
+    lcp: "e0-1"
-    lcp: e0-1
 
   GigabitEthernet2/0/0:
     description: "Infra: LAG to xsw0"
@@ -26,7 +26,7 @@ interfaces:
     description: "Infra: LAG to xsw1"
 
   GigabitEthernet3/0/0:
-    description: "Infra: Bridge Doamin 10"
+    description: "Infra: Bridge Domain 10"
 
   BondEthernet0:
     description: "Bond, James Bond!"
@@ -34,15 +34,19 @@ interfaces:
     lcp: "bond0"
     sub-interfaces:
       200:
+        lcp: "bond0.1000"
         encapsulation:
           dot1q: 1000
+          exact-match: True
       201:
         encapsulation:
           dot1ad: 1000
       202:
+        lcp: "bond0.1000.1234"
         encapsulation:
           dot1q: 1000
           inner-dot1q: 1234
+          exact-match: True
         addresses: [ 192.168.1.1/24 ]
       203:
         encapsulation:

unittest/test_interface.yaml

@@ -26,31 +26,34 @@ interfaces:
       100:
         lcp: "foo"
         addresses: [ "10.0.0.1/24", "10.0.0.2/24", "2001:db8:2::1/64" ]
+      101:
+        encapsulation:
+          dot1ad: 100
+          exact-match: True
+        lcp: "e1.100"
+        addresses: [ "10.0.2.1/30" ]
+      102:
+        encapsulation:
+          dot1ad: 100
+          inner-dot1q: 100
+          exact-match: True
+        lcp: "e1.100.100"
       200:
         mtu: 9000
         encapsulation:
           dot1q: 1000
-        addresses: [ "10.0.1.1/30" ]
       201:
-        encapsulation:
-          dot1ad: 1000
-        addresses: [ "10.0.2.1/30" ]
-      202:
         encapsulation:
           dot1q: 1000
           inner-dot1q: 1234
-        addresses: [ "10.0.3.1/30" ]
+      202:
+        encapsulation:
+          dot1ad: 1000
       203:
-        encapsulation:
-          dot1ad: 1000
-          inner-dot1q: 1000
-        addresses: [ "10.0.4.1/30" ]
-      204:
         encapsulation:
           dot1ad: 1000
           inner-dot1q: 1000
           exact-match: True
-        addresses: [ "10.0.5.1/30" ]
 
   GigabitEthernet2/0/0:
     description: "This interface has no sub-ints"

unittest/yaml/correct-address.yaml

@@ -10,9 +10,11 @@ interfaces:
     sub-interfaces:
       100:
         description: "Overlapping IP addresses are fine, if in the same prefix"
+        lcp: e0-0.100
         addresses: [ 192.0.2.9/29, 192.0.2.10/29 ]
       101:
         description: ".. and for IPv6 also, provided the same prefix is used"
+        lcp: e0-0.101
         addresses: [ 2001:db8:2::1/64, 2001:db8:2::2/64 ]
 
   GigabitEthernet3/0/0:

unittest/yaml/correct-bondethernet.yaml

@@ -33,4 +33,5 @@ interfaces:
     sub-interfaces:
       100:
         mtu: 2000
+        lcp: "be1.2000"
         addresses: [ 192.0.2.9/29, 2001:db8:1::1/64 ]

unittest/yaml/correct-example1.yaml

@@ -16,6 +16,7 @@ interfaces:
     sub-interfaces:
       100:
         description: "Cust: hvn0.nlams0.ipng.ch"
+        lcp: e0-0.100
         addresses: [ 94.142.241.185/29, 2a02:898:146::1/64 ]
       101:
         description: "Infra: L2 for FrysIX AS112"
@@ -40,8 +41,10 @@ interfaces:
     sub-interfaces:
       200:
         description: "This subint is needed to build the parent LCP bond0.1000 for QinQ subint 202 bond0.1000.1234"
+        lcp: "bond0.1000"
         encapsulation:
           dot1q: 1000
+          exact-match: True
       201:
         encapsulation:
           dot1ad: 1000
@@ -49,6 +52,8 @@ interfaces:
         encapsulation:
           dot1q: 1000
           inner-dot1q: 1234
+          exact-match: True
+        lcp: "bond0.1000.1234"
         addresses: [ 192.168.1.1/24 ]
       203:
         encapsulation:

unittest/yaml/error-subinterface2.yaml

@@ -2,7 +2,7 @@ test:
   description: "A subinterface cannot have an LCP if the parent doesn't have one"
   errors:
     expected:
-     - "sub-interface .* has LCP but .* does not have LCP"
+     - "sub-interface .* has LCP name .* but .* does not have LCP"
     count: 1
 ---
 interfaces:

unittest/yaml/error-subinterface3.yaml

@@ -1,24 +1,39 @@
 test:
-  description: "The length of the LCP name is too long"
+  description: "Children with an LCP require their parent to have one too"
   errors:
     expected:
-    - "sub-interface .* has LCP with too long name .*"
+    - "sub-interface .* has LCP name .* but .* does not have LCP"
-    count: 2
+    - "sub-interface .* is QinX and has LCP name .* but .* does not have LCP"
+    - "sub-interface .* has LCP name .* but its encapsulation is not exact-match"
+    - "sub-interface .* has invalid encapsulation"
+    count: 4
 ---
 interfaces:
   GigabitEthernet1/0/0:
-    lcp: "e23456789012"
     sub-interfaces:
       100:
-        description: "VLAN 100"
+        lcp: "e0.100"
+        description: "VLAN 100 has an LCP, but Gi1/0/0 does not"
+
   GigabitEthernet1/0/1:
-    lcp: "e2345678"
+    lcp: "e1"
     sub-interfaces:
       100:
         description: "VLAN 100"
       101:
-        description: "QinQ 101"
+        description: "QinQ 101 has an LCP but VLAN 100 does not"
         encapsulation:
           dot1q: 100
           inner-dot1q: 100
+          exact-match: True
+        lcp: "e1.100.100"
 
+  GigabitEthernet1/0/2:
+    lcp: "e2"
+    sub-interfaces:
+      100:
+        description: "Sub-interfaces must be exact-match in order to have an LCP"
+        encapsulation:
+          dot1q: 100
+          exact-match: False
+        lcp: "e2.100"

unittest/yaml/error-subinterface8.yaml

@@ -3,7 +3,8 @@ test:
   errors:
     expected:
      - "sub-interface GigabitEthernet1/0/0.(101|102) has invalid encapsulation"
-    count: 2
+     - "sub-interface .* has LCP name .* but its encapsulation is not exact-match"
+    count: 4
 ---
 interfaces:
   GigabitEthernet1/0/0:

validator/interface.py

@@ -35,16 +35,19 @@ def get_qinx_parent_by_name(yaml, ifname):
     if not qinx_encap:
         return None,None
 
-    for ifname, iface in yaml['interfaces'].items():
+    parent_ifname, parent_iface = get_parent_by_name(yaml, ifname)
-        for subid, sub_iface in iface['sub-interfaces'].items():
+    if not parent_iface:
-            sub_ifname = "%s.%d" % (ifname, subid)
+        return None,None
-            sub_encap = get_encapsulation(yaml, sub_ifname)
+
-            if not sub_encap:
+    for subid, sub_iface in parent_iface['sub-interfaces'].items():
-                continue
+        sub_ifname = "%s.%d" % (parent_ifname, subid)
-            if qinx_encap['dot1q'] > 0 and sub_encap['dot1q'] == qinx_encap['dot1q']:
+        sub_encap = get_encapsulation(yaml, sub_ifname)
-                return sub_ifname, sub_iface
+        if not sub_encap:
-            if qinx_encap['dot1ad'] > 0 and sub_encap['dot1ad'] == qinx_encap['dot1ad']:
+            continue
-                return sub_ifname, sub_iface
+        if qinx_encap['dot1q'] > 0 and sub_encap['dot1q'] == qinx_encap['dot1q']:
+            return sub_ifname, sub_iface
+        if qinx_encap['dot1ad'] > 0 and sub_encap['dot1ad'] == qinx_encap['dot1ad']:
+            return sub_ifname, sub_iface
     return None,None
 
 
@@ -201,6 +204,9 @@ def get_encapsulation(yaml, ifname):
         return None
 
     ifname, iface = get_by_name(yaml, ifname)
+    if not iface:
+        return None
+
     parent_ifname, parent_iface = get_parent_by_name(yaml, ifname)
     if not iface or not parent_iface:
         return None
@@ -319,44 +325,9 @@ def get_lcp(yaml, ifname):
     Return None if no LCP can be found. """
 
     ifname, iface = get_by_name(yaml, ifname)
-    if not iface:
+    if iface and 'lcp' in iface:
-        return None
-
-    parent_ifname, parent_iface = get_parent_by_name(yaml, ifname)
-    if 'lcp' in iface:
         return iface['lcp']
-    if is_l2(yaml, ifname):
+    return None
-        return None
-    if parent_iface and not 'lcp' in parent_iface:
-        return None
-    if not 'encapsulation' in iface:
-        if not '.' in ifname:
-            ## Not a sub-int and no encap? Should not happen
-            return None
-        ifname, subid = ifname.split('.')
-        subid = int(subid)
-        return "%s.%d" % (parent_iface['lcp'], subid)
-
-    dot1q = 0
-    dot1ad = 0
-    inner_dot1q = 0
-    if 'dot1q' in iface['encapsulation']:
-        dot1q = iface['encapsulation']['dot1q']
-    elif 'dot1ad' in iface['encapsulation']:
-        dot1ad = iface['encapsulation']['dot1ad']
-    if 'inner-dot1q' in iface['encapsulation']:
-        inner_dot1q = iface['encapsulation']['inner-dot1q']
-    if inner_dot1q and dot1ad:
-        lcp = "%s.%d.%d" % (parent_iface['lcp'], dot1ad, inner_dot1q)
-    elif inner_dot1q and dot1q:
-        lcp = "%s.%d.%d" % (parent_iface['lcp'], dot1q, inner_dot1q)
-    elif dot1ad:
-        lcp = "%s.%d" % (parent_iface['lcp'], dot1ad)
-    elif dot1q:
-        lcp = "%s.%d" % (parent_iface['lcp'], dot1q)
-    else:
-        return None
-    return lcp
 
 def get_mtu(yaml, ifname):
     """ Returns MTU of the interface. If it's not set, return the parent's MTU, and
@@ -391,25 +362,22 @@ def validate_interfaces(yaml):
             result = False
 
         iface_mtu = get_mtu(yaml, ifname)
-        iface_lcp = has_lcp(yaml, ifname)
+        iface_lcp = get_lcp(yaml, ifname)
         iface_address = has_address(yaml, ifname)
 
         if iface_address and not iface_lcp:
             msgs.append("interface %s has an address but no LCP" % ifname)
             result = False
-        iface_lcp = get_lcp(yaml, ifname)
         if iface_lcp and not lcp.is_unique(yaml, iface_lcp):
             msgs.append("interface %s does not have a unique LCP name %s" % (ifname, iface_lcp))
             result = False
-        if iface_lcp and len(iface_lcp)>15:
-            msgs.append("interface %s has LCP with too long name %s" % (fname, iface_lcp))
-            result = False
 
         if 'addresses' in iface:
             for a in iface['addresses']:
                 if not address.is_allowed(yaml, ifname, iface['addresses'], a):
                     msgs.append("interface %s IP address %s conflicts with another" % (ifname, a))
                     result = False
+
         if 'l2xc' in iface:
             if has_sub(yaml, ifname):
                 msgs.append("interface %s has l2xc so it cannot have sub-interfaces" % (ifname))
@@ -452,21 +420,31 @@ def validate_interfaces(yaml):
                     result = False
                     continue
 
-                sub_lcp = get_lcp(yaml, sub_ifname)
-                if sub_lcp and len(sub_lcp)>15:
-                    msgs.append("sub-interface %s has LCP with too long name %s" % (sub_ifname, sub_lcp))
-                    result = False
-                if sub_lcp and not lcp.is_unique(yaml, sub_lcp):
-                    msgs.append("sub-interface %s does not have a unique LCP name %s" % (sub_ifname, sub_lcp))
-                    result = False
                 sub_mtu = get_mtu(yaml, sub_ifname)
                 if sub_mtu > iface_mtu:
                     msgs.append("sub-interface %s has MTU %d higher than parent MTU %d" % (sub_ifname, sub_iface['mtu'], iface_mtu))
                     result = False
-                if has_lcp(yaml, sub_ifname):
+
-                    if not iface_lcp:
+                sub_lcp = get_lcp(yaml, sub_ifname)
-                        msgs.append("sub-interface %s has LCP but %s does not have LCP" % (sub_ifname, ifname))
+                if sub_lcp and not lcp.is_unique(yaml, sub_lcp):
+                    msgs.append("sub-interface %s does not have a unique LCP name %s" % (sub_ifname, sub_lcp))
+                    result = False
+                if sub_lcp and not iface_lcp:
+                    msgs.append("sub-interface %s has LCP name %s but %s does not have LCP" % (sub_ifname, sub_lcp, ifname))
+                    result = False
+                if sub_lcp and is_qinx(yaml, sub_ifname):
+                    mid_ifname, mid_iface = get_qinx_parent_by_name(yaml, sub_ifname)
+                    if not mid_iface:
+                        msgs.append("sub-interface %s is QinX and has LCP name %s which requires a parent" % (sub_ifname, sub_lcp))
                         result = False
+                    elif not get_lcp(yaml, mid_ifname):
+                        msgs.append("sub-interface %s is QinX and has LCP name %s but %s does not have LCP" % (sub_ifname, sub_lcp, mid_ifname))
+                        result = False
+                if sub_lcp and 'encapsulation' in sub_iface and 'exact-match' in sub_iface['encapsulation'] and not sub_iface['encapsulation']['exact-match']:
+                    msgs.append("sub-interface %s has LCP name %s but its encapsulation is not exact-match" % (sub_ifname, sub_lcp))
+                    result = False
+
+
                 if has_address(yaml, sub_ifname):
                     ## The sub_iface lcp is not required: it can be derived from the iface_lcp, which has to be set
                     if not iface_lcp:

validator/test_interface.py

@@ -9,21 +9,24 @@ class TestInterfaceMethods(unittest.TestCase):
 
     def test_enumerators(self):
         ifs = interface.get_interfaces(self.cfg)
-        self.assertEqual(len(ifs), 15)
+        self.assertEqual(len(ifs), 16)
         self.assertIn("GigabitEthernet1/0/1", ifs)
         self.assertIn("GigabitEthernet1/0/1.200", ifs)
-        self.assertIn("GigabitEthernet1/0/1.204", ifs)
 
         ifs = interface.get_sub_interfaces(self.cfg)
-        self.assertEqual(len(ifs), 10)
+        self.assertEqual(len(ifs), 11)
         self.assertNotIn("GigabitEthernet1/0/1", ifs)
         self.assertIn("GigabitEthernet1/0/1.200", ifs)
-        self.assertIn("GigabitEthernet1/0/1.204", ifs)
+        self.assertIn("GigabitEthernet1/0/1.201", ifs)
+        self.assertIn("GigabitEthernet1/0/1.202", ifs)
+        self.assertIn("GigabitEthernet1/0/1.203", ifs)
 
         ifs = interface.get_qinx_interfaces(self.cfg)
         self.assertEqual(len(ifs), 3)
         self.assertNotIn("GigabitEthernet1/0/1.200", ifs)
-        self.assertIn("GigabitEthernet1/0/1.204", ifs)
+        self.assertNotIn("GigabitEthernet1/0/1.202", ifs)
+        self.assertIn("GigabitEthernet1/0/1.201", ifs)
+        self.assertIn("GigabitEthernet1/0/1.203", ifs)
 
     def test_mtu(self):
         self.assertEqual(interface.get_mtu(self.cfg, "GigabitEthernet1/0/1"), 9216)
@@ -36,12 +39,10 @@ class TestInterfaceMethods(unittest.TestCase):
         self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.200"),
             { 'dot1q': 1000, 'dot1ad': 0, 'inner-dot1q': 0, 'exact-match': False })
         self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.201"),
-            { 'dot1q': 0, 'dot1ad': 1000, 'inner-dot1q': 0, 'exact-match': False })
-        self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.202"),
             { 'dot1q': 1000, 'dot1ad': 0, 'inner-dot1q': 1234, 'exact-match': False })
+        self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.202"),
+            { 'dot1q': 0, 'dot1ad': 1000, 'inner-dot1q': 0, 'exact-match': False })
         self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.203"),
-            { 'dot1q': 0, 'dot1ad': 1000, 'inner-dot1q': 1000, 'exact-match': False })
-        self.assertEqual(interface.get_encapsulation(self.cfg, "GigabitEthernet1/0/1.204"),
             { 'dot1q': 0, 'dot1ad': 1000, 'inner-dot1q': 1000, 'exact-match': True })
 
         self.assertFalse(interface.valid_encapsulation(self.cfg, "GigabitEthernet1/0/0.100"))
@@ -61,13 +62,12 @@ class TestInterfaceMethods(unittest.TestCase):
         self.assertTrue(interface.is_sub(self.cfg, "GigabitEthernet1/0/1.200"))
 
     def test_is_qinx(self):
-        self.assertTrue(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.202"))
-        self.assertTrue(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.203"))
-        self.assertTrue(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.204"))
-
         self.assertFalse(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1"))
         self.assertFalse(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.200"))
-        self.assertFalse(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.201"))
+        self.assertFalse(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.202"))
+
+        self.assertTrue(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.201"))
+        self.assertTrue(interface.is_qinx(self.cfg, "GigabitEthernet1/0/1.203"))
 
     def test_lcp(self):
         self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/0"))
@@ -75,16 +75,13 @@ class TestInterfaceMethods(unittest.TestCase):
 
         self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1"), "e1")
         self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.100"), "foo")
+        self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.101"), "e1.100")
+        self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.102"), "e1.100.100")
 
-        self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.200"), "e1.1000")
+        self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.200"))
-        self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.201"), "e1.1000")
+        self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.201"))
-
+        self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.202"))
-        ## TODO(pim) - sub 201-203 cannot have an LCP, their encap is not exact-match
+        self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.203"))
-        ## self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.200"))
-        ## self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.201"))
-        ## self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.202"))
-        ## self.assertIsNone(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.203"))
-        self.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.204"), "e1.1000.1000")
 
     def test_address(self):
         self.assertFalse(interface.has_address(self.cfg, "GigabitEthernet1/0/0"))
@@ -132,5 +129,4 @@ class TestInterfaceMethods(unittest.TestCase):
         self.assertIsNone(ifname)
 
         ifname, iface = interface.get_qinx_parent_by_name(self.cfg, "GigabitEthernet1/0/1.201")
-        self.assertIsNone(iface)
+        self.assertEqual(ifname, "GigabitEthernet1/0/1.200")
-        self.assertIsNone(ifname)