-
Notifications
You must be signed in to change notification settings - Fork 46
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
PeeringDB AS-set name format should not be used in AS-set IRR object export #126
Comments
Hello @bluikko, thanks for reporting this issue, and also for the very detailed analysis and proposal. I've just pushed a commit in a branch where I implemented a fix for this. First, the code of The second part of the implementation is about the addition of two command-line options, The proposal that you documented for the case n. 2 is very detailed, but it's also quite complex to implement. There's also the risk that an object in one IRR DB goes out of sync with the same object inside the registry for which the members list is generated, potentially leading to more confusion and ambiguity. |
Wise words.
In my opinion you've yet again struck a great balance with the commit already. It should be enough -- at least I know it is perfectly suitable for our needs. Now that I see your solution I believe my detailed proposal in the OP goes beyond the necessary. Also: I did a very short survey inspecting a grand total of 2 smaller IX AS-sets and they just included the out-of-registry object names such as "AS-HURRICANE" as-is. Maybe it's common to just strip the Had finally the chance to start testing this. Hit a traceback (using the latest Docker version)...
Traceback (most recent call last):
File "/usr/local/bin/arouteserver", line 64, in <module>
if main():
^^^^^^
File "/usr/local/bin/arouteserver", line 53, in main
return cmd.run()
^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/commands/tpl_rendering.py", line 423, in run
return super(IRRASSetCommand, self).run()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/commands/tpl_rendering.py", line 187, in run
builder.render_template(output_file=self.args.output_file)
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/builder.py", line 816, in render_template
self.enrich_j2_environment(env)
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/builder.py", line 1452, in enrich_j2_environment
members.update(self._get_valid_as_sets(as_sets_from_pdb))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/builder.py", line 1370, in _get_valid_as_sets
source, as_set = self._get_as_set_info(raw_as_set)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pierky/arouteserver/builder.py", line 1363, in _get_as_set_info
return source_asset.group(1), source_asset.group(2)
^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'group' The preceding line is the first one of the processing batch with a hierarchical AS-set name -- ASNs processed earlier are non-hierarchical AS-set names (e.g. ARouteServer 2023-12-01 00:39:45,481 INFO No AS-SETs provided for the 'AS65530_1' client. Using AS65530 + those obtained from PeeringDB: AS65530:AS-REDACTED. Went looking at the diff if I see anything obvious. The following comment & code seems suspicious for other (possibly unrelated) reasons: # Converting "SOURCE:AS-FOO" format (single colon) to "SOURCE::AS-FOO"
pattern = re.compile("^([^:]+):([^:].+)$", flags=re.IGNORECASE)
v, _ = pattern.subn("\\1::\\2", v) But this syntax is not to refer to a IRRDB of name |
Even if that's a common practice, it's probably more secure to let the operator judge on a case-by-case basis and "own" the decision on whether to include the AS-SET into the
Doh! Sorry for that, my fault. Going to look into it right now.
Of course, yet another fault on my end. Of course it must be allowed as is. |
@bluikko I've just pushed a commit that should fix both situations. CI/CD is running right now, once done (hopefully 1 hour from now) a pre-release should be out: v1.21.5-alpha1. You can install it following the instructions at https://arouteserver.readthedocs.io/en/dev/INSTALLATION.html#development-and-pre-release-versions (it's on the Test PyPi instance, so If you confirm it works in your use case, I'll make it official. Otherwise, pls share more details on how I could reproduce the issue. Thanks a lot for your feedback and the detailed analysis that you've provided once again! |
Had other things to do during the weekend so I apologize for taking so long.
I am testing with the Docker image tag I am seeing some new output from the jobs, for example: ARouteServer 2023-12-04 03:07:55,322 INFO Checking latest version (current version is 1.21.5-alpha1)...
/opt/pypy/lib/pypy3.9/site-packages/certifi/cacert.pem None
/opt/pypy/lib/pypy3.9/site-packages/certifi/cacert.pem None
ARouteServer 2023-12-04 03:07:57,166 INFO Started processing configuration for /etc/arouteserver/templates/bird/main.j2
ARouteServer 2023-12-04 03:07:57,168 INFO Enricher 'PeeringDB AS-SET' started
/opt/pypy/lib/pypy3.9/site-packages/certifi/cacert.pem None
/opt/pypy/lib/pypy3.9/site-packages/certifi/cacert.pem None
ARouteServer 2023-12-04 03:08:00,256 INFO Enricher 'PeeringDB AS-SET' completed successfully after 3 seconds In the above output the I'd say this issue can be closed then. Edited to add: looking at the diff... I just cannot believe what PeeringDB has done. Seriously, there is no set format for referring to an IRRdb? There can be 3 different formats (3 separators: |
Thanks for confirming! The pipeline to get the final release out is running now, if everything goes well, we'll have 1.21.5 out in about 1 hour.
You might be interested in this issue on the PeeringDB repo: peeringdb/peeringdb#151 |
If AS-set name data for peers was enriched from PeeringDB it is often in the format "source::as-set", for example "APNIC::AS-EXAMPLE" or "RADB::AS-EXAMPLEV6".
When generating an AS-set IRR object with arouteserver this same format is used in the resulting IRR object. This seems to not be a valid format when updating IRR objects.
Trying to use this IRR object in e.g. APNIC will result in an error.
Fixing the problem might not be totally straightforward since there are 2 cases:
The seemingly best (?) solution to case 2 above would perhaps be to:
The text was updated successfully, but these errors were encountered: