Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document GPRC Connection's TLS option #1290

Merged
merged 6 commits into from
Aug 14, 2023
Merged

Document GPRC Connection's TLS option #1290

merged 6 commits into from
Aug 14, 2023

Conversation

olegbespalov
Copy link
Contributor

What?

Document Connection TLS param from the grafana/k6#3159

Why?

It needs to be documented.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

There's a version of the docs published here:

https://mdr-ci.staging.k6.io/docs/refs/pull/1290/merge

It will be deleted automatically in 30 days.

Copy link
Collaborator

@heitortsergent heitortsergent left a comment

Choose a reason for hiding this comment

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

Left a few comments, mostly suggestions. :shipit:

Co-authored-by: Heitor Tashiro Sergent <heitortsergent@gmail.com>
Copy link
Collaborator

@heitortsergent heitortsergent left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

Comment on lines 72 to 93
// note: the services in this example don't exist. If you would like
// to run this example, make sure to replace the URLs, and
// the cacerts, cert, key, and password variables.
const params = {
'foo1.grpcbin.test.k6.io:9001': {
plaintext: false,
tls: {
cacerts: [open('cacerts0.pem')],
cert: open('cert0.pem'),
key: open('key0.pem'),
},
},
'foo2.grpcbin.test.k6.io:9002': {
plaintext: false,
tls: {
cacerts: open('cacerts1.pem'),
cert: open('cert1.pem'),
key: open('key1.pem'),
password: 'cert1-passphrase',
},
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This was briefly discussed in the original PR - the certificates and keys are likely going to be big enough that loading each and everyone for each VU will be wasteful.

As such I would recommend using SharedArray to anyone who will use this to have different certificates per connection/VU.

I would also prefer if any examples we provide follow this advice as users commonly copy paste examples as-is.

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, except for the not usage of SharedArray

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

With this example we showcase how to use different tls settings between VUs which seems to be closer to the original feature request.

It is also is done a way that will scale better as it uses SharedArray

oleiade and others added 2 commits August 14, 2023 14:33
… grpc/20 Client/20-Client-connect-connect-address-params.md

Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
…ent/20-Client-connect-connect-address-params.md

Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
@oleiade oleiade requested a review from mstoykov August 14, 2023 12:34
… grpc/20 Client/20-Client-connect-connect-address-params.md

Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
…ent/20-Client-connect-connect-address-params.md
@oleiade oleiade merged commit 12e0c49 into main Aug 14, 2023
4 checks passed
@oleiade oleiade deleted the feat/grpc-docs branch August 14, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants