From c196c0a74c951a3eff43fb1ea886812a50869ab1 Mon Sep 17 00:00:00 2001 From: David Morrison Date: Fri, 8 Nov 2024 10:36:15 -0800 Subject: [PATCH] chore: pull json_patch_ext into its own crate --- Cargo.lock | 49 ++++++++-- Cargo.toml | 4 +- sk-cli/Cargo.toml | 2 +- sk-core/Cargo.toml | 1 - .../src/{jsonutils/hash.rs => jsonutils.rs} | 0 sk-core/src/jsonutils/mod.rs | 11 --- sk-core/src/jsonutils/patch_ext.rs | 98 ------------------- sk-core/src/jsonutils/tests/mod.rs | 5 - sk-core/src/jsonutils/tests/patch_ext_test.rs | 54 ---------- sk-core/src/k8s/pod_lifecycle.rs | 16 --- sk-driver/Cargo.toml | 1 + sk-driver/src/mutation.rs | 16 ++- sk-driver/src/runner.rs | 36 +++---- 13 files changed, 77 insertions(+), 216 deletions(-) rename sk-core/src/{jsonutils/hash.rs => jsonutils.rs} (100%) delete mode 100644 sk-core/src/jsonutils/mod.rs delete mode 100644 sk-core/src/jsonutils/patch_ext.rs delete mode 100644 sk-core/src/jsonutils/tests/mod.rs delete mode 100644 sk-core/src/jsonutils/tests/patch_ext_test.rs diff --git a/Cargo.lock b/Cargo.lock index dd807a50..9bcc32a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1822,6 +1822,30 @@ dependencies = [ "thiserror", ] +[[package]] +name = "json-patch" +version = "3.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "863726d7afb6bc2590eeff7135d923545e5e964f004c2ccf8716c25e70a86f08" +dependencies = [ + "jsonptr", + "serde", + "serde_json", + "thiserror", +] + +[[package]] +name = "json-patch-ext" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "43b15fa8e1747529b482cda6caff763a207911b7bcb3352130f20520a3cf0087" +dependencies = [ + "json-patch 3.0.1", + "jsonptr", + "serde_json", + "thiserror", +] + [[package]] name = "jsonpath_lib" version = "0.3.0" @@ -1833,6 +1857,16 @@ dependencies = [ "serde_json", ] +[[package]] +name = "jsonptr" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5dea2b27dd239b2556ed7a25ba842fe47fd602e7fc7433c2a8d6106d4d9edd70" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "k8s-openapi" version = "0.19.0" @@ -1908,7 +1942,7 @@ dependencies = [ "chrono", "form_urlencoded", "http 0.2.12", - "json-patch", + "json-patch 1.4.0", "k8s-openapi", "once_cell", "schemars", @@ -1942,7 +1976,7 @@ dependencies = [ "derivative", "futures", "hashbrown", - "json-patch", + "json-patch 1.4.0", "k8s-openapi", "kube-client", "parking_lot", @@ -3378,12 +3412,13 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.118" +version = "1.0.132" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d947f6b3163d8857ea16c4fa0dd4840d52f3041039a85decd46867eb1abef2e4" +checksum = "d726bfaff4b320266d395898905d0eba0345aae23b54aee3a737e260fd46db03" dependencies = [ "indexmap", "itoa", + "memchr", "ryu", "serde", ] @@ -3507,7 +3542,6 @@ dependencies = [ "clockabilly", "http 0.2.12", "httpmock", - "json-patch", "k8s-openapi", "kube", "mockall", @@ -3564,7 +3598,8 @@ dependencies = [ "clockabilly", "either", "httpmock", - "json-patch", + "json-patch 1.4.0", + "json-patch-ext", "k8s-openapi", "kube", "mockall", @@ -3639,7 +3674,7 @@ dependencies = [ "derive_setters", "dirs", "insta", - "json-patch", + "json-patch-ext", "k8s-openapi", "kube", "lazy_static", diff --git a/Cargo.toml b/Cargo.toml index 2f7c5ece..c7caa4c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,7 +39,9 @@ derive_setters = "0.1.6" dirs = "5.0.1" either = "1.12.0" futures = "0.3.28" -json-patch = "1.2.0" +# remove this when we bump kube-rs +json-patch = { version = "1.0.0" } +json-patch-ext = "0.1.0" k8s-openapi = { version = "0.19.0", features = ["v1_27"] } lazy_static = "1.5.0" object_store = { version = "0.11.0", features = ["aws", "gcp", "azure", "http"] } diff --git a/sk-cli/Cargo.toml b/sk-cli/Cargo.toml index 3f983571..ad68c287 100644 --- a/sk-cli/Cargo.toml +++ b/sk-cli/Cargo.toml @@ -19,7 +19,7 @@ clap = { workspace = true } clap_complete = { workspace = true } derive_setters = { workspace = true } dirs = { workspace = true } -json-patch = { workspace = true } +json-patch-ext = { workspace = true } kube = { workspace = true } k8s-openapi = { workspace = true } lazy_static = { workspace = true } diff --git a/sk-core/Cargo.toml b/sk-core/Cargo.toml index 2be7e23e..06a75232 100644 --- a/sk-core/Cargo.toml +++ b/sk-core/Cargo.toml @@ -17,7 +17,6 @@ async-recursion = { workspace = true } async-trait = { workspace = true } bytes = { workspace = true } clockabilly = { workspace = true } -json-patch = { workspace = true } kube = { workspace = true } k8s-openapi = { workspace = true } object_store = { workspace = true } diff --git a/sk-core/src/jsonutils/hash.rs b/sk-core/src/jsonutils.rs similarity index 100% rename from sk-core/src/jsonutils/hash.rs rename to sk-core/src/jsonutils.rs diff --git a/sk-core/src/jsonutils/mod.rs b/sk-core/src/jsonutils/mod.rs deleted file mode 100644 index 2acee788..00000000 --- a/sk-core/src/jsonutils/mod.rs +++ /dev/null @@ -1,11 +0,0 @@ -mod hash; -pub mod patch_ext; - -pub use hash::{ - hash, - hash_option, -}; -pub use patch_ext::escape; - -#[cfg(test)] -mod tests; diff --git a/sk-core/src/jsonutils/patch_ext.rs b/sk-core/src/jsonutils/patch_ext.rs deleted file mode 100644 index 2f2fe8a1..00000000 --- a/sk-core/src/jsonutils/patch_ext.rs +++ /dev/null @@ -1,98 +0,0 @@ -use serde_json::Value; - -use crate::errors::*; - -// This module provides some unofficial "extensions" to the [jsonpatch](https://jsonpatch.com) -// format for describing changes to a JSON document. In particular, it adds the `*` operator as a -// valid token for arrays in a JSON document. It means: apply this change to all elements of this -// array. For example, consider the following document: -// -// ```json -// { -// "foo": { -// "bar": [ -// {"baz": 1}, -// {"baz": 2}, -// {"baz": 3}, -// ] -// } -// } -// ``` -// -// The pathspec `/foo/bar/*/baz` would reference the `baz` field of all three array entries in the -// `bar` array. It is an error to use `*` to reference a field that is not an array. Currently -// the only supported operations are `add` and `remove`. - -err_impl! {JsonPatchError, - #[error("invalid JSON pointer: {0}")] - InvalidPointer(String), - - #[error("index out of bounds at {0}")] - OutOfBounds(String), - - #[error("unexpected type at {0}")] - UnexpectedType(String), -} - -pub fn escape(path: &str) -> String { - let path = path.replace('~', "~0"); - path.replace('/', "~1") -} - -pub fn add(path: &str, key: &str, value: &Value, obj: &mut Value, overwrite: bool) -> EmptyResult { - let parts: Vec<_> = path.split('*').collect(); - for v in patch_ext_helper(&parts, obj).ok_or(JsonPatchError::invalid_pointer(path))? { - match v { - Value::Object(map) => { - if overwrite || !map.contains_key(key) { - map.insert(key.into(), value.clone()); - } - }, - Value::Array(vec) => { - if key == "-" { - vec.push(value.clone()); - } else if let Ok(idx) = key.parse::() { - ensure!(idx <= vec.len(), JsonPatchError::out_of_bounds(&format!("{path}/{key}"))); - vec.insert(idx, value.clone()); - } else { - bail!(JsonPatchError::out_of_bounds(path)); - } - }, - _ => bail!(JsonPatchError::unexpected_type(path)), - } - } - - Ok(()) -} - -pub fn remove(path: &str, key: &str, obj: &mut Value) -> EmptyResult { - let parts: Vec<_> = path.split('*').collect(); - for v in patch_ext_helper(&parts, obj).ok_or(JsonPatchError::invalid_pointer(path))? { - v.as_object_mut().ok_or(JsonPatchError::unexpected_type(path))?.remove(key); - } - - Ok(()) -} - -// Given a list of "path parts", i.e., paths split by `*`, recursively walk through all the -// possible "end" values that the path references; return a mutable reference so we can make -// modifications at those points. We assume that this function is never called with an empty -// `parts` array, which is valid in normal use since "some_string".split('*') will return -// ["some_string"]. -fn patch_ext_helper<'a>(parts: &[&str], value: &'a mut Value) -> Option> { - if parts.len() == 1 { - return Some(vec![value.pointer_mut(parts[0])?]); - } - - let mut res = vec![]; - - // If there was an array value, e.g., /foo/bar/*/baz, our path parts will look like - // /foo/bar/ and /baz; so we need to strip off the trailing '/' in our first part - let len = parts[0].len(); - let next_array_val = value.pointer_mut(&parts[0][..len - 1])?.as_array_mut()?; - for v in next_array_val { - let cons = patch_ext_helper(&parts[1..], v)?; - res.extend(cons); - } - Some(res) -} diff --git a/sk-core/src/jsonutils/tests/mod.rs b/sk-core/src/jsonutils/tests/mod.rs deleted file mode 100644 index 98b8b81e..00000000 --- a/sk-core/src/jsonutils/tests/mod.rs +++ /dev/null @@ -1,5 +0,0 @@ -mod patch_ext_test; - -use rstest::*; - -use super::*; diff --git a/sk-core/src/jsonutils/tests/patch_ext_test.rs b/sk-core/src/jsonutils/tests/patch_ext_test.rs deleted file mode 100644 index fc7daa73..00000000 --- a/sk-core/src/jsonutils/tests/patch_ext_test.rs +++ /dev/null @@ -1,54 +0,0 @@ -use serde_json::{ - json, - Value, -}; - -use super::*; - -#[fixture] -fn data() -> Value { - json!({ - "foo": [ - {"baz": {"buzz": 0}}, - {"baz": {"quzz": 1}}, - {"baz": {"fixx": 2}}, - ], - }) -} - - -#[rstest] -#[case::overwrite(true)] -#[case::no_overwrite(false)] -fn test_patch_ext_add(mut data: Value, #[case] overwrite: bool) { - let path = "/foo/*/baz"; - let res = patch_ext::add(path, "buzz", &json!(42), &mut data, overwrite); - assert!(res.is_ok()); - assert_eq!( - data, - json!({ - "foo": [ - {"baz": {"buzz": if overwrite { 42 } else { 0 } }}, - {"baz": {"quzz": 1, "buzz": 42}}, - {"baz": {"fixx": 2, "buzz": 42}}, - ], - }) - ); -} - -#[rstest] -fn test_patch_ext_remove(mut data: Value) { - let path = "/foo/*/baz"; - let res = patch_ext::remove(path, "quzz", &mut data); - assert!(res.is_ok()); - assert_eq!( - data, - json!({ - "foo": [ - {"baz": {"buzz": 0}}, - {"baz": {}}, - {"baz": {"fixx": 2}}, - ], - }) - ); -} diff --git a/sk-core/src/k8s/pod_lifecycle.rs b/sk-core/src/k8s/pod_lifecycle.rs index 15e7125f..ec527c55 100644 --- a/sk-core/src/k8s/pod_lifecycle.rs +++ b/sk-core/src/k8s/pod_lifecycle.rs @@ -6,15 +6,9 @@ use std::cmp::{ }; use clockabilly::Clockable; -use json_patch::{ - AddOperation, - PatchOperation, -}; use kube::ResourceExt; -use serde_json::Value; use super::*; -use crate::jsonutils; use crate::prelude::*; // A PodLifecycleData object is how we track the length of time a pod was running in a cluster. It @@ -147,16 +141,6 @@ impl PodLifecycleData { pub fn finished(&self) -> bool { matches!(self, PodLifecycleData::Finished(..)) } - - pub fn to_annotation_patch(&self) -> Option { - match self { - PodLifecycleData::Empty | PodLifecycleData::Running(_) => None, - PodLifecycleData::Finished(start_ts, end_ts) => Some(PatchOperation::Add(AddOperation { - path: format!("/metadata/annotations/{}", jsonutils::escape(LIFETIME_ANNOTATION_KEY)), - value: Value::String(format!("{}", end_ts - start_ts)), - })), - } - } } // We implement PartialOrd and PartialEq for PodLifecycleData; this is maybe a little bit magic, diff --git a/sk-driver/Cargo.toml b/sk-driver/Cargo.toml index d03a0215..76d674e7 100644 --- a/sk-driver/Cargo.toml +++ b/sk-driver/Cargo.toml @@ -19,6 +19,7 @@ either = { workspace = true } kube = { workspace = true } k8s-openapi = { workspace = true } json-patch = { workspace = true } +json-patch-ext = { workspace = true } rocket = { workspace = true } serde_json = { workspace = true } sk-api = { workspace = true } diff --git a/sk-driver/src/mutation.rs b/sk-driver/src/mutation.rs index 468e4b8c..795cb6a2 100644 --- a/sk-driver/src/mutation.rs +++ b/sk-driver/src/mutation.rs @@ -6,6 +6,7 @@ use json_patch::{ Patch, PatchOperation, }; +use json_patch_ext::escape; use kube::core::admission::{ AdmissionRequest, AdmissionResponse, @@ -21,6 +22,7 @@ use sk_core::jsonutils; use sk_core::k8s::{ KubeResourceExt, PodExt, + PodLifecycleData, }; use sk_core::prelude::*; @@ -101,7 +103,7 @@ fn add_simulation_labels(ctx: &DriverContext, pod: &corev1::Pod, patches: &mut V patches.push(PatchOperation::Add(AddOperation { path: "/metadata/labels".into(), value: json!({}) })); } patches.push(PatchOperation::Add(AddOperation { - path: format!("/metadata/labels/{}", jsonutils::escape(SIMULATION_LABEL_KEY)), + path: format!("/metadata/labels/{}", escape(SIMULATION_LABEL_KEY)), value: Value::String(ctx.name.clone()), })); @@ -126,7 +128,7 @@ fn add_lifecycle_annotation( let seq = mut_data.count(hash); let lifecycle = ctx.store.lookup_pod_lifecycle(&owner_ns_name, hash, seq); - if let Some(patch) = lifecycle.to_annotation_patch() { + if let Some(patch) = to_annotation_patch(&lifecycle) { info!("applying lifecycle annotations (hash={hash}, seq={seq})"); if pod.metadata.annotations.is_none() { patches.push(PatchOperation::Add(AddOperation { @@ -170,3 +172,13 @@ fn into_pod_review(resp: AdmissionResponse) -> AdmissionReview { response: Some(resp), } } + +fn to_annotation_patch(pld: &PodLifecycleData) -> Option { + match pld { + PodLifecycleData::Empty | PodLifecycleData::Running(_) => None, + PodLifecycleData::Finished(start_ts, end_ts) => Some(PatchOperation::Add(AddOperation { + path: format!("/metadata/annotations/{}", escape(LIFETIME_ANNOTATION_KEY)), + value: Value::String(format!("{}", end_ts - start_ts)), + })), + } +} diff --git a/sk-driver/src/runner.rs b/sk-driver/src/runner.rs index 7c4cb447..f707b274 100644 --- a/sk-driver/src/runner.rs +++ b/sk-driver/src/runner.rs @@ -10,6 +10,13 @@ use clockabilly::{ UtcClock, }; use either::Either; +use json_patch_ext::{ + add_operation, + escape, + format_ptr, + patch_ext, + remove_operation, +}; use kube::api::{ DeleteParams, DynamicObject, @@ -20,7 +27,6 @@ use kube::api::{ use kube::ResourceExt; use serde_json::json; use sk_core::errors::*; -use sk_core::jsonutils; use sk_core::k8s::{ add_common_metadata, build_global_object_meta, @@ -71,22 +77,14 @@ pub fn build_virtual_obj( klabel_insert!(vobj, VIRTUAL_LABEL_KEY => "true"); if let Some(pod_spec_template_path) = maybe_pod_spec_template_path { - jsonutils::patch_ext::add(pod_spec_template_path, "metadata", &json!({}), &mut vobj.data, false)?; - jsonutils::patch_ext::add( - &format!("{}/metadata", pod_spec_template_path), - "annotations", - &json!({}), - &mut vobj.data, - false, - )?; - jsonutils::patch_ext::add( - &format!("{}/metadata/annotations", pod_spec_template_path), - ORIG_NAMESPACE_ANNOTATION_KEY, - &json!(original_ns), + patch_ext( &mut vobj.data, - true, + add_operation( + format_ptr!("{pod_spec_template_path}/metadata/annotations/{}", escape(ORIG_NAMESPACE_ANNOTATION_KEY)), + json!(original_ns), + ), )?; - jsonutils::patch_ext::remove("", "status", &mut vobj.data)?; + patch_ext(&mut vobj.data, remove_operation(format_ptr!("/status")))?; // We remove all container ports from the pod specification just before applying, because it is // _possible_ to create a pod with duplicate container ports, but the apiserver will _reject_ a @@ -94,13 +92,11 @@ pub fn build_virtual_obj( // reason to expose the ports. We do this here because we still want the ports to be a part of // the podspec when we're computing its hash, i.e., changes to the container ports will still // result in changes to the pod in the trace/simulation - jsonutils::patch_ext::remove( - &format!("{}/spec/containers/*", pod_spec_template_path), - "ports", - &mut vobj.data, - )?; + patch_ext(&mut vobj.data, remove_operation(format_ptr!("{pod_spec_template_path}/spec/containers/*/ports")))?; } + println!("{vobj:?}"); + Ok(vobj) }