Support for per-vu TLS configuration (grpc.Client.connect only)

[chrismoran-mica]

What? and Why?

You can now use per-vu TLS configuration on grpc.Client connect params. Previously, grpc.Client connect made use of a shared (immutable) tls.Config cloned into every VU. Now, the connect params argument can contain a tls key that loosely resembles the TLSAuth struct. This will create allow a per-vu TLS configuration for the grpc.Client* allowing you to connect to multiple mTLS endpoints in a single VU. Note: the global tls.Config will remain unmodified.

*Note: the grpc.Client, once connected will utilize the param tlsconfig regardless of the VU that uses the grpc.Client. In order to truly have per-vu tls configuration steps must be taken on the test to ensure each VU uses the grpc.Client with its custom tlsconfig.
For example, in my use case I use multiple VUs to connect to a configurable number of endpoints. Each of these endpoints are mTLS secured with potentially different CA-signed certificates. Each VU will use the grpc.Client with the tlsconfig targeting the relevant endpoint. As an implementation of this may not be obvious, I've included a trivial example below. Allowing different VUs to use different endpoint values I assume is trivial (or it can be considered an exercise for the reader).

HINT: one way would be to load aSharedArray (or SharedArrays) with shards of data. Each index in the SharedArray(s) would be used by a VU. So if you have 10 sets of data and run 10 VUs, you can have each VU pull from their own index in the SharedArrays.

// init phase (1)
const params = {
  "grpcbin.test.notk6.io:9001": {
    plaintext: false,
    tls: {
      cacerts: [open("cacerts0.pem")],
      cert: open("cert0.pem"),
      key: open("key0.pem"),
    }, // password omitted to demonstrate 'optional password'
  },
  "grpcbin.test.fakek6.io:9001": {
    plaintext: false,
    tls: {
      cacerts: open("cacerts1.pem"),
      cert: open("cert1.pem"),
      key: open("key1.pem"),
      password: "cert1-passphrase",
    }, // cacerts as a 'string' to demonstrate string|string[] typing of cacerts
  },
};
const clients = {
  "grpcbin.test.notk6.io:9001": new grpc.Client(),
  "grpcbin.test.fakek6.io:9001": new grpc.Client(),
};
...

// k6 - VU code phase (3)
if (__ITER === 0) {
  clients["grpcbin.test.notk6.io:9001"]
    .connect("grpcbin.test.notk6.io:9001", params["grpcbin.test.notk6.io:9001"]);
  clients["grpcbin.test.notk6.io:9001"]
    .connect("grpcbin.test.fakek6.io:9001", params["grpcbin.test.fakek6.io:9001"]);
}
...

Usage:

// VU 1 - code phase (3)
const hostPort = "grpcbin.test.notk6.io:9001";
const client = clients[hostPort];

const data = { greeting: 'Bert' };
const response = client.invoke('hello.HelloService/SayHello', data);

check(response, {
  'status is OK': (r) => r && r.status === grpc.StatusOK,
});

console.log(JSON.stringify(response.message));
...
// VU 2 - code phase (3)
const hostPort = "grpcbin.test.fakek6.io:9001";
const client = clients[hostPort];

const data = { greeting: 'Ernie' };
const response = client.invoke('hello.HelloService/SayHello', data);

check(response, {
  'status is OK': (r) => r && r.status === grpc.StatusOK,
});

console.log(JSON.stringify(response.message));

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have updated tests to consider my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run linter locally (make ci-like-lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

This PR does not close an issue but it does reference at least one conversation about per-vu mTLS: https://community.k6.io/t/grpc-mtls-per-vu-iteration/1868

[CLAassistant]

All committers have signed the CLA.

[imiric]

Hi there, thanks a lot for this PR! 🙇

I think it makes sense to do this for the gRPC client, but I'm not so sure about the global tlsAuth change. And I'd like to see some gRPC tests for this.

[chrismoran-mica]

Thank you for your feedback @imiric.

I've addressed all of the feedback requests and added some (admittedly not 100% coverage) test cases for the new connectParams tls option. Let me know what I've missed that must/should be included before merge.

(I've also modified the PR description and title to reflect the narrower scope of this).

Please let me know what more I can do to facilitate this PR. I noticed the documentation-needed tag. Shall I create a new PR with the relevant updated documentation?

[mstoykov]

LGTM in general! Thanks for the contribution 🙇

I have left a bunch of change requests and some questions.

My two concerns (which are not inline) are:

1. Organizational - we now have an experimental grpc module where we have some changes. This change will now need to be transfered there. While we might've prefered it the other way around. cc @olegbespalov
2. API one - k6 has problems with big files arguably with the handling of any files to be honest, just big ones make it worse. This particular case seems okay to me. Especially if users figure out how to load the key/cert once . Whihc arguably is doable with a SharedArray with element per each cert+key+password. My other problem is that currently you went with the key and cert being strings instead of binary data. Which I guess is based on what formats golang supports? I do ask the same question inline.

And both of those questions could've been answered if we had an issue to discuss this before hand.

p.s. Your example in the description shows "cert" (and all the other big options) as what looks like file names. But they are actually the contents of those. I would prefer to have the example either just use open("./cert.pem") or be the string (with a lot of ...) that is the actual contents. As in this way it is confusing for both me as reviewer and likely anyone else who in the future tries to figure out what was added here.

[chrismoran-mica]

@mstoykov

I've updated my trivial examples with open statements to address your concern mentioned above.

[chrismoran-mica]

I believe I've addressed all of the feedback so far. Please let me know what else I can do to prepare this for v0.46.0.

[mstoykov]

LGTM! thanks for the contribution.

[olegbespalov]

LGTM, left a few non-blocking suggestions 👍

Thanks for the contribution!

The global tlsAuth was addressed

[chrismoran-mica]

I approved the requested changes but unfortunately one of them broke a couple tests :)

I've fixed them I believe. Sorry for the ping-pong!

[mstoykov]

For future reference while squashing the changes I did delete the mention of the global tlsAuth.

But unfortunately did not fix the example to use the actual contents of a pem file and instead it still has the pem file name 🤦 .

This likely doesn't really matter as both the current PR body and any documentation will have this correct.

[chrismoran-mica]

I updated the example to use “open”. 🤔On Jul 12, 2023, at 12:07 PM, Mihail Stoykov ***@***.***> wrote: For future reference while squashing the changes I did delete the mention of the global tlsAuth. But unfortunately did not fix the example to use the actual contents of a pem file and instead it still has the pem file name 🤦 . This likely doesn't really matter as both the current PR body and any documentation will have this correct. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[mstoykov]

@chrismoran-mica I should've been more specific. This wasn't your fault.
When you squash on github it presents you with the commit messages not the PR description.

Arguably you could've changed the commit message and force pushed, but I would probably also forget to do it or not even think of it.

I should've just been more careful and arguably just written a completely knew message when squashing. It is my fault