Details

Reviewers

• jon
varun
marcin
tomek

Commits

rCOMM51f80d6bdde0: [services] Tunnelbroker - Adding of gRPC `SessionSignature` implementation

Summary

This diff introduces the implementation of SessionSignature gRPC handler using the Rust Tonic gRPC server and by calling the sessionSignatureHandler C++ handler wrapper function. The new implementation is a wrapper of the current SessionSignature C++ handler.

The server/tools.rs file was created with the create_tonic_status function, which returns a Tonic status. The reason to create this function is that we can't cast the tonic::Code enum based on the number that received from the C++ sessionSignatureHandler function. Also, we can't use tonic::Code as a ffi/cxx return type, because the cxx build fails with an unsupported type error. I think that's because of the impl for the enum which C++/cxx doesn't support.

The grpcResult struct is created in the ffi section to pass gRPC errors from C++ handler to the Rust gRPC handler and return the error code and text by the gRPC server.

Linear task: ENG-2058

Test Plan

1. Building:
The service was successfully built with the CI process or can be built manually using the cargo build command in services/tunnelbroker directory.

2. gRPC testings:
Run the service using the cargo run command. Open the gRPC client and call the SessionSignature request with the valid deviceID.
For example:

{
  "deviceID": "mobile:foS4IBADcn3uUNF8DEoH2jj6b8hujMsKUtNAKhJID4U50oalGWyg9rdLpgJwlmRy"
}

The expected result should be the toSign generated string from the server:

{
  "toSign": "7u1iiQNBlrsn0qy9BhlX8BJxttS6cBNnuCtJR1N9Mp8up56Qc4BEZWc9OvJYjlou"
}

Diff Detail

Repository

rCOMM Comm

Branch

add-grpc-session-signature-handler

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Oct 26 2022, 9:37 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 26 2022, 9:37 AM

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

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

Harbormaster completed remote builds in B13051: Diff 17921.Oct 26 2022, 9:48 AM

• max mentioned this in D5486: [services] Tunnelbroker - Move ffi from `main.rs` to the `cxx_bridge.rs` file.Oct 26 2022, 10:02 AM

• max published this revision for review.Oct 26 2022, 10:14 AM

• 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 edited the summary of this revision. (Show Details)Oct 26 2022, 10:18 AM

• max added inline comments.Oct 26 2022, 10:21 AM

services/tunnelbroker/src/server/tools.rs
3 ↗	(On Diff #17921)	The reason to create this function is that we can't cast the `tonic::Code` enum based on the number that received from the C++ `sessionSignatureHandler` function. Also, we can't use `tonic::Code` as a ffi/cxx return type, because the cxx build fails with an unsupported type error. I think that's because of the impl for the enum which C++/cxx doesn't support. That's why we match the Code enum and return the `tonic::Status` by this function.

• jon added inline comments.Oct 26 2022, 1:49 PM

services/tunnelbroker/src/server/tools.rs
3 ↗	(On Diff #17921)	could we do something like an `AsRef<Status> for tonic::Code` implementation? then we we might get marshalling for free. Similar to how a `String` has a `AsRef<str>` implementation.

• max added a child revision: D5490: [services] Tunnelbroker - Adding of gRPC `NewSession` wrapper implementation.Oct 27 2022, 6:15 AM

Fixing gRPC struct from grpcResult to idiomatic GrpcResult name.

Harbormaster completed remote builds in B13075: Diff 17952.Oct 27 2022, 8:10 AM

Move cxx_bridge to the use super::cxx_bridge::ffi for shorter calls instead of the full path.

Harbormaster completed remote builds in B13168: Diff 18072.Nov 4 2022, 6:30 AM

• max planned changes to this revision.Nov 4 2022, 6:53 AM

• max edited parent revisions, added: D5477: [services] Tunnelbroker - Adding Tonic gRPC server with empty handlers; removed: D5486: [services] Tunnelbroker - Move ffi from `main.rs` to the `cxx_bridge.rs` file.Nov 7 2022, 10:18 AM

• max mentioned this in rCOMM89f260e671ae: [services] Tunnelbroker - Move ffi from `main.rs` to the `cxx_bridge.rs` file.Nov 7 2022, 10:31 AM

• max removed a parent revision: D5477: [services] Tunnelbroker - Adding Tonic gRPC server with empty handlers.Nov 8 2022, 6:04 AM

Rebasing on master changes.

• max planned changes to this revision.Nov 8 2022, 6:16 AM

Harbormaster completed remote builds in B13255: Diff 18187.Nov 8 2022, 6:31 AM

• max requested review of this revision.Nov 16 2022, 4:32 AM

• max marked an inline comment as done.

• max added inline comments.

services/tunnelbroker/src/server/tools.rs
3 ↗	(On Diff #17921)	could we do something like an `AsRef<Status> for tonic::Code` implementation? then we we might get marshalling for free. Similar to how a `String` has a `AsRef<str>` implementation. I've tried this approach but seems it doesn't work (( Thanks @jon !

• max added a reviewer: tomek.Nov 16 2022, 4:32 AM

tomek requested changes to this revision.Nov 16 2022, 9:54 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
45 ↗	(On Diff #18187)	I mentioned it somewhere that we shouldn't use magic numbers. Ideally we would have an enum, but if that's not possible, we can at least have some named constants.
services/tunnelbroker/src/server/tools.rs
3 ↗	(On Diff #17921)	It would be a lot more efficient to share some details about what was the issue with the proposed approach, e.g. a type error returned by Rust. It might be possible that the initial idea might be enhanced based on this.

This revision now requires changes to proceed.Nov 16 2022, 9:54 AM

• max planned changes to this revision.Nov 17 2022, 5:23 AM

• max marked an inline comment as done.

Changes to use enum for the gRPC status codes.

• max marked 2 inline comments as done.Nov 17 2022, 7:44 AM

• max added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
45 ↗	(On Diff #18187)	I mentioned it somewhere that we shouldn't use magic numbers. Ideally we would have an enum, but if that's not possible, we can at least have some named constants. It's better to go with the enum. I've updated to use an enum with the gRPC codes in Rust and in C++ way through the cxx bridge. It looks better now.
services/tunnelbroker/src/server/tools.rs
3 ↗	(On Diff #17921)	It would be a lot more efficient to share some details about what was the issue with the proposed approach, e.g. a type error returned by Rust. It might be possible that the initial idea might be enhanced based on this. To use `AsRef` we should have this trait implemented, but we can't pass the `Code` into the CXX bridge because the CXX doesn't support enums and structs with the `impl` in them. We can do the `impl` for the `AsRef` for the `GRPCStatusCodes` enum because CXX and C++ don't support it. We can only use a "clean" enum or struct without any `impl` or `derives`. Unfortunately, it's hard to marshal it in an "easy way".