Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Add IPv6 support" #52

Merged

Conversation

Alextopher
Copy link
Contributor

Reverts #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
Copy link
Contributor Author

Alextopher commented Jan 17, 2024

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

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
Copy link
Contributor

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
Copy link
Contributor

@talhahwahla thoughts too?

@Alextopher
Copy link
Contributor Author

Alextopher commented Jan 17, 2024

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
Copy link
Contributor

talhahwahla commented Jan 18, 2024

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 ipinfo/go#62 we also allow client level configuration. So maybe we can do that too, with v4 as default. Will change the ui though.

@talhahwahla talhahwahla merged commit e085dd3 into ipinfo:master Jan 18, 2024
1 check failed
@Alextopher Alextopher deleted the revert-51-talhahwahla/ipv6-support branch January 18, 2024 13:58
@talhahwahla talhahwahla mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants