Pass handles between nodes using Protocol Buffers #1108

wildarch · 2020-06-05T16:11:00Z

Checklist

Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
- I have written tests that cover the code changes.
- I have checked that these tests are run by
  Cloudbuild
- I have updated documentation accordingly.
- I have raised an issue to
  cover any TODOs and/or unfinished work.
Pull request includes prototype/experimental work that is under
construction.

This is very much still a draft, but tests are passing (at least locally).

Todo:

Finish injection example
Test all the things!
Cleanups
Documentation

Fixes #673.

wildarch · 2020-06-12T14:52:13Z

I think this PR is ready for review now.

Once we can agree on the code I will update the documentation to reflect the changes 😄.

wildarch · 2020-06-12T14:53:31Z

@tiziano88 would you mind reviewing this?

tiziano88 · 2020-06-12T17:37:50Z

Sure, I'll take a look next week. Could you rebase on master first, so that there are no unnecessary conflicts to resolve? Also, for vendoring prost, can you make sure there is an initial pristine commit, and then any subsequent changes are applied on top of that?

tiziano88

Thanks @wildarch , this is looking great! And the changes seem very well contained, which I am sure must have taken a lot of attempts to get to this stage! Have a few comments, the main one related to visitor vs traversable / foldable as the main type for manipulating handles, then I'll take a closer look.

oak/server/rust/oak_runtime/Cargo.toml

sdk/rust/oak/src/handle.rs

sdk/rust/oak_derive/src/lib.rs

sdk/rust/oak_derive/tests/visit.rs

daviddrysdale · 2020-06-16T06:05:29Z

It's a shame that Cargo doesn't support (rust-lang/cargo#4648) applying a patch file to an existing repository (unlike Bazel); is it worth having a play with https://github.com/mettke/cargo-patch ? It might avoid us having to check in an entire copy of prost.

(Alternatively, are the deltas to prost something that might be taken upstream eventually, or are they entirely Oak-specific? If the former, we could put the changes in a forked repo of prost and use the functionality that Cargo's built-in [patch] does support to pull from that until upstream accepts the change.)

wildarch · 2020-06-16T08:22:31Z

I wasn't aware of cargo-patch, looks useful! when checking the readme I stumbled across this: https://github.com/mettke/cargo-patch#limitations. That seems to suggest it only works for binary crates, which would be a problem for a library like the SDK.

@tiziano88 and I did discuss a fork at some point, but apparently that would have to go through some kind of approval process? I'm not sure how this would work, @tiziano88 can you comment on this?

I'm convinced that all the changes that we need to prost can be upstreamed in a more generalized form, I have a PR open to start this process (https://github.com/danburkert/prost/pull/317), but the crate author has not responded to it yet.

sdk/rust/oak/src/handle.rs

sdk/rust/oak/tests/handle_extract_inject.rs

sdk/rust/oak/src/handle.rs

wildarch · 2020-06-24T12:32:42Z

@tiziano88 I think there are some issues with dependency vetting to be resolved, other than that do you think this PR is in a good state to merge?

tiziano88 · 2020-06-24T13:17:08Z

As it's quite a large change (hint: next time perhaps try to make it smaller / more incremental if possible), I'd like someone else from the team to also review it. @daviddrysdale would you have time to do that? I will also take another closer look shortly.

daviddrysdale

Just a first pass so far, mostly focusing on the prost vendoring stuff.

114 / 136 files viewed

third_party/prost/vendor.sh

oak/server/rust/oak_runtime/build.rs

examples/trusted_information_retrieval/backend/build.rs

examples/injection/client/injection.cc

examples/chat/module/rust/build.rs

sdk/rust/oak_derive/Cargo.toml

daviddrysdale

Second pass and I'm starting to understand (and appreciate) what's going on – neat stuff!

123 / 136 files viewed

oak_utils/src/lib.rs

sdk/rust/oak/src/handle.rs

sdk/rust/oak/tests/handle_extract_inject.rs

sdk/rust/oak_derive/tests/visit.rs

daviddrysdale

136 / 136 files viewed

sdk/rust/oak/src/io/receiver.rs

examples/injection/module/rust/src/lib.rs

tiziano88 · 2020-06-25T16:58:39Z

Sorry I'm being slow on this, I am planning to also take a closer look soon.

tiziano88

I have now looked more closely at the first few files, I expect this needs to be rebased on top of #1199 anyways?

examples/chat/module/rust/src/backend.rs

examples/chat/proto/chat.proto

tiziano88 · 2020-06-29T15:49:09Z

examples/chat/proto/chat.proto

+
+message Command {
+  oneof command {
+    oak.handle.Sender join = 1 [(oak.handle.message_type) = ".oak.encap.GrpcResponse"];


Suggested change

oak.handle.Sender join = 1 [(oak.handle.message_type) = ".oak.encap.GrpcResponse"];

oak.handle.Sender join_room = 1 [(oak.handle.message_type) = ".oak.encap.GrpcResponse"];

I think some extra comments would help to explain what's going on here, especially since there are not many other examples of Inter-Node Communication, so people will probably look at this one for reference.

I have added more details here to all the protos, and split them into external gRPC and internal sections.

tiziano88 · 2020-06-29T15:55:13Z

examples/injection/module/rust/src/lib.rs

+
+    fn get_interface(&mut self) -> &BlobStoreInterface {
+        // Make sure it is cached
+        if let CachedStore::NotCached { sender, receiver } = &self.store {


Can this be moved into the match below, and avoid the unreachable?

I tried to get this to work, but I'm not sure it's possible. The problem is that when I do

self.store = CachedStore::Cached(iface);

I can't also do a return &iface, as iface has already been moved. I also can't extract a reference through self.store without doing a match somewhere, so I think this is the best we can get here. Do you have other ideas?

tiziano88 · 2020-06-29T16:01:53Z

examples/injection/proto/injection.proto

+
+package oak.examples.injection;
+
+message BlobStoreInterface {


Isn't this the same (or very similar) as what we usually call "invocation" in the code?

I think it is similar yes. But in this case the handles can be used to serve multiple requests, so I'm not sure we should call it an 'invocation'.

sdk/rust/oak/src/handle.rs

sdk/rust/oak/tests/handle_extract_inject.rs

daviddrysdale · 2020-07-07T07:14:51Z

@wildarch please could you rebase (again, sorry!) now #1225 is in?

wildarch · 2020-07-07T08:34:33Z

@wildarch please could you rebase (again, sorry!) now #1225 is in?

No worries, all done 😄

daviddrysdale

Looks like there's some (minor) comments from my second pass that you haven't responded to – is GitHub's UI confusing things again?

sdk/rust/oak_derive/Cargo.toml

daviddrysdale · 2020-07-07T15:59:36Z

examples/injection/module/rust/src/lib.rs

+//
+//! Example module showcasing handle passing inside of Protocol Buffers.
+//!
+//! The following diagram illustrates how the system stores client blobs:


Thanks, much clearer.

wildarch · 2020-07-07T16:38:42Z

Thanks David, it seems I did indeed miss a few comments. Hopefully I've now addressed all of them 😄

daviddrysdale · 2020-07-07T16:41:00Z

BUILD

@@ -1,35 +0,0 @@
-#


Is removing this file related?

This should not have been removed, how I've managed to delete this is a mystery to me. Thanks for spotting it! 😄

tiziano88 · 2020-07-07T19:28:21Z

sdk/rust/oak/src/handle.rs

+/// Order is significant: handles are injected starting at the first field, recursing
+/// into nested structs before moving on to the next field.


In order of field declaration, or of tag number? Perhaps add a test case that has fields in non-standard order (e.g. 3-1-2).

In order of field declaration, handle extraction is not tied directly to protobuf types in any way (i.e. you can derive HandleVisit for any type). I'll try and make this more clear in the comment, I'm not too sure we need such a test since the two are unrelated.

tiziano88 · 2020-07-07T19:29:19Z

sdk/rust/oak/src/handle.rs

+/// If the number of handles provided is not exactly equal to the number of handles needed to fill
+/// the message, an error is returned.
+///
+/// Order is significant: handles are injected starting at the first field, recursing


I guess this also means there is no backwards compatibility between different versions of the same message, right? i.e. adding a new field

That is correct yes.

If we want this in future it might be possible, I'm unsure if our procedural macro could read the tag annotations meant for prost, though 🤔

daviddrysdale · 2020-07-08T09:57:28Z

Not sure whether you can see CI results (#1112), but there's a few invalid doc links. Other than that, ready to merge I think!

Step #21 - "check_docs": + cargo deadlinks --dir target/doc
Step #21 - "check_docs": Found invalid urls in /workspace/sdk/target/doc/oak/handle/derive.HandleVisit.html:
Step #21 - "check_docs": 	Linked file at path /workspace/sdk/target/doc/oak/oak/handle/trait.HandleVisit.html does not exist!
Step #21 - "check_docs": Found invalid urls in /workspace/sdk/target/doc/oak/handle/index.html:
Step #21 - "check_docs": 	Linked file at path /workspace/sdk/target/doc/oak/oak/handle/trait.HandleVisit.html does not exist!
Step #21 - "check_docs": Found invalid urls in /workspace/sdk/target/doc/oak/handle/fn.extract_handles.html:
Step #21 - "check_docs": 	Linked file at path /workspace/sdk/target/doc/oak_abi/constant.INVALID_HANDLE.html does not exist!
Step #21 - "check_docs": + echo 'Found deadlinks in the generated docs.'
Step #21 - "check_docs": + exit 1
Step #21 - "check_docs": Found deadlinks in the generated docs.

wildarch · 2020-07-08T10:00:12Z

Thanks @daviddrysdale, I saw them but wasn't able to reproduce them locally. Turns out I had some outdated docs in the target/doc dir that don't get removed on new builds, but the checker still tripped over them before getting to that error 😓.

I can reproduce the errors locally now so I should be able to address them, thanks! 😄

Also includes: - Custom derive macro to visit all handles in a struct/enum (crate oak_derive) - New example "injection" to showcase passing handles around.

daviddrysdale · 2020-07-08T11:00:30Z

@tiziano88 happy to merge?

tiziano88 · 2020-07-08T11:02:47Z

@tiziano88 happy to merge?

Sure, go ahead @daviddrysdale , thanks! And thanks @wildarch for all the work!

wildarch · 2020-07-08T11:30:49Z

Yay, awesome! 😃 🎉

Thanks for all the reviews guys 😄

github-actions · 2020-07-08T12:01:08Z

Reproducibility Index:

41e274c3757a74826f327ee287295854521fb9b232c69c3a8ce424f663d06bb8  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
7cc210ce2abc06e438bf6b2fa760d8a971558a635ee4d453ce553f0336440c50  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
b1499033dc9c01664b60a81cf058b1f3796cfba19ef835619fa2dabd7cb4d21d  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
d53a8d0506c38532648fc3ce442abb35ff170aee96d81c3f53cb96a000eaddb9  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
9d3fd950c5f27f5fe2484a62088de3c31b34a7e168c0156d140924f9c7fe426e  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
55115a2a0893a01587930ca9c074abb7023a8cdd1c3864e73e55474692e7fcd0  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
c3e02ae9ce838f488601f6536aa2c7ae5069db05426da530e0a5d29164af4d9a  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
17299bcb7f0642ff6c5c3cec4366283fdc0430b2b9ccd5113762b66b3f425888  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
25ceadbdcf66e9b7a8e1a1b096e5efabef0d4b9bd8060c62549e38e6f8e231a0  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
bacd8942470cc84d379a88809f351f0d817d12a31a8608296c3743bd61ca3faa  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
ab4205f9351a85f7d201021530d5045da1f4e1682cf5c1c99caf604a4fa2c737  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
fa2598827a3b34a4cc16b31eb095c2f00862b0641c3e4e545f769a6d2fed1f68  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
f7e04fec862016c4c4dfdb816062cd5227cf8325b3340e5cdbf7d29ff7d5a714  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index f55aea2..047e0b5 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,12 +1,13 @@
-27b563913abba69ab96699ca4576a5406231800739297d43f14647dfe28182d3  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-aaa701e9c0fc58fd65e5956bf07dbc17a9fe30170d776f8c734e833ecc30718e  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-4b88fc85c633e5413002b05ff66bec9ec653397f07f6799f889aeb7af14fb6e1  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-1e8c5a8ff13e83e228fa461eb049e7eb37600b980ef17e1e42aed2b9b14c8cc1  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-400c13da142185bb700bf6daaed1de58f38a6ebc98d52a67a0c93f425a806c77  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
-9035e13788e682a01e22c1ba42c125fccbfae4df296d7bd46318fb56044e19a2  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-c5646c404e5d4f4d354f1961e39bc9ca23989922741efee90b9f270625bdcd29  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-3267614a8c860eef6449c40856cef55b04aceb95a206fe9d3cd70d2b3931d967  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-656e89a34c1d964fd003e7421d204f96fd5c3853d27b142a9eec619f6f86f96b  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-aea5aa8876b8158f0d781665e089307e9de53b59104407e71422a58ed4cff6d2  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-080c53ca4491dbd9e279725cc32abddca83d0da14e645915981455b5b9f8ecd2  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
+41e274c3757a74826f327ee287295854521fb9b232c69c3a8ce424f663d06bb8  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+7cc210ce2abc06e438bf6b2fa760d8a971558a635ee4d453ce553f0336440c50  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+b1499033dc9c01664b60a81cf058b1f3796cfba19ef835619fa2dabd7cb4d21d  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+d53a8d0506c38532648fc3ce442abb35ff170aee96d81c3f53cb96a000eaddb9  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+9d3fd950c5f27f5fe2484a62088de3c31b34a7e168c0156d140924f9c7fe426e  ./examples/target/wasm32-unknown-unknown/release/database_proxy.wasm
+55115a2a0893a01587930ca9c074abb7023a8cdd1c3864e73e55474692e7fcd0  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+c3e02ae9ce838f488601f6536aa2c7ae5069db05426da530e0a5d29164af4d9a  ./examples/target/wasm32-unknown-unknown/release/injection.wasm
+17299bcb7f0642ff6c5c3cec4366283fdc0430b2b9ccd5113762b66b3f425888  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+25ceadbdcf66e9b7a8e1a1b096e5efabef0d4b9bd8060c62549e38e6f8e231a0  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+bacd8942470cc84d379a88809f351f0d817d12a31a8608296c3743bd61ca3faa  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+ab4205f9351a85f7d201021530d5045da1f4e1682cf5c1c99caf604a4fa2c737  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+fa2598827a3b34a4cc16b31eb095c2f00862b0641c3e4e545f769a6d2fed1f68  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
 f7e04fec862016c4c4dfdb816062cd5227cf8325b3340e5cdbf7d29ff7d5a714  ./oak/server/target/x86_64-unknown-linux-musl/release/oak_loader

googlebot added the cla: yes label Jun 5, 2020

wildarch force-pushed the protobuf_handle_extract branch from ef7e606 to caa442b Compare June 12, 2020 13:57

wildarch marked this pull request as ready for review June 12, 2020 14:52

tiziano88 self-requested a review June 12, 2020 17:36

wildarch force-pushed the protobuf_handle_extract branch 2 times, most recently from aadc30e to 16a4611 Compare June 12, 2020 22:22

tiziano88 reviewed Jun 15, 2020

View reviewed changes

tiziano88 reviewed Jun 16, 2020

View reviewed changes

jul-sh changed the base branch from master to main June 16, 2020 17:21

wildarch force-pushed the protobuf_handle_extract branch 4 times, most recently from 2752257 to daccc10 Compare June 19, 2020 16:17

tiziano88 requested review from tiziano88 and daviddrysdale June 24, 2020 13:17

tiziano88 mentioned this pull request Jun 24, 2020

Clarify inter-node communication (INC) model #1192

Closed

daviddrysdale reviewed Jun 24, 2020

View reviewed changes

daviddrysdale reviewed Jun 25, 2020

View reviewed changes

wildarch mentioned this pull request Jun 26, 2020

Vendor and patch Prost #1200

Closed

6 tasks

daviddrysdale mentioned this pull request Jun 26, 2020

Move client proto generation to oak_utils #1205

Merged

1 task

tiziano88 reviewed Jun 29, 2020

View reviewed changes

wildarch requested review from daviddrysdale and tiziano88 July 3, 2020 13:59

wildarch force-pushed the protobuf_handle_extract branch 2 times, most recently from 5f89e89 to d3cb2ed Compare July 3, 2020 15:18

wildarch force-pushed the protobuf_handle_extract branch from d3cb2ed to a5c02c2 Compare July 7, 2020 08:33

wildarch force-pushed the protobuf_handle_extract branch 2 times, most recently from 3ff2cac to abcdab9 Compare July 7, 2020 13:33

daviddrysdale approved these changes Jul 7, 2020

View reviewed changes

wildarch force-pushed the protobuf_handle_extract branch 2 times, most recently from 65b125f to d3bc5e0 Compare July 7, 2020 16:37

daviddrysdale reviewed Jul 7, 2020

View reviewed changes

tiziano88 approved these changes Jul 7, 2020

View reviewed changes

wildarch force-pushed the protobuf_handle_extract branch from d3bc5e0 to e8a54ba Compare July 8, 2020 09:08

Pass handles as fields in Protobuf messages.

ef50128

Also includes: - Custom derive macro to visit all handles in a struct/enum (crate oak_derive) - New example "injection" to showcase passing handles around.

wildarch force-pushed the protobuf_handle_extract branch from e8a54ba to ef50128 Compare July 8, 2020 10:16

daviddrysdale merged commit 8d54581 into project-oak:main Jul 8, 2020

wildarch deleted the protobuf_handle_extract branch July 8, 2020 11:31

daviddrysdale mentioned this pull request Jul 8, 2020

Separate out generation of protobuf messages and gRPC services #1093

Closed

conradgrobler mentioned this pull request Jul 17, 2020

Use crate prefix when generating prost code #1263

Closed

2 tasks

tiziano88 mentioned this pull request Oct 19, 2020

unused closure in handle visit usage #1599

Closed

wildarch mentioned this pull request Jan 8, 2021

evaluate FIDL for inter-node communication #426

Closed

	oak.handle.Sender join = 1 [(oak.handle.message_type) = ".oak.encap.GrpcResponse"];
	oak.handle.Sender join_room = 1 [(oak.handle.message_type) = ".oak.encap.GrpcResponse"];

		/// Order is significant: handles are injected starting at the first field, recursing
		/// into nested structs before moving on to the next field.


		package oak.examples.injection;

		message BlobStoreInterface {

Pass handles between nodes using Protocol Buffers #1108

Pass handles between nodes using Protocol Buffers #1108

Conversation

wildarch commented Jun 5, 2020 • edited

Checklist

wildarch commented Jun 12, 2020

wildarch commented Jun 12, 2020

tiziano88 commented Jun 12, 2020

tiziano88 left a comment

Choose a reason for hiding this comment

daviddrysdale commented Jun 16, 2020

wildarch commented Jun 16, 2020

wildarch commented Jun 24, 2020

tiziano88 commented Jun 24, 2020

daviddrysdale left a comment

Choose a reason for hiding this comment

daviddrysdale left a comment

Choose a reason for hiding this comment

daviddrysdale left a comment

Choose a reason for hiding this comment

tiziano88 commented Jun 25, 2020

tiziano88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Jul 7, 2020

wildarch commented Jul 7, 2020

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wildarch commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Jul 8, 2020

wildarch commented Jul 8, 2020

daviddrysdale commented Jul 8, 2020

tiziano88 commented Jul 8, 2020

wildarch commented Jul 8, 2020

github-actions bot commented Jul 8, 2020

wildarch commented Jun 5, 2020 •

edited