From c18f04fa551f892c24369db5162d638ef0ae9904 Mon Sep 17 00:00:00 2001 From: Pim van Pelt Date: Mon, 21 Mar 2022 11:06:15 +0000 Subject: [PATCH] Refactor: stop trying to derive implicit LCP names. Make it mandatory and explicitly configured --- example.yaml | 12 ++- unittest/test_interface.yaml | 27 ++++--- unittest/yaml/correct-address.yaml | 2 + unittest/yaml/correct-bondethernet.yaml | 1 + unittest/yaml/correct-example1.yaml | 5 ++ unittest/yaml/error-subinterface2.yaml | 2 +- unittest/yaml/error-subinterface3.yaml | 29 +++++-- unittest/yaml/error-subinterface8.yaml | 3 +- validator/interface.py | 102 ++++++++++-------------- validator/test_interface.py | 46 +++++------ 10 files changed, 117 insertions(+), 112 deletions(-) diff --git a/example.yaml b/example.yaml index 8c74511..a20478e 100644 --- a/example.yaml +++ b/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: diff --git a/unittest/test_interface.yaml b/unittest/test_interface.yaml index c315b4d..2b7150f 100644 --- a/unittest/test_interface.yaml +++ b/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" diff --git a/unittest/yaml/correct-address.yaml b/unittest/yaml/correct-address.yaml index eea1343..e53afd4 100644 --- a/unittest/yaml/correct-address.yaml +++ b/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: diff --git a/unittest/yaml/correct-bondethernet.yaml b/unittest/yaml/correct-bondethernet.yaml index aad04b4..b999fda 100644 --- a/unittest/yaml/correct-bondethernet.yaml +++ b/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 ] diff --git a/unittest/yaml/correct-example1.yaml b/unittest/yaml/correct-example1.yaml index 67fc435..aa4bd47 100644 --- a/unittest/yaml/correct-example1.yaml +++ b/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: diff --git a/unittest/yaml/error-subinterface2.yaml b/unittest/yaml/error-subinterface2.yaml index 76ec93e..022387d 100644 --- a/unittest/yaml/error-subinterface2.yaml +++ b/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: diff --git a/unittest/yaml/error-subinterface3.yaml b/unittest/yaml/error-subinterface3.yaml index cb93df4..87709f5 100644 --- a/unittest/yaml/error-subinterface3.yaml +++ b/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 .*" - count: 2 + - "sub-interface .* has LCP name .* but .* does not have LCP" + - "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" diff --git a/unittest/yaml/error-subinterface8.yaml b/unittest/yaml/error-subinterface8.yaml index 3dfa888..5faf101 100644 --- a/unittest/yaml/error-subinterface8.yaml +++ b/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: diff --git a/validator/interface.py b/validator/interface.py index 07a251b..216454d 100644 --- a/validator/interface.py +++ b/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(): - for subid, sub_iface in iface['sub-interfaces'].items(): - sub_ifname = "%s.%d" % (ifname, subid) - sub_encap = get_encapsulation(yaml, sub_ifname) - if not sub_encap: - continue - 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 + parent_ifname, parent_iface = get_parent_by_name(yaml, ifname) + if not parent_iface: + return None,None + + for subid, sub_iface in parent_iface['sub-interfaces'].items(): + sub_ifname = "%s.%d" % (parent_ifname, subid) + sub_encap = get_encapsulation(yaml, sub_ifname) + if not sub_encap: + continue + 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: - return None - - parent_ifname, parent_iface = get_parent_by_name(yaml, ifname) - if 'lcp' in iface: + if iface and 'lcp' in iface: return iface['lcp'] - if is_l2(yaml, ifname): - 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 + return None 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: - msgs.append("sub-interface %s has LCP but %s does not have LCP" % (sub_ifname, ifname)) + + sub_lcp = get_lcp(yaml, sub_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: diff --git a/validator/test_interface.py b/validator/test_interface.py index f3a82ba..a2dedc1 100644 --- a/validator/test_interface.py +++ b/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.assertEqual(interface.get_lcp(self.cfg, "GigabitEthernet1/0/1.201"), "e1.1000") - - ## 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.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") + 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")) 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.assertIsNone(ifname) + self.assertEqual(ifname, "GigabitEthernet1/0/1.200")