-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix high cpu usage with IPv6 recursor address. Closes #6120 #6128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. While it would definitely fix the linked issue I believe there is a small hole that would allow input to be used that was not valid (such as when foo::53
is provided as the recursor address).
I made some suggestions for how to fix it. (basically just ensure that we have a plain IPv6 address when we get that particular error).
agent/dns.go
Outdated
@@ -270,7 +270,7 @@ func recursorAddr(recursor string) (string, error) { | |||
// Add the port if none | |||
START: | |||
_, _, err := net.SplitHostPort(recursor) | |||
if ae, ok := err.(*net.AddrError); ok && ae.Err == "missing port in address" { | |||
if ae, ok := err.(*net.AddrError); ok && (ae.Err == "missing port in address" || ae.Err == "too many colons in address") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look at the implementation of SplitHostPort
and there are several other real error cases where the too many colons in address
can be returned. In those cases we would not want to use the FormatAddressPort
logic with the default port but rather return the error.
I think something like the following should be sufficient to only assume a non-bracketed IPv6 address when we get that error.
if ae, ok := err.(*net.AddrError); ok {
if ae.Err == "missing port in address" {
recursor = ipaddr.FormatAddressPort(recursor, 53)
goto START
} else if ae.Err == "too many colons in address" {
if ip := net.ParseIp(recursor); ip != nil && ip.To4() == nil {
recursor = ipaddr.FormatAddressPort(recursor, 53)
goto START
}
}
}
I haven't tested that myself but basically it just ensures that in the case of the too many colons error that the address is a valid IPv6 address and we didn't get that error for any other reason.
In addition, it would be great to also add a test for foo::53
as that should return with the too many colons in address
error but we should not attempt to turn it into an actual recursor.
What is the status of this? I'm hitting this error too. |
@42wim This looks like something we would like to fix. Any updates on this one? |
@i0rek seems like I forgot about this issue, I'll try to fix this soon. |
Thank you! |
changes pushed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks great now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.