Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IX-F export suggestions #89

Closed
bluikko opened this issue Nov 9, 2021 · 7 comments
Closed

IX-F export suggestions #89

bluikko opened this issue Nov 9, 2021 · 7 comments

Comments

@bluikko
Copy link
Contributor

bluikko commented Nov 9, 2021

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.

  • Include a file inside the ixp_list attribute: a file could be included in this section so that IXP could define its country, full name, peeringdb_id, url, various [...]_email attributes, etc. Or even a list of switches.
    Instead of adding a bunch of options to arouteserver to include various extra data, the obvious way would be to just include a named file so that operators can include whatever data they want in the ixp_list.
    Perhaps it could be a switch passed to arouteserver that enables inclusion of a named file passed to the option, and rendered after the currently generated ixp_list.vlan, just before the end of ixp_list.
  • In the documentation https://arouteserver.readthedocs.io/en/latest/USAGE.html#ix-f-member-export-json-file-from-clients-yml there is a link to https://ml.ix-f.net/ and this link is broken - maybe it should point to https://github.com/euro-ix/json-schemas instead?
  • Prefix information: I think adding the peering VLAN prefix information (ixp_list.vlan.ipv4, ixp_list.vlan.ipv6) to the vlan list (ixp_list.vlan) would be much more difficult since arouteserver does not really have this information; the closest is the LOCAL_PREFIXES information. That option could have much more than just the peering VLAN prefixes; maybe it could be cross-checked with the IP addresses assigned to members and then pick those prefixes from LOCAL_PREFIXES that have IP addresses assigned to members/peers.
  • Individual vlans in connection_list: according to PeeringDB the current way to list each member IP address in its own vlan_list element could be understood as each IPv4/IPv6 address being their own interface (=IXP port). The trouble here I think is that arouteserver does not really have the interface/port-level information and could not associate each IPv4 address with the corresponding IPv6 address.
    Currently we just list IPv4 and IPv6 addresses in clients.yml without any way to distinguish which port they belong to.
    Individual addresses within the same address family (multiple IPv4 addresses/multiple IPv6 addresses) muddy the waters here - should they be kept in their individual vlan_list elements? It seems to me so and that would make the difficulty. How to associate IPv4 address with its IPv6 counterpart to put them in the same vlan_list element, while keeping a separate vlan_list element for some other IPv4+IPv6 pair? It could be much easier if all IPv4+IPv6 addresses could be lumped in the same vlan_list element but I am not sure if this would still be any better to PeeringDB than having all of the IP addresses in their own individual vlan_list elements. Will update if I get a response to that question.
    Example:
{
  "_comment": "Current definition",
  "asnum": 65520,
  "name": "Member",
  "connection_list": [
    {
      "ixp_id": 0,
      "vlan_list": [
        {
          "vlan_id": 0,
          "ipv4": {
            "address": "192.0.2.1"
          }
        },
        {
          "vlan_id": 0,
          "ipv6": {
            "address": "2001:db8::1"
          }
        }
      ]
    }
  ]
}

{
  "_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
Copy link
Owner

pierky commented Nov 11, 2021

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 added a commit that referenced this issue Nov 18, 2021
pierky added a commit that referenced this issue Nov 18, 2021
Issue #89
@pierky
Copy link
Owner

pierky commented Nov 18, 2021

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
Copy link
Contributor Author

bluikko commented Nov 19, 2021

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
Copy link
Contributor Author

bluikko commented Nov 19, 2021

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 added a commit that referenced this issue Nov 21, 2021
The JSON schema example gives "24/7" and not "24x7".

Issue #89
pierky added a commit that referenced this issue Nov 21, 2021
pierky added a commit that referenced this issue Nov 21, 2021
pierky added a commit that referenced this issue Nov 21, 2021
pierky added a commit that referenced this issue Nov 21, 2021
@pierky
Copy link
Owner

pierky commented Nov 21, 2021

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
Copy link
Contributor Author

bluikko commented Nov 22, 2021

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
Copy link
Owner

pierky commented Nov 29, 2021

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!

@pierky pierky closed this as completed Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants