Skip to content

chore: resolve external IP with just an http request#8516

Merged
DaniPopes merged 2 commits intomainfrom
dani/one-nat-resolver
May 30, 2024
Merged

chore: resolve external IP with just an http request#8516
DaniPopes merged 2 commits intomainfrom
dani/one-nat-resolver

Conversation

@DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented May 30, 2024

Removes two dependencies used solely for getting the external IP:

  • igd-next
  • public-ip, unmaintained for 2 years

which will be duplicating old versions of reqwest, hyper and http after #7018

@DaniPopes DaniPopes marked this pull request as ready for review May 30, 2024 14:51
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with removing igd-next, don't think this is actually useful.

I also, don't think we really need DNS queries

Comment on lines +183 to +186
let response = reqwest::get("http://ipinfo.io/ip").await.ok()?;
let response = response.error_for_status().ok()?;
let text = response.text().await.ok()?;
text.trim().parse().ok()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public_ip::addr does a bit more than that, for example DNS queries

but we can add support natively I think, and this should be sufficient

but can we do "http://bot.whatismyipaddress.com" as well? then we're on par with public_ip for the HTTP part at least

turns out this API was shut down:

https://whatismyipaddress.com/api

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public_ip::addr does a bit more than that, for example DNS queries

yes would be nice if it was maintained tho :D

@mattsse mattsse added C-debt A clean up/refactor of existing code A-networking Related to networking in general labels May 30, 2024
@DaniPopes DaniPopes enabled auto-merge May 30, 2024 15:29
@DaniPopes DaniPopes force-pushed the dani/one-nat-resolver branch from 3379521 to 05ead79 Compare May 30, 2024 15:30
@DaniPopes DaniPopes added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit 9a08ad7 May 30, 2024
@DaniPopes DaniPopes deleted the dani/one-nat-resolver branch May 30, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-networking Related to networking in general C-debt A clean up/refactor of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants