chore(allow_hosts): Use getaddrinfo instead of gethostbyname

[hasier]

#189 introduced hostname resolution to allow passing actual host names rather than IPs to the allowlist. This used getaddrinfo to get the actual IP addresses of hosts, but this is limited to only 1 IPv4 address result, where the address may have more than 1, or maybe even IPv6. This is a limitation stated in the Python docs, and the suggested alternative is to use gethostbyname.

I assumed the same tests from #189 should apply, as the functionality itself is not changing. Please let me know if there are any other tests you'd like to see.

As a side effect, I have also slightly improved the error message for SocketConnectBlockedError, as I found it'd help me with some of the tests I've been building lately. Where the error message used to be like

A test tried to use socket.socket.connect() with host "172.217.169.3" (allowed: "google.com").

now will look more like

A test tried to use socket.socket.connect() with host "172.217.169.3" (allowed: "google.com (172.217.169.14)").

[hasier]

Seems like https://httpbin.org/ is having some issues, it's 504ing in the browser for me as well. I'll keep an eye on it to see if we can rerun when it's stable again.

[hasier]

@miketheman https://httpbin.org/ seems to be better now, feel free to approve the test run whenever you prefer 🙂

[miketheman]

@hasier Thanks for working through this - glad to see the existing behaviors are not affected.
There's enough code changes that don't have any new tests - can you please add some in another commit to detail what the behaviors that you're seeing trigger these new code paths?

[hasier]

@hasier Thanks for working through this - glad to see the existing behaviors are not affected. There's enough code changes that don't have any new tests - can you please add some in another commit to detail what the behaviors that you're seeing trigger these new code paths?

@miketheman the underlying behaviour was not changed, so I could only think of a test that used a hostname directly as an argument, which I added in 75c0a8f. Let me know what you think 🙂

EDIT: pastebin seems to be having some issues again, but they don't seem to be related to the changes (as happened last time), a re-run when it's stable again should confirm this.

[hasier]

@miketheman thanks for re-running the tests! Any thoughts on the changes? 🙂

[miketheman]

Just got back from PyCon, gonna need a few days to recover. 😉

[hasier]

@miketheman any chance you can take a look at this again, whenever you have a bit of time? 😊

[hasier]

@miketheman I have updated the PR with the changes in main, could approve the workflow run and review again whenever you have some time, please? Thanks!

EDIT: pastebin seems to be having some issues once again, but they don't seem to be related to the changes (as happened in the previous times), a re-run when it's stable again should confirm this.

[hasier]

@miketheman seems like HTTPBin is somewhat stable now (at least for the few request I just tried), maybe we can re-run CI one last time? 🤞

[miketheman]

@hasier Thankfully, httpbin played nice today.
We got an interesting error: https://github.com/miketheman/pytest-socket/actions/runs/5305161721/jobs/9601944686#step:6:157

pytest_socket.SocketConnectBlockedError: A test tried to use socket.socket.connect() with host "192.30.255.113" (allowed: "github.com (192.30.255.112)").

Is this due to now how the functionality works with multiple DNS addresses?

[hasier]

@hasier Thankfully, httpbin played nice today. We got an interesting error: https://github.com/miketheman/pytest-socket/actions/runs/5305161721/jobs/9601944686#step:6:157

pytest_socket.SocketConnectBlockedError: A test tried to use socket.socket.connect() with host "192.30.255.113" (allowed: "github.com (192.30.255.112)").

Is this due to now how the functionality works with multiple DNS addresses?

Thanks for rerunning it @miketheman!

Hmm interesting, not sure it's directly related to the new functionality, the tests in the rest of Python versions succeeded, and it did locally for me too, so the resolution seems accurate most of the time 🤔 The new way of resolving IPs should simply yield multiple results, as opposed to just 1 from the previous method, but in the test there was only 1 result returned (note how there's only 1 IP allowed in the message (allowed: "github.com (192.30.255.112)")). Considering we are reaching out to a real address and the OS will be resolving DNSs each time, maybe we are more liable to this sort of tiny variations as Github will regularly change their external IPs, which will make this test flaky...

I have changed the test to check values only against "localhost", similar to what other tests do, also ensuring that it still resolves the hostname to its 127.0.0.1 and ::1 IPs to test the new functionality. This way we avoid the flakiness of a real address changing IPs, while still ensuring that "localhost" is correctly resolved to IP addresses. wdyt?

[codeclimate]

Code Climate has analyzed commit 44636ea and detected 0 issues on this pull request.

View more on Code Climate.

[miketheman]

@hasier thanks again for persisting! It's been a bit rocky for sure, but I think we got to a better solution overall.

[hasier]

Amazing, thanks very much to you @miketheman for bearing with all these reruns! 🙇