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 [email protected]

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 <[email protected]>

- Include tests for denied hosts. - Include copyright notice in new file. Signed-off-by: David Calavera <[email protected]>

- Fix default ports for tests. Signed-off-by: David Calavera <[email protected]>

Clean lint errors. Signed-off-by: David Calavera <[email protected]>

Add new tests for dialer and options. Signed-off-by: David Calavera <[email protected]>

Signed-off-by: David Calavera <[email protected]>

Add more test cases. Signed-off-by: David Calavera <[email protected]>

That way we avoid a round trip for IP addresses. Signed-off-by: David Calavera <[email protected]>

Signed-off-by: David Calavera <[email protected]>

Copy the struct before setting the new port. Signed-off-by: David Calavera <[email protected]>

Signed-off-by: David Calavera <[email protected]>

lib/netext/dialer.go

Signed-off-by: David Calavera <[email protected]>

imiric

This LGTM, save for the typo nitpick. Thanks for your work @calavera and apologies about the delay with merging this.

lib/netext/dialer.go

lib/netext/dialer_test.go

Signed-off-by: David Calavera <[email protected]>

calavera · 2020-08-26T16:16:17Z

I've fixed those typos, thanks for the review!

na--

LGTM, though we should manually test this with k6 archive (for cloud issues) and envconfig issues (K6_HOSTS)

imiric suggested changes Jun 8, 2020

View reviewed changes

lib/options.go Outdated Show resolved Hide resolved

lib/options.go Outdated Show resolved Hide resolved

lib/options.go Outdated Show resolved Hide resolved

lib/netext/dialer_test.go Show resolved Hide resolved

mstoykov requested changes Jun 8, 2020

View reviewed changes

lib/testutils/httpmultibin/httpmultibin.go Outdated Show resolved Hide resolved

lib/netext/dialer_test.go Outdated Show resolved Hide resolved

lib/netext/dialer.go Outdated Show resolved Hide resolved

This was referenced Jul 21, 2020

Fix IPv6 support #1562

Closed

IPv6 support #1537

Closed

imiric reviewed Jul 21, 2020

View reviewed changes

lib/netext/dialer_test.go Outdated Show resolved Hide resolved

calavera force-pushed the host_options_with_port branch from 985e556 to 9262820 Compare July 27, 2020 00:04

imiric reviewed Jul 27, 2020

View reviewed changes

lib/netext/dialer.go Show resolved Hide resolved

mstoykov reviewed Jul 28, 2020

View reviewed changes

calavera requested review from mstoykov and imiric July 31, 2020 17:56

mstoykov reviewed Aug 13, 2020

View reviewed changes

lib/netext/dialer.go Outdated Show resolved Hide resolved

lib/netext/dialer.go Outdated Show resolved Hide resolved

lib/netext/dialer.go Outdated Show resolved Hide resolved

calavera requested a review from mstoykov August 13, 2020 14:28

na-- added this to the v0.28.0 milestone Aug 14, 2020

mstoykov reviewed Aug 17, 2020

View reviewed changes

lib/netext/dialer.go Outdated Show resolved Hide resolved

calavera added 8 commits August 22, 2020 09:46

Add optional port to host mappings.

7bc1ce2

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 <[email protected]>

Alias HostAddress with net.TCPAddr

2e3e951

- Include tests for denied hosts. - Include copyright notice in new file. Signed-off-by: David Calavera <[email protected]>

Fix serialization.

7bef016

- Fix default ports for tests. Signed-off-by: David Calavera <[email protected]>

Extract fetchHostAddress method.

561088a

Clean lint errors. Signed-off-by: David Calavera <[email protected]>

Make host & port work with IPv6.

c867963

Add new tests for dialer and options. Signed-off-by: David Calavera <[email protected]>

Remove unused constant.

71b471f

Signed-off-by: David Calavera <[email protected]>

Ensure ip lookup.

7de1c61

Add more test cases. Signed-off-by: David Calavera <[email protected]>

Parse IP before dns lookup.

3cc8eee

That way we avoid a round trip for IP addresses. Signed-off-by: David Calavera <[email protected]>

calavera added 4 commits August 22, 2020 09:46

Refactor to have less nested conditions.

e882e17

Signed-off-by: David Calavera <[email protected]>

Refactor dialer methods.

8a4a7fa

Signed-off-by: David Calavera <[email protected]>

Fix IPNet declaration.

85aa6f1

Signed-off-by: David Calavera <[email protected]>

Don't modify the configuration pointer.

c2ee499

Copy the struct before setting the new port. Signed-off-by: David Calavera <[email protected]>

calavera force-pushed the host_options_with_port branch 3 times, most recently from 92728b0 to 361e9e2 Compare August 22, 2020 16:57

Extract function to fetch remote from resolver.

97ba9ff

Signed-off-by: David Calavera <[email protected]>

calavera force-pushed the host_options_with_port branch from 361e9e2 to 97ba9ff Compare August 22, 2020 16:58

calavera requested a review from mstoykov August 22, 2020 17:12

calavera force-pushed the host_options_with_port branch 2 times, most recently from 21f9732 to 3532479 Compare August 22, 2020 17:45

Fix lint errors.

3731805

Signed-off-by: David Calavera <[email protected]>

calavera force-pushed the host_options_with_port branch from 3532479 to 3731805 Compare August 22, 2020 17:47

mstoykov reviewed Aug 24, 2020

View reviewed changes

lib/netext/dialer.go Outdated Show resolved Hide resolved

lib/netext/dialer.go Outdated Show resolved Hide resolved

Cleanup code.

b408bb8

Signed-off-by: David Calavera <[email protected]>

imiric previously approved these changes Aug 26, 2020

View reviewed changes

lib/netext/dialer.go Outdated Show resolved Hide resolved

lib/netext/dialer_test.go Outdated Show resolved Hide resolved

imiric mentioned this pull request Aug 26, 2020

Add blocking by hostname patterns (--block-hostname) #1532

Merged

Fix typos.

bb293f2

Signed-off-by: David Calavera <[email protected]>

calavera dismissed imiric’s stale review via bb293f2 August 26, 2020 16:15

calavera requested a review from imiric August 26, 2020 16:16

imiric approved these changes Aug 27, 2020

View reviewed changes

na-- approved these changes Aug 27, 2020

View reviewed changes

mstoykov approved these changes Aug 28, 2020

View reviewed changes

na-- merged commit 3f1bafa into grafana:master Aug 28, 2020

calavera deleted the host_options_with_port branch August 28, 2020 15:38

mstoykov added a commit to grafana/k6-docs that referenced this pull request Sep 23, 2020

Document grafana/k6#1489

52eab26

ppcano pushed a commit to grafana/k6-docs that referenced this pull request Sep 23, 2020

Document grafana/k6#1489 (#113)

ed5c680

simskij pushed a commit to grafana/k6-docs that referenced this pull request Nov 16, 2020

Document grafana/k6#1489 (#113)

882aac1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add optional port to host mappings. #1489

feature: Add optional port to host mappings. #1489

calavera commented Jun 7, 2020

CLAassistant commented Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jun 7, 2020 •

edited

Loading

imiric left a comment

mstoykov left a comment

calavera commented Jun 8, 2020

mstoykov commented Jun 8, 2020

calavera commented Jun 21, 2020

mstoykov commented Jul 20, 2020

calavera commented Jul 20, 2020

imiric left a comment

mstoykov left a comment

mstoykov Jul 28, 2020

calavera Jul 30, 2020 •

edited

Loading

calavera commented Aug 13, 2020

imiric left a comment

calavera commented Aug 26, 2020

na-- left a comment

feature: Add optional port to host mappings. #1489

feature: Add optional port to host mappings. #1489

Conversation

calavera commented Jun 7, 2020

CLAassistant commented Jun 7, 2020 • edited Loading

codecov-commenter commented Jun 7, 2020 • edited Loading

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

calavera commented Jun 8, 2020

mstoykov commented Jun 8, 2020

calavera commented Jun 21, 2020

mstoykov commented Jul 20, 2020

calavera commented Jul 20, 2020

imiric left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Jul 28, 2020

Choose a reason for hiding this comment

calavera Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

calavera commented Aug 13, 2020

imiric left a comment

Choose a reason for hiding this comment

calavera commented Aug 26, 2020

na-- left a comment

Choose a reason for hiding this comment

CLAassistant commented Jun 7, 2020 •

edited

Loading

codecov-commenter commented Jun 7, 2020 •

edited

Loading

calavera Jul 30, 2020 •

edited

Loading