Re: [PATCH 1/2] location-importer: Introduce auxiliary function to sanitise ASNs

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 3809 bytes --]

Hi,

> On 13 Oct 2021, at 17:33, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> Hello Michael,
> 
> thanks for your reply.
> 
>> Hello,
>> 
>>> On 10 Oct 2021, at 17:16, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>> 
>>> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
>>> ---
>>> src/python/location-importer.in | 20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/src/python/location-importer.in b/src/python/location-importer.in
>>> index da058d3..c2b3e41 100644
>>> --- a/src/python/location-importer.in
>>> +++ b/src/python/location-importer.in
>>> @@ -574,6 +574,22 @@ class CLI(object):
>>> 		# be suitable for libloc consumption...
>>> 		return True
>>> 
>>> +	def _check_parsed_asn(self, asn):
>>> +		"""
>>> +			Assistive function to filter Autonomous System Numbers not being suitable
>>> +			for adding to our database. Returns False in such cases, and True otherwise.
>>> +		"""
>>> +
>>> +		if not asn or not isinstance(asn, int):
>>> +			return False
>> 
>> Does this happen that a non-integer is being passed to this function?
> 
> What's wrong with input validation? I _like_ input validation. :-)

There is nothing wrong with that. You are just checking the developer here and I am not sure whether you want that or not.

> Seriously: Anything else than an integer does not make sense for an ASN. Sure, this
> function is not intended to get anything else, but we will never know. Better to be
> safe than sorry.

Not entirely. You want code to perform. If you want to be 100% use, Python isn’t the language this parser should be written in.

Nothing else but an integer makes sense. The question is how do you want to treat zero?

>> You also return False for zero without logging the message.
> 
> True. Since there will probably a second version of this patchset, I will ensure it
> logs anything useful in this case.
> 
>> I would suggest to drop the check above.
> 
> Frankly, I don't see why.
> 
>>> +
>>> +		if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)):
>>> +			log.debug("Skipping invalid ASN: %s" % asn)
>>> +			return False
>> 
>> This works, but I do not consider this very Pythonic.
>> 
>> I would have written a tuple which conatins one tuple for each range and then iterate over that until you find a match.
> 
> Far from being a Python developer, this wouldn't have come to my mind. But if it's
> Pythonic, I'll do so. When in Rome...

I don’t make the rules. That is just how I would do it:

* Data in one place

* A short algorithm that works on the data

In C I would hope that the compiler makes it fast.

> 
>> 
>>> +
>>> +		# ASN is fine if we made it here...
>>> +		return True
>> 
>> Ellipses in comments are sometimes weird...
> 
> ???

This one left the comment kind of open ended. Making it sound kind of unlikely.

> 
> Thanks, and best regards,
> Peter Müller
> 
>> 
>>> +
>>> 	def _parse_block(self, block, source_key, validcountries = None):
>>> 		# Get first line to find out what type of block this is
>>> 		line = block[0]
>>> @@ -829,8 +845,8 @@ class CLI(object):
>>> 					log.debug("Skipping ARIN AS names line not containing an integer for ASN")
>>> 					continue
>>> 
>>> -				if not ((1 <= asn and asn <= 23455) or (23457 <= asn and asn <= 64495) or (131072 <= asn and asn <= 4199999999)):
>>> -					log.debug("Skipping ARIN AS names line not containing a valid ASN: %s" % asn)
>>> +				# Filter invalid ASNs...
>>> +				if not self._check_parsed_asn(asn):
>>> 					continue
>>> 
>>> 				# Skip any AS name that appears to be a placeholder for a different RIR or entity...
>>> -- 
>>> 2.26.2