You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@teaclave.apache.org by "henrysun007 (via GitHub)" <gi...@apache.org> on 2023/06/01 04:07:02 UTC
[GitHub] [incubator-teaclave] henrysun007 commented on a diff in pull request #691: Replace current rpc with grpc
henrysun007 commented on code in PR #691:
URL: https://github.com/apache/incubator-teaclave/pull/691#discussion_r1212505399
##########
cmake/tomls/Cargo.sgx_trusted_lib.toml:
##########
@@ -58,10 +58,12 @@ simple_asn1 = { git = "https://github.com/acw/simple_asn1", rev = "7db7a48
gbdt = { git = "https://github.com/apache/incubator-teaclave-crates" }
getrandom = { git = "https://github.com/apache/incubator-teaclave-crates" }
image = { git = "https://github.com/apache/incubator-teaclave-crates" }
+mio = { git = "https://github.com/GeminiCarrie/incubator-teaclave-crates", branch = "rustls_0.19.1"}
Review Comment:
Please open a PR in https://github.com/apache/incubator-teaclave-crates.
##########
rpc/README.md:
##########
@@ -4,40 +4,20 @@ permalink: /docs/codebase/rpc
# RPC
-This directory contains an RPC implementation over attested TLS connection
-written in Rust, providing trusted channels to send and handle requests.
-RPC interfaces and request/response messages can be defined in ProtoBuf and
-used for generating Rust structs and traits to implement services or client
-function to send requests.
+This directory contains TLS config over attested TLS connection, providing trusted channels to send and handle requests.
-Similar with other RPC frameworks, there are several concepts of RPC in
-Teaclave.
+Re-export `tonic` to support general gRPC framework. `tonic` is a gRPC over HTTP/2 implementation focused on high performance, interoperability, and flexibility.
Review Comment:
A link to tonic website can be given here.
##########
rpc/src/config.rs:
##########
@@ -224,3 +238,12 @@ impl SgxTrustedTlsClientConfig {
Ok(config)
}
}
+
+impl From<SgxTrustedTlsClientConfig> for ClientTlsConfig {
+ fn from(config: SgxTrustedTlsClientConfig) -> Self {
+ let mut client_config = config.client_config;
+ // Yout must set the 'h2' negotiation flag.
Review Comment:
=> `h2` negotiation flag must be set.
##########
rpc/README.md:
##########
@@ -4,40 +4,20 @@ permalink: /docs/codebase/rpc
# RPC
-This directory contains an RPC implementation over attested TLS connection
-written in Rust, providing trusted channels to send and handle requests.
-RPC interfaces and request/response messages can be defined in ProtoBuf and
-used for generating Rust structs and traits to implement services or client
-function to send requests.
+This directory contains TLS config over attested TLS connection, providing trusted channels to send and handle requests.
-Similar with other RPC frameworks, there are several concepts of RPC in
-Teaclave.
+Re-export `tonic` to support general gRPC framework. `tonic` is a gRPC over HTTP/2 implementation focused on high performance, interoperability, and flexibility.
## Channel and Client
-A channel in RPC represent a connection to the target service. Clients can use
-the channel to send requests. In Teaclave, we implement `SgxTrustedTlsChannel`,
-which can establish and attested a remote connection. For example, to connect
-the management service, you need to establish a trusted channel with the service
-first. Then, create a client of management service with the channel. At last,
-you can use this client to send requests like `InvokeTask`.
+A channel in gRPC represent a connection to the target service. Clients can use
+the channel to send requests. When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup TLS and attestation configs, so that we can establish and attested a remote connection. For example, to connect the management service, you need to establish a trusted channel with the service first. Then, create a client of management service with the channel. At last, you can use this client to send requests like `InvokeTask`.
-When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup
-TLS and attestation configs.
## Server and Service
-Server is an entity to listening a network address, processing incoming
-messages, and forwarding requests to certain service. Similar with channel in
-Teaclave, we implement `SgxTrustedTlsServer`, which can establish an attested TLS
-channel with clients.
+Server is an entity to listening a network address, processing incoming messages, and forwarding requests to certain service. Similar with the client, you can use `SgxTrustedTlsServerConfig` to setup TLS and attestation configs for chanenel with clients.
-Similar with the client, you can use `SgxTrustedTlsServerConfig` to setup TLS
-and attestation configs.
+## Interceptor
-## Protocol
-
-There are many RPC protocols that can be implemented in the RPC framework. Currently,
-there's only one simple protocol called `JsonProtocol`. Simply speaking, for
-the json protocol, one RPC message will contain a length of the following
-requests (in big endian) and a json serialized request.
+In Teaclave, We implement `CredentialService` base on the `Interceptor` trait to add credential in the MetadataMap of each request before it is sent, so servers can check the authentication credential of each request.
Review Comment:
Add more descriptions about `Interceptor`. It is `tonic` specific.
##########
services/proto/src/proto/teaclave_authentication_service.proto:
##########
@@ -85,21 +82,17 @@ message UserChangePasswordRequest {
string password = 1;
}
-message UserChangePasswordResponse {}
-
message DeleteUserRequest {
string id = 1;
}
-message DeleteUserResponse {}
-
service TeaclaveAuthenticationApi {
- rpc UserRegister(UserRegisterRequest) returns (UserRegisterResponse);
- rpc UserUpdate(UserUpdateRequest) returns (UserUpdateResponse);
+ rpc UserRegister(UserRegisterRequest) returns (google.protobuf.Empty);
Review Comment:
Can we use only `Empty` here?
##########
services/proto/src/teaclave_frontend_service.rs:
##########
@@ -15,282 +15,156 @@
// specific language governing permissions and limitations
// under the License.
-use crate::teaclave_common::{i32_from_task_status, i32_to_task_status};
use crate::teaclave_frontend_service_proto as proto;
-use crate::teaclave_management_service::TeaclaveManagementRequest;
-use crate::teaclave_management_service::TeaclaveManagementResponse;
-use anyhow::anyhow;
use anyhow::{Error, Result};
use core::convert::TryInto;
use std::collections::HashMap;
-use teaclave_rpc::into_request;
use teaclave_types::{
Executor, ExecutorType, ExternalID, FileAuthTag, FileCrypto, FunctionArgument,
FunctionArguments, FunctionBuilder, FunctionInput, FunctionOutput, OwnerList, TaskFileOwners,
- TaskResult, TaskStatus, UserID, UserList,
};
use url::Url;
-pub use proto::TeaclaveFrontend;
-pub use proto::TeaclaveFrontendClient;
-pub use proto::TeaclaveFrontendRequest;
-pub use proto::TeaclaveFrontendResponse;
-
-#[into_request(TeaclaveFrontendRequest::RegisterInputFile)]
-#[into_request(TeaclaveManagementRequest::RegisterInputFile)]
-#[derive(Debug, PartialEq)]
-pub struct RegisterInputFileRequest {
- pub url: Url,
- pub cmac: FileAuthTag,
- pub crypto_info: FileCrypto,
-}
+pub use proto::teaclave_frontend_client::TeaclaveFrontendClient;
+pub use proto::teaclave_frontend_server::TeaclaveFrontend;
Review Comment:
Can be combined.
##########
services/storage/enclave/src/error.rs:
##########
@@ -28,9 +28,15 @@ pub(crate) enum StorageServiceError {
Service(#[from] anyhow::Error),
}
-impl From<StorageServiceError> for TeaclaveServiceResponseError {
+impl From<StorageServiceError> for teaclave_rpc::Status {
fn from(error: StorageServiceError) -> Self {
log::debug!("StorageServiceError: {:?}", error);
- TeaclaveServiceResponseError::RequestError(error.to_string())
+ let msg = error.to_string();
+ let code = match error {
+ StorageServiceError::Database(_) => Code::DataLoss,
Review Comment:
Is `database` error necessarily be a `data loss`?
##########
services/proto/src/teaclave_frontend_service.rs:
##########
@@ -363,51 +238,49 @@ impl RegisterFunctionRequestBuilder {
}
// We explicitly construct Function here in case of missing any field
-impl From<RegisterFunctionRequest> for FunctionBuilder {
- fn from(request: RegisterFunctionRequest) -> Self {
- FunctionBuilder::new()
+impl std::convert::TryFrom<RegisterFunctionRequest> for FunctionBuilder {
+ type Error = Error;
+ fn try_from(request: RegisterFunctionRequest) -> Result<Self> {
+ Ok(FunctionBuilder::new()
.name(request.name)
.description(request.description)
.public(request.public)
- .executor_type(request.executor_type)
+ .executor_type(request.executor_type.try_into()?)
.payload(request.payload)
- .arguments(request.arguments)
- .inputs(request.inputs)
- .outputs(request.outputs)
+ .arguments(
+ request
Review Comment:
Use
```
let request = request .arguments
.into_iter()
.map(FunctionArgument::try_from)
.collect::<Result<_>>()?
```
Do not put all the calling as an argument.
##########
cmake/scripts/test.sh:
##########
@@ -152,7 +152,13 @@ run_functional_tests() {
./teaclave_functional_tests -t end_to_end
- # Run script tests
+ python3 -m grpc_tools.protoc \
Review Comment:
Can we abstract these commands into a function? It seems to appear in many places.
##########
rpc/README.md:
##########
@@ -4,40 +4,20 @@ permalink: /docs/codebase/rpc
# RPC
-This directory contains an RPC implementation over attested TLS connection
-written in Rust, providing trusted channels to send and handle requests.
-RPC interfaces and request/response messages can be defined in ProtoBuf and
-used for generating Rust structs and traits to implement services or client
-function to send requests.
+This directory contains TLS config over attested TLS connection, providing trusted channels to send and handle requests.
-Similar with other RPC frameworks, there are several concepts of RPC in
-Teaclave.
+Re-export `tonic` to support general gRPC framework. `tonic` is a gRPC over HTTP/2 implementation focused on high performance, interoperability, and flexibility.
## Channel and Client
-A channel in RPC represent a connection to the target service. Clients can use
-the channel to send requests. In Teaclave, we implement `SgxTrustedTlsChannel`,
-which can establish and attested a remote connection. For example, to connect
-the management service, you need to establish a trusted channel with the service
-first. Then, create a client of management service with the channel. At last,
-you can use this client to send requests like `InvokeTask`.
+A channel in gRPC represent a connection to the target service. Clients can use
+the channel to send requests. When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup TLS and attestation configs, so that we can establish and attested a remote connection. For example, to connect the management service, you need to establish a trusted channel with the service first. Then, create a client of management service with the channel. At last, you can use this client to send requests like `InvokeTask`.
Review Comment:
attested => attest
##########
rpc/README.md:
##########
@@ -4,40 +4,20 @@ permalink: /docs/codebase/rpc
# RPC
-This directory contains an RPC implementation over attested TLS connection
-written in Rust, providing trusted channels to send and handle requests.
-RPC interfaces and request/response messages can be defined in ProtoBuf and
-used for generating Rust structs and traits to implement services or client
-function to send requests.
+This directory contains TLS config over attested TLS connection, providing trusted channels to send and handle requests.
-Similar with other RPC frameworks, there are several concepts of RPC in
-Teaclave.
+Re-export `tonic` to support general gRPC framework. `tonic` is a gRPC over HTTP/2 implementation focused on high performance, interoperability, and flexibility.
## Channel and Client
-A channel in RPC represent a connection to the target service. Clients can use
-the channel to send requests. In Teaclave, we implement `SgxTrustedTlsChannel`,
-which can establish and attested a remote connection. For example, to connect
-the management service, you need to establish a trusted channel with the service
-first. Then, create a client of management service with the channel. At last,
-you can use this client to send requests like `InvokeTask`.
+A channel in gRPC represent a connection to the target service. Clients can use
+the channel to send requests. When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup TLS and attestation configs, so that we can establish and attested a remote connection. For example, to connect the management service, you need to establish a trusted channel with the service first. Then, create a client of management service with the channel. At last, you can use this client to send requests like `InvokeTask`.
-When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup
-TLS and attestation configs.
## Server and Service
-Server is an entity to listening a network address, processing incoming
-messages, and forwarding requests to certain service. Similar with channel in
-Teaclave, we implement `SgxTrustedTlsServer`, which can establish an attested TLS
-channel with clients.
+Server is an entity to listening a network address, processing incoming messages, and forwarding requests to certain service. Similar with the client, you can use `SgxTrustedTlsServerConfig` to setup TLS and attestation configs for chanenel with clients.
Review Comment:
Please seperate this one line to different ones as the left does for easy reading.
##########
rpc/src/macros.rs:
##########
@@ -15,12 +15,17 @@
// specific language governing permissions and limitations
// under the License.
-#[allow(dead_code)]
-#[cfg(feature = "mesalock_sgx")]
-pub(crate) fn get_tcs_num() -> usize {
- if sgx_trts::enclave::rsgx_is_supported_EDMM() {
- sgx_trts::enclave::SgxGlobalData::new().get_dyn_tcs_num() as usize
- } else {
- (sgx_trts::enclave::SgxGlobalData::new().get_tcs_max_num() - 1) as usize
- }
+#[macro_export]
+macro_rules! ensure {
Review Comment:
Is this necessary? Will `From<Error>` work here?
##########
services/execution/app/Cargo.toml:
##########
@@ -38,6 +39,7 @@ anyhow = { version = "1.0.26" }
libc = { version = "0.2.66" }
log = { version = "0.4.17", features = ["release_max_level_info"] }
signal-hook = { version = "0.1.13" }
+tokio = { version = "1.0", features = ["rt-multi-thread", "time", "macros"], optional = true }
Review Comment:
What is the concern about using an optional tokio?
##########
services/proto/src/teaclave_frontend_service.rs:
##########
@@ -485,1181 +365,283 @@ impl UpdateFunctionRequestBuilder {
}
// We explicitly construct Function here in case of missing any field
-impl From<UpdateFunctionRequest> for FunctionBuilder {
- fn from(request: UpdateFunctionRequest) -> Self {
- FunctionBuilder::new()
- .id(request.function_id.uuid)
+impl std::convert::TryFrom<UpdateFunctionRequest> for FunctionBuilder {
+ type Error = Error;
+ fn try_from(request: UpdateFunctionRequest) -> Result<Self> {
+ let function_id: ExternalID = request.function_id.try_into()?;
+ Ok(FunctionBuilder::new()
+ .id(function_id.uuid)
.name(request.name)
.description(request.description)
.public(request.public)
- .executor_type(request.executor_type)
+ .executor_type(request.executor_type.try_into()?)
.payload(request.payload)
- .arguments(request.arguments)
- .inputs(request.inputs)
- .outputs(request.outputs)
+ .arguments(
+ request
Review Comment:
The same as before. Use a seperate variable.
##########
services/proto/build.rs:
##########
@@ -34,54 +30,16 @@ fn main() {
let out_dir = env::var("OUT_DIR").expect("$OUT_DIR not set. Please build with cargo");
println!("cargo:rerun-if-changed=build.rs");
- println!("cargo:rerun-if-changed=proto_gen/templates/proto.j2");
- println!("cargo:rerun-if-changed=proto_gen/main.rs");
for pf in proto_files.iter() {
println!("cargo:rerun-if-changed={}", pf);
}
- let target_dir = match env::var("TEACLAVE_SYMLINKS") {
- Ok(teaclave_symlinks) => {
- Path::new(&teaclave_symlinks).join("teaclave_build/target/proto_gen")
- }
- Err(_) => env::current_dir().unwrap().join("target/proto_gen"),
- };
- let current_dir: PathBuf = match env::var("MT_SGXAPP_TOML_DIR") {
- Ok(sgxapp_toml_dir) => Path::new(&sgxapp_toml_dir).into(),
- // This fallback is only for compiling rust client sdk with cargo
- Err(_) => Path::new("../../").into(),
- };
-
- let proto_files = [
- "services/proto/src/proto/teaclave_access_control_service.proto",
- "services/proto/src/proto/teaclave_authentication_service.proto",
- "services/proto/src/proto/teaclave_common.proto",
- "services/proto/src/proto/teaclave_storage_service.proto",
- "services/proto/src/proto/teaclave_frontend_service.proto",
- "services/proto/src/proto/teaclave_management_service.proto",
- "services/proto/src/proto/teaclave_scheduler_service.proto",
- ];
- let mut c = Command::new("cargo");
- // Use CARGO_ENCODED_RUSTFLAGS to override RUSTFLAGS which makes the run fail.
- c.current_dir(¤t_dir)
- .env("CARGO_ENCODED_RUSTFLAGS", "");
- c.args([
- "run",
- "--target-dir",
- &target_dir.to_string_lossy(),
- "--manifest-path",
- "services/proto/proto_gen/Cargo.toml",
- ]);
-
- c.args(["--", "-i", "services/proto/src/proto", "-d", &out_dir, "-p"])
- .args(proto_files);
- let output = c.output().expect("Generate proto failed");
- if !output.status.success() {
- panic!(
- "stdout: {:?}, stderr: {:?}",
- str::from_utf8(&output.stderr).unwrap(),
- str::from_utf8(&output.stderr).unwrap()
- );
+ if let Err(e) = tonic_build::configure()
Review Comment:
Seems an unwrap or expect can do the same as the panic.
##########
sdk/rust/Cargo.toml:
##########
@@ -27,16 +27,17 @@ edition = "2021"
crate-type = ["lib", "cdylib", "staticlib"]
[dependencies]
-teaclave_types = { path = "../../types", features = ["app"] }
-teaclave_attestation = { path = "../../attestation" }
-teaclave_rpc = { path = "../../rpc" }
-teaclave_proto = { path = "../../services/proto" }
-anyhow = { version = "1.0.26" }
-url = { version = "2.1.1" }
-serde_json = { version = "1.0.39" }
-serde = { version = "1.0.92" }
-pem = "0.7.0"
-libc = "0.2.68"
+teaclave_types = { path = "../../types", features = ["app"] }
Review Comment:
Please sort in the alphabetical order.
##########
services/proto/src/teaclave_authentication_service.rs:
##########
@@ -15,33 +15,20 @@
// specific language governing permissions and limitations
// under the License.
-use anyhow::anyhow;
use anyhow::{Error, Result};
-use core::convert::TryInto;
-use teaclave_rpc::into_request;
use crate::teaclave_authentication_service_proto as proto;
use crate::teaclave_common;
+pub use proto::teaclave_authentication_api_client::TeaclaveAuthenticationApiClient;
+pub use proto::teaclave_authentication_api_server::TeaclaveAuthenticationApi;
Review Comment:
Can be combind to one line.
##########
services/storage/enclave/src/proxy.rs:
##########
@@ -17,41 +17,76 @@
use crate::error::StorageServiceError;
use anyhow::anyhow;
-use std::sync::mpsc::{channel, Sender};
-use teaclave_proto::teaclave_storage_service::{TeaclaveStorageRequest, TeaclaveStorageResponse};
-use teaclave_rpc::Request;
-use teaclave_types::TeaclaveServiceResponseResult;
+use teaclave_proto::teaclave_storage_service::*;
+use teaclave_rpc::{Request, Response, Status};
+use tokio::sync::mpsc::{unbounded_channel, UnboundedSender};
#[derive(Clone)]
pub(crate) struct ProxyService {
- sender: Sender<ProxyRequest>,
+ sender: UnboundedSender<ProxyRequest>,
}
impl ProxyService {
- pub(crate) fn new(sender: Sender<ProxyRequest>) -> Self {
+ pub(crate) fn new(sender: UnboundedSender<ProxyRequest>) -> Self {
Self { sender }
}
}
-impl teaclave_rpc::TeaclaveService<TeaclaveStorageRequest, TeaclaveStorageResponse>
- for ProxyService
-{
- fn handle_request(
- &self,
- request: Request<TeaclaveStorageRequest>,
- ) -> TeaclaveServiceResponseResult<TeaclaveStorageResponse> {
- let (sender, receiver) = channel();
- self.sender
- .send(ProxyRequest { sender, request })
+macro_rules! handle_request {
+ ($service: ident,$request:expr,$fun:ident,$response:ident) => {{
+ let (sender, mut receiver) = unbounded_channel();
+ $service
+ .sender
+ .send(ProxyRequest {
+ sender,
+ request: Request::new(TeaclaveStorageRequest::$fun($request)),
+ })
.map_err(|_| StorageServiceError::Service(anyhow!("send ProxyRequest error")))?;
- receiver
- .recv()
- .map_err(|_| StorageServiceError::Service(anyhow!("recv ProxyRequest error")))?
+ match receiver.recv().await {
+ Some(Ok(TeaclaveStorageResponse::$response(re))) => return Ok(Response::new(re)),
+ _ => return Err(teaclave_rpc::Status::internal("invalid response")),
+ }
+ }};
+
+ ($service: ident,$request:expr,$fun:ident) => {
+ handle_request!($service, $request, $fun, $fun)
+ };
+}
+
+#[teaclave_rpc::async_trait]
+impl TeaclaveStorage for ProxyService {
Review Comment:
The first inputs are all `self`. How about using an associated function here?
##########
services/scheduler/enclave/src/error.rs:
##########
@@ -30,9 +29,17 @@ pub enum SchedulerServiceError {
StorageError,
}
-impl From<SchedulerServiceError> for TeaclaveServiceResponseError {
+impl From<SchedulerServiceError> for Status {
fn from(error: SchedulerServiceError) -> Self {
log::debug!("SchedulerServiceError: {:?}", error);
- TeaclaveServiceResponseError::RequestError(error.to_string())
+ let msg = error.to_string();
+ let code = match error {
+ SchedulerServiceError::StorageError | SchedulerServiceError::TaskQueueEmpty => {
Review Comment:
It seems `TaskQueueEmpty` is not neccary to be `data loss`.
##########
tests/utils/src/lib.rs:
##########
@@ -155,3 +178,35 @@ where
}
}
}
+
+#[allow(clippy::print_literal)]
+pub fn async_test<F>(ncases: &mut u64, failurecases: &mut Vec<String>, f: F, name: &str)
+where
+ F: FnOnce() -> BoxFuture<'static, ()> + std::panic::UnwindSafe,
+{
+ *ncases += 1;
+
+ let t = || -> f64 {
Review Comment:
please resue the code of function `test`.
##########
services/storage/enclave/src/service.rs:
##########
@@ -174,18 +164,46 @@ impl TeaclaveStorageService {
}
}
}
+
+ fn dispatch(
+ &self,
+ request: teaclave_rpc::Request<TeaclaveStorageRequest>,
+ ) -> std::result::Result<TeaclaveStorageResponse, StorageServiceError> {
+ match request.into_inner() {
+ TeaclaveStorageRequest::Get(r) => {
+ let response = self.get(r)?;
+ Ok(response).map(TeaclaveStorageResponse::Get)
+ }
+ TeaclaveStorageRequest::Put(r) => {
Review Comment:
It seems the code can be simplified by combinding different lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@teaclave.apache.org
For additional commands, e-mail: notifications-help@teaclave.apache.org