Fix ipv6 support in domain lookups #575
Conversation
Pleasing the linter, and some more documentation
|
|
| @@ -228,13 +256,21 @@ def check_domain(self, domain: str) -> SiteCheck: | |||
| the best matching IP range for the ip address it resolves to, | |||
| or a 'grey' Sitecheck | |||
| """ | |||
| UNRESOLVED_ADDRESS = "0.0.0.0" | |||
There was a problem hiding this comment.
I'm aware that 0.0.0.0 has a special meaning when we pass it to servers. I'm using this placeholder, as I'm not aware of an equivalent IP address that means "not a real address", and in a few places we always assume an ip address string is returned.
I think our workers expect an ip string when writing to the big greencheck table, and I don't want this PR to result in us needing to change out database schema.
| # Ideally we'd check that ALL the IP addresses resolved are within our | ||
| # green IP ranges but until we know how much this impacts performance | ||
| # we choose the first one. | ||
| ip_info = socket.getaddrinfo(domain, None) |
There was a problem hiding this comment.
As the comments around this say - we acknowledge that we really should check that every ip range is green. Ths priority is fixing this first, before adding that functionality in later. See #576 for more details, and future work.
1 similar comment
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR is intended to resolve #573 , which came up when we realised that domain lookups that resolve to an ipv6 address were not working properly.
It covers both cases, and lays the path for a updating this to check ALL ip addresses a domain might resolve to in future, not just the first one.
We don't immediately moved to checking every domain, as I'm not sure of the performance implications, and there are a few places that expect just one ip address to be returned - we'd need to make quite a few changes elsewhere.