45 set up an example for d41 #46
Conversation
Otherwise it is difficult to interpret the error returned when running the CLI without a configuration file
| Build { | ||
| spec_file: String, | ||
| }, | ||
| Invoke { |
There was a problem hiding this comment.
We might not want to advertise this feature too much as that could be problematic once we have encryption (and the client might need another cert for that if we do mutual auth), or if we have a more complex dataplane. Still this is useful for debugging.
There was a problem hiding this comment.
We do have a requirement for directly addressable functions, precisely for development/troubleshooting reasons.
Anyway, this requirement can be fulfilled only partially at the moment, because you can only cast (not call) a function since that would require the dataplane to be reconfigured on the fly.
I added this (tiny) feature as a placeholder of the requirement: in the future we can expand things or suppress direct invocation (and the requirement).
There was a problem hiding this comment.
We can certainly make it work even in the more complex cases (I was not aware this was a requirement).
edgeless_cli/src/edgeless_cli.rs
Outdated
| function_id: uuid::Uuid::parse_str(&function_id)?, | ||
| }, | ||
| source: edgeless_api::function_instance::InstanceId { | ||
| node_id: uuid::uuid!("00000000-0000-0000-0000-ffff00000000"), |
There was a problem hiding this comment.
didn't you use that for "all-nodes" elsewhere? While this does not cause a direct issue here, probably you should generate some unique instance_id so any responses are routed towards that (and dropped by the dataplane).
There was a problem hiding this comment.
Yeah, actually that was for the "no workflow". I was in a rush so I did not define a special value also for the "no node" and "no function", though that would be a good idea (will do).
edgeless_cli/src/edgeless_cli.rs
Outdated
| stream_id: 0, | ||
| data: match event_type.as_str() { | ||
| "cast" => edgeless_api::invocation::EventData::Cast(payload), | ||
| _ => edgeless_api::invocation::EventData::Err, |
There was a problem hiding this comment.
Would probably use an empty-message cast as the default case. While we haven't used err yet, this sounds like it could trigger some unexpected code. An alternative would be to not send a message at all if it is sth else than cast.
edgeless_cli/src/edgeless_cli.rs
Outdated
| _ => edgeless_api::invocation::EventData::Err, | ||
| }, | ||
| }; | ||
| if let edgeless_api::invocation::EventData::Err = event.data { |
There was a problem hiding this comment.
Ok, this is better, no actual error-message is sent.
You could move that return to the match in line 239 to make things more readable.
| body: None, | ||
| headers: std::collections::HashMap::<String, String>::new(), | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Not sth. you need to change, but that seems quite verbose as only the message changes.
Might be able to partially solve this by making some contents of EdgelessHTTPResponse optional, creating a builder or just refactoring this specific instance.
| &"external_sink", | ||
| &edgeless_http::request_to_string(&edgeless_http::EdgelessHTTPRequest { | ||
| protocol: edgeless_http::EdgelessHTTPProtocol::HTTP, | ||
| host: CONFIGURATION.get().unwrap().lock().unwrap().target_host.clone(), |
There was a problem hiding this comment.
Also not something specific to this pr but we really need to find a way to make state examples look less weird.
There was a problem hiding this comment.
Agree... Anyway in this example CONFIGURATION is not even a state. I will simplify a bit by dropping the Mutex.
| "version": "0.1", | ||
| "include_code_file": "./http_read_number/http_read_number.wasm", | ||
| "output_callbacks": [ | ||
| "cb_success" |
There was a problem hiding this comment.
Minor nitpick but it is probably more readable if those have proper names.
There was a problem hiding this comment.
For cb_success you mean? For me that's a proper name ;-) since it is the callback (cb) that is invoked in case of success.
By the way: I like this callback mechanism very much. Indeed, I am not sure I see a reason to keep the "call" invocation method at all. Let's discuss this later.
There was a problem hiding this comment.
It was just that in this workflow there are multiple cb_success. Something like result might be better for the mathematical operators or parsed_value for the ingress-bridge. But as said above, this was a minor nitpick.
Cast without alias can be useful if we send instance_ids over the network (which we do with the sender, essentially this is continuation-passing). Ofc that has some lifetime assumptions but this works well in Erlang.
There was a problem hiding this comment.
OK, your names are better
In addition to the example
simple_workflow_httpnote:edgeless_cli function invokemode, which is actually unrelated to this PR 🤦handle_init()aspayload(before the hardcoded"test"string was passed)