diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index 7e496013..6291cee1 100644 --- a/.github/workflows/verify.yml +++ b/.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/action@v3.0.1 diff --git a/sk-cli/src/validation/annotated_trace.rs b/sk-cli/src/validation/annotated_trace.rs index 042cf38b..1d4f7870 100644 --- a/sk-cli/src/validation/annotated_trace.rs +++ b/sk-cli/src/validation/annotated_trace.rs @@ -30,13 +30,13 @@ pub struct AnnotatedTracePatch { pub ops: Vec, } -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Annotation { pub code: ValidatorCode, pub patches: Vec, } -#[derive(Clone, Default)] +#[derive(Clone, Debug, Default)] pub struct AnnotatedTraceEvent { pub data: TraceEvent, pub annotations: BTreeMap>, @@ -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); } diff --git a/sk-cli/src/validation/rules/service_account_missing.rs b/sk-cli/src/validation/rules/service_account_missing.rs index 556db2b8..6970e820 100644 --- a/sk-cli/src/validation/rules/service_account_missing.rs +++ b/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, + pub(crate) seen_service_accounts: HashSet, } 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 { diff --git a/sk-cli/src/validation/rules/tests/mod.rs b/sk-cli/src/validation/rules/tests/mod.rs index 9bf19f56..4c942210 100644 --- a/sk-cli/src/validation/rules/tests/mod.rs +++ b/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()), + ]), + } +} diff --git a/sk-cli/src/validation/rules/tests/service_account_missing_test.rs b/sk-cli/src/validation/rules/tests/service_account_missing_test.rs index 24c228ef..b410aca9 100644 --- a/sk-cli/src/validation/rules/tests/service_account_missing_test.rs +++ b/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![&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![&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![&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![&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); } diff --git a/sk-cli/src/validation/rules/tests/status_field_populated_test.rs b/sk-cli/src/validation/rules/tests/status_field_populated_test.rs index 8953be97..02738ba7 100644 --- a/sk-cli/src/validation/rules/tests/status_field_populated_test.rs +++ b/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![&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); } diff --git a/sk-cli/src/validation/tests/annotated_trace_test.rs b/sk-cli/src/validation/tests/annotated_trace_test.rs index 4049cbe2..34cc2ec0 100644 --- a/sk-cli/src/validation/tests/annotated_trace_test.rs +++ b/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(); diff --git a/sk-cli/src/validation/tests/mod.rs b/sk-cli/src/validation/tests/mod.rs index b2fbcb84..d7f7ff12 100644 --- a/sk-cli/src/validation/tests/mod.rs +++ b/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 { + 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 { - vec![add_operation(format_ptr!("/foo"), "bar".into())] - } - fn reset(&mut self) {} } diff --git a/sk-cli/src/validation/tests/validation_store_test.rs b/sk-cli/src/validation/tests/validation_store_test.rs index cff0e54b..f104ce23 100644 --- a/sk-cli/src/validation/tests/validation_store_test.rs +++ b/sk-cli/src/validation/tests/validation_store_test.rs @@ -1,6 +1,7 @@ use assertables::*; use super::*; +use crate::validation::annotated_trace::Annotation; #[rstest] fn test_validate_trace(test_validation_store: ValidationStore, mut annotated_trace: AnnotatedTrace) { @@ -8,9 +9,9 @@ fn test_validate_trace(test_validation_store: ValidationStore, mut annotated_tra 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); } diff --git a/sk-core/src/constants.rs b/sk-core/src/constants.rs index 52204287..91a8bab2 100644 --- a/sk-core/src/constants.rs +++ b/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"); } } diff --git a/sk-core/src/k8s/testutils/objs.rs b/sk-core/src/k8s/testutils/objs.rs index b06f271a..3471443c 100644 --- a/sk-core/src/k8s/testutils/objs.rs +++ b/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) +}