[services] Tunnelbroker - Adding Tonic gRPC server with empty handlers
ClosedPublic
Actions

Authored by • max on Oct 25 2022, 10:32 AM.

Details

Reviewers

• jon
varun
marcin
tomek

Commits

rCOMM907d9033cce5: [services] Tunnelbroker - Adding Tonic gRPC server with empty handlers

Summary

This diff introduces adding of the gRPC server empty core implementation using Tonic.

The server module with the empty API handlers is added (handlers return a "Not implemented yet" error).
Tonic forces us to implement all of the existing handlers in the imported proto file, that's why the deprecated API empty handlers are implemented with the error code "Deprecated". Otherwise, the cargo build will fail.
These old API handlers will be removed after removing the old API from a proto file in ENG-1334.

The new API handlers implementations will be added as a follow-up diffs as a handler struct implementation.

Linear task: ENG-2094

Test Plan

1. Manually testing:
In the services/tunnelbroker directory run cargo run command. This command will build and run the server.
The gRPC server will listen to the default port. Requesting the gRPC API using the gRPC client will result in a "Not implemented yet" or "Deprecated" returns.

2. CI testing:
Passing the CI builds.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Oct 25 2022, 10:32 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 25 2022, 10:32 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D5360: [services] Tunnelbroker - Add protobuf file compilation to Cargo `build.rs`.Oct 25 2022, 10:34 AM

Harbormaster completed remote builds in B13033: Diff 17902.Oct 25 2022, 10:45 AM

• max published this revision for review.Oct 25 2022, 4:39 PM

• max retitled this revision from [services] Tunnelbroker - Add Tonic gRPC server with empty handlers to [services] Tunnelbroker - Adding Tonic gRPC server with empty handlers.

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: varun, marcin.

• max mentioned this in D5436: [services] Tunnelbroker - Creating Cargo config file for Tunnelbroker as a Rust app.Oct 26 2022, 2:48 AM

• max added a child revision: D5486: [services] Tunnelbroker - Move ffi from `main.rs` to the `cxx_bridge.rs` file.Oct 26 2022, 9:36 AM

otherwise LGTM

services/tunnelbroker/src/main.rs
14–16 ↗	(On Diff #17902)	Should logically be the same
services/tunnelbroker/src/server/mod.rs
93–95 ↗	(On Diff #17902)	should also be the same

This revision is now accepted and ready to land.Oct 26 2022, 1:45 PM

• max added a reviewer: tomek.Oct 27 2022, 2:31 AM

This revision now requires review to proceed.Oct 27 2022, 2:31 AM

tomek accepted this revision.Oct 27 2022, 5:20 AM

tomek added inline comments.

services/tunnelbroker/src/server/mod.rs
32 ↗	(On Diff #17902)	Is this repetition intentional?
32–36 ↗	(On Diff #17902)	Why do we need to `Pin` the result?
85 ↗	(On Diff #17902)	Not sure, but maybe we should use `anyhow`?
86 ↗	(On Diff #17902)	Do we have a const with the port number?

This revision is now accepted and ready to land.Oct 27 2022, 5:20 AM

• max mentioned this in rCOMM3fb5533c5d5d: [services] Tunnelbroker - Creating Cargo config file for Tunnelbroker as a Rust….Nov 7 2022, 6:11 AM

• max removed a child revision: D5486: [services] Tunnelbroker - Move ffi from `main.rs` to the `cxx_bridge.rs` file.Nov 7 2022, 10:18 AM

• max added a child revision: D5487: [services] Tunnelbroker - Adding of gRPC `SessionSignature` implementation.

• max removed a parent revision: D5360: [services] Tunnelbroker - Add protobuf file compilation to Cargo `build.rs`.

Passing the error from the run_grpc_server(), adding the constant for the gRPC listening port.

services/tunnelbroker/src/main.rs
14–16 ↗	(On Diff #17902)	Should logically be the same Thanks, @jon!
services/tunnelbroker/src/server/mod.rs
32 ↗	(On Diff #17902)	Is this repetition intentional? This is an auto-generated trait from the proto file. We have `MessagesStream` and the auto-generated stream for it is `MessagesStreamStream`.
32–36 ↗	(On Diff #17902)	Why do we need to `Pin` the result? That's a Tonic requirement to use Box stream: Wrap the stream in a Box, pinning it.
85 ↗	(On Diff #17902)	Not sure, but maybe we should use `anyhow`? Makes sense to be flexible here and go with anyhow instead. I've made changes.
86 ↗	(On Diff #17902)	Do we have a const with the port number? No (( I've updated the diff and put the const with the port into the constants.