fix: allow Domains is empty in TLSAuth

[rainingmaster]

Hi, I found when I set the option like this, the certificates won't be pass to server, and have no error will be reported:

export const options = {
  tlsAuth: [
    {
      cert: open('mycert.pem'),
      key: open('mycert-key.pem'),
    },
  ],
};

I think it is reasonable to allow user set an empty domains filed, so I raise this PR.

Beside, I found the certificates in test are expired since 2018, and I can't found the key of ca in the project. So I have generate some new certificates which will expired after 1000 years, and make the test more clear.

Could you help me review this PR, so Thanks!

[CLAassistant]

All committers have signed the CLA.

[rainingmaster]

I have paste the ca key as comment here, so we can resign or do other thing if we need.

[olegbespalov]

Hi @rainingmaster

Thanks for your PR, It's looks good to me!

[codecov-commenter]

Codecov Report

Merging #2349 (f41c1d9) into master (42f2e60) will increase coverage by 2.47%.
The diff coverage is 77.15%.

❗ Current head f41c1d9 differs from pull request most recent head 0b6d759. Consider uploading reports for the commit 0b6d759 to get more accurate results

@@            Coverage Diff             @@
##           master    #2349      +/-   ##
==========================================
+ Coverage   72.71%   75.19%   +2.47%     
==========================================
  Files         184      193       +9     
  Lines       14571    15392     +821     
==========================================
+ Hits        10596    11574     +978     
+ Misses       3333     3117     -216     
- Partials      642      701      +59     

Flag	Coverage Δ
ubuntu	75.13% <77.15%> (+2.48%)	⬆️
windows	74.94% <77.09%> (+2.57%)	⬆️

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/v1/client/client.go	0.00% <0.00%> (ø)
api/v1/client/metrics.go	0.00% <0.00%> (ø)
api/v1/client/status.go	0.00% <0.00%> (ø)
api/v1/group.go	54.05% <ø> (-35.01%)	⬇️
api/v1/metric.go	54.54% <ø> (-45.46%)	⬇️
api/v1/routes.go	55.31% <ø> (ø)
api/v1/status.go	100.00% <ø> (ø)
cmd/inspect.go	0.00% <0.00%> (ø)
cmd/login_cloud.go	0.00% <0.00%> (ø)
... and 164 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 46d1d98...0b6d759. Read the comment docs.

[rainingmaster]

Just change the err to errC here to fix a shadowing problem by update the commit.

By the way, may I run the CI manually before review?

[olegbespalov]

Changes look good to me! 👍

By the way, may I run the CI manually before review?

Unfortunately, this is a limitation for first-time contributors. But next time should not be the case 😉

To handle this limitation, we implemented a Makefile target check that runs the linter the same version as the CI (inside the docker container) and runs the tests. For sure, it's not the same as the CI, but at least address the linter issues. I hope that helps!

[codebien]

Hi @rainingmaster,
the current implementation LGTM but apparently, the NameToCertificate had been deprecated which seems to be the main reason why nameToCert exists. I think we could accept Go's suggestion and migrate to auto-detection. Does it make sense for you?

[rainingmaster]

Hi @rainingmaster, the current implementation LGTM but apparently, the NameToCertificate had been deprecated which seems to be the main reason why nameToCert exists. I think we could accept Go's suggestion and migrate to auto-detection. Does it make sense for you?

Hi @codebien
Thanks for your review, and I think you have point out below common:

// Deprecated: NameToCertificate only allows associating a single
// certificate with a given name. Leave this field nil to let the library
// select the first compatible chain from Certificates.

Sorry I didn't notice this, and in fact, I have use NameToCertificate before, so I omit this setting in k6 too...

So do you mean we should remove domains in TLSAuth, and we will leave NameToCertificate as nil all times.

If what I said is correct, I agree and will do it, do you mind if I implement it in the this MR?

[codebien]

Hi @rainingmaster,

So do you mean we should remove domains in TLSAuth, and we will leave NameToCertificate as nil all times.

Unfortunately, I don't think we can do it, it would be a breaking change for the k6 API. We can deprecate it but we have to continue to support the previous code.

Instead, we can support the new cases in the new method (e.g. the one you're adding). So if domains is provided then NameToCertificate must be used otherwise is set to nil.
In the case domains is provided we should write a log warnings about the deprecation.

If what I said is correct, I agree and will do it, do you mind if I implement it in the this MR?

It will still support the original proposal and the change will be only an internal refactor, for that I think it's totally fine to have it here.

[rainingmaster]

Hi @codebien

Instead, we can support the new cases in the new method (e.g. the one you're adding). So if domains is provided then NameToCertificate must be used otherwise is set to nil.
In the case domains is provided we should write a log warnings about the deprecation.

I see, so I think what I need to do in this MR is add a log warnings about the deprecation? Please correct me if I'm missing other points.

[codebien]

@rainingmaster and init nameToCert only when len (auth.Domains) > 0

The test of your use case and the test changes added are still nice to have so we can keep them

[rainingmaster]

@rainingmaster and init nameToCert only when len (auth.Domains) > 0

The test of your use case and the test changes added are still nice to have so we can keep them

Hi @codebien

I have add a judgement here and leave some comment.

However, I didn't find a good way to add a log warnings if domains is provided or NameToCertificate is not empty, and I didn't found any deprecation mechanism in Options, or we can add some judgement for the Options when new a runner? I think I need more advice for you to add this log. Thanks!

[rainingmaster]

Hi @codebien
I have try to add some log here, we will get the log like this:

  scenarios: (100.00%) 1 scenario, 20 max VUs, 3m30s max duration (incl. graceful stop):
           * default: Up to 20 looping VUs for 3m0s over 3 stages (gracefulRampDown: 30s, gracefulStop: 30s)

WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here
WARN[0000] the message here

It is because the log will be print when we new each VU, is it acceptable? Or we should log it when new a runner?

[rainingmaster]

I saw a data race error here. I think this error was not introduced by this change, but existed before: because multiple VUs will share the same Option, there will be a competition for reading and writing here when newUV is used.

[codebien]

Hi @rainingmaster,
sorry for the late response.

It is because the log will be print when we new each VU, is it acceptable? Or we should log it when new a runner?

We can use a sync.Once to print the message only the first time. Make sure to exec the code only after the if condition so it doesn't impact any type of run but only when the domains field is provided.

I saw a data race error here

The data race is dependent on the test's design. The real code will invoke the Parse branch only once during the UnmarhalJSON execution.

k6/lib/options.go

Lines 150 to 152 in 77ff732

	if _, err := c.Certificate(); err != nil {
	return err
	}

You should be able to fix the data race invoking the Certificate method before starting the tests so just after the option set, like:

opt.TLSAuth = []*lib.TLSAuth{
...
opt.TLSAuth[0].Certificate()

[rainingmaster]

Hi @codebien. Thanks for your reply. In fact I am doing some job with this PR when you response it, lol.

We can use a sync.Once to print the message only the first time. Make sure to exec the code only after the if condition so it doesn't impact any type of run but only when the domains field is provided.

Do you mean we add a sync.Once for log the NameToCertificate had been deprecated only, because I think it can't shared with other situation

You should be able to fix the data race invoking the Certificate method before starting the tests so just after the option set

Thanks for your remind, I just notice this point. I will improve it.

[rainingmaster]

Hi @codebien
I have try to update

type TLSAuth {
    Origin []*TLSAuthFields
}

instead of

type TLSAuth {
    TLSAuthFields
}

And we can get handle these logic (all Certificates and NameToCertificate) when UnmarshalJSON. So newVU can get data directly instead use a loop to get them every times, I think it will be more efficient

[rainingmaster]

We can use a sync.Once to print the message only the first time. Make sure to exec the code only after the if condition so it doesn't impact any type of run but only when the domains field is provided.

Do you mean we add a sync.Once for log the NameToCertificate had been deprecated only, because I think it can't shared with other situation

By the way, I didn't find a good place to add the sync.Once, due to it will use for log the NameToCertificate had been deprecated only ...

[rainingmaster]

Hi @codebien I have add a log here, I not sure it is OK for you?

[rainingmaster]

Hi @codebien , could you help me review it when you free? Thanks!

[codebien]

Hi @rainingmaster, thanks for your effort on this PR, and sorry for my late response, I was busy with other topics. I added some comments.

It could be good for you to know that we have make lint and make tests commands that mostly replicate the CI jobs, run them before pushing so you can get faster feedback about your code.

[rainingmaster]

Also, I think it's better to maintain the logic for NameToCertificate close to the deprecation warning, so we will change only one place when we will rid the support.

Cool, this is reasonable. I have revert to the old version and add the Once.Do. And all make lint and make tests in pass in my local machine, could you help review again? THX! @codebien

[codebien]

it looks good to me, just a simple comment

[olegbespalov]

@rainingmaster excellent work 👍 Thanks again for the contribution!

[codebien]

Hi @rainingmaster,
it should be nice to mention what are the plans on the nolint directive. After that, I think we should be mostly ready to merge this PR at the beginning of the next week when the v0.38 cycle will start.

[rainingmaster]

Hi @codebien , could you help me review again? Thx!