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

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

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

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?

You also return False for zero without logging the message.

I would suggest to drop the check above.

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

> +
> +		# ASN is fine if we made it here...
> +		return True

Ellipses in comments are sometimes weird...

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