Add support for gRPC calls. #100

Shikugawa · 2021-04-01T08:46:44Z

On the next release of Envoy, we will utilize cluster-specific gRPC call. We introduce this feature on the Rust SDK side in this PR.

PiotrSikora

Could you rebase this PR on top of current master? Also, please regenerate Bazel rules using cargo-raze version mentioned in the README.md, and add the example to BUILD file.

examples/grpc_call/helloworld.rs

src/hostcalls.rs

examples/grpc_call/main.rs

Signed-off-by: Rei Shimizu <[email protected]> Signed-off-by: Shikugawa <[email protected]>

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-04-02T12:46:20Z

.github/workflows/rust.yml

@@ -132,7 +133,7 @@ jobs:
      run: cargo doc --no-deps --target=wasm32-unknown-unknown

    - name: Package (publish)
-      run: cargo publish --dry-run --target=wasm32-unknown-unknown
+      run: cargo publish --dry-run --target=wasm32-unknown-unknown --no-verify


This change is because CI will generate example/grpc_call/helloworld.rs while publishing but the commit doesn't have this file and to resolve this mismatch. If we include this file in this codebase, we will face mismatch also while publishing because built file doesn't have licence but committed file has this one. licencer doesn't have `exclude' option to skip verify while validation on dir walk. so we must add licence to the file while committing. If we have this PR on licencer, we don't have this workaround.

Shikugawa · 2021-04-07T15:43:10Z

@PiotrSikora friendly ping

PiotrSikora · 2021-04-08T09:44:03Z

@Shikugawa having protobuf compiler as a build dependency for the library is quite unfortunate, and I don't this acceptable.

I see 3 options:

Find a way to execute build.rs and the protobuf compiler only when building examples.
Make grpc_call a standalone crate under examples and remove references to it from the main Cargo.toml.
Remove grpc_call example.

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-04-11T12:07:28Z

@PiotrSikora For now we should delete grpc callout example.

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-04-13T13:29:52Z

@PiotrSikora Could we merge this?

PiotrSikora · 2021-04-14T11:00:21Z

@PiotrSikora Could we merge this?

I'll get back to reviewing this in a day or two, sorry.

src/hostcalls.rs

src/dispatcher.rs

Signed-off-by: Shikugawa <[email protected]>

src/dispatcher.rs

Signed-off-by: Shikugawa <[email protected]>

src/dispatcher.rs

src/hostcalls.rs

Signed-off-by: Shikugawa <[email protected]>

src/dispatcher.rs

PiotrSikora · 2021-04-26T09:39:31Z

src/traits.rs

+            message,
+            timeout,
+        )
+    }


Sorry for missing it earlier, but you're missing grpc_cancel host call (it should probably be called cancel_grpc_call to match the existing naming convention).

src/dispatcher.rs

Signed-off-by: Shikugawa <[email protected]>

src/hostcalls.rs

PiotrSikora · 2021-04-27T09:59:15Z

src/hostcalls.rs

+            status => panic!("unexpected status: {}", status as u32),
+        }
+    }
+}


I don't believe grpc_close is used for gRPC call, it's effectively an alias for grpc_cancel.

I agree with it. But host provides a path to reset gRPC call so that we should provide it on SDK side, I think.

What I'm saying is that both grpc_cancel and grpc_close are doing exactly the same thing in Envoy for gRPC calls:

Context::grpcClose:
https://github.com/envoyproxy/envoy/blob/87d5eb7d44e4b24b2d098ce073f8742610c5d03e/source/extensions/common/wasm/context.cc#L1886-L1895

Context::grpcCancel:
https://github.com/envoyproxy/envoy/blob/87d5eb7d44e4b24b2d098ce073f8742610c5d03e/source/extensions/common/wasm/context.cc#L1913-L1922

It would be actually interesting to see what happens on the wire, i.e.is it RST_STREAM(CANCEL)?

Ok. so we should provide cancel_grpc_call on unary call, and cancel_grpc_stream which send RST_STREAM and close_grpc_stream which transit the caller state to Half-Closed, Right?

src/traits.rs

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora

I'll run some end-to-end tests tomorrow before merging, but everything looks good.

Thanks and sorry for the nitpicking!

PiotrSikora

There is no support for reading the gRPC response. Please add it.

PiotrSikora · 2021-04-28T08:43:05Z

There is no support for reading the gRPC response. Please add it.

You only need to add get_grpc_call_response_body in src/traits.rs and GrpcCallResponseBody in src/types.rs.

Signed-off-by: shikugawa <[email protected]>

Shikugawa · 2021-05-04T08:41:50Z

@PiotrSikora Sorry for my delay! I added get_grpc_call_response_body as well as get_grpc_call_initial/trailing_metadata.

PiotrSikora · 2021-05-04T08:55:14Z

I added [...] get_grpc_call_initial/trailing_metadata.

Those are actually not available in unary gRPC calls. Compare Envoy interfaces for gRPC calls and gRPC streams.

Shikugawa · 2021-05-04T09:06:04Z

@PiotrSikora After merged this, I will continue to work #101. So I don't think it should be a problem for users.

PiotrSikora · 2021-05-04T09:10:58Z

@PiotrSikora After merged this, I will continue to work #101. So I don't think it should be a problem for users.

They are not part of the "gRPC calls" feature, so they should not be part of this PR.
They are named get_grpc_call_..., and not get_grpc_stream_....

If you want to include them in #101 that's great, but I think they should be removed from this PR.

Signed-off-by: shikugawa <[email protected]>

PiotrSikora

LGTM, sans nit.

src/traits.rs

Signed-off-by: shikugawa <[email protected]>

PiotrSikora · 2021-05-04T09:50:28Z

Thanks!

Shikugawa requested a review from PiotrSikora as a code owner April 1, 2021 08:46

Shikugawa force-pushed the grpc-support branch 4 times, most recently from 4b76db7 to d83447f Compare April 1, 2021 16:36

PiotrSikora reviewed Apr 2, 2021

View reviewed changes

examples/grpc_call/helloworld.rs Outdated Show resolved Hide resolved

src/hostcalls.rs Outdated Show resolved Hide resolved

examples/grpc_call/main.rs Outdated Show resolved Hide resolved

Shikugawa force-pushed the grpc-support branch 2 times, most recently from afcd8ac to b978698 Compare April 2, 2021 12:08

add gRPC callout support

93fadae

Signed-off-by: Rei Shimizu <[email protected]> Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from b978698 to 93fadae Compare April 2, 2021 12:09

Shikugawa changed the title ~~add gRPC call dispatch support~~ add gRPC callout support Apr 2, 2021

not to push generated protobuf code

21a0cd6

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from d3e9ce0 to 21a0cd6 Compare April 2, 2021 12:22

exec raze

9632564

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from c4c244c to 9632564 Compare April 2, 2021 12:29

no-verify while publish

2adf4b3

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from 9ed9037 to 2adf4b3 Compare April 2, 2021 12:44

Shikugawa commented Apr 2, 2021

View reviewed changes

remove grpc example

6d0c3bd

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from b9b7850 to 6d0c3bd Compare April 11, 2021 12:06

delete

4c277f9

Signed-off-by: Shikugawa <[email protected]>

Shikugawa mentioned this pull request Apr 13, 2021

Add support for gRPC streams. #101

Merged

PiotrSikora reviewed Apr 19, 2021

View reviewed changes

fix

b634b08

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora reviewed Apr 20, 2021

View reviewed changes

src/dispatcher.rs Outdated Show resolved Hide resolved

Shikugawa force-pushed the grpc-support branch from 21ac2f4 to cac2474 Compare April 20, 2021 08:29

fix

26f916a

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from cac2474 to 26f916a Compare April 20, 2021 08:33

PiotrSikora reviewed Apr 20, 2021

View reviewed changes

src/dispatcher.rs Outdated Show resolved Hide resolved

src/dispatcher.rs Outdated Show resolved Hide resolved

src/hostcalls.rs Outdated Show resolved Hide resolved

src/hostcalls.rs Outdated Show resolved Hide resolved

src/hostcalls.rs Outdated Show resolved Hide resolved

fix

b6a5e7a

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from 054dced to b6a5e7a Compare April 20, 2021 09:37

PiotrSikora reviewed Apr 26, 2021

View reviewed changes

fix

27648e9

Signed-off-by: Shikugawa <[email protected]>

PiotrSikora reviewed Apr 27, 2021

View reviewed changes

fix

91e1d65

Signed-off-by: Shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from 54d5024 to 91e1d65 Compare April 27, 2021 10:10

PiotrSikora changed the title ~~add gRPC callout support~~ Add support for gRPC calls. Apr 27, 2021

PiotrSikora approved these changes Apr 27, 2021

View reviewed changes

PiotrSikora requested changes Apr 28, 2021

View reviewed changes

fix

dc0ea2a

Signed-off-by: shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from a417fda to dc0ea2a Compare May 4, 2021 08:41

cleanup

c641e4a

Signed-off-by: shikugawa <[email protected]>

PiotrSikora approved these changes May 4, 2021

View reviewed changes

src/traits.rs Outdated Show resolved Hide resolved

nit

24102b9

Signed-off-by: shikugawa <[email protected]>

Shikugawa force-pushed the grpc-support branch from 7d9ab40 to 24102b9 Compare May 4, 2021 09:23

PiotrSikora merged commit 30066a7 into proxy-wasm:master May 4, 2021

Shikugawa deleted the grpc-support branch May 4, 2021 11:48

PiotrSikora mentioned this pull request May 28, 2021

gRPC support #90

Closed

PiotrSikora mentioned this pull request Jul 3, 2024

Add support for nested gRPC callouts. #240

Merged

Add support for gRPC calls. #100

Add support for gRPC calls. #100

Uh oh!

Conversation

Shikugawa commented Apr 1, 2021

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shikugawa Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shikugawa commented Apr 7, 2021

Uh oh!

PiotrSikora commented Apr 8, 2021

Uh oh!

Shikugawa commented Apr 11, 2021

Uh oh!

Shikugawa commented Apr 13, 2021

Uh oh!

PiotrSikora commented Apr 14, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrSikora Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PiotrSikora Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Shikugawa Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Shikugawa Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

PiotrSikora Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Apr 28, 2021

Uh oh!

Shikugawa commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrSikora commented May 4, 2021

Uh oh!

Shikugawa commented May 4, 2021

Uh oh!

PiotrSikora commented May 4, 2021

Uh oh!

PiotrSikora left a comment

Shikugawa Apr 2, 2021 •

edited

Loading

Shikugawa commented May 4, 2021 •

edited

Loading