feat: fix issues discovered by validation

sk-cli/Cargo.toml

@@ -13,6 +13,7 @@ testutils = []
 
 [dependencies]
 anyhow = { workspace = true }
+bytes = { workspace = true }
 chrono = { workspace = true }
 clockabilly = { workspace = true }
 clap = { workspace = true }
@@ -25,6 +26,7 @@ k8s-openapi = { workspace = true }
 lazy_static = { workspace = true }
 ratatui = { workspace = true }
 reqwest = { workspace = true }
+rmp-serde = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
 serde_yaml = { workspace = true }

sk-cli/src/validation/annotated_trace.rs

@@ -1,9 +1,7 @@
-use std::collections::{
-    btree_map,
-    BTreeMap,
-};
+use std::collections::BTreeMap;
 use std::slice;
 
+use json_patch_ext::prelude::*;
 use sk_core::external_storage::{
     ObjectStoreWrapper,
     SkObjectStore,
@@ -32,15 +30,34 @@ impl AnnotatedTraceEvent {
 
         AnnotatedTraceEvent { data, annotations }
     }
+
+    pub fn clear_annotations(&mut self) {
+        self.annotations = vec![vec![]; self.data.len()];
+    }
+}
+
+pub enum PatchLocations {
+    #[allow(dead_code)]
+    Everywhere,
+    AffectedObjects(ValidatorCode),
+    #[allow(dead_code)]
+    ObjectReference(TypeMeta, String),
+}
+
+pub struct AnnotatedTracePatch {
+    pub locations: PatchLocations,
+    pub op: PatchOperation,
 }
 
+type AnnotationSummary = BTreeMap<ValidatorCode, usize>;
+
 #[derive(Default)]
 pub struct AnnotatedTrace {
-    #[allow(dead_code)]
-    base: TraceStore,
     path: String,
+    base: TraceStore,
+    patches: Vec<AnnotatedTracePatch>,
+
     events: Vec<AnnotatedTraceEvent>,
-    summary: BTreeMap<ValidatorCode, usize>,
 }
 
 impl AnnotatedTrace {
@@ -57,24 +74,74 @@ impl AnnotatedTrace {
         })
     }
 
-    pub fn validate(&mut self, validators: &mut BTreeMap<ValidatorCode, Validator>) {
+    pub fn apply_patch(&mut self, patch: AnnotatedTracePatch) -> anyhow::Result<usize> {
+        let mut count = 0;
+        for event in self.events.iter_mut() {
+            for (i, obj) in event
+                .data
+                .applied_objs
+                .iter_mut()
+                .chain(event.data.deleted_objs.iter_mut())
+                .enumerate()
+            {
+                let should_apply_here = match patch.locations {
+                    PatchLocations::Everywhere => true,
+                    PatchLocations::AffectedObjects(code) => event.annotations[i].contains(&code),
+                    PatchLocations::ObjectReference(ref type_, ref ns_name) => {
+                        obj.types.as_ref().is_some_and(|t| t == type_) && &obj.namespaced_name() == ns_name
+                    },
+                };
+
+                if should_apply_here {
+                    count += 1;
+                    patch_ext(&mut obj.data, patch.op.clone())?;
+                }
+            }
+        }
+        self.patches.push(patch);
+
+        Ok(count)
+    }
+
+    pub fn export(&self) -> anyhow::Result<Vec<u8>> {
+        let trace = self
+            .base
+            .clone_with_events(self.events.iter().map(|a_event| a_event.data.clone()).collect());
+        trace.export_all()
+    }
+
+    pub fn validate(&mut self, validators: &BTreeMap<ValidatorCode, Validator>) -> AnnotationSummary {
+        let mut summary = BTreeMap::new();
         for event in self.events.iter_mut() {
-            for (code, validator) in validators.iter_mut() {
+            event.clear_annotations();
+            for (code, validator) in validators.iter() {
                 let affected_indices = validator.check_next_event(event);
                 let count = affected_indices.len();
-                self.summary.entry(*code).and_modify(|e| *e += count).or_insert(count);
+                summary.entry(*code).and_modify(|e| *e += count).or_insert(count);
 
                 for i in affected_indices {
                     event.annotations[i].push(*code);
                 }
             }
         }
+        summary
     }
 
     pub fn get_event(&self, idx: usize) -> Option<&AnnotatedTraceEvent> {
         self.events.get(idx)
     }
 
+    pub fn get_next_error(&self) -> Option<ValidatorCode> {
+        for event in &self.events {
+            for annotation in &event.annotations {
+                if let Some(code) = annotation.first() {
+                    return Some(*code);
+                }
+            }
+        }
+        None
+    }
+
     pub fn get_object(&self, event_idx: usize, obj_idx: usize) -> Option<&DynamicObject> {
         let event = self.events.get(event_idx)?;
         let applied_len = event.data.applied_objs.len();
@@ -107,19 +174,11 @@ impl AnnotatedTrace {
     pub fn start_ts(&self) -> Option<i64> {
         self.events.first().map(|evt| evt.data.ts)
     }
-
-    pub fn summary_iter(&self) -> btree_map::Iter<'_, ValidatorCode, usize> {
-        self.summary.iter()
-    }
 }
 
 #[cfg(test)]
 impl AnnotatedTrace {
     pub fn new_with_events(events: Vec<AnnotatedTraceEvent>) -> AnnotatedTrace {
         AnnotatedTrace { events, ..Default::default() }
     }
-
-    pub fn summary_for(&self, code: &ValidatorCode) -> Option<usize> {
-        self.summary.get(code).cloned()
-    }
 }

sk-cli/src/validation/mod.rs

@@ -1,20 +1,28 @@
 mod annotated_trace;
 mod status_field_populated;
+mod summary;
 mod validation_store;
 mod validator;
 
+use bytes::Bytes;
 use clap::{
     value_parser,
     Subcommand,
     ValueEnum,
 };
+use sk_core::external_storage::{
+    ObjectStoreWrapper,
+    SkObjectStore,
+};
 use sk_core::prelude::*;
 
 pub use self::annotated_trace::{
     AnnotatedTrace,
     AnnotatedTraceEvent,
+    AnnotatedTracePatch,
+    PatchLocations,
 };
-pub use self::validation_store::ValidationStore;
+pub use self::validation_store::VALIDATORS;
 pub use self::validator::{
     ValidatorCode,
     ValidatorType,
@@ -36,6 +44,17 @@ pub enum ValidateSubcommand {
 pub struct CheckArgs {
     #[arg(long_help = "location of the input trace file")]
     pub trace_path: String,
+
+    #[arg(long, long_help = "fix all discovered issues")]
+    pub fix: bool,
+
+    #[arg(
+        short,
+        long,
+        long_help = "output path for modified trace (REQUIRED if --fix is set)",
+        required_if_eq("fix", "true")
+    )]
+    pub output: Option<String>,
 }
 
 #[derive(clap::Args)]
@@ -65,26 +84,25 @@ pub struct PrintArgs {
 }
 
 pub async fn cmd(subcommand: &ValidateSubcommand) -> EmptyResult {
-    let mut validators = ValidationStore::default();
     match subcommand {
         ValidateSubcommand::Check(args) => {
             let mut trace = AnnotatedTrace::new(&args.trace_path).await?;
-            validators.validate_trace(&mut trace);
-            print_summary(&trace, &validators)?;
+            let summary = VALIDATORS.validate_trace(&mut trace, args.fix)?;
+            if let Some(output_path) = &args.output {
+                let trace_data = trace.export()?;
+                let object_store = SkObjectStore::new(output_path)?;
+                object_store.put(Bytes::from(trace_data)).await?;
+            }
+            println!("{summary}");
         },
-        ValidateSubcommand::Explain(args) => validators.explain(&args.code)?,
-        ValidateSubcommand::Print(args) => validators.print(&args.format)?,
+        ValidateSubcommand::Explain(args) => VALIDATORS.explain(&args.code)?,
+        ValidateSubcommand::Print(args) => VALIDATORS.print(&args.format)?,
     }
     Ok(())
 }
 
-fn print_summary(trace: &AnnotatedTrace, validators: &ValidationStore) -> EmptyResult {
-    for (code, count) in trace.summary_iter() {
-        let name = validators.lookup(code)?.name;
-        println!("{name} ({code}): {count:.>30}");
-    }
-    Ok(())
-}
+#[cfg(test)]
+pub use self::validation_store::ValidationStore;
 
 #[cfg(test)]
 pub mod tests;

sk-cli/src/validation/status_field_populated.rs

@@ -1,3 +1,10 @@
+use std::sync::{
+    Arc,
+    RwLock,
+};
+
+use json_patch_ext::prelude::*;
+
 use super::annotated_trace::AnnotatedTraceEvent;
 use super::validator::{
     Diagnostic,
@@ -35,6 +42,10 @@ impl Diagnostic for StatusFieldPopulated {
             .collect()
     }
 
+    fn fixes(&self) -> Vec<PatchOperation> {
+        vec![remove_operation(format_ptr!("/status"))]
+    }
+
     fn reset(&mut self) {}
 }
 
@@ -43,6 +54,6 @@ pub(super) fn validator() -> Validator {
         type_: ValidatorType::Warning,
         name: "status_field_populated",
         help: HELP,
-        diagnostic: Box::new(StatusFieldPopulated::default()),
+        diagnostic: Arc::new(RwLock::new(StatusFieldPopulated::default())),
     }
 }

sk-cli/src/validation/summary.rs

@@ -0,0 +1,37 @@
+use std::collections::BTreeMap; // BTreeMap sorts by key, HashMap doesn't
+use std::fmt;
+
+use super::validator::ValidatorCode;
+use super::VALIDATORS;
+
+const WIDTH: usize = 70;
+
+#[derive(Default)]
+pub struct ValidationSummary {
+    pub annotations: BTreeMap<ValidatorCode, usize>,
+    pub patches: usize,
+}
+
+impl fmt::Display for ValidationSummary {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        writeln!(f, "\nValidation errors found:")?;
+        writeln!(f, "{}", "-".repeat(WIDTH))?;
+        for (code, count) in self.annotations.iter() {
+            if *count == 0 {
+                continue;
+            }
+            let name = VALIDATORS.lookup(code).map(|v| v.name).unwrap_or("<unknown>");
+            let left = format!("{name} ({code})");
+            let right = format!("{count}");
+            let mid_width = WIDTH.saturating_sub(left.len()).saturating_sub(right.len()).saturating_sub(2); // two chars for extra spaces
+            writeln!(f, "{left} {} {right}", ".".repeat(mid_width))?;
+        }
+
+        if self.patches > 0 {
+            writeln!(f, "{}", "-".repeat(WIDTH))?;
+            writeln!(f, "Patches applied: {}", self.patches)?;
+            writeln!(f, "0 problems remaining")?;
+        }
+        Ok(())
+    }
+}

sk-cli/src/validation/tests/annotated_trace_test.rs

@@ -0,0 +1,43 @@
+use assertables::*;
+use json_patch_ext::prelude::*;
+
+use super::*;
+
+#[rstest]
+fn test_apply_patch_everywhere(mut annotated_trace: AnnotatedTrace) {
+    annotated_trace
+        .apply_patch(AnnotatedTracePatch {
+            locations: PatchLocations::Everywhere,
+            op: add_operation(format_ptr!("/foo"), "bar".into()),
+        })
+        .unwrap();
+
+    for event in annotated_trace.iter() {
+        for obj in event.data.applied_objs.iter().chain(event.data.deleted_objs.iter()) {
+            assert_eq!(obj.data.get("foo").unwrap(), "bar");
+        }
+    }
+}
+
+#[rstest]
+fn test_apply_patch_object_reference(mut annotated_trace: AnnotatedTrace) {
+    annotated_trace
+        .apply_patch(AnnotatedTracePatch {
+            locations: PatchLocations::ObjectReference(
+                DEPL_GVK.into_type_meta(),
+                format!("{TEST_NAMESPACE}/test_depl1"),
+            ),
+            op: add_operation(format_ptr!("/foo"), "bar".into()),
+        })
+        .unwrap();
+
+    for event in annotated_trace.iter() {
+        for obj in event.data.applied_objs.iter().chain(event.data.deleted_objs.iter()) {
+            if obj.metadata.name == Some("test_depl1".into()) {
+                assert_eq!(obj.data.get("foo").unwrap(), "bar");
+            } else {
+                assert_none!(obj.data.get("foo"));
+            }
+        }
+    }
+}