Fix blacklistIPs JS configuration #1004

THoelzel · 2019-04-21T18:38:47Z

Net.IPNet does not support JSON unmarshalling, causing the blacklistIPs
configuration to fail when set through JS.
Fixed by wrapping net.IPNet for the purpose of JSON unmarshalling.

Fixes #973

Net.IPNet does not support JSON unmarshalling, causing the blacklistIPs configuration to fail when set through JS. Fixed by wrapping net.IPNet for the purpose of JSON unmarshalling. Fixes grafana#973

CLAassistant · 2019-04-21T18:38:54Z

All committers have signed the CLA.

cmd/options.go

lib/options.go

js/runner_test.go

codecov · 2019-04-21T18:51:04Z

Codecov Report

Merging #1004 into master will decrease coverage by 0.15%.
The diff coverage is 85%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   72.31%   72.16%   -0.16%     
==========================================
  Files         132      131       -1     
  Lines        9703     9635      -68     
==========================================
- Hits         7017     6953      -64     
+ Misses       2272     2268       -4     
  Partials      414      414

Impacted Files	Coverage Δ
cmd/options.go	`66.37% <0%> (ø)`	⬆️
lib/netext/dialer.go	`94.36% <100%> (ø)`	⬆️
lib/options.go	`92.67% <100%> (+0.57%)`	⬆️
lib/netext/httpext/tracer.go	`96.42% <0%> (-1.79%)`	⬇️
core/engine.go	`92.99% <0%> (-0.94%)`	⬇️
js/modules/k6/http/request.go	`79.78% <0%> (-0.71%)`	⬇️
lib/netext/httpext/request.go	`88.17% <0%> (-0.17%)`	⬇️
lib/netext/httpext/error_codes.go	`100% <0%> (ø)`	⬆️
lib/netext/httpext/compression_type_gen.go

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 3fc300c...820ff7c. Read the comment docs.

codecov · 2019-04-21T18:51:04Z

Codecov Report

Merging #1004 into master will decrease coverage by 0.2%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   72.31%   72.11%   -0.21%     
==========================================
  Files         132      131       -1     
  Lines        9703     9640      -63     
==========================================
- Hits         7017     6952      -65     
+ Misses       2272     2271       -1     
- Partials      414      417       +3

Impacted Files	Coverage Δ
lib/netext/dialer.go	`94.36% <ø> (ø)`	⬆️
cmd/options.go	`66.37% <0%> (ø)`	⬆️
lib/options.go	`89.79% <68.42%> (-2.3%)`	⬇️
lib/netext/httpext/tracer.go	`96.42% <0%> (-1.79%)`	⬇️
core/engine.go	`92.99% <0%> (-0.94%)`	⬇️
js/modules/k6/http/request.go	`79.78% <0%> (-0.71%)`	⬇️
lib/netext/httpext/request.go	`88.17% <0%> (-0.17%)`	⬇️
lib/netext/httpext/error_codes.go	`100% <0%> (ø)`	⬆️
lib/netext/httpext/compression_type_gen.go

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 3fc300c...3c90d08. Read the comment docs.

codecov · 2019-04-21T18:51:04Z

Codecov Report

Merging #1004 into master will decrease coverage by 0.2%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   72.31%   72.11%   -0.21%     
==========================================
  Files         132      131       -1     
  Lines        9703     9640      -63     
==========================================
- Hits         7017     6952      -65     
+ Misses       2272     2271       -1     
- Partials      414      417       +3

Impacted Files	Coverage Δ
lib/netext/dialer.go	`94.36% <ø> (ø)`	⬆️
cmd/options.go	`66.37% <0%> (ø)`	⬆️
lib/options.go	`89.79% <68.42%> (-2.3%)`	⬇️
lib/netext/httpext/tracer.go	`96.42% <0%> (-1.79%)`	⬇️
core/engine.go	`92.99% <0%> (-0.94%)`	⬇️
js/modules/k6/http/request.go	`79.78% <0%> (-0.71%)`	⬇️
lib/netext/httpext/request.go	`88.17% <0%> (-0.17%)`	⬇️
lib/netext/httpext/error_codes.go	`100% <0%> (ø)`	⬆️
lib/netext/httpext/compression_type_gen.go

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 3fc300c...3c90d08. Read the comment docs.

codecov · 2019-04-21T18:51:04Z

Codecov Report

Merging #1004 into master will decrease coverage by 0.2%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   72.31%   72.11%   -0.21%     
==========================================
  Files         132      131       -1     
  Lines        9703     9640      -63     
==========================================
- Hits         7017     6952      -65     
+ Misses       2272     2271       -1     
- Partials      414      417       +3

Impacted Files	Coverage Δ
lib/netext/dialer.go	`94.36% <ø> (ø)`	⬆️
cmd/options.go	`66.37% <0%> (ø)`	⬆️
lib/options.go	`89.79% <68.42%> (-2.3%)`	⬇️
lib/netext/httpext/tracer.go	`96.42% <0%> (-1.79%)`	⬇️
core/engine.go	`92.99% <0%> (-0.94%)`	⬇️
js/modules/k6/http/request.go	`79.78% <0%> (-0.71%)`	⬇️
lib/netext/httpext/request.go	`88.17% <0%> (-0.17%)`	⬇️
lib/netext/httpext/error_codes.go	`100% <0%> (ø)`	⬆️
lib/netext/httpext/compression_type_gen.go

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 3fc300c...3c90d08. Read the comment docs.

lib/options_test.go

lib/options.go

mstoykov

Thanks very much for this PR.
It looks awesome and I just have a comment about the not using "ipnet" directly in ParseCIDR.

I was also wondering if we shouldn't be a little bit more backwards compatible as previously ipnet was definitely marshallable and unmarshalable it was just very inconvenient. although I don't know if anyone has actually used it and whether it is worth it. @na-- ?

mstoykov · 2019-04-22T05:48:34Z

lib/options.go

+	}
+
+	return &IPNet{
+		IPNet: net.IPNet{


Why is this not just ipnet ?

na-- · 2019-04-23T07:14:32Z

@mstoykov I'm not sure if we need to support the previous (unintentional) ways of encoding and decoding values... I'm not sure if anyone used it, since this bug wasn't reported before I accidentally found it. And in the docs we clearly state that blacklisted IPs have the following JS config format:

export let options = {
    blacklistIPs: ["10.0.0.0/8"]
};

So that was definitely a bug. My only concern is that someone could have used the CLI --blacklist-ip flag and then executed k6 cloud (which bundles the script and all options in a .tar archive like k6 archive). In that case, the tar archive's metadata.json file would have contained something like this:

{
  // ...
  blacklistIPs: [
    {"IP":"10.0.0.0","Mask":"/wAAAA=="}
  ],
  //...
}

Which could have actually worked: https://play.golang.org/p/ZHq6_Rs6YsZ

na--

Thanks for this fix, LGTM, besides that minor simplification of having IPNet be directly type IPNet net.IPNet.

As mentioned in the main thread, we might have to expand the UnmarshalJSON() function a little, to support both strings like "10.0.0.0/8" and JS objects like {"IP":"10.0.0.0","Mask":"/wAAAA=="}, if it turns out that a lot of users used the --blacklist-ip CLI flag before this. However, I think we can do that in a separate PR later, after we find out how many users used this in the cloud execution.

lib/options.go

na--

LGTM

mstoykov

LGTM

Fix blacklistIPs JS configuration

3c90d08

Net.IPNet does not support JSON unmarshalling, causing the blacklistIPs configuration to fail when set through JS. Fixed by wrapping net.IPNet for the purpose of JSON unmarshalling. Fixes grafana#973

golangcibot reviewed Apr 21, 2019

View reviewed changes

cmd/options.go Outdated Show resolved Hide resolved

lib/options.go Outdated Show resolved Hide resolved

js/runner_test.go Show resolved Hide resolved

golangcibot reviewed Apr 21, 2019

View reviewed changes

lib/options_test.go Outdated Show resolved Hide resolved

lib/options_test.go Show resolved Hide resolved

lib/options_test.go Show resolved Hide resolved

lib/options_test.go Show resolved Hide resolved

lib/options.go Outdated Show resolved Hide resolved

Improve code coverage

9ec56c8

THoelzel force-pushed the fix/blacklistips-js branch from af2100f to 9ec56c8 Compare April 21, 2019 23:24

mstoykov reviewed Apr 22, 2019

View reviewed changes

lib/options.go Outdated

}

return &IPNet{

IPNet: net.IPNet{

Copy link

Contributor

mstoykov Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not just ipnet ?

Code cleanup

a8f50f1

na-- reviewed Apr 23, 2019

View reviewed changes

lib/options.go Outdated Show resolved Hide resolved

Switch to UnmarshalText

820ff7c

na-- approved these changes Apr 25, 2019

View reviewed changes

mstoykov approved these changes Apr 25, 2019

View reviewed changes

na-- merged commit 1621aad into grafana:master Apr 25, 2019

imiric mentioned this pull request Jul 5, 2021

blacklistIPs JS configuration isn't parsed correctly, pt. 2 #2083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix blacklistIPs JS configuration #1004

Fix blacklistIPs JS configuration #1004

THoelzel commented Apr 21, 2019

CLAassistant commented Apr 21, 2019 •

edited

Loading

codecov bot commented Apr 21, 2019 •

edited

Loading

codecov bot commented Apr 21, 2019

codecov bot commented Apr 21, 2019

codecov bot commented Apr 21, 2019

mstoykov left a comment

mstoykov Apr 22, 2019

na-- commented Apr 23, 2019

na-- left a comment

na-- left a comment

mstoykov left a comment

Fix blacklistIPs JS configuration #1004

Fix blacklistIPs JS configuration #1004

Conversation

THoelzel commented Apr 21, 2019

CLAassistant commented Apr 21, 2019 • edited Loading

codecov bot commented Apr 21, 2019 • edited Loading

Codecov Report

codecov bot commented Apr 21, 2019

Codecov Report

codecov bot commented Apr 21, 2019

Codecov Report

codecov bot commented Apr 21, 2019

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Apr 22, 2019

Choose a reason for hiding this comment

na-- commented Apr 23, 2019

na-- left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

CLAassistant commented Apr 21, 2019 •

edited

Loading

codecov bot commented Apr 21, 2019 •

edited

Loading