Rust gRPC server pseudo-node #772

ipetr0v · 2020-03-26T13:08:51Z

This change adds a Rust gRPC server pseudo-node.

Fixes #751

TLS support is tracked by #806
Unit tests are tracked by #807

Checklist

Pull request includes prototype/experimental work that is under
construction.

ipetr0v · 2020-03-28T19:32:38Z

oak/server/rust/oak_runtime/src/runtime/mod.rs

@@ -630,6 +631,7 @@ impl RuntimeRef {
        entrypoint: &str,
        label: &oak_abi::label::Label,
        reader: Handle,
+        writer: Handle,


If I add a writer handle as a parameter to this function, it means that I will need to pass some writer here:

oak/oak/server/rust/oak_runtime/src/node/wasm.rs

Lines 198 to 211 in 589b2d2

let channel_ref = self.readers.get(&initial_handle).ok_or(()).map_err(|_| {

error!("node_create: Invalid handle");

OakStatus::ErrBadHandle

})?;

self.runtime

.node_create(

self.node_id,

&config_name,

&entrypoint,

// TODO(#630): Let caller provide this label via the Wasm ABI.

&oak_abi::label::Label::public_trusted(),

channel_ref.clone(),

)

And IIUC we don't have None handles, right?
cc @tiziano88 @daviddrysdale @blaxill

I have added writer as a parameter to the node_create:

oak/oak/server/rust/oak_runtime/src/runtime/mod.rs

Lines 773 to 780 in c2b2ff4

pub fn node_create(

self: Arc<Self>,

node_id: NodeId,

module_name: &str,

entrypoint: &str,

label: &Label,

reader: Handle,

) -> Result<(), OakStatus> {

But this function is called inside a Wasm node, when oak_abi funciton node_create is called (and it doesn't provide a writer).

Is it ok to search for this writer in the list of channels and pass to the node_create in the Wasm node, or it may affect the Control Flow?

I think perhaps we should have a pre-requisite to this PR to make gRPC node creation more similar to the creation of other pseudo-nodes, i.e. let the wasm node create them. This is what we do for logging, for instance.

I already done this in this PR:
https://github.com/project-oak/oak/pull/772/files#diff-ce2a63846754333df981075c4b07ec96R93-R101
https://github.com/project-oak/oak/pull/772/files#diff-6c581d36a36131023ea8060843bb2dffR126-R129

e.g.:

oak/sdk/rust/oak/src/logger/mod.rs

Lines 80 to 99 in 006b921

/// Initialize Node-wide logging via a channel to a logging pseudo-Node.

///

/// Initialize logging at the given level, creating a logging pseudo-Node

/// whose configuration is identified by the given name.

///

/// # Errors

///

/// An error is returned if a logger has already been set.

pub fn init(level: Level, config: &str) -> Result<(), SetLoggerError> {

// Create a channel and pass the read half to a fresh logging Node.

let (write_handle, read_handle) = crate::channel_create().expect("could not create channel");

crate::node_create(config, "oak_main", read_handle).expect("could not create node");

crate::channel_close(read_handle.handle).expect("could not close channel");

log::set_boxed_logger(Box::new(OakChannelLogger {

channel: crate::io::Sender::new(write_handle),

}))?;

log::set_max_level(level.to_level_filter());

Ok(())

}

this is what creates a log pseudo-node, and we also have a default config name for it. We could have the same for gRPC.

So pseudo-nodes should be created by the Oak Node, right?
But Runtime also creates a pseudo-node (and passes it a reader):
https://github.com/project-oak/oak/pull/772/files#diff-6c581d36a36131023ea8060843bb2dffR126

Ah, makes sense, but then I think we should also flip the direction of the channel, so that the gRPC server pseudo-node would have an incoming read channel, like every other node, over which the caller passes a single write end of a channel to receive invocations. Would that work?

Yes, I think it will work.
So before starting the server, gRPC pseudo-node should wait on the channel to receive the writer.

I think the server may as well start as soon as possible, but then at the first incoming gRPC request it would block (wait_on_channels) to receive the channel to which to write the invocation, store it somewhere in its local state, and then use that channel for all subsequent invocations.

Done
Created an init function:
https://github.com/project-oak/oak/pull/772/files#diff-1211ded35ba967a10825322cdf0cbd20R362-R378

It returns a read handle to read gRPC invocations from. This handle should be used by the Runtime, but I haven't found a loop/server inside the Runtime that reads invocations from a channel and dispatches them (runs corresponding gRPC method).

ipetr0v · 2020-03-28T19:33:35Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+
+impl HttpStatusCode for OakStatus {
+    fn status_code(&self) -> http::StatusCode {
+        match self {


WDYH about HTTP status codes that we need to return based on the ChannelReadStatus and OakStatus ?
cc @tiziano88 @daviddrysdale

oak/server/rust/oak_runtime/src/node/grpc_server.rs

tiziano88 · 2020-03-29T00:11:07Z

It may be worth hooking up some grpc tests (possibly the existing c++ ones) before proceeding too much with this.

ipetr0v · 2020-03-29T15:15:02Z

Do you mean tests from abitest (will require finishing #749 first)?
Because there are no unit tests for the C++ of gRPC server.

tiziano88 · 2020-03-29T16:37:22Z

It's fine to merge something that does not work yet, but yes, we should be in a better position to test everything once the rust oak_loader is in place

tiziano88

Thanks Ivan, this looks like in the correct direction, I left a few comments around, happy to take a closer look again later on!

tiziano88 · 2020-03-31T18:26:50Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+    writer: Handle,
+}
+
+impl GrpcService {


Why could this not be implemented on GrpcServerNode above? May as well use the same struct, if possible?

Created node thread uses Service inside it for a Hyper server.
And GrpcServerNode contains a thread reference.
So it would require creating a custom (non-derive) clone function that doesn't copy this thread reference and sets it to None.
Not sure if it will be readable.

WDYT ?

Makes sense, but perhaps at least create the GrpcService instance as part of GrpcServerNode itself, rather than repeat all the fields?

Also I am confused by the naming, why is one called service and the other server? I think we should stick to one of them, and I think server would be appropriate.

oak/server/rust/oak_runtime/src/node/grpc_server.rs

tiziano88 · 2020-03-31T18:35:13Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+        );
+
+        // Create a notification message and attach the method-invocation specific channels to it.
+        let notification = crate::Message {


This should probably be called invocation, also we should have a dedicated type for it somewhere at some point (not in this PR).

Done.
This class exists but it's not a message:

oak/sdk/rust/oak/src/grpc/invocation.rs

Lines 19 to 22 in 158ff84

pub struct Invocation {

pub request_receiver: crate::io::Receiver<crate::proto::grpc_encap::GrpcRequest>,

pub response_sender: crate::io::Sender<crate::proto::grpc_encap::GrpcResponse>,

}

oak/server/rust/oak_runtime/src/node/grpc_server.rs

tiziano88 · 2020-03-31T18:40:39Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+use protobuf::well_known_types::Any;
+
+use oak_abi::{ChannelReadStatus, OakStatus};
+use oak::grpc::GrpcRequest;


This is adding a dependency on the Rust / Wasm Oak SDK, which I am not sure is appropriate. I guess it's fine because we just need the protobuf generated files, but it feels we should try to split them out if we need to keep using them here perhaps. In fact GrpcRequest should probably move to oak_abi anyways?

Agree, but currently it's in the oak/src/proto - so it's a generated Rust Protobuf file.
Is is ok to move it to oak_abi?

Perhaps move that in a separate PR, either before or after this?

Created an issue #799

oak/server/rust/oak_runtime/src/node/grpc_server.rs

Add call function Fix service creation gRPC pseudo-node creation gRPC request processing Clean use Update after review Fix after review Update comments Add config parsing

oak/server/rust/oak_runtime/src/node/mod.rs

oak/server/rust/oak_runtime/src/node/grpc_server.rs

conradgrobler

I think it looks good so far, but does not seem quite ready for merging. Specifically the Hyper server coode looks as I would have expected.

oak/server/rust/oak_runtime/src/node/grpc_server.rs

ipetr0v · 2020-04-03T14:56:24Z

@conradgrobler I was planning to add TLS and tests in separate PRs - not to clutter this one.

conradgrobler · 2020-04-06T08:47:42Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+
+/// Oak Node implementation for the gRPC server.
+impl super::Node for GrpcServerNode {
+    fn start(&mut self) -> Result<(), OakStatus> {


The gRPC seems to be combining two different Hyper concepts into one: The server, which is listening for new requests and the service, which handles each request. I think this is why there are so many clones statements needed.

This logic was merged into the Node based on the following thread: #772 (comment)
But even if we will have a separate service object, we'll still need to clone it, since it should live inside a Future.

As part of the cleanup in #813, it might be worth looking at using a Tower service (implemented as a gRPC service node), which could encapsulate the needed state so that it does not have to be cloned so many times. It would also reduce the need for so many nested moves and async moves.

@conradgrobler 's comment suggests to me that @ipetr0v 's initial approach was perhaps more sensible than what I suggested in that thread? I was not aware of the distinction of those concepts in Hyper. The split should be determined on exactly what is needed inside the future and what not. For instance, I am guessing config_name and address are not relevant inside the future?

They are necessary in the Node thread, but not in the Future, yes.
But Future requires a RuntimeProxy reference.

conradgrobler · 2020-04-06T08:53:29Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+                });
+
+                // Start the HTTP server.
+                let result = block_on(hyper::Server::bind(&server.address).serve(service));


I think this will block as long as the server is listening on the port, which means that start will never return.

Also, I think this should not be using block_on, which uses a threadpool executor, but should rather be using Tokio executors as Hyper is based on Tokio. (https://docs.rs/tokio/0.2.16/tokio/runtime/struct.Runtime.html)

This is happening inside a newly created thread: https://github.com/project-oak/oak/pull/772/files/327276097e2c3593e513d92efa4f8ce713ab6a0a#diff-475373f331232762d978c36eb8027376R288-R290
Same is happening in Logger node:

oak/oak/server/rust/oak_runtime/src/node/logger.rs

Lines 61 to 65 in 006b921

let thread_handle = thread::Builder::new()

.name(self.to_string())

.spawn(move || {

let pretty_name = pretty_name_for_thread(&thread::current());

let result = logger(&pretty_name, &runtime, reader);

Where logger is an infinite loop.

Done.
Added a tokio runtime for executing a Future.
https://github.com/project-oak/oak/pull/772/files#diff-475373f331232762d978c36eb8027376R313-R317

conradgrobler · 2020-04-06T13:38:59Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+
+/// Oak Node implementation for the gRPC server.
+impl super::Node for GrpcServerNode {
+    fn start(&mut self) -> Result<(), OakStatus> {


As part of the cleanup in #813, it might be worth looking at using a Tower service (implemented as a gRPC service node), which could encapsulate the needed state so that it does not have to be cloned so many times. It would also reduce the need for so many nested moves and async moves.

oak/server/rust/oak_runtime/src/node/grpc_server.rs

tiziano88 · 2020-04-06T13:40:28Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+                                    "gRPC pseudo-node should be specified with a single `writer` \
+                            handle, found {}",


Did rustfmt split the string in this odd way? :)

No, I was trying to fit into 100 characters.
And the rustfmt changed alignment.
Done

oak/server/rust/oak_runtime/src/node/grpc_server.rs

tiziano88 · 2020-04-06T13:58:40Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

+
+/// Oak Node implementation for the gRPC server.
+impl super::Node for GrpcServerNode {
+    fn start(&mut self) -> Result<(), OakStatus> {


@conradgrobler 's comment suggests to me that @ipetr0v 's initial approach was perhaps more sensible than what I suggested in that thread? I was not aware of the distinction of those concepts in Hyper. The split should be determined on exactly what is needed inside the future and what not. For instance, I am guessing config_name and address are not relevant inside the future?

sdk/rust/oak/src/grpc/mod.rs

tiziano88 · 2020-04-06T14:01:09Z

sdk/rust/oak/src/grpc/mod.rs

+    init(DEFAULT_CONFIG_NAME).unwrap();
+}
+
+/// Initialize a gRPC pseudo-node and pass it a handle to write invocations to.


I thought this method returns a handle on which to read invocations?

Yes, changed description to Returns a ['Handle'] to read invocations from

oak/server/rust/oak_runtime/src/node/mod.rs

tiziano88

It seems there is still room for improvement, and I am not sure some things actually work at all (e.g. the Any conversion), but I guess you can submit as it is, and things should be clearer once we have some tests to exercise the new logic.

sdk/rust/oak/src/grpc/mod.rs

Auto-generated from commit 41b2777 ("Rust gRPC server pseudo-node (#772)").

Protocol buffer messages that are exchanged in serialized form between Wasm Nodes and Oak-provided pseudo-Nodes are effectively part of the Oak ABI, so move the generated code for them into the oak_abi crate. This allows us to remove the oak_runtime->oak dependency that was introduced by #772. Along the way, put the gRPC encapsulation protobuf messages into an `encap` submodule/namespace. Fixes #764.

Commit 41b2777 ("Rust gRPC server pseudo-node (#772)") changed the gRPC server pseudo-Node to be created explicitly by the Application rather than being implicitly created by the Runtime. Update the docs to reflect that.

ipetr0v added the WIP Work in progress label Mar 26, 2020

googlebot added the cla: yes label Mar 26, 2020

ipetr0v force-pushed the grpc_pseudo_node branch from 7f70a7e to fe1ab1b Compare March 28, 2020 04:38

ipetr0v commented Mar 28, 2020

View reviewed changes

oak/server/rust/oak_runtime/src/node/grpc_server.rs Show resolved Hide resolved

ipetr0v requested a review from tiziano88 March 31, 2020 19:30

tiziano88 reviewed Mar 31, 2020

View reviewed changes

ipetr0v force-pushed the grpc_pseudo_node branch from e05b00b to 9b93306 Compare March 31, 2020 19:46

ipetr0v mentioned this pull request Apr 1, 2020

Move GrpcRequest to oak_abi #799

Closed

tiziano88 mentioned this pull request Apr 1, 2020

Move node thread creation to Runtime #802

Closed

6 tasks

ipetr0v force-pushed the grpc_pseudo_node branch from 6fdb112 to 1ea14fd Compare April 2, 2020 12:32

Initial commit

fd17b2d

Add call function Fix service creation gRPC pseudo-node creation gRPC request processing Clean use Update after review Fix after review Update comments Add config parsing

ipetr0v force-pushed the grpc_pseudo_node branch from 1ea14fd to fd17b2d Compare April 2, 2020 12:35

ipetr0v added 3 commits April 2, 2020 14:01

Update internal structures

c6af3d4

Delete unnecessary crates

21f364c

Update comments

bd65c9e

ipetr0v marked this pull request as ready for review April 2, 2020 13:20

ipetr0v requested review from tiziano88, daviddrysdale, conradgrobler and blaxill April 2, 2020 13:22

ipetr0v removed the WIP Work in progress label Apr 2, 2020

ipetr0v changed the title ~~gRPC pseudo-node~~ Rust gRPC server pseudo-node Apr 2, 2020

Change address variable

9a249e6

tiziano88 reviewed Apr 2, 2020

View reviewed changes

oak/server/rust/oak_runtime/src/node/mod.rs Outdated Show resolved Hide resolved

oak/server/rust/oak_runtime/src/node/mod.rs Outdated Show resolved Hide resolved

oak/server/rust/oak_runtime/src/node/grpc_server.rs Outdated Show resolved Hide resolved

conradgrobler reviewed Apr 2, 2020

View reviewed changes

oak/server/rust/oak_runtime/src/node/grpc_server.rs Outdated Show resolved Hide resolved

oak/server/rust/oak_runtime/src/node/grpc_server.rs Outdated Show resolved Hide resolved

ipetr0v added 2 commits April 3, 2020 15:50

Changes after review

e26119c

Format

c5a846a

ipetr0v requested a review from conradgrobler April 3, 2020 14:51

ipetr0v added 2 commits April 3, 2020 16:00

Update comments

79d30d5

Pass writer to gRPC node on initialization

acb3d35

ipetr0v requested a review from tiziano88 April 5, 2020 13:52

Format

3272760

conradgrobler reviewed Apr 6, 2020

View reviewed changes

Use Tokio runtime

fa621d3

conradgrobler approved these changes Apr 6, 2020

View reviewed changes

tiziano88 approved these changes Apr 6, 2020

View reviewed changes

Change after review

40a7918

tiziano88 reviewed Apr 6, 2020

View reviewed changes

oak/server/rust/oak_runtime/src/node/mod.rs Outdated Show resolved Hide resolved

tiziano88 approved these changes Apr 6, 2020

View reviewed changes

sdk/rust/oak/src/grpc/mod.rs Show resolved Hide resolved

ipetr0v mentioned this pull request Apr 7, 2020

Rust Runtime features for running Oak applications #818

Closed

15 tasks

ipetr0v added 2 commits April 7, 2020 12:52

Change after review

9f488f9

Change after clippy

02a34f8

ipetr0v merged commit 41b2777 into project-oak:master Apr 7, 2020

ipetr0v deleted the grpc_pseudo_node branch April 7, 2020 12:40

tiziano88 added a commit that referenced this pull request Apr 7, 2020

Update gh-pages from 41b2777

6be028d

Auto-generated from commit 41b2777 ("Rust gRPC server pseudo-node (#772)").

This was referenced Apr 8, 2020

Switch from rust_protobuf to prost for protobuf generation #668

Closed

Move generated protos into oak_abi #834

Merged

	let channel_ref = self.readers.get(&initial_handle).ok_or(()).map_err(\|_\| {
	error!("node_create: Invalid handle");
	OakStatus::ErrBadHandle
	})?;

	self.runtime
	.node_create(
	self.node_id,
	&config_name,
	&entrypoint,
	// TODO(#630): Let caller provide this label via the Wasm ABI.
	&oak_abi::label::Label::public_trusted(),
	channel_ref.clone(),
	)

	pub fn node_create(
	self: Arc<Self>,
	node_id: NodeId,
	module_name: &str,
	entrypoint: &str,
	label: &Label,
	reader: Handle,
	) -> Result<(), OakStatus> {

	/// Initialize Node-wide logging via a channel to a logging pseudo-Node.
	///
	/// Initialize logging at the given level, creating a logging pseudo-Node
	/// whose configuration is identified by the given name.
	///
	/// # Errors
	///
	/// An error is returned if a logger has already been set.
	pub fn init(level: Level, config: &str) -> Result<(), SetLoggerError> {
	// Create a channel and pass the read half to a fresh logging Node.
	let (write_handle, read_handle) = crate::channel_create().expect("could not create channel");
	crate::node_create(config, "oak_main", read_handle).expect("could not create node");
	crate::channel_close(read_handle.handle).expect("could not close channel");

	log::set_boxed_logger(Box::new(OakChannelLogger {
	channel: crate::io::Sender::new(write_handle),
	}))?;
	log::set_max_level(level.to_level_filter());
	Ok(())
	}

	pub struct Invocation {
	pub request_receiver: crate::io::Receiver<crate::proto::grpc_encap::GrpcRequest>,
	pub response_sender: crate::io::Sender<crate::proto::grpc_encap::GrpcResponse>,
	}

	let thread_handle = thread::Builder::new()
	.name(self.to_string())
	.spawn(move \|\| {
	let pretty_name = pretty_name_for_thread(&thread::current());
	let result = logger(&pretty_name, &runtime, reader);

		"gRPC pseudo-node should be specified with a single `writer` \
		handle, found {}",

Rust gRPC server pseudo-node #772

Rust gRPC server pseudo-node #772

Conversation

ipetr0v commented Mar 26, 2020 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v Apr 3, 2020 • edited Loading

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

tiziano88 commented Mar 29, 2020

ipetr0v commented Mar 29, 2020 • edited Loading

tiziano88 commented Mar 29, 2020

tiziano88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v Apr 1, 2020 • edited Loading

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

Choose a reason for hiding this comment

conradgrobler left a comment

Choose a reason for hiding this comment

ipetr0v commented Apr 3, 2020

Choose a reason for hiding this comment

ipetr0v Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v Apr 6, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 left a comment

Choose a reason for hiding this comment

ipetr0v commented Mar 26, 2020 •

edited

Loading

ipetr0v Apr 3, 2020 •

edited

Loading

ipetr0v commented Mar 29, 2020 •

edited

Loading

ipetr0v Apr 1, 2020 •

edited

Loading

ipetr0v Apr 6, 2020 •

edited

Loading

ipetr0v Apr 6, 2020 •

edited

Loading