Add multi source IP support #1682

mstoykov · 2020-10-22T15:43:49Z

fixes #476

na-- · 2020-10-26T09:22:11Z

cmd/options.go

@@ -93,6 +93,7 @@ func optionFlagSet() *pflag.FlagSet {
 	flags.StringSlice("tag", nil, "add a `tag` to be applied to all samples, as `[name]=[value]`")
 	flags.String("console-output", "", "redirects the console logging to the provided output file")
 	flags.Bool("discard-response-bodies", false, "Read but don't process or save HTTP response bodies")
+	flags.String("client-ips", "", "Client IP Ranges and/or CIDRs from which each VU will be making requests")


--local-ips or --local-addr seems slightly better as a name. This is also what Go uses, see https://golang.org/pkg/net/#IPConn.LocalAddr, https://golang.org/pkg/net/#DialTCP

na--

LGTM, besides the option name and the few minor nitpicks I noted inline.

na-- · 2020-10-26T10:04:46Z

lib/types/ipblock.go

+		return ipBlockFromCIDR(s)
+	default:
+		if net.ParseIP(s) == nil {
+			return nil, fmt.Errorf("%s is not a valid ip, range ip or CIDR", s)


Suggested change

return nil, fmt.Errorf("%s is not a valid ip, range ip or CIDR", s)

return nil, fmt.Errorf("%s is not a valid IP, IP range or CIDR", s)

lib/types/ipblock.go

na-- · 2020-10-26T10:20:50Z

lib/types/ipblock.go

+		block.firstIP.SetBytes(ip0.To4())
+		block.count.SetBytes(ip1.To4())
+	}
+	block.count.Sub(block.count, block.firstIP)


Please add a comment that ip0<ip1 is validated elsewhere, I missed the negative ip range check on the first reading

Also having some tests for this would be good.

lib/types/ipblock.go

imiric · 2020-10-26T10:51:56Z

lib/types/ipblock.go

+		block.firstIP.SetBytes(ip0.To4())
+		block.count.SetBytes(ip1.To4())
+	}
+	block.count.Sub(block.count, block.firstIP)


Also having some tests for this would be good.

lib/types/ipblock.go

codecov-io · 2020-10-26T12:12:40Z

Codecov Report

Merging #1682 into master will increase coverage by 0.27%.
The diff coverage is 77.84%.

@@            Coverage Diff             @@
##           master    #1682      +/-   ##
==========================================
+ Coverage   72.10%   72.37%   +0.27%     
==========================================
  Files         167      176       +9     
  Lines       12882    13536     +654     
==========================================
+ Hits         9288     9797     +509     
- Misses       3031     3126      +95     
- Partials      563      613      +50

Flag	Coverage Δ
#ubuntu	`72.33% <77.84%> (+0.24%)`	⬆️
#windows	`70.97% <77.84%> (+0.41%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/bundle.go	`90.20% <ø> (ø)`
lib/testutils/httpmultibin/httpmultibin.go	`89.88% <ø> (-2.52%)`	⬇️
lib/types/dns_policy_gen.go	`36.36% <36.36%> (ø)`
lib/types/dns_select_gen.go	`45.45% <45.45%> (ø)`
lib/netext/dialer.go	`93.75% <50.00%> (-2.17%)`	⬇️
cmd/options.go	`60.46% <52.63%> (-0.98%)`	⬇️
lib/types/dns.go	`68.29% <68.29%> (ø)`
js/runner.go	`80.13% <74.28%> (-2.20%)`	⬇️
lib/netext/resolver.go	`84.21% <84.21%> (ø)`
lib/testutils/mockresolver/resolver.go	`85.71% <85.71%> (ø)`
... and 18 more

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 0e39d45...ef7eb9d. Read the comment docs.

imiric

LGTM, just a comment nitpick.

imiric · 2020-10-26T14:32:55Z

lib/types/ipblock.go

+}
+
+// ipPoolBlock is almost the same as ipBlock but is put in a IPPool and knows it's start id indest
+// of it's size/count


Please revise this comment. I'm not sure what it's start id indest means, and it's should be its in this case.

"ipPoolBlock is similar to ipBlock but instead of knowing its count/size it knows the first index from which it starts in an IPPool" ?

divfor · 2020-10-26T15:33:26Z

lib/types/ipblock.go

+}
+
+func ipBlockFromCIDR(s string) (*ipBlock, error) {
+	_, pnet, err := net.ParseCIDR(s)


_, pnet, err := net.ParseCIDR(strings.TrimSpace(s))

I don't think the trimming is needed - I just don't think people should add a random amount of additional space in their argument and then tools to just remove them for them.

I am actually thinking if I shouldn't drop it from the places I have apparently not dropped it from 🤔
cc @na-- @imiric

at least it should keep as same as ipBlockFromRange() which is using trimming.

I don't have a strong preference either way, but we should keep it consistent, so removing trimming LGTM.

divfor · 2020-10-26T15:39:59Z

lib/types/ipblock.go

+	// bigger than startIndex but it will be true always for the first block which is with
+	// startIndex 0. This can also be fixed by iterating in reverse but this seems better
+	for i := 0; i < len(pool.list)/2; i++ {
+		pool.list[i], pool.list[len(pool.list)/2-i] = pool.list[len(pool.list)/2-i], pool.list[i]


a reverse should be: pool.list[i], pool.list[len(pool.list)-i] = pool.list[len(pool.list)-i], pool.list[i]

divfor · 2020-10-26T15:51:26Z

js/runner.go

@@ -170,6 +170,10 @@ func (r *Runner) newVU(id int64, samplesOut chan<- stats.SampleContainer) (*VU,
 		BlockedHostnames: r.Bundle.Options.BlockedHostnames.Trie,
 		Hosts:            r.Bundle.Options.Hosts,
 	}
+	if r.Bundle.Options.LocalIPs.Valid {
+		dialer.Dialer.LocalAddr = &net.TCPAddr{IP: r.Bundle.Options.LocalIPs.Pool.GetIP(uint64(id - 1))}


From my testing, id will be 0 in the first VU allocation. so a check of id > 0 should be set.

if r.Bundle.Options.LocalIPs.Valid && id > 0 {

there is a fact: VU (id==0) will never sends requests... I think this is a bug

VU with id 0 is generally used for stuff that "don't" do requests but ... setup and teardown functions can make them and they need to also use one of the provided IPs to make the requests.

Actually speaking of this ... maybe they should use some "randomish" one but instead the first one? Currently They will use (2**64 -1) % IPPool.count which IMO is fine - they use an IP from the provided ones and arguably every IP should be viable, but maybe they should use the first one instead for more consistancy?
cc @imiric @na--

I don't think we should add any special handling for 0 ID VUs, but we should fix the uint64 underflow for the id=0 case. Picking the first one seems fine to me.

from my view, take the first IP is not good since the vu id == 0 will take initialising requests - I guess it is DNS resolving?
if skipped, first VU(id == 0) will be using OS interface ip address (OS to decide what interface & its IP address to reach dns servers). this is needed because most of outside dns servers may not have route to responding to k6's --local-ips.

@divfor

thanks to @divfor

cmd/options.go

js/runner.go

Co-authored-by: na-- <n@andreev.sh>

imiric

LGTM, just remember to squash.

na-- · 2020-10-27T15:34:22Z

lib/options.go

@@ -388,6 +388,9 @@ type Options struct {

 	// Redirect console logging to a file
 	ConsoleOutput null.String `json:"-" envconfig:"K6_CONSOLE_OUTPUT"`
+
+	// Specify client IP ranges and/or CIDR from which VUs will make requests
+	LocalIPs types.NullIPPool `json:"-" envconfig:"K6_LOCAL_IPS"`


I am not sure why this shouldn't be configured via the JS options and from config.json. We can validate so we don't allow it in the cloud, but I don't see to disable it in general.

Why should it be configurable? I find it very unlikely that will be a needed thing for anyone and setting an environment variable won't be a good enough solution as well.

I am for leaving this for when someone actually asks for it as we can always add it

Why should it be configurable?

Because there's no good reason not to be? And, whenever possible, most things in k6 are configurable in 4 different ways - CLI flags, env vars, JS options, JSON options. When that's not the case, it's usually because there is a good reason for it. For example, we need to know the option's value before we execute the init context. Or we think the option is so minor that it'll overcrowd the CLI flags. Or there's a bug in envconfig, or something like that.

The more inconsistencies we have with the k6 options, the worse the user experience is. It's not a big deal, but constantly breaking expectations in this way is bound to get annoying.

na--

LGTM besides the lack of js/json option support, but we can fix that in the future, along with all of the already existing config inconsistencies 😞

Regarding future developments in this direction (weights, randomization and such), I was thinking that shoehorning them in a super-complex global option might not be the best way to implement the feature. I had a brief look at the other PR and it was very difficult to understand things. Rather, given that this is essentially a per-VU setting, why can't we have a JS API to control the source IPs for each VU?

Basically, leave the --local-ips exactly as it is, without any further complications. Instead, in the future, we can add a new JS API that allow users to control different aspects of the VU, including the local IPs. I don't see a problem with setting the local IP in the init context, or even on every iteration... The --local-ips value can be used for validation - if set, allow the JS function to only select IPs from its pool, otherwise allow users to set any local IPs.

More thought obviously has to go into this, but I think it would be better than having increasingly more complex global options. "Pick a random IP out of this pool of IP ranges with different weights" is much more cleanly expressed in a programming language like JS than through some obscure configuration syntax.

And we actually won't have to add this huge complexity to k6 for the few users like @divfor who might need it. We'd just offer a simple API for setting the source IP. Then every user can have an arbitrary complex scheme for picking random or sequential values however they like. The new HTTP client API should also probably have the ability to set a local IP... And this way, local IPs will work much better in conjunction with scenarios - different scenarios can have different pools of local IPs, which seems potentially useful.

na-- · 2020-10-28T08:29:37Z

lib/options.go

@@ -388,6 +388,9 @@ type Options struct {

 	// Redirect console logging to a file
 	ConsoleOutput null.String `json:"-" envconfig:"K6_CONSOLE_OUTPUT"`
+
+	// Specify client IP ranges and/or CIDR from which VUs will make requests
+	LocalIPs types.NullIPPool `json:"-" envconfig:"K6_LOCAL_IPS"`


Why should it be configurable?

Because there's no good reason not to be? And, whenever possible, most things in k6 are configurable in 4 different ways - CLI flags, env vars, JS options, JSON options. When that's not the case, it's usually because there is a good reason for it. For example, we need to know the option's value before we execute the init context. Or we think the option is so minor that it'll overcrowd the CLI flags. Or there's a bug in envconfig, or something like that.

The more inconsistencies we have with the k6 options, the worse the user experience is. It's not a big deal, but constantly breaking expectations in this way is bound to get annoying.

@divfor

fixes grafana#476 thanks to @divfor Co-authored-by: divfor <divfor@gmail.com> Co-authored-by: na-- <n@andreev.sh>

Add multi source IP support

ae6c52c

fixes #476

mstoykov requested review from imiric and na-- October 22, 2020 15:43

na-- reviewed Oct 26, 2020

View reviewed changes

imiric reviewed Oct 26, 2020

View reviewed changes

mstoykov added 7 commits October 26, 2020 13:15

add copyright

02531b5

Drop fmt.Pritnln lines

dc2cf50

changes to comments

01408f7

add ipPoolBlock for better readability

ebd5b47

Rename to local-ips

9a3f095

fixup! add ipPoolBlock for better readability

da3f6b1

Add some error cases tests

645a450

mstoykov requested review from imiric and na-- October 26, 2020 13:39

imiric previously approved these changes Oct 26, 2020

View reviewed changes

divfor reviewed Oct 26, 2020

View reviewed changes

Better ipPoolBlock comment

9a18622

divfor reviewed Oct 26, 2020

View reviewed changes

Fix reversing the ips in IPPool

bd1c03e

thanks to @divfor

divfor mentioned this pull request Oct 26, 2020

add support to ip range #1674

Closed

mstoykov dismissed imiric’s stale review via bd1c03e October 26, 2020 16:05

Drop trimming client-ips everywhere

7e2f8d7

mstoykov requested a review from imiric October 27, 2020 08:58

imiric reviewed Oct 27, 2020

View reviewed changes

cmd/options.go Outdated Show resolved Hide resolved

mstoykov added 2 commits October 27, 2020 14:02

Add example for local-ips cli arg

63d5f1b

Fix overflowing ip index on VU with id 0

df51955

na-- reviewed Oct 27, 2020

View reviewed changes

js/runner.go Outdated Show resolved Hide resolved

Update js/runner.go

ef7eb9d

Co-authored-by: na-- <n@andreev.sh>

imiric approved these changes Oct 27, 2020

View reviewed changes

na-- reviewed Oct 27, 2020

View reviewed changes

na-- approved these changes Oct 28, 2020

View reviewed changes

mstoykov merged commit bf826ba into master Oct 28, 2020

mstoykov deleted the feature/multiSourceIPs branch October 28, 2020 08:50

na-- added this to the v0.29.0 milestone Oct 28, 2020

mstoykov mentioned this pull request Nov 2, 2020

Changes for k6 v0.29.0 grafana/k6-docs#144

Merged

7 tasks

salem84 pushed a commit to salem84/k6 that referenced this pull request Nov 5, 2020

Add multi source IP support (grafana#1682)

a550bec

fixes grafana#476 thanks to @divfor Co-authored-by: divfor <divfor@gmail.com> Co-authored-by: na-- <n@andreev.sh>

This was referenced Nov 16, 2020

New JS API for controlling and inspecting VU behavior #1724

Open

IPv4 and IPv6 selection compatibility between --local-ips and --dns #1733

Open

na-- mentioned this pull request Jan 21, 2021

K6 and IP spoofing #721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multi source IP support #1682

Add multi source IP support #1682

mstoykov commented Oct 22, 2020

na-- Oct 26, 2020

na-- left a comment

na-- Oct 26, 2020

na-- Oct 26, 2020

imiric Oct 26, 2020

imiric Oct 26, 2020

codecov-io commented Oct 26, 2020 •

edited

imiric left a comment

imiric Oct 26, 2020

mstoykov Oct 26, 2020

divfor Oct 26, 2020

mstoykov Oct 26, 2020

divfor Oct 26, 2020

imiric Oct 27, 2020

divfor Oct 26, 2020

divfor Oct 26, 2020

divfor Oct 26, 2020

divfor Oct 26, 2020

mstoykov Oct 26, 2020

imiric Oct 27, 2020

divfor Oct 28, 2020

imiric left a comment

na-- Oct 27, 2020

mstoykov Oct 27, 2020

na-- Oct 28, 2020

na-- left a comment

na-- Oct 28, 2020

	return nil, fmt.Errorf("%s is not a valid ip, range ip or CIDR", s)
	return nil, fmt.Errorf("%s is not a valid IP, IP range or CIDR", s)

Add multi source IP support #1682

Add multi source IP support #1682

Conversation

mstoykov commented Oct 22, 2020

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 26, 2020 • edited

Codecov Report

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 26, 2020 •

edited