fix/add new tests

.github/workflows/verify.yml

@@ -74,6 +74,8 @@ jobs:
       # tomlq (included with yq) is needed to check the package version in Cargo.toml
       - name: Install tomlq
         run: pip install yq
+      - name: cargo clippy version
+        run: cargo clippy --version
       - name: Run pre-commit
         uses: pre-commit/[email protected]
 

sk-cli/src/validation/annotated_trace.rs

@@ -30,13 +30,13 @@ pub struct AnnotatedTracePatch {
     pub ops: Vec<PatchOperation>,
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct Annotation {
     pub code: ValidatorCode,
     pub patches: Vec<AnnotatedTracePatch>,
 }
 
-#[derive(Clone, Default)]
+#[derive(Clone, Debug, Default)]
 pub struct AnnotatedTraceEvent {
     pub data: TraceEvent,
     pub annotations: BTreeMap<usize, Vec<Annotation>>,
@@ -136,7 +136,7 @@ impl AnnotatedTrace {
 
     pub fn get_next_annotation(&self) -> Option<&Annotation> {
         for event in &self.events {
-            for (_obj_index, annotation_list) in &event.annotations {
+            for annotation_list in event.annotations.values() {
                 if let Some(annotation) = annotation_list.first() {
                     return Some(annotation);
                 }

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

@@ -30,7 +30,7 @@ if their service account does not exist."#;
 
 #[derive(Default)]
 pub struct ServiceAccountMissing {
-    seen_service_accounts: HashSet<String>,
+    pub(crate) seen_service_accounts: HashSet<String>,
 }
 
 impl Diagnostic for ServiceAccountMissing {
@@ -54,7 +54,8 @@ impl Diagnostic for ServiceAccountMissing {
         for (i, obj) in event.data.applied_objs.iter().enumerate() {
             let gvk = GVK::from_dynamic_obj(obj)?;
             if let Some(pod_spec_template_path) = config.pod_spec_template_path(&gvk) {
-                let sa_ptrs = vec![
+                let sa_ptrs = [
+                    // serviceAccount is deprecated but still supported (for now)
                     format_ptr!("{pod_spec_template_path}/spec/serviceAccount"),
                     format_ptr!("{pod_spec_template_path}/spec/serviceAccountName"),
                 ];
@@ -76,7 +77,9 @@ impl Diagnostic for ServiceAccountMissing {
         Ok(BTreeMap::from_iter(patches))
     }
 
-    fn reset(&mut self) {}
+    fn reset(&mut self) {
+        self.seen_service_accounts.clear();
+    }
 }
 
 pub fn validator() -> Validator {

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

@@ -1,8 +1,31 @@
 mod service_account_missing_test;
 mod status_field_populated_test;
 
+use std::collections::HashMap;
+
 use rstest::*;
 use sk_core::prelude::*;
+use sk_store::{
+    TracerConfig,
+    TrackedObjectConfig,
+};
 
 use super::*;
+use crate::validation::validator::Diagnostic;
 use crate::validation::AnnotatedTraceEvent;
+
+#[fixture]
+fn test_trace_config() -> TracerConfig {
+    TracerConfig {
+        tracked_objects: HashMap::from([
+            (
+                DEPL_GVK.clone(),
+                TrackedObjectConfig {
+                    pod_spec_template_path: Some("/spec/template".into()),
+                    ..Default::default()
+                },
+            ),
+            (SA_GVK.clone(), Default::default()),
+        ]),
+    }
+}

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

@@ -1,36 +1,113 @@
-
 use assertables::*;
 use serde_json::json;
-use sk_store::TraceEvent;
+use sk_store::{
+    TraceEvent,
+    TracerConfig,
+};
 
+use super::service_account_missing::ServiceAccountMissing;
 use super::*;
 
-#[rstest]
-fn test_status_field_populated(test_deployment: DynamicObject) {
-    let v = status_field_populated::validator();
-    let mut evt = AnnotatedTraceEvent {
+#[fixture]
+fn depl_event(test_deployment: DynamicObject, #[default("serviceAccount")] sa_key: &str) -> AnnotatedTraceEvent {
+    AnnotatedTraceEvent {
         data: TraceEvent {
             ts: 1,
-            applied_objs: vec![test_deployment.data(json!({"status": {"availableReplicas": 3}}))],
+            applied_objs: vec![test_deployment.data(json!({"spec": {"template": {"spec": {sa_key: "foobar"}}}}))],
+            deleted_objs: vec![],
+        },
+        ..Default::default()
+    }
+}
+
+#[fixture]
+fn sa_event(test_service_account: DynamicObject) -> AnnotatedTraceEvent {
+    AnnotatedTraceEvent {
+        data: TraceEvent {
+            ts: 0,
+            applied_objs: vec![test_service_account],
             deleted_objs: vec![],
         },
         ..Default::default()
+    }
+}
+
+#[rstest]
+#[case("serviceAccount")]
+#[case("serviceAccountName")]
+fn test_service_account_missing(test_deployment: DynamicObject, test_trace_config: TracerConfig, #[case] sa_key: &str) {
+    let mut v = ServiceAccountMissing::default();
+    let mut evt = depl_event(test_deployment, sa_key);
+    let annotations = v.check_next_event(&mut evt, &test_trace_config).unwrap();
+
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
+}
+
+#[rstest]
+fn test_service_account_missing_deleted(
+    mut depl_event: AnnotatedTraceEvent,
+    mut sa_event: AnnotatedTraceEvent,
+    test_service_account: DynamicObject,
+    test_trace_config: TracerConfig,
+) {
+    let mut v = ServiceAccountMissing::default();
+    let mut sa_event_del = AnnotatedTraceEvent {
+        data: TraceEvent {
+            ts: 0,
+            applied_objs: vec![],
+            deleted_objs: vec![test_service_account],
+        },
+        ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_bag_eq!(indices, &[0]);
+    v.check_next_event(&mut sa_event, &test_trace_config).unwrap();
+    v.check_next_event(&mut sa_event_del, &test_trace_config).unwrap();
+    let annotations = v.check_next_event(&mut depl_event, &test_trace_config).unwrap();
+
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
+}
+
+#[rstest]
+fn test_service_account_not_missing(
+    mut depl_event: AnnotatedTraceEvent,
+    mut sa_event: AnnotatedTraceEvent,
+    test_trace_config: TracerConfig,
+) {
+    let mut v = ServiceAccountMissing::default();
+    v.check_next_event(&mut sa_event, &test_trace_config).unwrap();
+    let annotations = v.check_next_event(&mut depl_event, &test_trace_config).unwrap();
+
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
 }
 
 #[rstest]
-fn test_status_field_not_populated(test_deployment: DynamicObject) {
-    let v = status_field_populated::validator();
-    let mut evt = AnnotatedTraceEvent {
+fn test_service_account_not_missing_same_evt(
+    test_deployment: DynamicObject,
+    test_service_account: DynamicObject,
+    test_trace_config: TracerConfig,
+) {
+    let mut v = ServiceAccountMissing::default();
+    let mut depl_evt = AnnotatedTraceEvent {
         data: TraceEvent {
             ts: 1,
-            applied_objs: vec![test_deployment.data(json!({}))],
+            applied_objs: vec![
+                test_deployment
+                    .data(json!({"spec": {"template": {"spec": {"serviceAccountName": TEST_SERVICE_ACCOUNT}}}})),
+                test_service_account,
+            ],
             deleted_objs: vec![],
         },
         ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_is_empty!(indices);
+    let annotations = v.check_next_event(&mut depl_evt, &test_trace_config).unwrap();
+
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
+}
+
+#[rstest]
+fn test_service_account_reset(mut depl_event: AnnotatedTraceEvent, test_trace_config: TracerConfig) {
+    let mut v = ServiceAccountMissing::default();
+    v.check_next_event(&mut depl_event, &test_trace_config).unwrap();
+    v.reset();
+
+    assert_is_empty!(v.seen_service_accounts);
 }

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

@@ -15,8 +15,8 @@ fn test_status_field_populated(test_deployment: DynamicObject) {
         },
         ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_bag_eq!(indices, &[0]);
+    let annotations = v.check_next_event(&mut evt, &Default::default()).unwrap();
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
 }
 
 #[rstest]
@@ -30,6 +30,6 @@ fn test_status_field_not_populated(test_deployment: DynamicObject) {
         },
         ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_is_empty!(indices);
+    let annotations = v.check_next_event(&mut evt, &Default::default()).unwrap();
+    assert_is_empty!(annotations);
 }

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,
-            ops: add_operation(format_ptr!("/foo"), "bar".into()),
+            ops: vec![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"),
             ),
-            ops: add_operation(format_ptr!("/foo"), "bar".into()),
+            ops: vec![add_operation(format_ptr!("/foo"), "bar".into())],
         })
         .unwrap();
 

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

@@ -10,10 +10,14 @@ use std::sync::{
 use json_patch_ext::prelude::*;
 use rstest::*;
 use sk_core::prelude::*;
-use sk_store::TraceEvent;
+use sk_store::{
+    TraceEvent,
+    TracerConfig,
+};
 
 use super::annotated_trace::AnnotatedTraceEvent;
 use super::validator::{
+    CheckResult,
     Diagnostic,
     Validator,
     ValidatorCode,
@@ -52,18 +56,20 @@ pub fn annotated_trace() -> AnnotatedTrace {
 struct TestDiagnostic {}
 
 impl Diagnostic for TestDiagnostic {
-    fn check_next_event(&mut self, evt: &mut AnnotatedTraceEvent) -> Vec<usize> {
+    fn check_next_event(&mut self, evt: &mut AnnotatedTraceEvent, _: &TracerConfig) -> CheckResult {
         if evt.data.applied_objs.len() > 1 && evt.data.applied_objs[1].data.get("foo").is_none() {
-            vec![1]
+            Ok(BTreeMap::from([(
+                1,
+                vec![AnnotatedTracePatch {
+                    locations: PatchLocations::Everywhere,
+                    ops: vec![add_operation(format_ptr!("/foo"), "bar".into())],
+                }],
+            )]))
         } else {
-            vec![]
+            Ok(BTreeMap::new())
         }
     }
 
-    fn fixes(&self) -> Vec<PatchOperation> {
-        vec![add_operation(format_ptr!("/foo"), "bar".into())]
-    }
-
     fn reset(&mut self) {}
 }
 

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

@@ -1,16 +1,17 @@
 use assertables::*;
 
 use super::*;
+use crate::validation::annotated_trace::Annotation;
 
 #[rstest]
 fn test_validate_trace(test_validation_store: ValidationStore, mut annotated_trace: AnnotatedTrace) {
     let summary = test_validation_store.validate_trace(&mut annotated_trace, false).unwrap();
 
     for event in annotated_trace.iter() {
         if event.data.applied_objs.len() > 1 {
-            assert_eq!(event.annotations[1], vec![TEST_VALIDATOR_CODE]);
+            assert_all!(event.annotations[&1].iter(), |a: &Annotation| a.code == TEST_VALIDATOR_CODE);
         } else {
-            for annotation in &event.annotations {
+            for annotation in event.annotations.values() {
                 assert_is_empty!(annotation);
             }
         }
@@ -25,15 +26,14 @@ fn test_fix_trace(test_validation_store: ValidationStore, mut annotated_trace: A
     let summary = test_validation_store.validate_trace(&mut annotated_trace, true).unwrap();
 
     for event in annotated_trace.iter() {
-        println!("{:?}", event.data);
         if event.data.applied_objs.len() > 1 {
             assert_eq!(event.data.applied_objs[1].data.get("foo").unwrap(), "bar");
         }
-        for annotation in &event.annotations {
+        for annotation in event.annotations.values() {
             assert_is_empty!(annotation);
         }
     }
 
     assert_eq!(*summary.annotations.get(&TEST_VALIDATOR_CODE).unwrap(), 1);
-    assert_eq!(summary.patches, 1);
+    assert_eq!(summary.patches, 5);
 }

sk-core/src/constants.rs

@@ -36,6 +36,8 @@ mod test_constants {
 
     pub const EMPTY_POD_SPEC_HASH: u64 = 17506812802394981455;
     pub const TEST_DEPLOYMENT: &str = "the-deployment";
+    pub const TEST_DAEMONSET: &str = "the-daemonset";
+    pub const TEST_SERVICE_ACCOUNT: &str = "the-service-account";
     pub const TEST_NAMESPACE: &str = "test-namespace";
     pub const TEST_SIM_NAME: &str = "test-sim";
     pub const TEST_SIM_ROOT_NAME: &str = "test-sim-root";
@@ -47,6 +49,7 @@ mod test_constants {
     lazy_static! {
         pub static ref DEPL_GVK: GVK = GVK::new("apps", "v1", "Deployment");
         pub static ref DS_GVK: GVK = GVK::new("apps", "v1", "DaemonSet");
+        pub static ref SA_GVK: GVK = GVK::new("", "v1", "ServiceAccount");
     }
 }
 

sk-core/src/k8s/testutils/objs.rs

@@ -16,8 +16,13 @@ pub fn test_deployment(#[default(TEST_DEPLOYMENT)] name: &str) -> DynamicObject
 }
 
 #[fixture]
-pub fn test_daemonset(#[default(TEST_DEPLOYMENT)] name: &str) -> DynamicObject {
+pub fn test_daemonset(#[default(TEST_DAEMONSET)] name: &str) -> DynamicObject {
     DynamicObject::new(&name, &ApiResource::from_gvk(&DS_GVK))
         .within(TEST_NAMESPACE)
         .data(json!({"spec": {"updateStrategy": {"type": "onDelete"}}}))
 }
+
+#[fixture]
+pub fn test_service_account(#[default(TEST_SERVICE_ACCOUNT)] name: &str) -> DynamicObject {
+    DynamicObject::new(&name, &ApiResource::from_gvk(&SA_GVK)).within(TEST_NAMESPACE)
+}