feat(local): load system cert pool. Resolves #2364

[tbourrely]

Hey 🙂

This is what I understood of the #2364 issue. I'd like some input on how to test this programmatically. I don't see much change when running k6 with or without the line.

Regards,

[codecov-commenter]

Codecov Report

Merging #2660 (86180c3) into master (7f823f0) will increase coverage by 0.20%. The diff coverage is 87.99%.

@@            Coverage Diff             @@
##           master    #2660      +/-   ##
==========================================
+ Coverage   75.59%   75.79%   +0.20%     
==========================================
  Files         202      207       +5     
  Lines       15992    16407     +415     
==========================================
+ Hits        12089    12436     +347     
- Misses       3151     3204      +53     
- Partials      752      767      +15     

Flag	Coverage Δ
ubuntu	75.58% <87.99%> (+0.18%)	:arrow_up:
windows	75.40% <87.99%> (+0.11%)	:arrow_up:

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

Impacted Files	Coverage Δ
api/common/context.go	100.00% <ø> (ø)
api/server.go	71.42% <ø> (ø)
api/v1/client/client.go	0.00% <ø> (ø)
api/v1/client/metrics.go	0.00% <ø> (ø)
api/v1/client/status.go	0.00% <ø> (ø)
api/v1/errors.go	76.92% <ø> (ø)
api/v1/group.go	54.05% <ø> (ø)
api/v1/group_jsonapi.go	95.74% <ø> (ø)
api/v1/group_routes.go	60.86% <ø> (ø)
api/v1/metric.go	77.27% <ø> (+9.09%)	:arrow_up:
... and 244 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[olegbespalov]

Hey @tbourrely !

Thanks for the contribution, I've just wanted to say that currently, the team is busy with a new release, but we'll get back to you straight after that!

Cheers!

[codebien]

Hi @tbourrely, thanks for your contribution. :pray:

I think a better place for this code could be closer to the TLS configuration initialisation: https://github.com/grafana/k6/blob/e09bb87277865d668586429eee97158fbbfc58e5/js/runner.go#L169-L177

You can test checking the p95 and avg metrics are equal (with some tolerance) to the min metric. An example for testing the Runner code with TLS can be found here: https://github.com/grafana/k6/blob/e09bb87277865d668586429eee97158fbbfc58e5/js/runner_test.go#L958

@olegbespalov and @mstoykov WDYT? @tbourrely wait for more opinions from the other reviewers before submitting changes.

[mstoykov]

It likely is better near to the tls configurations, but it also might be better to be inside of sync.Once as in practice we are trying to do this once so that we preload it before the actual test starts :thinking:.

It might even be better to just do it in a goroutine earlier, maybe while making a new Runner :thinking: this way it can go in the background while we continue with the other operations, hopefully it will be done before we need it and if not the program will still work :shrug:

This likely needs to be looked into further, but all of those sound as an improvement.

But I definitely would want this to be tested as I do remember that this function had some problems on specific OSes

[mstoykov]

Actually yes if you look into the test failure you will see how it fails with

crypto/x509: system root pool is not available on Windows

I don't know though why it only happens for the 1.17.x :shrug:

edit: seems to have been fixed with https://github.com/golang/go/commit/3544082f75fd3d2df7af237ed9aef3ddd499ab9c ... so this might be blocked on us dropping 1.17.x support - very likely for the next release when we move to 1.19.x as the primary version we release with

[codebien]

Now you remember me that it has a strong dependency from the OS I would prefer to have it in the cmd package, where the buildTestRunState method seems to me a good candidate. https://github.com/grafana/k6/blob/93b3a309944aec2cc8284e115334c015eb76e708/cmd/test_load.go#L243-L258

In this way, it makes more sense to have an integration test because it has the tight dependency on the OS.

@mstoykov WDYT?

[mstoykov]

I am fine with it being in cmd - not so certain where exactly.

But if we pull it earlier and earlier I am definitely for it to be in a separate goroutine, and preferably to not abort the test even if it fails, but instead report a warning :shrug:. Although I guess after the windows fix it won't be returning an error :shrug:

WDYT @na-- @olegbespalov

[tbourrely]

Hey 😊

Thanks for your inputs. I will have a look at it as soon as I can and comeback to you if some things are still unclear.

[tbourrely]

Hey @codebien @mstoykov @olegbespalov can you confirm that the changes corresponds to what you had in mind ? if so, I will move forward with the integration test 🙂

[codebien]

Hi @tbourrely, this seems ready to be reviewed again. Can you rebase the branch and resolve the conflict, please? We would like to include this at the beginning of the next cycle.

[tbourrely]

Hey 👋

I was not super convinced of my test but I'll gladly rebase and put it to review.

Sorry for the delay !

[tbourrely]

Rebased & squashed 👍