pierky / arouteserver

A tool to automatically build (and test) feature-rich configurations for BGP route servers.
https://arouteserver.readthedocs.org/
GNU General Public License v3.0
288 stars 46 forks source link

IX-F export suggestions #89

Closed bluikko closed 2 years ago

bluikko commented 2 years ago

We recently enabled the IX-F member export JSON schema generated by arouteserver and PeeringDB had some suggestions to make the data more useful. Some of the suggestion below come from that feedback. We could do these by post-processing the generated JSON file but I would expect anyone using this arouteserver feature could benefit and PeeringDB could need to send less mails if the features were provided by arouteserver out of the box. It could also promote the usage and usefulness of the JSON schema.

{ "_comment": "New definition", "asnum": 65520, "name": "Member", "connection_list": [ { "ixp_id": 0, "vlan_list": [ { "vlan_id": 0, "ipv4": { "address": "192.0.2.1" }, "ipv6": { "address": "2001:db8::1" } } ] } ] }

pierky commented 2 years ago

Thanks a lot @bluikko, I’ll go over your suggestions one at a time and see what I can do to make the IX-F data handling better.

pierky commented 2 years ago

Hi @bluikko, thanks again for the suggestions.

I've just pushed a new commit to the dev branch in which I've introduced a new option for the ixf-member-export command: --merge-file. You can read more about it in the changelog from dev and in the staging instance of the docs web site.

I think this new feature could help with the top 2 bullet points from your message above: to add more info about the IXP and to add peering VLAN prefix information. (The URL has been fixed in another commit, unrelated to this new feature.)

The CI/CD pipeline is now running, and if everything goes well (🤞) a pre-release should be out soon on the test instance of PyPi: v1.12.0-alpha1

It'd be great if you could give it a try, by following the instructions reported on the docs about how to install pre-releases: https://arouteserver.readthedocs.io/en/latest/INSTALLATION.html#development-and-pre-release-versions

Regarding the 4th bullet point, as you said it's not trivial. I'll take a better look at it in the next days, however at the moment I can't see a way to solve it with the information present in ARouteServer.

Thanks

bluikko commented 2 years ago

I've just pushed a new commit to the dev branch in which I've introduced a new option for the ixf-member-export command: --merge-file.

So arouteserver does a dict merge between the generated result and the merge file. That sounds great.

Will test it soon.

For the 4th point, I've found out that in PeeringDB each of the IP addresses indeed shows as a separate interface. But it is possible to manually edit that data at PeeringDB after it has imported the IX-F data. So seems the feedback was given in order to avoid this manual editing.

bluikko commented 2 years ago

It works excellently. It took me a few runs to realize that the ixp_id attribute in the merge file cannot be zero (it would be 0 by default if not otherwise defined by the arg). Otherwise an error (+ error handling the exception) is encountered.
The same non-zero ixp_id must be passed on the command line: otherwise arouteserver will use ixp_id 0 and not the ixp_id in the merge file. This was not very logical and could probably be either documented or improved. The first example JSON file in documentation also uses ixp_id of 0.

I would propose to update the documentation

      "support_contact_hours": "24x7",

to

      "support_contact_hours": "24/7",

Since the JSON schema example gives "24/7" and not "24x7".

And a second nit, on the switches:

usage: arouteserver ixf-member-export [-h] [--cfg FILE] [--cache-dir DIR] [--logging-config-file LOGGING_CONFIG_FILE] [--logging-level {DEBUG,INFO,WARNING,ERROR,CRITICAL}]
                                      [--clients FILE] [--ixp_id IXP_ID] [--vlan-id VLAN_ID] [--merge-file MERGE_FILE] [-o OUTPUT_FILE]
                                      shortname IXF_ID

The ixp_id uses underscore while the arg vlan-id (+ others) uses a dash. I don't know if it breaks too many people's systems but for consistency maybe it should be ixp-id (since the other such args use a dash). The argument could also be said for making vlan-id instead use an underscore to keep them consistent with the JSON schema instead of other args. Maybe it needs to wait until a major release that can have breaking changes.

Edit: when the merge YAML file has phone numbers, for example:

    emergency_phone: +1235556789

The value in the result JSON file is incorrectly type int and not a string. Of course this can be fixed by changing it to

    emergency_phone: "+1235556789"

but it is a bit counterintuitive since YAML does not usually need quotation marks. Don't know if this behavior should have change to force phone numbers to strings.

Also, the IX-F JSON examples list phone numbers with spaces so perhaps the documentation merge file example could also group the phone digits with spaces, e.g. "+123 456 789".
Similarly to the "prefixes" in the documentation merge file example, the IX-F JSON examples list prefixes in format "192.0.2.0" and "2001:db8::" without the masks (the masks are only in the mask_length attributes.

And yet another small suggestion: arouteserver prints a line every time about checking for updates. This could be a good opportunity to print the current version.

ARouteServer 2021-11-19 01:52:28,693 INFO Checking latest version
pierky commented 2 years ago

Thanks for testing it @bluikko, and for providing all these comments.

I've added several additional sanity checks against the well known keys for the ixp_list object; they should help the operator to figure out issues and inconsistencies between the values used by ARouteServer to build the member list (which are those from the CLI) and any eventual conflicting value from the merge file. I've also fixed the example used for the doc, which was wrong of course due to what you said.

support_contact_hours example has been fixed, and also the phone number format. However, I'm not planning to add code to handle the YAML-to-JSON translation of it, since it's really a corner case due to YAML, and the user can easily fix it by quoting the value. IP prefixes have also been fixed in the example, removing the mask.

I've also implemented a deprecation for --ixp_id: I'll use the approach of having all "dash"-based options, which is more oriented towards the GNU way of handling CLI arguments.

The pipeline for a new alpha2 release is running; it'd be great if you could give it a try (assuming it will succeed!)

Thanks again.

Also, the tool now shows the current version while checking for any new one.

bluikko commented 2 years ago

The pipeline for a new alpha2 release is running; it'd be great if you could give it a try (assuming it will succeed!)

Tested with alpha5 and it seems to work well. Thanks for your work for creating a viable option for IXP Manager, it is very important for avoiding a total monoculture.

pierky commented 2 years ago

Hello @bluikko, I've just released 1.12.0, which contains the improvements/bugfixes for most of the points from this ticket.

I've also created #91 and I've tagged you there: in this new ticket I've captured your comment about the individual VLANs in connection_list. My idea is to attack it as part of a different ticket (assuming I'll find a way to work it out 😄) I hope it makes sense for you.

I'm going to close this ticket now, feel free to reopen it if you find something wrong in the new release or if there's anything missing. Thanks for your feedback!