hadiasghari / pyasn

Python IP address to Autonomous System Number lookup module. (Supports fast local lookups, and historical lookups using archived BGP dumps.)
Other
292 stars 72 forks source link

Issue with multihomed IP addresses #46

Closed gmmoura closed 4 years ago

gmmoura commented 7 years ago

The tools seems to report only 1 AS out of multiple that multihomed IPs have.

E.g: In [4]: asndb.lookup('37.209.196.7')

Out[4]: (134390, '37.209.196.0/24')

While IP2ASN

whois -h whois.cymru.com " -v 37.209.196.7 2017-06-10 00:00:00 GMT" |cut -d'|' -f1,2,3

AS | IP | BGP Prefix 18210 | 37.209.196.7 | 37.209.196.0/24
134390 | 37.209.196.7 | 37.209.196.0/24
134391 | 37.209.196.7 | 37.209.196.0/24
134396 | 37.209.196.7 | 37.209.196.0/24`

hadiasghari commented 7 years ago

Hi @gmmoura , thanks for the input.

Most probably this is the line of code that is responsible in the MRT to IPASN converter:

for prefix, origin in prefixes.items():
    if not debug_write_sets and isinstance(origin, set):
        origin = list(origin)[0]  # get an AS randomly if more than one

This could be changed to save the whole set during conversion, and return the whole set by the lookup method.

Could you elaborate a bit on the use-case and how important it is to all ASes returned for these multi-homed IPs?

Cheers, Hadi

hadiasghari commented 7 years ago

I'd appreciate some input to know if this is important to fix.

aaronkaplan commented 7 years ago

On 24 Jul 2017, at 15:40, Hadi Asghari notifications@github.com wrote:

I'd appreciate some input to know if this is important to fix.

I'd suggest that if support for it is implemented, that the lookup function shall have a conditional flag lookup_multi=<bool> which defaults to False (i.e. keep the existing functionality as default).

If you enable it by default, it would confuse existing users of the library.

Best, a.

hadiasghari commented 7 years ago

@aaronkaplan that's exactly my thinking, and if the change is useful only for rare cases, then it has little priority.

hadiasghari commented 7 years ago

BTW, I also wonder if this issue relates to #37.

aaronkaplan commented 7 years ago

On 25 Jul 2017, at 12:10, Hadi Asghari notifications@github.com wrote:

@aaronkaplan that's exactly my thinking, :)

and if the change is useful only for rare cases, then it has little priority.

Yes and no. There are cases where the info is necessary. Since the library can not know what the requirements of the application are, it's a good thing to implement it, especially since it's a cheap , solvable issue. But, I would definitely not change the default behaviour to return only one ASN. But rather document that the multi-ASN feature exists.

My 2 cents, a.

hadiasghari commented 7 years ago

The change requires producing IPASNDAT files with multiple ASes listed for some prefixes, and this might be incompatible with existing versions of the library (specifically the load function in the C code). This might be OK, but I'd prefer to avoid it if possible. Will check the code.

ghost commented 7 years ago

Then the new version is simply 2.0 :)

hadiasghari commented 4 years ago

@wagner-certat, do you have any code/workaround on this for a possible PR? If so I'd be happy to do a merge. Otherwise I'm closing the issue as a won't fix since the original poster doesn't have an interest in it either.

ghost commented 4 years ago

I did not work on it. Does the closing of the issue mean that the feature request is rejected?

hadiasghari commented 4 years ago

The idea is not rejected: I agree that it could be a useful addition. The question is if anyone wants the feature enough to implement it, and the moment the answer to that question is negative. Do you suggest I keep these kind of issues open?