Exa-Networks / exabgp

The BGP swiss army knife of networking
Other
2.07k stars 443 forks source link

bgp-ls: SrCapabilities json encoding and sr-base decode is incorrect #1082

Closed joshqoon closed 2 years ago

joshqoon commented 2 years ago

couple of more issues in the json generated for sr capabilities:

$ ./sbin/exabgp version
ExaBGP : master
Python : 3.10.4 (main, Apr  2 2022, 12:32:41) [GCC 9.4.0]
Uname  : Linux sr-lab-exabgp 5.4.0-107-generic #121-Ubuntu SMP Thu Mar 24 16:04:27 UTC 2022 x86_64

Two issues in the json below :

"attribute": { 
    "origin": "igp", 
    "as-path": {"0": {"element": "as-sequence", "value": [15924]}},
    "bgp-ls": { 
        "generic-lsid-266": ["0x010A"], 
        "node-name": "xxxx", 
        "area-id": "4784272", 
        "local-te-router-ids": ["10.134.0.41"], 
        "sr_capability_flags": "sr_capability_flags": {"I": 1, "V": 0, "RSV": 0},    <-- repeating key 
        "sids": [(8000, 8000)],                                                      <-- list of tuples and range decode is not right
        "sr-algorithms": [0, 1], 
        "generic-lsid-1036": ["0x00000003E804890003003A98"] 
    } 
}

sr_capability_flags is generated from the line below:

https://github.com/Exa-Networks/exabgp/blob/b3f4dd8dfd5582fc50a25704bc3b9e2baaf5105e/src/exabgp/bgp/message/update/attribute/bgpls/node/srcap.py#L85

here is a diff that works for me:

diff --git a/src/exabgp/bgp/message/update/attribute/bgpls/node/srcap.py b/src/exabgp/bgp/message/update/attribute/bgpls/node/srcap.py
index e463a33b..80d73bd2 100644
--- a/src/exabgp/bgp/message/update/attribute/bgpls/node/srcap.py
+++ b/src/exabgp/bgp/message/update/attribute/bgpls/node/srcap.py
@@ -42,7 +42,7 @@ from exabgp.bgp.message.notification import Notify
 @LinkState.register()
 class SrCapabilities(FlagLS):
     REPR = 'SR Capability Flags'
-    JSON = 'sr_capability_flags'
+    JSON = 'sr-capability-flags'
     TLV = 1034
     FLAGS = ['I', 'V', 'RSV', 'RSV', 'RSV', 'RSV', 'RSV', 'RSV']

@@ -73,13 +73,13 @@ class SrCapabilities(FlagLS):
             if t != 1161:
                 raise Notify(3, 5, "Invalid sub-TLV type: {}".format(t))
             if l == 3:
-                sids.append((range_size, unpack('!L', bytes([0]) + data[:3])[0]))
+                sids.append([range_size, unpack('!I', bytes([0]) + data[7 : l + 7])[0] & 0xfffff])
             elif l == 4:
                 # XXX: really we are reading 7+ but then re-parsing it again ??
-                sids.append((range_size, unpack('!I', data[7 : l + 7])[0]))
+                sids.append([range_size, unpack('!I', data[7 : l + 7])[0]])
             data = data[l + 7 :]

         return cls(flags, sids)

     def json(self, compact=None):
-        return '"{}": {}, "sids": {}'.format(self.JSON, FlagLS.json(self), self.sids)
+        return '{}, "sids": {}'.format(FlagLS.json(self), self.sids)
thomas-mangin commented 2 years ago

I will accept the proposed change as it but could I have a update with it so I can add a test for it.

joshqoon commented 2 years ago

would this help?

{
    "exabgp": "5.0.0",
    "time": 1651219870.115168,
    "host": "sr-lab-exabgp",
    "pid": 4042882,
    "ppid": 84633,
    "counter": 84,
    "type": "update",
    "header": "0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00A402",
    "body": "0x0000008D900E003440044704C0A8640200000100270200000000000002BC0100001A0200000400003E3402010004000000000203000601013400004140010100400206020100003E34801D45010A0002010A04020006726F7574657204030003490090040400040A860029040A000C8000001F4004890003003E80040B00020001040C000C00000003E804890003003A98",
    "neighbor": {
        "address": {
            "local": "192.168.100.1",
            "peer": "192.168.100.2"
        },
        "asn": {
            "local": 65050,
            "peer": 15924
        },
        "direction": "receive",
        "message": {
            "update": {
                "attribute": {
                    "origin": "igp",
                    "as-path": {
                        "0": {
                            "element": "as-sequence",
                            "value": [
                                15924
                            ]
                        }
                    },
                    "bgp-ls": {
                        "generic-lsid-266": [
                            "0x010A"
                        ],
                        "node-name": "router",
                        "area-id": "4784272",
                        "local-te-router-ids": [
                            "10.134.0.41"
                        ],
                        "sr-capability-flags": {
                            "I": 1,
                            "V": 0,
                            "RSV": 0
                        },
                        "sids": [
                            [
                                8000,
                                16000
                            ]
                        ],
                        "sr-algorithms": [
                            0,
                            1
                        ],
                        "generic-lsid-1036": [
                            "0x00000003E804890003003A98"
                        ]
                    }
                },
                "announce": {
                    "bgp-ls bgp-ls": {
                        "192.168.100.2": [
                            {
                                "ls-nlri-type": "bgpls-node",
                                "l3-routing-topology": 700,
                                "protocol-id": 2,
                                "node-descriptors": [
                                    {
                                        "autonomous-system": 15924
                                    },
                                    {
                                        "bgp-ls-identifier": "0"
                                    },
                                    {
                                        "router-id": "010134000041"
                                    }
                                ],
                                "nexthop": "192.168.100.2"
                            }
                        ]
                    }
                }
            }
        }
    }
}
thomas-mangin commented 2 years ago

Thank you. Yes, the BGP payload is in the header and body fields.