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

["Peter Müller" <peter.mueller@ipfire.org>]

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

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. :-)

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.

> 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...

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

???

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
>