pass fixes through to annotations

sk-cli/src/validation/annotated_trace.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::BTreeMap; // BTreeMap sorts by key, HashMap doesn't
 use std::slice;
 
 use json_patch_ext::prelude::*;
@@ -18,37 +18,40 @@ use super::validator::{
     ValidatorCode,
 };
 
+#[derive(Clone, Debug)]
+pub enum PatchLocations {
+    Everywhere,
+    ObjectReference(TypeMeta, String),
+}
+
+#[derive(Clone, Debug)]
+pub struct AnnotatedTracePatch {
+    pub locations: PatchLocations,
+    pub ops: Vec<PatchOperation>,
+}
+
+#[derive(Clone)]
+pub struct Annotation {
+    pub code: ValidatorCode,
+    pub patches: Vec<AnnotatedTracePatch>,
+}
+
 #[derive(Clone, Default)]
 pub struct AnnotatedTraceEvent {
     pub data: TraceEvent,
-    pub annotations: Vec<Vec<ValidatorCode>>,
+    pub annotations: BTreeMap<usize, Vec<Annotation>>,
 }
 
 impl AnnotatedTraceEvent {
     pub fn new(data: TraceEvent) -> AnnotatedTraceEvent {
-        let annotations = vec![vec![]; data.len()];
-
-        AnnotatedTraceEvent { data, annotations }
+        AnnotatedTraceEvent { data, ..Default::default() }
     }
 
     pub fn clear_annotations(&mut self) {
-        self.annotations = vec![vec![]; self.data.len()];
+        self.annotations.clear();
     }
 }
 
-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)]
@@ -77,28 +80,24 @@ impl AnnotatedTrace {
     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()
-            {
+            for obj in event.data.applied_objs.iter_mut().chain(event.data.deleted_objs.iter_mut()) {
                 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())?;
+                    for op in &patch.ops {
+                        patch_ext(&mut obj.data, op.clone())?;
+                    }
                 }
             }
         }
         self.patches.push(patch);
+        println!("{:?}", self.patches);
 
         Ok(count)
     }
@@ -115,12 +114,16 @@ impl AnnotatedTrace {
         for event in self.events.iter_mut() {
             event.clear_annotations();
             for (code, validator) in validators.iter() {
-                let affected_indices = validator.check_next_event(event, self.base.config())?;
-                let count = affected_indices.len();
+                let event_patches = validator.check_next_event(event, self.base.config())?;
+                let count = event_patches.len();
                 summary.entry(*code).and_modify(|e| *e += count).or_insert(count);
 
-                for i in affected_indices {
-                    event.annotations[i].push(*code);
+                for (i, obj_patches) in event_patches {
+                    event
+                        .annotations
+                        .entry(i)
+                        .or_insert(vec![])
+                        .push(Annotation { code: *code, patches: obj_patches });
                 }
             }
         }
@@ -131,11 +134,11 @@ impl AnnotatedTrace {
         self.events.get(idx)
     }
 
-    pub fn get_next_error(&self) -> Option<ValidatorCode> {
+    pub fn get_next_annotation(&self) -> Option<&Annotation> {
         for event in &self.events {
-            for annotation in &event.annotations {
-                if let Some(code) = annotation.first() {
-                    return Some(*code);
+            for (_obj_index, annotation_list) in &event.annotations {
+                if let Some(annotation) = annotation_list.first() {
+                    return Some(annotation);
                 }
             }
         }

sk-cli/src/validation/rules/service_account_missing.rs

@@ -1,4 +1,7 @@
-use std::collections::HashSet;
+use std::collections::{
+    BTreeMap,
+    HashSet,
+};
 use std::sync::{
     Arc,
     RwLock,
@@ -9,12 +12,17 @@ use sk_core::k8s::GVK;
 use sk_core::prelude::*;
 use sk_store::TracerConfig;
 
-use crate::validation::annotated_trace::AnnotatedTraceEvent;
 use crate::validation::validator::{
+    CheckResult,
     Diagnostic,
     Validator,
     ValidatorType,
 };
+use crate::validation::{
+    AnnotatedTraceEvent,
+    AnnotatedTracePatch,
+    PatchLocations,
+};
 
 const HELP: &str = r#"A pod needs a service account that is not present in
 the trace file.  The simulation will fail because pods cannot be created
@@ -25,26 +33,8 @@ pub struct ServiceAccountMissing {
     seen_service_accounts: HashSet<String>,
 }
 
-fn get_service_account(obj: &DynamicObject, config: &TracerConfig) -> anyhow::Result<Option<String>> {
-    let gvk = GVK::from_dynamic_obj(obj)?;
-    if let Some(pod_spec_template_path) = config.pod_spec_template_path(&gvk) {
-        let sa_ptr = format_ptr!("{pod_spec_template_path}/spec/serviceAccount");
-        let sa_name_ptr = format_ptr!("{pod_spec_template_path}/spec/serviceAccountName");
-        if let Ok(sa) = sa_ptr.resolve(&obj.data) {
-            return Ok(Some(sa.to_string()));
-        } else if let Ok(sa) = sa_name_ptr.resolve(&obj.data) {
-            return Ok(Some(sa.to_string()));
-        }
-    }
-    Ok(None)
-}
-
 impl Diagnostic for ServiceAccountMissing {
-    fn check_next_event(
-        &mut self,
-        event: &mut AnnotatedTraceEvent,
-        config: &TracerConfig,
-    ) -> anyhow::Result<Vec<usize>> {
+    fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent, config: &TracerConfig) -> CheckResult {
         for obj in &event.data.applied_objs {
             if let Some(ref type_meta) = obj.types {
                 if &type_meta.kind == "ServiceAccount" {
@@ -60,21 +50,30 @@ impl Diagnostic for ServiceAccountMissing {
             }
         }
 
-        let mut indices = vec![];
+        let mut patches = vec![];
         for (i, obj) in event.data.applied_objs.iter().enumerate() {
-            let maybe_sa = get_service_account(obj, config)?;
-            if let Some(sa) = maybe_sa {
-                if !self.seen_service_accounts.contains(&sa) {
-                    indices.push(i);
+            let gvk = GVK::from_dynamic_obj(obj)?;
+            if let Some(pod_spec_template_path) = config.pod_spec_template_path(&gvk) {
+                let sa_ptrs = vec![
+                    format_ptr!("{pod_spec_template_path}/spec/serviceAccount"),
+                    format_ptr!("{pod_spec_template_path}/spec/serviceAccountName"),
+                ];
+                if let Some(sa) = sa_ptrs.iter().filter_map(|ptr| ptr.resolve(&obj.data).ok()).next() {
+                    if !self.seen_service_accounts.contains(sa.as_str().expect("expected string")) {
+                        let fix = AnnotatedTracePatch {
+                            locations: PatchLocations::ObjectReference(
+                                obj.types.clone().unwrap_or_default(),
+                                obj.namespaced_name(),
+                            ),
+                            ops: sa_ptrs.iter().map(|ptr| remove_operation(ptr.clone())).collect(),
+                        };
+                        patches.push((i, vec![fix]));
+                    }
                 }
             }
         }
 
-        Ok(indices)
-    }
-
-    fn fixes(&self) -> Vec<PatchOperation> {
-        vec![remove_operation(format_ptr!("/spec/serviceAccount"))]
+        Ok(BTreeMap::from_iter(patches))
     }
 
     fn reset(&mut self) {}

sk-cli/src/validation/rules/status_field_populated.rs

@@ -4,14 +4,20 @@ use std::sync::{
 };
 
 use json_patch_ext::prelude::*;
+use lazy_static::lazy_static;
 use sk_store::TracerConfig;
 
-use crate::validation::annotated_trace::AnnotatedTraceEvent;
 use crate::validation::validator::{
+    CheckResult,
     Diagnostic,
     Validator,
     ValidatorType,
 };
+use crate::validation::{
+    AnnotatedTraceEvent,
+    AnnotatedTracePatch,
+    PatchLocations,
+};
 
 const HELP: &str = r#"Indicates that the status field of a Kubernetes object in
 the trace is non-empty; status fields are updated by their controlling objects
@@ -21,8 +27,15 @@ better to clean them up (and also they take up a lot of space."#;
 #[derive(Default)]
 pub struct StatusFieldPopulated {}
 
+lazy_static! {
+    static ref FIX: AnnotatedTracePatch = AnnotatedTracePatch {
+        locations: PatchLocations::Everywhere,
+        ops: vec![remove_operation(format_ptr!("/status"))],
+    };
+}
+
 impl Diagnostic for StatusFieldPopulated {
-    fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent, _: &TracerConfig) -> anyhow::Result<Vec<usize>> {
+    fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent, _: &TracerConfig) -> CheckResult {
         Ok(event
             .data
             .applied_objs
@@ -36,17 +49,14 @@ impl Diagnostic for StatusFieldPopulated {
                     .get("status")
                     .is_some_and(|v| !v.is_null())
                 {
-                    return Some(i);
+                    Some((i, vec![FIX.clone()]))
+                } else {
+                    None
                 }
-                None
             })
             .collect())
     }
 
-    fn fixes(&self) -> Vec<PatchOperation> {
-        vec![remove_operation(format_ptr!("/status"))]
-    }
-
     fn reset(&mut self) {}
 }
 

sk-cli/src/validation/rules/tests/mod.rs

@@ -1,3 +1,4 @@
+mod service_account_missing_test;
 mod status_field_populated_test;
 
 use rstest::*;

sk-cli/src/validation/rules/tests/service_account_missing_test.rs

@@ -0,0 +1,36 @@
+
+use assertables::*;
+use serde_json::json;
+use sk_store::TraceEvent;
+
+use super::*;
+
+#[rstest]
+fn test_status_field_populated(test_deployment: DynamicObject) {
+    let v = status_field_populated::validator();
+    let mut evt = AnnotatedTraceEvent {
+        data: TraceEvent {
+            ts: 1,
+            applied_objs: vec![test_deployment.data(json!({"status": {"availableReplicas": 3}}))],
+            deleted_objs: vec![],
+        },
+        ..Default::default()
+    };
+    let indices = v.check_next_event(&mut evt);
+    assert_bag_eq!(indices, &[0]);
+}
+
+#[rstest]
+fn test_status_field_not_populated(test_deployment: DynamicObject) {
+    let v = status_field_populated::validator();
+    let mut evt = AnnotatedTraceEvent {
+        data: TraceEvent {
+            ts: 1,
+            applied_objs: vec![test_deployment.data(json!({}))],
+            deleted_objs: vec![],
+        },
+        ..Default::default()
+    };
+    let indices = v.check_next_event(&mut evt);
+    assert_is_empty!(indices);
+}

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

@@ -8,7 +8,7 @@ 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()),
+            ops: add_operation(format_ptr!("/foo"), "bar".into()),
         })
         .unwrap();
 
@@ -27,7 +27,7 @@ fn test_apply_patch_object_reference(mut annotated_trace: AnnotatedTrace) {
                 DEPL_GVK.into_type_meta(),
                 format!("{TEST_NAMESPACE}/test_depl1"),
             ),
-            op: add_operation(format_ptr!("/foo"), "bar".into()),
+            ops: add_operation(format_ptr!("/foo"), "bar".into()),
         })
         .unwrap();
 

sk-cli/src/validation/validation_store.rs

@@ -13,8 +13,6 @@ use super::validator::{
 };
 use super::{
     AnnotatedTrace,
-    AnnotatedTracePatch,
-    PatchLocations,
     PrintFormat,
 };
 
@@ -42,25 +40,15 @@ impl ValidationStore {
                 break;
             }
 
-            let Some(next_error) = trace.get_next_error() else {
+            let Some(next_annotation) = trace.get_next_annotation() else {
                 break;
             };
 
-            let Some(op) = self
-                .validators
-                .get(&next_error)
-                .ok_or(anyhow!("validation error"))?
-                .fixes()
-                .first()
-                .cloned()
-            else {
-                println!("no fix available for {next_error}; continuing");
+            let Some(patch) = next_annotation.patches.first().cloned() else {
+                println!("no fix available for {}; continuing", next_annotation.code);
                 break;
             };
-            summary.patches += trace.apply_patch(AnnotatedTracePatch {
-                locations: PatchLocations::AffectedObjects(next_error),
-                op,
-            })?;
+            summary.patches += trace.apply_patch(patch)?;
         }
 
         Ok(summary)