-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Rust Loader for running examples #993
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
roughtime: oak::roughtime::Roughtime::new("roughtime-client"), | ||
misconfigured_roughtime: oak::roughtime::Roughtime::new("roughtime-misconfig"), | ||
} | ||
} | ||
} | ||
|
||
#[no_mangle] | ||
pub extern "C" fn frontend_oak_main(in_handle: u64) { | ||
pub extern "C" fn frontend_oak_main(_: u64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the name of the parameter, just add an underscore as prefix.
"tokio 0.2.20 (registry+https://github.com/rust-lang/crates.io-index)", | ||
"tonic 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", | ||
"tonic 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this downgrade intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were different versions of tonic
in Cargo.lock
examples/abitest/client/abitest.cc
Outdated
std::string private_key_path = absl::GetFlag(FLAGS_private_key); | ||
std::string cert_chain_path = absl::GetFlag(FLAGS_cert_chain); | ||
if (private_key_path.empty()) { | ||
OAK_LOG(FATAL) << "No private key file specified."; | ||
} | ||
if (cert_chain_path.empty()) { | ||
OAK_LOG(FATAL) << "No certificate chain file specified."; | ||
} | ||
std::string private_key = oak::utils::read_file(private_key_path); | ||
std::string cert_chain = oak::utils::read_file(cert_chain_path); | ||
|
||
grpc::SslServerCredentialsOptions::PemKeyCertPair key_cert_pair = {private_key, cert_chain}; | ||
grpc::SslServerCredentialsOptions options; | ||
options.pem_root_certs = ca_cert; | ||
options.pem_key_cert_pairs.push_back(key_cert_pair); | ||
return grpc::SslServerCredentials(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why this was not needed before and now is needed. Is it because the Rust gRPC server pseudo-node requires TLS? Is there no way to make it work with insecure credentials? I don't know if it's still blocking you, but if it is, I think it would make sense to have support for insecure to make the transition easier, and then add TLS later on. Apologies if we already talked about it, I don't remember the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this is more because the Rust gRPC client pseudo-Node now always uses TLS, so the test server that it connects to (which runs in parallel inside the abitest
client program) now needs to act as TLS server side?
// Retrieve intersection. | ||
std::vector<std::string> intersection_0 = RetrieveIntersection(stub_0.get()); | ||
LOG(INFO) << "client 0 intersection:"; | ||
for (auto item : intersection_0) { | ||
LOG(INFO) << "- " << item; | ||
} | ||
assert(std::set<std::string>(intersection_0.begin(), intersection_0.end()) == expected_set); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to #1006
let private_key = tokio::fs::read(&opt.tls_private_key).await?; | ||
let certificate = tokio::fs::read(&opt.tls_certificate).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, so much parallelism :) (well, actually they are serialized anyways)
|
||
readonly APPLICATION="${PWD}/bazel-client-bin/examples/aggregator/config/config.bin" | ||
exec ./bazel-bin/oak/server/loader/oak_runner \ | ||
exec ./bazel-clang-bin/oak/server/rust/oak_loader/oak_loader \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why are we not using the cargo version yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably my fault – I switched to a Bazel-build rust_binary
target in 210911e (back when we were planning to have a Bazel-built Rust + C++ cross-linked binary). We could/should revert that commit.
grpc::SslServerCredentialsOptions::PemKeyCertPair key_cert_pair = {private_key, cert_chain}; | ||
grpc::SslServerCredentialsOptions options; | ||
options.pem_root_certs = ca_cert; | ||
options.pem_key_cert_pairs.push_back(key_cert_pair); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Aside: if we weren't about to delete it, I'd suggest commonizing this code with the equivalent code in oak_runner_main.cc
. But we are so I won't.)
examples/abitest/client/abitest.cc
Outdated
std::string private_key_path = absl::GetFlag(FLAGS_private_key); | ||
std::string cert_chain_path = absl::GetFlag(FLAGS_cert_chain); | ||
if (private_key_path.empty()) { | ||
OAK_LOG(FATAL) << "No private key file specified."; | ||
} | ||
if (cert_chain_path.empty()) { | ||
OAK_LOG(FATAL) << "No certificate chain file specified."; | ||
} | ||
std::string private_key = oak::utils::read_file(private_key_path); | ||
std::string cert_chain = oak::utils::read_file(cert_chain_path); | ||
|
||
grpc::SslServerCredentialsOptions::PemKeyCertPair key_cert_pair = {private_key, cert_chain}; | ||
grpc::SslServerCredentialsOptions options; | ||
options.pem_root_certs = ca_cert; | ||
options.pem_key_cert_pairs.push_back(key_cert_pair); | ||
return grpc::SslServerCredentials(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this is more because the Rust gRPC client pseudo-Node now always uses TLS, so the test server that it connects to (which runs in parallel inside the abitest
client program) now needs to act as TLS server side?
|
||
let identity = Identity::from_pem(certificate, private_key); | ||
|
||
let address = "[::1]:8888".parse()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same as localhost:8888
? If so, can we use localhost
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a IPv6 synonym for localhost
.
Changed to [::]:8888
that is a synonym to 0.0.0.0
.
Using IPv6 so we won't need to change it everywhere if we switch to IPv6 (since it's backwards compatible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -26,17 +26,27 @@ node_configs { | |||
address: "test.invalid:9999" | |||
} | |||
} | |||
node_configs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going away soon anyways.
Add gRPC client Add gRPC client Update comments Move client to a new grpc directory Move codec to a separate file Format Update comments Format Refactor Change after review Update Node macro Run running_average Update scripts Update examples Run examples with the Rust Oak Loader Delete default from rustfmt Decode GrpcResponse Use Rust Loader in aggregator Remove io Remove additional rust configs Update scripts Reenable tests; Add Node stubs Fix gRPC client connection Work on abitest fix Temporary comment gRPC abitest Update Aggregator scripts Update Aggregator scripts Format Change after review Fix backend wasm Nodes Format Fix non-Ok gRPC responses Disable gRPC stop via notification Fix gRPC abitest Fix example tests Use cargo build Fix build_docker Change after review Fix server termination Fix running_average
Reproducibility index:
|
Changes required by commit 68fbbe3 ("Use Rust Loader for running examples (#993)"): - Add and use a config that doesn't include a translator node. - Add C++ code that is equivalent to oak::grpc::server::init(). Change required by commit 64d3f90 ("Use bytes to represent gRPC message body (#833)"): - Re-work hand-coded response message for current GrpcResponse proto message definition. For #1009
This change:
Fixes #725
Fixes #874
Fixes #901
Ref #945
Checklist
cover any TODOs and/or unfinished work.