Support binary passed in GRPC metadata #3234
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #3234 +/- ##
==========================================
- Coverage 73.19% 73.15% -0.05%
==========================================
Files 258 256 -2
Lines 19884 19885 +1
==========================================
- Hits 14555 14546 -9
- Misses 4405 4409 +4
- Partials 924 930 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@sapphire-janrain Thanks for the contribution 🙇 left a few comments |
olegbespalov
added
the
documentation-needed
There was a problem hiding this comment.
Hey @sapphire-janrain, thanks for your contribution 🙇
LGTM, just a minor comment. Note: You have to rebase the branch on top of master for fixing the xk6 steps of the CI.
js/modules/k6/grpc/client.go
Outdated
| // The spec defines that Binary-valued keys end in -bin | ||
| // https://grpc.io/docs/what-is-grpc/core-concepts/#metadata | ||
| if strings.HasSuffix(hk, "-bin") { | ||
| binval, ok := kv.([]byte) | ||
| if !ok { | ||
| return result, fmt.Errorf("metadata %q value must be a string or binary", hk) | ||
| } | ||
| strval = string(binval) | ||
| } else { | ||
| return result, fmt.Errorf("metadata %q value must be a string", hk) | ||
| } |
There was a problem hiding this comment.
A passing comment without much knowledge about gRPC:
- I find it strange that we do not check the suffix before we try to make it into a string.
- I find it strange that after we get something that isn't as string as per it's definition we just cast it into a string.
There was a problem hiding this comment.
I find it strange that we do not check the suffix before we try to make it into a string.
I agree, good suggestion
There was a problem hiding this comment.
I find it strange that we do not check the suffix before we try to make it into a string.
My main motivation for this is that more often than not it's going to be a string, so I was avoiding this change causing any impact. Happy to change if that's preferred.
I find it strange that after we get something that isn't as string as per it's definition we just cast it into a string.
This is all the GRPC go library does too. If you're expecting some kind of encoding, that wouldn't be desirable. It's just an unfortunate side-effect of the GRPC library having no means of accepting binary data directly.
There was a problem hiding this comment.
I agree w @mstoykov' suggestion (and @olegbespalov now 🎉), so marking as Request changes again.
Use Uint8Array type in metadata declaration to avoid any reencoding issues in goja.
|
@sapphire-janrain looks good to me 👍 let's resolve the linter issue: and I'll approve that. Thanks for the contribution! 🙇 |
| if !ok { | ||
| return result, fmt.Errorf("metadata %q value must be binary", hk) | ||
| } | ||
| strval = string(binval) |
There was a problem hiding this comment.
Can we link to https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#storing-binary-data-in-metadata so that whoever comes after us knows why this is how it is done instead of keeping it in binary and giving it to the grpc library as binary
There was a problem hiding this comment.
LGTM! Thanks for the work 🙇
I will still like a comment as I had to go on a wide goose chase to figure out that the grpc library actually wants string and that it is supposed to work like that.
Which I still find strange, but at least is what is expect so 🤷
Additionally this brings the question of how do we handle it if we get binary metadata back 🤔
|
I made it a more permanent link in case future docs changes on their end rename or reorganize it. |
There was a problem hiding this comment.
LGTM! Thanks for the contribution! 🙇
@codebien will postpone merging action to you 😅
There was a problem hiding this comment.
LGTM, thanks again for your contribution @sapphire-janrain 🎉 I will merge as soon as the CI will be green.
Add support for binary metadata (postfixed with `-bin`). This ports changes from the grafana/k6#3234
Add support for binary metadata (postfixed with `-bin`). This ports changes from the grafana/k6#3234
Support binary data for Metadata following the specification that requires a -bin suffix.
Use Uint8Array type in metadata declaration to avoid any reencoding issues in goja.
What?
Changes k6's GRPC client to accept Uint8Array types passed as metadata values.
Why?
Currently k6 does not support adding binary data in the metadata. The suggestion seems to have been to convert it to a string of some sort but there are issues therein:
-binmetadata fields itself.As such, it would be best for k6 to properly accept Uint8Arrays in metadata.
Checklist
make ci-like-lint) and all checks pass.make tests) and all tests pass.Caveats
You cannot store Uint8Array in the returned data object from setup (possibly related to #2579 ) as it's converted to a base64'd string before it's passed to the scenario. I'm not sure why exactly and didn't investigate, I assume that's expected behavior (though it was surprising to me). I'm not inclined to fix it here as it's simple to work around for my use case, but happy to create a ticket if it does happen to be unexpected.