Interoperability with kubernetes crate

[AaronFriel]

I recently published a comment and tagged @Arnavion in anguslees/kubernetes-rs/issues/9.

While working on trying to get the kubernetes and k8s-openapi crates to work together, I came up with a list of ergonomic changes I'd like to run by the author, work with to implement, or consider forking this project to experiment with these ideas. I'd really like your thoughts on the feasibility and ergonomics of these changes.

1. Use structs with default params instead of lists of arguments for constructing requests. A trait to generically create variants of these (e.g.: with watch: true, continue: true, resourceVersion: X) would also be very useful.

2. Helper methods and a common trait for accessing important primitives such as ObjectMeta, ListMeta, and their intersection which contains important values like resourceVersion and continue.

3. API cleanup, in particular try_from_parts wasn't ergonomic because there were too many response types, so a cleaned up way to parse a response as either an enum of Response(...), WatchEvent(…), Status(Status), StatusCode(StatusCode) would be nice.

4. Version merging or some better story for API handling. i.e.: if Kubernetes 1.12 adds a new field to Pods that is foo: Option<Bar>, then it is safe to merge that into the constructor for the Kubernetes 1.11 API. It looks like with this approach we would probably end up with a lot more code re-use and forward compatibility, maybe less need for the various features.

I haven't begun to work on these things, but I think I might like to.

[Arnavion]

1. Use structs with default params instead of lists of arguments for constructing requests. A trait to generically create variants of these (e.g.: with watch: true, continue: true, resourceVersion: X) would also be very useful.

2. Helper methods and a common trait for accessing important primitives such as ObjectMeta, ListMeta, and their intersection which contains important values like resourceVersion and continue.

My expectation was to have this be just a codegen crate, and to have other crates that use this one to implement those convenience methods (example), but I don't have any particular aversion to adding it to this crate's codegen either. Can you list the traits and functions you want to see?

---

3. API cleanup, in particular try_from_parts wasn't ergonomic because there were too many response types, so a cleaned up way to parse a response as either an enum of Response(...), WatchEvent(…), Status(Status), StatusCode(StatusCode) would be nice.

What do you mean? For example, Pod::create_core_v1_namespaced_pod returns CreateCoreV1NamespacedPodResponse. How would you want it to look like instead?

If you mean a single type like this

enum Response<R> {
    Response(R),
    Status(Status),
    Other(StatusCode),
}

and have it return Response<Pod>, then this loses the ability to determine which status code resulted in that response. It also still requires code unique to Pod::create_core_v1_namespaced_pod that understands how to parse the response bytes + status code combination correctly (200 / 201 / 202 -> parse JSON as Pod, ignore others) which cannot be shared with any other type.

But let's say we assume that all API in the spec follow a convention, eg all create_ API have 200, 201 and 202 return the same type, all read_ API return the same type in the 200 response, etc. That would allow this crate to have a single CreateResponse<T> { Ok(T), Created(T), Accepted(T), StatusCode(StatusCode) } and specialize it for every create_ API. But it's already not true, like with CreateApiextensionsV1beta1CustomResourceDefinitionResponse, and there are probably other examples (that particular one did get fixed in 1.9).

It also would not work for those requests which have disparate responses depending on the status code, but I didn't find an example of this so there might not be any.

---

4. Version merging or some better story for API handling. i.e.: if Kubernetes 1.12 adds a new field to Pods that is foo: Option, then it is safe to merge that into the constructor for the Kubernetes 1.11 API

It would still be a semver-major change since it would break anyone who wrote let pod = Pod { existing_fields: some_value }; Also, the spec makes breaking changes all the time, mostly around changing fields from optional to non-optional or vice-versa, so for these you have no choice but to give two different types so that the user can pick the one that corresponds to their server.

[AaronFriel]

Thanks for the really quick response! Re: 1, 2. 3, I admit, I haven't fully fleshed out what I would want for the API, and I'm hesitant to commit to particular traits before I hear back from the authors of the Kubernetes crate.

My goal is to create an easy to use generic API that lets someone do something close to:

fn foo() {
    let client = Client::new()?;
    
    client
        // automatically list all pods, call list again with continue field if necessary
        // then begin a watch and yield results as a Stream
        .list_and_watch(list_core_v1_namespaced_pod(ListPodOpts {
            // be able to construct requests with only the arguments that are necessary, more ergonomic
            namespace: "kube-system",
            ..Default::default,
            // implicitly: to meet the `list_and_watch` contract this struct would need to be able to take:
            // * a `continue` value to handle large lists
            // a `resourceVersion` and a `watch` parameter to watch for changes 
            // likewise, the return value of the request would need to be parsable by a trait that provides:
            // * a continue value if necessary
            // * a resourceVersion for beginning and resuming watches from the last seen version
            // * an associated type for its list and item variants
            // * a parsing method that can take a response body or a chunked response (watch) and return a list or item stream?
        }))
        // alternatively, have the result be just an individual pod and don't aggregate the partial results?
        // this is all just for demonstration purposes, anyhow
        .for_each(|result| match result {
            ListPart(podlist) => ...
            List(podlist) => ...
            Item(pod) => ...
        })

I think this sort of uniformity is necessary for a high quality Kubernetes library, and as you can see most of the complexity will revolve around the various ways of getting, listing, and watching resources. I don't think that sort of uniformity of return value parsing is as necessary for creating, replacing, patching, etc. That's just not going to be as feasible, or useful. Users will want to parse those responses and carefully consider return values from those requests. But for general APIs: getting, listing, watching, iterating over resources, I think we can probably find a common API surface? Or I hope we can.

Re: 4. I think that's a good point. A lot of what I'm describing would be breaking changes if implemented, and this is (obviously) your library. I do think that in concert with the kubernetes library, a lot of interesting things could be done (and a lot more safely!) if there were some really good traits, associated types, and simpler constructors for building requests.

Here is an example fork of the kubernetes crate demonstrating a resilient Observer pattern: https://github.com/AaronFriel/kubernetes-rs/blob/master/examples/client-watch.rs

This isn't as good as I'd like it to be. It depends a lot on specifying a bunch of types, a boxed closure to create variants of the options for the request, and the Observer object makes some assumptions that should be encoded as trait bounds about whether there ought to be a continue or a resource version in the response.

That said, I think the observer stream here is as good or better than the Go examples I've seen, and didn't require complicated codegen steps for the user. That demonstrates the value proposition of Rust's type system with powerful generics and concurrency primitives from the futures library. And this observer, unlike most of the other examples on the internet, won't die or stop yielding results when the Kubernetes API server hiccups, terminates a connection, or because internal loadbalancers time out stale connections or other issues that have caused CRD-dependent services to "not notice" changes.

[Arnavion]

A generic list_and_watch will have to assume all list-and-watch-able objects follow the same standard, which is the same problem I'd written earlier earlier:

But let's say we assume that all API in the spec follow a convention, eg all create_ API have 200, 201 and 202 return the same type", [...]

That is, you have to assume that all watch API in the spec return a WatchEvent for 200. Though the advantage for watch_ API is that the spec clearly marks it with "x-kubernetes-action": "watch" or "watchlist", so the codegen can reliably make that assumption even if the spec doesn't. (The codegen can't do that for create_ because create_ are marked with a generic "x-kubernetes-action": "post").

That also applies to the list_ API.

So with that assumption:

// k8s-openapi

trait Watchable {
    type Parameters: WatchParameters;

    fn watch(parameters: Self::Parameters) -> http::Request);
}

fn try_watch_response_from_parts<T: Watchable>(status_code: StatusCode, buf: &[u8]) -> Result<ParsedWatchResponse<T>, ResponseError> {
    // https://docs.rs/k8s-openapi/0.2.0/src/k8s_openapi/v1_11/api/core/v1/pod.rs.html#2505-2520
    // but also re-deserialize the `WatchEvent::object` as T
}

enum ParsedWatchResponse<T> {
    Ok { item: T, offset: usize },
    End { continue: Option<String> },
    Other(StatusCode),
}

impl Watchable for Pod {
    type Parameters = WatchCoreV1NamespacedPodParameters;

    fn watch(parameters: Self::Parameters) -> (http::Request, WatchResponse<Self>) {
        // https://docs.rs/k8s-openapi/0.2.0/src/k8s_openapi/v1_11/api/core/v1/pod.rs.html#2459-2492
    }
}

struct WatchCoreV1NamespacedPodParameters {
    continue: Option<String>,
    namespace: &str,
    // ...
}

trait WatchParameters {
    fn continue(&self) -> Option<&str>;
    fn set_continue(&mut self, String);
    // ...
}

// Downstream

impl Client {
    fn watch<T: Watchable>(parameters: <T as Watchable>::Parameters) {
        let request = <T as Watchable>::watch(parameters);
        let (status_code, response) = ...;
        // Loop sync or async, as required
        match try_watch_response_from_parts<T>(status_code, response)? {
            // ...
        }
    }
}

could work for watch_, and a similar one for list_

[AaronFriel]

Yes, and more complicated primitives (like the Go controller examples of writing informers/reflectors) can arise from those traits.

This would be sufficient to write some of the more complicated behaviors involving Kubernetes API objects in a generic way. Some nit picks about your example aside, do you think this would be a feasible first step?

[Arnavion]

A few things happened here with v0.4.0:

• The list_ family of functions no longer have a watch: bool parameter. The presence of this parameter meant the functions could return either a ListResponse or a WatchResponse. Instead, the list_ functions now always return a ListResponse. For watching, the corresponding watch_ functions should be used instead.

• The corresponding watch_ functions are not actually generated from their original definitions in the OpenAPI spec (the spec deprecates the watch functions in favor of using the list_ endpoint with watch=true). Instead they're generated from the list_ functions' spec, but with an implicit watch=true appended to the URL's query string, and the continue parameter removed. All other parameters from the list_ function are retained.

• For all operation functions, their optional parameters are now in a separate Defaultable struct. This is quite close to the code sample in Interoperability with kubernetes crate #20 (comment) . Specifically it's Pod::watch_namespaced_pod("kube-system", Default::default()) to not pass any optional parameters, and Pod::watch_namespaced_pod("kube-system", WatchNamespacedPodOptional { field_selector: "foo=bar", ..Default::default() }) to set the parameters you want.

The remaining piece is to make a trait for all List*Optional and Watch*Optional structs to handle continue and resource_version generically, and to have strongly typed WatchEvents.

[quodlibetor]

On possible major breaking API change which would make it possible for downstream crates to implement clients for the entire API would I think be to create an Exec trait in k8s-openapi that you can use internally to wrap everything up. I'm imagining something like this, but I haven't tried to prototype it or anything so hopefully I'm not missing anything too big:

trait Exec {
    type Response: SomeK8sOpenApiTrait;
    type Error: std::error::Error + SomeK8sOpenApiTrait;
    fn exec(&mut self, http::HttpRequest) -> Result<Self::Response, Self::Error>;
    // exec_watch probably can't be implemented in here without the separate `Watchable` trait
    // mentioned in this thread, but I thought I'd mention it in this strawman
    fn exec_watch(&mut self, http::HttpRequest) ->Result<impl Iterator<Item=Self::Response>, Self::Error>;
}

This would allow k8s-openapi to to implement the gruntwork around generating requests and parsing responses, and downstream client crates to just create an Exec implementation and k8s-openapi would generate something like:

impl Service for T where T: Exec {
    fn get_namespaced_pod(&mut self, namespace: &str, name: &str, opts: &Options {..}) -> Result<GetPodResponse, Self::Error> {
        let request = Pod::get_namespaced_pod(namespace, name, opts);
        Ok(Pod::parse_get_response(self.exec(request)?)?)
    }
}

Obviously this is a huge API change expansion, but the benefit is that we can get asymptotically closer to client implementations having 100% coverage of the kube API.

[Arnavion]

@quodlibetor Clients should already have 100% coverage. You can write that today without any new traits. For example, the tests do:

https://github.com/Arnavion/k8s-openapi/blob/v0.4.0/k8s-openapi-tests/src/pod.rs#L6-L12

https://github.com/Arnavion/k8s-openapi/blob/v0.4.0/k8s-openapi-tests/src/lib.rs#L239-L248

You could equivalently combine execute and get_single_value into a single method of your Client:

struct Client;

impl Client {
    fn get_single_value<F, R>(
        request: (http::Request<Vec<u8>>, F),
    ) -> Result<R, crate::Error> where
        F: fn(http::StatusCode) -> k8s_openapi::ResponseBody<R>,
        R: k8s_openapi::Response,
    {
        // Execute the http::Request
        // Pipe the http::Response into the ResponseBody
        // response_body.parse()
    }
}

so that your users could write:

let pod_list =
    client.get_single_value(
        api::Pod::list_namespaced_pod("kube-system", Default::default())?,
    )?;

[quodlibetor]

Ah, interesting, that does indeed make it possible to have a single function that does everything that I am interested in.

I believe that creating a trait that implements get_single_value and then adding a trait that implements all the existing methods around that trait would make for a slightly more obvious API for users, but I'm significantly less confident that it's worth the doubling of the API surface to make it happen than I was when I wrote my original comment.

[Arnavion]

@quodlibetor

One thing to add to the example I gave above: As of 0.4.0, it is in a client's best interests to also provide the raw &[u8] to its caller, not just the R. This is because the bindings are not complete. Eg if Pod::list_namespaced_pod fails with HTTP 403, the response is ListNamespacedPodResponse::Unauthorized which has no associated data in the enum variant. However the actual HTTP response does contain a JSON object. The client's caller will need access to the HTTP response's raw bytes to be able to deserialize it as a Status or serde_json::Value.

Another example is CustomResourceDefinition::create_custom_resource_definition in v1.8 where the CreateCustomResourceDefinitionResponse response type does not have a Created variant even though that is what the API server returns on success. So the caller needs to parse the response manually.

This is why the get_single_value function used by the tests (that I linked above) also take a separate FnMut(R, http::StatusCode &[u8]) -> Result<ValueResult> parameter.

This will likely become unnecessary once #40 is fixed.

---

As for an Exec and Service trait, there are two problems:

1. The API operation functions are associated with resources, so you have Pod::list_namespaced_pods instead of corev1::list_namespaced_pods (the latter is what a default Swagger-generated client would have). This is an intentional design choice, both in terms of discoverability and being similar to the Golang API.

   But it also means there would need to be a separate Service trait for each resource type. Eg there needs to be a PodService trait with fn list_namespaced_pods, a SecretService trait with fn list_namespaced_secrets, and so on. Apart from increasing compilation time, it'll also require the user to import the PodService trait to be able to use the client in addition to importing the Pod type itself.

2. Something like:

   impl Service for T where T: Exec {
       fn get_namespaced_pod(&mut self, namespace: &str, name: &str, opts: &Options {..}) -> Result<GetPodResponse, Self::Error> {

   has to make a choice whether the response is computed synchronously or asynchronously. (The example you gave computes it synchronously.) For full generality there would need to be ExecSync and ExecAsync traits that return Result and Future respectively, and then two Service traits like impl PodServiceSync for T where T: ExecSync and impl PodServiceAsync for T: where T: ExecAsync.

   Similarly there are at least two functions needed for "multiple-value" operations - they can return either Iterator or Stream.

   (Well, it's probably not impossible to make a monadic trait that abstracts Result / Iterator and Future / Stream such that it can work with only one Exec and one PodService trait, but it seems like unnecessary overkill.)

[Arnavion]

The remaining piece is to make a trait for all List*Optional and Watch*Optional structs to handle continue and resource_version generically, and to have strongly typed WatchEvents.

This was done in 5f9bf59, which made all list and watch operations have a single common type for their optional parameters - k8s_openapi::ListOptional and k8s_openapi::WatchOptional respectively.

So you don't need a trait to set the resource_version and continue_ fields; mutate them on the values directly, or copy the values into new ones with struct-update syntax to handle the other varying fields.

[Arnavion]

I think everything is done here, and has been released as part of v0.5.0 a few minutes ago.