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

Use socket.getaddrinfo instead of socket.gethostbyname #305

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

ott
Copy link
Contributor

@ott ott commented Dec 30, 2021

socket.gethostbyname only returns a single IPv4 address. Any additional
IPv4 addresses and all IPv6 addresses are ignored. As a consequence,
hosts without IPv4 addresses are ignored and any additional information
from additional IPv4 addresses is ignored. If IPv4 addresses are rotated
by the implementation of socket.gethostbyname, for example, common with
DNS servers and resolvers, the geolocation information for the host can
change without every call of socket.gethostbyname.

It seems better to consider all addresses, both all IPv4 and IPv6
addresses, and to summarize the geolocation information when only a
single result is required.

@ott
Copy link
Contributor Author

ott commented Dec 30, 2021

This pull request is a first step to proper IPv6 support and support for IPv6-only mirrors.

@ott
Copy link
Contributor Author

ott commented Dec 30, 2021

IPv6 support is tracked in issue #306 for the moment.

@ott ott force-pushed the getaddrinfo branch 2 times, most recently from a358946 to 5cfd112 Compare December 31, 2021 00:14
@abompard abompard force-pushed the master branch 4 times, most recently from 861f200 to d9d0078 Compare July 12, 2024 10:19
socket.gethostbyname only returns a single IPv4 address. Any additional
IPv4 addresses and all IPv6 addresses are ignored. As a consequence,
hosts without IPv4 addresses are ignored and any additional information
from additional IPv4 addresses is ignored. If IPv4 addresses are rotated
by the implementation of socket.gethostbyname, for example, common with
DNS servers and resolvers, the geolocation information for the host can
change without every call of socket.gethostbyname.

It seems better to consider all addresses, both all IPv4 and IPv6
addresses, and to summarize the geolocation information when only a
single result is required.

Signed-off-by: Matthias-Christian Ott <[email protected]>
@ott
Copy link
Contributor Author

ott commented Aug 16, 2024

I have updated this pull request and would appreciate if it could be merged before larger changes are made again that create conflicts and would require me to update this pull request again.

@abompard
Copy link
Member

Thanks for updating it! I've fixed a couple remaining issues due to the rebase, and refactored the common code.

@abompard abompard merged commit f68c665 into fedora-infra:master Aug 19, 2024
4 checks passed
@ott
Copy link
Contributor Author

ott commented Aug 19, 2024

Now that it is possible to store the geographic location of a host in the database, it would perhaps also be a good idea to use this information in the world map and perhaps also in the crawler if it is available.

@abompard
Copy link
Member

The world map is generated from the mirror coordinates in the database already. Or did I misunderstood your question?

@ott
Copy link
Contributor Author

ott commented Sep 7, 2024

No, it is not. The location is generated from the city geolocation. See mirrormanager2/utility/generate_worldmap.py:

            host.latitude = city.location.latitude
            host.longitude = city.location.longitude

@abompard
Copy link
Member

abompard commented Sep 9, 2024

Ah, you may have been tricked into believing that the generate_worldmap.py script actually generates the worldmap. It would be too easy! It only stores the host coordinates in the database. The map itself is generated dynamically using the data from the mirrors_location view in views.py. The unfortunate name of this module is historic: it used to generate a static worldmap (in image format). Now it only stores the coordinates in the database.

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.

2 participants