fix: prefer IP addresses as "less" when comparing endpoints #2716
Conversation
Previously there was no distinction between an IP address and any other string when doing a comparison to determine which is "less" when determining which endpoint to actually create. This explicitly handles IP addresses and will always prefer them over non-IP addresses when determining which of two targets is less.
Required for net/netip package
|
Welcome @wbh1! |
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
size/M
|
/assign @Raffo |
|
ACK that I’ve seen this and I will take a look at it this week. |
There was a problem hiding this comment.
Change LGTM except for one minor comments on error handling. I want to call out explicitly that I am okay with the upgrade to Go 1.18 which is something we should have done anyway, but that will of course require some bake time in terms of testing the image although I don’t expect breaking changes.
endpoint/endpoint.go
Outdated
| return e < o[i] | ||
| // Explicitly prefers IP addresses (e.g. A records) over FQDNs (e.g. CNAMEs). | ||
| // This prevents behavior like `1-2-3-4.example.com` being "less" than `1.2.3.4` when doing lexicographical string comparison. | ||
| ipA, _ := netip.ParseAddr(e) |
There was a problem hiding this comment.
What happens if ParseAddr returns an error? Can we guarantee that it is impossible and thus ignore the error? If this is the case, can we add an error to clarify it to future readers? Same for the line right below this one.
There was a problem hiding this comment.
@Raffo Any parsing errors are caught when the IsValid() function is run below, since an invalid IP address is returned in those cases.
I will add debug-level logging to print out the error and a note to clarify, though.
There was a problem hiding this comment.
Added the aforementioned logging and additional comments. Worth mentioning that the debug logging will probably be noisy as I believe it'll result in at least 1 log line per DNS record being managed when debug logging is enabled.
Log whenever a target is not able to be parsed as an IP address. This is expected to occur fairly often (for example, with CNAME targets), but allows more visibility into how targets are being compared. NOTE: depending on the number and type of targets, this could be quite noisy.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo, wbh1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Description
This PR attempts to handle comparison between the targets of two endpoints more robustly by checking if they are IP addresses before falling back to the original string comparison behavior.
IP addresses are already typically preferred over a hostname, but there is an edge case (like the one linked in the issue below) in which a hostname could be preferred over an IP address. This occurs when the hostname sorts lexicographically before the IP address during string comparison. For example,
1password.comwould be preferred over8.8.8.8but8.8.8.8would be preferred overonepassword.com.Because I chose to use to the new
net/netippackage, I bumped to Go 1.18. I hope that's alright. If not, I can rewrite to use the oldnet.IPtype.Fixes #2499
Checklist