From 2ed4f94527be743d5be623f9343457282ef4917a Mon Sep 17 00:00:00 2001
From: Erik Reid <erik.reid@geant.org>
Date: Mon, 25 Jan 2021 18:58:20 +0100
Subject: [PATCH] also read top-level peerings

---
 inventory_provider/juniper.py        | 65 ++++++++++++++++++++++++++++
 test/per_router/test_data_routes.py  | 47 +-------------------
 test/per_router/test_juniper_data.py | 46 +-------------------
 test/test_worker_utils.py            | 28 +++++++++---
 4 files changed, 90 insertions(+), 96 deletions(-)

diff --git a/inventory_provider/juniper.py b/inventory_provider/juniper.py
index 723ab2bc..57ca5e81 100644
--- a/inventory_provider/juniper.py
+++ b/inventory_provider/juniper.py
@@ -107,6 +107,64 @@ UNIT_SCHEMA = """<?xml version="1.1" encoding="UTF-8" ?>
 </xs:schema>
 """  # noqa: E501
 
+PEERING_LIST_SCHEMA = {
+    "$schema": "http://json-schema.org/draft-07/schema#",
+    "definitions": {
+        "top-level-peering": {
+            "type": "object",
+            "properties": {
+                "group": {"type": "string"},
+                "description": {"type": "string"},
+                "address": {"type": "string"},
+                "remote-asn": {"type": "integer"},
+                "local-asn": {"type": "integer"}
+            },
+            # lots of internal peerings - so maybe no explicit asn's
+            "required": ["group", "address"],
+            "additionalProperties": False
+        },
+        "instance-peering": {
+            "type": "object",
+            "properties": {
+                "instance": {"type": "string"},
+                "group": {"type": "string"},
+                "description": {"type": "string"},
+                "address": {"type": "string"},
+                "remote-asn": {"type": "integer"},
+                "local-asn": {"type": "integer"},
+            },
+            # description and-or local-asn is not always present,
+            # just based on empirical tests - not a problem
+            "required": ["instance", "group", "address", "remote-asn"],
+            "additionalProperties": False
+        },
+        "logical-system-peering": {
+            "type": "object",
+            "properties": {
+                "logical-system": {"type": "string"},
+                "group": {"type": "string"},
+                "description": {"type": "string"},
+                "address": {"type": "string"},
+                "remote-asn": {"type": "integer"},
+                "local-asn": {"type": "integer"}
+            },
+            # local/remote-asn and/or description are not always present,
+            # just based on empirical tests - not a problem
+            "required": ["logical-system", "group", "address"],
+            "additionalProperties": False
+        },
+        "peering": {
+            "oneOf": [
+                {"$ref": "#/definitions/top-level-peering"},
+                {"$ref": "#/definitions/instance-peering"},
+                {"$ref": "#/definitions/logical-system-peering"}
+            ]
+        }
+    },
+    "type": "array",
+    "items": {"$ref": "#/definitions/peering"}
+}
+
 
 class NetconfHandlingError(Exception):
     pass
@@ -244,6 +302,13 @@ def all_bgp_peers(netconf_config):
             info['description'] = description.text
         return info
 
+    for group in netconf_config.xpath('//configuration/protocols/bgp/group'):
+        group_name = group.find('name').text
+        for neighbor in group.xpath('./neighbor'):
+            peer = _peering_params(neighbor)
+            peer['group'] = group_name
+            yield peer
+
     for instance in netconf_config.xpath(
             '//configuration/routing-instances/instance'):
         instance_name = instance.find('name').text
diff --git a/test/per_router/test_data_routes.py b/test/per_router/test_data_routes.py
index 4ac97975..b1fbdbe0 100644
--- a/test/per_router/test_data_routes.py
+++ b/test/per_router/test_data_routes.py
@@ -1,6 +1,7 @@
 import json
 import pytest
 import jsonschema
+from inventory_provider.juniper import PEERING_LIST_SCHEMA
 
 DEFAULT_REQUEST_HEADERS = {
     "Content-type": "application/json",
@@ -82,55 +83,11 @@ def test_router_bgp_routes(router, client):
         pytest.skip('%s is not expected to have bgp peers' % router)
         return
 
-    bgp_list_schema = {
-        "$schema": "http://json-schema.org/draft-07/schema#",
-        "definitions": {
-            "instance-peering": {
-                "type": "object",
-                "properties": {
-                    "instance": {"type": "string"},
-                    "group": {"type": "string"},
-                    "description": {"type": "string"},
-                    "address": {"type": "string"},
-                    "remote-asn": {"type": "integer"},
-                    "local-asn": {"type": "integer"}
-                },
-                # description and-or local-asn is not always present,
-                # just based on empirical tests - not a problem
-                "required": ["instance", "group", "address", "remote-asn"],
-                "additionalProperties": False
-            },
-            "logical-system-peering": {
-                "type": "object",
-                "properties": {
-                    "logical-system": {"type": "string"},
-                    "group": {"type": "string"},
-                    "description": {"type": "string"},
-                    "address": {"type": "string"},
-                    "remote-asn": {"type": "integer"},
-                    "local-asn": {"type": "integer"}
-                },
-                # local/remote-asn and/or description are not always present,
-                # just based on empirical tests - not a problem
-                "required": ["logical-system", "group", "address"],
-                "additionalProperties": False
-            },
-            "peering": {
-                "oneOf": [
-                    {"$ref": "#/definitions/instance-peering"},
-                    {"$ref": "#/definitions/logical-system-peering"}
-                ]
-            }
-        },
-        "type": "array",
-        "items": {"$ref": "#/definitions/peering"}
-    }
-
     rv = client.post(
         "/testing/bgp/" + router,
         headers=DEFAULT_REQUEST_HEADERS)
 
     assert rv.status_code == 200
     response = json.loads(rv.data.decode("utf-8"))
-    jsonschema.validate(response, bgp_list_schema)
+    jsonschema.validate(response, PEERING_LIST_SCHEMA)
     assert response  # at least shouldn't be empty
diff --git a/test/per_router/test_juniper_data.py b/test/per_router/test_juniper_data.py
index cb4a2d81..81418755 100644
--- a/test/per_router/test_juniper_data.py
+++ b/test/per_router/test_juniper_data.py
@@ -38,52 +38,8 @@ def test_interface_list(netconf_doc):
 
 def test_bgp_peering_data(netconf_doc):
 
-    schema = {
-        "$schema": "http://json-schema.org/draft-07/schema#",
-        "definitions": {
-            "instance-peering": {
-                "type": "object",
-                "properties": {
-                    "instance": {"type": "string"},
-                    "group": {"type": "string"},
-                    "description": {"type": "string"},
-                    "address": {"type": "string"},
-                    "remote-asn": {"type": "integer"},
-                    "local-asn": {"type": "integer"}
-                },
-                # description and-or local-asn is not always present,
-                # just based on empirical tests - not a problem
-                "required": ["instance", "group", "address", "remote-asn"],
-                "additionalProperties": False
-            },
-            "logical-system-peering": {
-                "type": "object",
-                "properties": {
-                    "logical-system": {"type": "string"},
-                    "group": {"type": "string"},
-                    "description": {"type": "string"},
-                    "address": {"type": "string"},
-                    "remote-asn": {"type": "integer"},
-                    "local-asn": {"type": "integer"}
-                },
-                # local/remote-asn and/or description are not always present,
-                # just based on empirical tests - not a problem
-                "required": ["logical-system", "group", "address"],
-                "additionalProperties": False
-            },
-            "peering": {
-                "oneOf": [
-                    {"$ref": "#/definitions/instance-peering"},
-                    {"$ref": "#/definitions/logical-system-peering"}
-                ]
-            }
-        },
-        "type": "array",
-        "items": {"$ref": "#/definitions/peering"}
-    }
-
     peerings = list(juniper.all_bgp_peers(netconf_doc))
-    jsonschema.validate(peerings, schema)
+    jsonschema.validate(peerings, juniper.PEERING_LIST_SCHEMA)
     assert peerings  # there's always at least one
 
     # confirm the addresses are in canonical (exploded) form
diff --git a/test/test_worker_utils.py b/test/test_worker_utils.py
index d0228efd..fda3215a 100644
--- a/test/test_worker_utils.py
+++ b/test/test_worker_utils.py
@@ -88,9 +88,26 @@ def test_build_juniper_peering_db(mocked_worker_module):
     :param mocked_worker_module: fixture
     """
 
-    peering_list_schema = {
+    # same as inventory_provider.juniper.PEERING_LIST_SCHEMA,
+    # but with "hostname" in every returned record
+
+    PEERING_LIST_SCHEMA = {
         "$schema": "http://json-schema.org/draft-07/schema#",
         "definitions": {
+            "top-level-peering": {
+                "type": "object",
+                "properties": {
+                    "group": {"type": "string"},
+                    "description": {"type": "string"},
+                    "address": {"type": "string"},
+                    "remote-asn": {"type": "integer"},
+                    "local-asn": {"type": "integer"},
+                    "hostname": {"type": "string"}
+                },
+                # lots of internal peerings - so maybe no explicit asn's
+                "required": ["group", "address"],
+                "additionalProperties": False
+            },
             "instance-peering": {
                 "type": "object",
                 "properties": {
@@ -104,9 +121,7 @@ def test_build_juniper_peering_db(mocked_worker_module):
                 },
                 # description and-or local-asn is not always present,
                 # just based on empirical tests - not a problem
-                "required": ["instance", "group",
-                             "address", "remote-asn",
-                             "hostname"],
+                "required": ["instance", "group", "address", "remote-asn"],
                 "additionalProperties": False
             },
             "logical-system-peering": {
@@ -122,11 +137,12 @@ def test_build_juniper_peering_db(mocked_worker_module):
                 },
                 # local/remote-asn and/or description are not always present,
                 # just based on empirical tests - not a problem
-                "required": ["logical-system", "group", "address", "hostname"],
+                "required": ["logical-system", "group", "address"],
                 "additionalProperties": False
             },
             "peering": {
                 "oneOf": [
+                    {"$ref": "#/definitions/top-level-peering"},
                     {"$ref": "#/definitions/instance-peering"},
                     {"$ref": "#/definitions/logical-system-peering"}
                 ]
@@ -167,7 +183,7 @@ def test_build_juniper_peering_db(mocked_worker_module):
                 assert address == canonical
             continue
 
-        jsonschema.validate(value, peering_list_schema)
+        jsonschema.validate(value, PEERING_LIST_SCHEMA)
 
     assert found_record
 
-- 
GitLab