-
Notifications
You must be signed in to change notification settings - Fork 5
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
Support unverified third party APIs in controllers #180
Conversation
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
src/reconciler/exec/io.rs
Outdated
|
||
verus! { | ||
|
||
pub enum Receiver<T: View> { |
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.
Could you add more comments on what should be T
here? Same for Response
|
||
verus! { | ||
|
||
pub trait ExternalLibrary<InputType: View, OutputType: View> { |
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 think this is used when the controller does not need to communicate with any external systems? Could we have some doc (comments) here?
@@ -19,4 +19,16 @@ pub enum ZookeeperReconcileStep { | |||
Error, | |||
} | |||
|
|||
pub enum Error { |
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.
Do we use these errors anywhere?
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
fn handle(&self, e: WatchedEvent) {} | ||
} | ||
|
||
pub fn empty_func(zk: String) -> Result<(), Error> { |
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 might miss sth, but what is this empty_func used for?
} | ||
} | ||
|
||
// pub open spec fn |
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.
nit: let's remove this line
src/reconciler/exec/io.rs
Outdated
// The response type should be consistent with the Request type. | ||
// T: the output type of the third-party library of the reconciler which should be defined by the developer. | ||
// A safe and common way is to have an enum type which has the corresponding types of the input type in the Request. | ||
// Anway, the process method in the ExternalLibrary, the input type in Request, output type in Response and the handling |
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.
Anyway?
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.
What should it be? I mean, although I introduce concretely what T
is for, it is up to the developers to define it together with the Lib, reconcile_core process.
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.
oh I was just saying there is a typo
// Similarly, the OutputType composes the Response<?> type of a reconciler. | ||
// Note that we can encapsulate all the required libraries here, so each reconciler only has one ExternalLibrary type. | ||
pub trait ExternalLibrary<InputType: View, OutputType: View> { | ||
fn process(input: InputType) -> Option<OutputType>; |
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.
Sometimes the third party APIs are async functions. Since process is not async, we probably should check whether Verus supports async block (in external functions) so that we can call async functions in process
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.
What does that mean? I think we have already used some asyn blocks.
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.
You mean the one in run_controller
let reconcile = |cr: Arc<K>, ctx: Arc<Data>| async move {
return reconcile_with::<K, ResourceWrapperType, ReconcilerType, ReconcileStateType>(
&ReconcilerType::default(), cr, ctx
).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.
It is an async closure but not an async block, tho they are very similar
@@ -1,4 +1,4 @@ | |||
// Copyright 2022 VMware, Inc. | |||
// SPDX-License-Identifier: MIT | |||
pub mod proof; | |||
// pub mod proof; |
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.
Do you want to fix the proof lemmas in this PR or later?
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.
Later.
Signed-off-by: Wenjie Ma <[email protected]>
Signed-off-by: Wenjie Ma <[email protected]>
src/reconciler/exec/io.rs
Outdated
} | ||
|
||
pub open spec fn opt_request_to_view<T: View>(receiver: &Option<Request<T>>) -> Option<RequestView<T::V>> { | ||
match receiver { |
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.
Can you change receiver to request here?
@euclidgame I also revised the description a bit. Please take a look to ensure it is still correct. |
Signed-off-by: Wenjie Ma <[email protected]>
This PR allows developers to call any (unverified) third-party APIs besides the Kubernetes client APIs when implementing the controller.
It adds
Lib
as a generic type toReconciler
.Lib
also takes two generic typesLibInputType
andLibOutputType
.Lib
wraps all interactions with the third-party APIs, and it implements the traitExternalLibrary<LibInputType, LibOutputType>
.ExternalLibrary
has a methodprocess
which calls the third-party API in a synchronous way: it takes the input fromreconcile_core
(i.e.,LibInputType
) and returns an output (Option<LibOutputType>
).During every reconcile loop, the reconciler will return an
Option<Request>
(instead of the originalOption<KubeAPIRequest>
) to theshim_layer
.Request
is an enum with two variants for the two types of effects issued by the controller: invoking Kubernetes client APIs or third-party APIs. The shim layer will do pattern matching to theRequest
accordingly, which will either invoke the Kubernetes client API as before, or invoke the third-party APIs by calling the developer-providedprocess
.Correspondingly, the input to the reconciler,
Response
, is also an enum that encodes the two types of responses from the Kubernetes client APIs or third-party APIs.