search

ipinfo / rust

IPinfo Rust library for IPinfo API (IP geolocation and other types of IP data)
https://ipinfo.io
Apache License 2.0
56 stars 14 forks source link

Revert "Add IPv6 support" #52

Closed Alextopher closed 10 months ago

Alextopher commented 10 months ago

Reverts ipinfo/rust#51

I think the motivation of this change is to allow users of this library to connect to the ipinfo.io API using IPv6. Before this change only the IPv4 domain is used.

In my opinion #51 did not improve the status-quo in the right way. A few thoughts:

lookup_v6 misleads users as to what it does. Because of the missing type information and documentation the name wrongly implies that lookup(&str) only works on IPv4 addresses while lookup_v6(&str) deals with IPv6 addresses. This is not true and is confusing.

If we do want to give users more control over these kinds lower level decisions then I think we would be better off allowing the user to provide their own reqwest::Client. Then they can use local_address to bind to a particular local address.

Choice between using lookup_v6 and lookup methods should almost never be a compile time decision. When I compile my binary I do not know if it will be running in an IPv4 only, IPv6 only, or Both environment.

On this note I think this change should be reworked. I would recommend making IPv4 vs IPv6 address choice transparent. Try to use v6 and then fallback to v4 after a connection failure.

Alextopher commented 10 months ago

Sorry for not catching this before the original PR was merged :chart_with_downwards_trend:. I'm now watching and stargazing this repository so that shouldn't happen again :smile:

I'm happy to make a new PR or edit this one with these suggestions materialized if @talhahwahla doesn't want to give it another try.

UmanShahzad commented 10 months ago

It's certainly liable to confusion. We should rethink it.

It doesn't make sense though to try with v6 first and then try v4, that's going to be a non-trivial expense depending on the network stack, and then negatively affect all calls by v4-only users.

Really the use case of this function is just one: look up your own v6 IP, if you're on a v6-viable stack, because v6.ipinfo.io only has a AAAA record and no A record.

Simplest solution for that use case is to just remove the input parameter and clearly write that this is for looking up your own IP. Plus rename to e.g. lookup_self_v6.

UmanShahzad commented 10 months ago

@talhahwahla thoughts too?

Alextopher commented 10 months ago

It doesn't make sense though to try with v6 first and then try v4, that's going to be a non-trivial expense depending on the network stack, and then negatively affect all calls by v4-only users.

I agree, but I would need to play around with reqwest's behavior in this case to see how it preforms.

Really the use case of this function is just one: look up your own v6 IP, if you're on a v6-viable stack, because v6.ipinfo.io only has a AAAA record and no A record.

IPv6 has some inherent advantages over IPv4. If we can support both without significant cost I don't see why we shouldn't.

talhahwahla commented 10 months ago

Simplest solution for that use case is to just remove the input parameter and clearly write that this is for looking up your own IP. Plus rename to e.g. lookup_self_v6.

Second that.

But I see here https://github.com/ipinfo/go/pull/62/ we also allow client level configuration. So maybe we can do that too, with v4 as default. Will change the ui though.