-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 IPv6 support #1562
Fix IPv6 support #1562
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1562 +/- ##
==========================================
- Coverage 77.11% 77.10% -0.02%
==========================================
Files 162 162
Lines 13192 13199 +7
==========================================
+ Hits 10173 10177 +4
- Misses 2499 2503 +4
+ Partials 520 519 -1
Continue to review full report at Codecov.
|
lib/netext/dialer.go
Outdated
ip := net.ParseIP(host) | ||
if ip == nil { | ||
// It's not an IP address, so lookup the hostname in the Hosts | ||
// option before trying to resolve DNS. | ||
var ok bool | ||
ip, ok = d.Hosts[host] | ||
if !ok { | ||
var dnsErr error | ||
ip, dnsErr = d.Resolver.FetchOne(host) | ||
if dnsErr != nil { | ||
return nil, dnsErr | ||
} | ||
} |
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.
This is probably good for a function and can be tested on it's own
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 agree, the IP:port
splitting and DNS resolving logic can be a separate function(s), so you can test it with unit tests with valid and invalid IPv4, IPv6, and domains. I also think we can also test the actual IPv6 connection logic as well, at least if we have IPv6 on our systems or CI. And we can reliably skip it if we don't.
We should be able to use net.Listen
with tcp6
for network
and port 0
. If it returns an error, then we t.Skip()
the test. If not, we can spin up a test httptest.Server
, or even httpmultibin there.
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.
We should be able to use net.Listen with tcp6 for network and port 0. If it returns an error, then we t.Skip() the test. If not, we can spin up a test httptest.Server, or even httpmultibin there.
I suppose this could be done, I am not sure it will test everything nicely as we can probably test it on the loopback interface, but not between machines .... 🤷 But we can at least try that :D
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.
The loopback interface should still be employing most of the networking stack, I think - from the standpoint of Go and k6, everything should be pretty much the same, only the OS cares... That said, besides running the tests in the IPv6-enabled Docker container, like @imiric showed, might be the only way to run that test, for now, given that GitHub Actions doesn't support it: actions/runner-images#668 (comment) 😞
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.
Let me know if 437d91f is enough.
Ideally resolveHost()
should work on a Resolver
interface so that we can mock out the net.LookupIP()
call. Right now this does an actual DNS lookup, which might fail if the DNS server doesn't support IPv6. Unlikely, but we should avoid environment issues like this, and there's no need to test the DNS. :)
Since we don't do much mocking elsewhere, it was easier to not do this, but let me know.
I'll look into the higher-level connection tests with httpmultibin
.
EDIT: Ah, spoke too soon. Maybe it should be mocked after all.
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.
@imiric I vote for t.Skip
on windows and unskipping it once on github actions for windows
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'm leaning towards the interface approach, as it's not an issue with Windows, but with the environment. This test could fail just as easily on a dev machine using a non-IPv6 DNS, so removing the environment out of the equation is the safest approach for these unit tests. For integration tests there should be no mocking, but we can mark them as such and run them in an integration environment eventually.
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.
Take a look at d6c6ecc.
I decided to extract more of DialContext
into that function, so now it's awkwardly named checkAndResolveAddress
, but I think having unit tests for this crucial part of k6 makes up for it.
lib/netext/dialer.go
Outdated
@@ -66,31 +65,42 @@ func (b BlackListedIPError) Error() string { | |||
return fmt.Sprintf("IP (%s) is in a blacklisted range (%s)", b.ip, b.net) | |||
} | |||
|
|||
func resolveHost(host string, hosts map[string]net.IP, resolver *dnscache.Resolver) (net.IP, error) { |
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.
A minor nitpick, but if this was a method of the Dialer
, you can skip the first 2 arguments while still having it very testable. And mocking it can easily be done by just defining a simple new interface
with a FetchOne()
method yourself, for now (there will probably be further methods once we fix the DNS issues) and using that in the Dialer
.
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, I did that in d6c6ecc, but only the hosts
argument can be removed, we need the other 2 :)
As discussed, closing this in favor of #1489, which also fixes this. |
Closes #1537
This was straightforward to fix once I realized I could test this with Docker 😄 It's trivial to setup, so we could have a few integration/E2E tests in CI. Other than that, I'm not sure how else to test this without substantially refactoring the
Dialer
to make it testable. Any other ideas?