Helper struct for patch_status with Patch::Apply

[Kritzefitz]

I was trying to modify the status of a managed CR somewhat like this:

let status_patch = json!({
  "status" new_status
});

cr_api.patch_status(
  name,
  &PatchParams::Apply("controller.example.com"),
  &Patch::Apply(status_patch),
).await?;

However, this fails with the following error:

ApiError: invalid object type: /, Kind=: BadRequest (ErrorResponse { status: "Failure", message: "invalid object type: /, Kind=", reason: "BadRequest", code: 400 })

This was a surprise to me, because this example on the website, does basically the same. AIUI, the only relevant difference in my scenario is that I'm using Patch::Apply where the example is using Patch::Merge.

The root of the problem seems to be, that the api server expects apiVersion and kind to be present in the input for server side apply patches, whereas it does not require these fields for strategic merges.

This seems annoying to me, because using server side apply for status update generally seems like a good idea (please correct me if I'm wrong here). But currently there seems to be no easy solution to construct an appropriate payload for that patch. Using the proper CustomResource type produces a payload that works, but depending on how its spec is defined, it might be impossible to construct a payload that updates only the status.

To make it easier to construct appropriate payloads, I think a helper struct like this would be useful:

    #[derive(Debug, Clone)]
    pub struct ResourceStatus<'a, R>
    where
        R: Resource + HasStatus,
    {
        dyn_type: &'a R::DynamicType,
        metadata: ObjectMeta,
        status: Option<R::Status>,
    }

    impl<'a, R> ResourceStatus<'a, R>
    where
        R: Resource + HasStatus,
    {
        pub fn from_object(
            object: &R,
            status: Option<R::Status>,
            dyn_type: &'a R::DynamicType,
        ) -> Self {
            let metadata = ObjectMeta {
                namespace: object.meta().namespace.clone(),
                name: object.meta().name.clone(),
                generate_name: object.meta().generate_name.clone(),
                ..Default::default()
            };
            Self {
                metadata,
                status,
                dyn_type,
            }
        }
    }

    impl<'a, R> Serialize for ResourceStatus<'a, R>
    where
        R: Resource + HasStatus,
        R::Status: Serialize,
    {
        fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
        where
            S: serde::Serializer,
        {
            let mut ser_struct = serializer.serialize_struct("ResourceStatus", 4)?;
            ser_struct.serialize_field("apiVersion", &R::api_version(self.dyn_type))?;
            ser_struct.serialize_field("kind", &R::kind(self.dyn_type))?;
            ser_struct.serialize_field("metadata", &self.metadata)?;
            ser_struct.serialize_field("status", &self.status)?;
            ser_struct.end()
        }
    }

This mimics the serialization of the actual resource, but only serializes the status subresource and enough metadata to identify the exact object being updated.

I've only talked about updating status above, but I think this concept could also be applied to updates to other subresources, though I'm not familiar enough with the semantics of other resources to hash out the details.

What do you think? Would this be a useful addition to kube?

[clux]

Hey, thanks for this. This seems accurate to me, and I am also a bit annoyed at the state of things w.r.t. apply implicits and things we need to forcibly option out.

From a first glance, I think your idea here might be a good one for client extensions #1032
(might be able to remove cloning of metadata props by taking a ref to ObjectMeta also).

You could possibly hide some of this inside of a module as well by having a Resource type that also implements HasStatus be able to convert to ResourceStatus, then have the signature of a Client::apply_status include an impl Into<ResourceStatus> as the argument for the thing to serialize.

[Kritzefitz]

I'm sure we could get rid of metadata cloning by referencing the existing ObjectMeta or individual properties of it. Though we have to be careful not to include too much metadata in patch requests. Apparently the ObjectMeta returned by the API contains fields that it won't accept back in a patch (e.g. .metadata.managedFields must be absent from a patch). That's why I chose to clone only certain fields of the metadata. Though a wrapper around a &ObjectMeta that only serializes certain fields or a helper struct that only references certain fields of an ObjectMeta would work just as well and avoid the cloning.

I'm not sure if simply converting from a Resource should be the standard way to construct a ResourceStatus. Sure, if you have already retrieved an API object, modifying its status in-place and then converting the modified object into a ResourceStatus for the update is simple and easy, but it only does the correct thing if your controller controls all the status field of your object. In other cases, where different fields of the status might be controlled by different controllers, reusing the status object retrieved from the API will cause your controller to spuriously take ownership of status fields you weren't actually interested in.

I guess to effectively cooperate with server side apply, you'd want to construct the desired ResourceStatus from scratch with only the fields your controller actively cares about. That's why my proposal for from_object above doesn't re-use the status from the object but requires you to pass an explicit status: Option<R::Status> instead.

If possible, I would also like to give ResourceStatus struct less exposure, by not requiring users to construct and pass it around explicitly. But if we want to allow users to pass in a new status unrelated to the existing status, I think doing so would require the new patch_status call to accept the existing ObjectMeta (or the whole object containing it) and the new status as separate parameters.