feature: Add optional port to host mappings. #1489

calavera · 2020-06-07T23:02:29Z

This is an extension for the host mapping feature. It allows people to map to a different port than the one in the original url.

This is very convenient when you run k6 against a fleet of docker
containers to test the same service but want to keep the port mapping.

Signed-off-by: David Calavera david.calavera@gmail.com

CLAassistant · 2020-06-07T23:02:33Z

All committers have signed the CLA.

codecov-commenter · 2020-06-07T23:25:12Z

Codecov Report

Merging #1489 into master will increase coverage by 0.03%.
The diff coverage is 82.41%.

@@            Coverage Diff             @@
##           master    #1489      +/-   ##
==========================================
+ Coverage   74.82%   74.85%   +0.03%     
==========================================
  Files         164      164              
  Lines       14035    14104      +69     
==========================================
+ Hits        10501    10558      +57     
- Misses       3002     3010       +8     
- Partials      532      536       +4

Impacted Files	Coverage Δ
lib/options.go	`84.34% <68.42%> (-3.35%)`	⬇️
lib/netext/dialer.go	`96.29% <91.30%> (-1.14%)`	⬇️
lib/testutils/httpmultibin/httpmultibin.go	`92.77% <100.00%> (+0.17%)`	⬆️
lib/testutils/minirunner/minirunner.go	`78.57% <0.00%> (-3.58%)`	⬇️
stats/cloud/collector.go	`79.29% <0.00%> (+0.54%)`	⬆️
lib/executors.go	`94.11% <0.00%> (+1.96%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c68a4eb...bb293f2. Read the comment docs.

imiric

Thanks for your contribution! I left a few comments, but don't see major blockers for merging this in.

Just a heads-up: this will likely have to wait until #1007 is finally merged (any day now... 🤞).

And I'm not sure how this could be defined with the K6_HOSTS environment variable, which even without ports is not intuitive or well documented. So maybe some more tests would be needed there, or it could wait for #883 and the removal of envconfig.

lib/options.go

lib/netext/dialer_test.go

mstoykov

Hi @calavera, Thanks for the PR!!! 🎉
I have left some comments, mostly around more tests :D, great job!

Unfortunately (as has been mentioned in other PRs as well) we are at the end of working on PR 1007 which is a quite a big change (I think you won't need to rebase, but don't quote me :D) and don't really have time for anything else(mostly because working on other things is one of the reasons why it has been so long in the making). For this, we are more or less not merging any PRs until we've merged (and possibly released) 1007 as it's quite huge on its own.

So unfortunately this (as well as a bunch of other PRs) will probably wait for at least another month (hopefully not more) until we get back to you :(. You can of course continue working on this, and if we have some time to spare we might look at it, but don't count on it :(.

Thanks again, and sorry for the inconvenience :(

lib/testutils/httpmultibin/httpmultibin.go

lib/netext/dialer_test.go

lib/netext/dialer.go

calavera · 2020-06-08T15:34:04Z

So unfortunately this (as well as a bunch of other PRs) will probably wait for at least another month (hopefully not more) until we get back to you

Thanks for the heads up @mstoykov. Don't worry about it, take your time with all other priorities that you have. I actually wrote this code months ago an never had time to push the PR. I'll address your comments for when you're ready to take a deeper look.

I was going to write some documentation about this change, but I couldn't find any place to do it. Any pointers will be appreciated.

mstoykov · 2020-06-08T15:40:13Z

I was going to write some documentation about this change, but I couldn't find any place to do it. Any pointers will be appreciated.

I meant to spare you that ... but, given that you asked :D , there is this k6-docs repo :D.

Also if you click on the suggest edits on top of this page you will be forwarded to the actual file you need to change (as the structure isn't very intuitive at least for me :( )

Thanks again for all the work :D

calavera · 2020-06-21T21:22:51Z

@mstoykov I've updated the code with your suggestions, thanks! 🙌

I'll wait for your review to make any changes in the docs.

mstoykov · 2020-07-20T09:00:08Z

Hi @calavera , we have released v0.27.0 last week, and while we are fixing up some stuff for v0.27.1, you should probably rebase this after we merge #1562 as that will have confilcts.
Hopefully we will be able to get this in v0.28.0 ;)

calavera · 2020-07-20T15:59:42Z

Thanks for the update @mstoykov 👍

imiric

Hey @calavera, thanks again for this work! 👍

I had forgotten about this PR and was stepping on your toes with #1562 😅, but we decided to avoid the merging conflicts and close that PR in favor of this one. The IPv6 fix will have to wait for 0.28.0 because of the additional port feature introduced here, but that's been broken for a long time now, so it can wait for a couple more weeks. :)

I took another look at this and it looks good, besides that test nitpick. If you can rebase your branch on current master, we'll merge this as soon as 0.27.1 is released (this or next week?). The rebase conflicts should be simple to fix 🤞

lib/netext/dialer_test.go

lib/netext/dialer.go

mstoykov

I took a quick glance with the morning coffee - so I might be completely wrong :D

lib/netext/dialer.go

mstoykov · 2020-07-28T07:09:48Z

lib/options.go

+	host := string(text)
+	var port string
+
+	if isHostPort(text) {
+		var err error
+		host, port, err = net.SplitHostPort(host)
+		if err != nil {
+			return nil, "", err
+		}
+	}
+
+	ip := net.ParseIP(host)
+	if ip == nil {
+		return nil, "", &net.ParseError{Type: "IP address", Text: host}
+	}
+


are you here trying to figure out if there is a port ? so that net.SplitHostPort doesn't return you an error? Because IMO you can just call it - and if there is no error - there is a port, if there is an error - there is no port :D
As far as I can see this code currently doesn't handle something.com:8413 for example correctly ?

I've removed the double check.

As far as I can see this code currently doesn't handle something.com:8413 for example correctly ?

I don't understand this comment. As far as I can tell, the current code expects the option to be a valid IP address, and something.com won't ever work as IP address.

lib/netext/dialer.go

calavera · 2020-08-13T14:29:10Z

Thanks for the review @mstoykov. I pushed new changes.

lib/netext/dialer.go

That way people can map a name under different ports. This is very convenient when you run k6 against a fleet of docker containers to test the same service but want to keep the port mapping. Signed-off-by: David Calavera <david.calavera@gmail.com>

- Include tests for denied hosts. - Include copyright notice in new file. Signed-off-by: David Calavera <david.calavera@gmail.com>

- Fix default ports for tests. Signed-off-by: David Calavera <david.calavera@gmail.com>

Clean lint errors. Signed-off-by: David Calavera <david.calavera@gmail.com>

Add new tests for dialer and options. Signed-off-by: David Calavera <david.calavera@gmail.com>