Closed bluikko closed 9 months ago
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 irr-as-set
now considers the source of an as-set and excludes it from the members
list if it's not matching the value used for source:
in the template. When that happens, a warning message is logged to inform the operator.
The second part of the implementation is about the addition of two command-line options, --include-members
and --exclude-members
, that can be passed to the command in order to forcedly include or exclude members from the list. With these two options I wanted to give the operator the freedom of customising the output members list by adding or removing as-set arbitrarily.
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. I'm torn on whether to implement it or not. It really goes a little beyond the scope of ARouteServer, which is basically to generate configs for route servers, and not really to provide a fully fledged comprehensive tool to manage an IXP. Additionally, I'm not sure it's worth it spending time trying to improve (or fix) the IRR ecosystem's inconsistencies nowadays; I see it like a uphill battle.
[...] worth it spending time trying to improve (or fix) the IRR ecosystem's inconsistencies
Wise words.
I'm torn on whether to implement it or not.
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 SOURCE::
and not care what IRRDB the objects exist in?
I could take a look at some more IX AS-sets if you'd like some numbers on whether everyone just simply strips the SOURCE::
and be done with it.
Had finally the chance to start testing this.
Hit a traceback (using the latest Docker version)...
--include-members AS-HURRICANE,AS-HURRICANEV6,<a few others redacted>
.arouteserver irr-as-set
as usual otherwise.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. RIPE::AS-EXAMPLE
and AS-EXAMPLE
)
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 SOURCE
- it is just the syntax for hierarchical AS-set names.
For example AS65530:AS-CUSTOMERS
refers to an object named AS-CUSTOMERS
of AS65530, not to an object named AS-CUSTOMERS
in a non-existing IRRDB with name AS65530.
Am I understanding this correctly that this mangles hierarchical AS-set names to format AS65530::AS-CUSTOMERS
? Wouldn't that be incorrect?
Maybe it's common to just strip the
SOURCE::
and not care what IRRDB the objects exist in?
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 members:
list or not (using the new CLI options).
Hit a traceback (using the latest Docker version)...
Doh! Sorry for that, my fault. Going to look into it right now.
But this syntax is not to refer to a IRRDB of name
SOURCE
- it is just the syntax for hierarchical AS-set names. For exampleAS65530:AS-CUSTOMERS
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 pip install
is not enough)
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!
CI/CD is running right now, once done (hopefully 1 hour from now) a pre-release should be out: v1.21.5-alpha1.
Had other things to do during the weekend so I apologize for taking so long.
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.
I am testing with the Docker image tag 1.21.5-alpha1-pypy3
. And now the AS-set generation works as expected.
Comparing the generated AS-set with output from 1.21.3 looks as expected.
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 cacert.pem None
lines. I guess they could be perhaps related to the use of the Alpha version and would not be present in a final release.
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: @
, ::
, :
)? I always thought they only accept double-colon ::
as separator but maybe they have some historical formats.
Pondering about raising this with them - they really should fix this to a single format only IMHO,
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.
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:
@
,::
,:
)? I always thought they only accept double-colon::
as separator but maybe they have some historical formats. Pondering about raising this with them - they really should fix this to a single format only IMHO
You might be interested in this issue on the PeeringDB repo: https://github.com/peeringdb/peeringdb/issues/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: