Potential extension of PatchParams::validate to cover for invalid patch footgun

[jmintb]

Would you like to work on this feature?

yes

What problem are you trying to solve?

When working on #1153 it became apparent that it is possible to submit invalid patches and receive no error. See this example from the ~~PR docs~~ :

edit: the docs have been moved to a new PR.

In the example below the patch contains a PodSpec and not a complete or partial Pod. It is therefore an invalid patch as the full structure of a resource, Pod in this case, is required when patching. The invalid patch will be accepted by the K8S API, but no changes will be made to the cluster.

An example with an invalid and valid patch:

use k8s_openapi::api::core::v1::{Pod, PodSpec};
use kube::{Api, api::{PatchParams, Patch}};

# async fn wrapper() -> Result<(), Box<dyn std::error::Error>> {
# let client = kube::Client::try_default().await?;
let pods: Api<Pod> = Api::namespaced(client, "apps");
let pp = PatchParams::default();

let invalid_patch: PodSpec = serde_json::from_value(serde_json::json!({
                "activeDeadlineSeconds": 5
}))?;

// This will have no effect on mypod.
pods.patch("mypod", &pp, &Patch::Strategic(invalid_patch)).await?;

let valid_patch: Pod = serde_json::from_value(serde_json::json!({
         "spec": {
                "activeDeadlineSeconds": 5
           }
}))?;

// This will set activeDeadlineSeconds to 5.
pods.patch("mypod", &pp, &Patch::Strategic(invalid_patch)).await?;

# Ok(())
# }

Describe the solution you'd like

Extend PatchParams::validate with the necessary checks to avoid "silently" invalid patches.

PatchParams::validate_strictseems to already be able to cover for this case, so it might make sense to reuse that functionality :thinking:

This issue is meant for discussion I don't have a clear solution yet.

Describe alternatives you've considered

~~An alternative could be to inform users to that PatchParams::validate_strict can mitigate this issue. The docs for the #1153 already indclude a mention of this.~~ this it not a solution, as validation_strict is supposed to only affect server-side style patches.

~~I think having some builtin validation in PatchParams::validate is preferable so user's can avoid invalid patches without requiring extra steps or knowledge of PatchParams::validate_strict.~~

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

kube-client

[clux]

Yeah, I think having some type of validation like this would be good.

Putting it on straight on PatchParams::validate could be sensible, although it is actually run by default inside core::Request so it would have to be fool-proof. As a stop-gap it is possible for us to do an extra opt-in thing, but it's not ideal because it requires the users to find and run it.

[jmintb]

I see, I wonder if we can do something that avoids core::Request, it seems unnecessary to me to perform this extra validation for every request kube makes when we are only interested in validating a subset of requests :thinking:. I am not familiar with the code so I could be completely of base here.

I will experiment a bit and make a draft PR this should help drive the discussion and help me actually know what I am talking about haha.

[jmintb]

Now that #1153 is wrapped up I would like to look at this. For context #1262 is essentially trying to document the behavior outlined in this issue.

[jmintb]

For context on why validation_strict seems to mitigate this issue even though it should have no effect on non server-side style patches: https://github.com/kube-rs/kube/pull/1153#discussion_r1271265768