From 7a81987d01b53a8d5a08d412834234247a0e4347 Mon Sep 17 00:00:00 2001 From: David Morrison Date: Sat, 26 Oct 2024 14:48:08 -0700 Subject: [PATCH] feat: service_account_missing validation check --- .pre-commit-config.yaml | 2 +- Makefile | 2 +- sk-cli/src/validation/README.md | 7 -- sk-cli/src/validation/annotated_trace.rs | 2 +- sk-cli/src/validation/rules/README.md | 8 ++ sk-cli/src/validation/rules/mod.rs | 1 + .../rules/service_account_missing.rs | 92 ++++++++++++++ .../rules/status_field_populated.rs | 3 +- sk-cli/src/validation/rules/tests/mod.rs | 24 ++++ .../tests/service_account_missing_test.rs | 113 ++++++++++++++++++ .../tests/status_field_populated_test.rs | 4 +- sk-cli/src/validation/tests/mod.rs | 7 +- sk-cli/src/validation/validation_store.rs | 1 + sk-cli/src/validation/validator.rs | 7 +- sk-core/src/constants.rs | 3 + sk-core/src/k8s/testutils/objs.rs | 7 +- 16 files changed, 264 insertions(+), 19 deletions(-) delete mode 100644 sk-cli/src/validation/README.md create mode 100644 sk-cli/src/validation/rules/README.md create mode 100644 sk-cli/src/validation/rules/service_account_missing.rs create mode 100644 sk-cli/src/validation/rules/tests/service_account_missing_test.rs diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index bb185bbc..4942b28f 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -52,4 +52,4 @@ repos: language: system entry: bash -c 'make validation_rules && git diff --quiet' pass_filenames: false - files: "sk-store/src/validation/.*" + files: "sk-cli/src/validation/.*" diff --git a/Makefile b/Makefile index 17736a6e..c02ab96f 100644 --- a/Makefile +++ b/Makefile @@ -88,7 +88,7 @@ crd: skctl pre-k8s:: crd .PHONY: validation_rules -validation_rules: VALIDATION_FILE=sk-cli/src/validation/README.md +validation_rules: VALIDATION_FILE=sk-cli/src/validation/rules/README.md validation_rules: skctl printf "# SimKube Trace Validation Checks\n\n" > $(VALIDATION_FILE) $(BUILD_DIR)/skctl validate print --format table >> $(VALIDATION_FILE) diff --git a/sk-cli/src/validation/README.md b/sk-cli/src/validation/README.md deleted file mode 100644 index 74dbe336..00000000 --- a/sk-cli/src/validation/README.md +++ /dev/null @@ -1,7 +0,0 @@ -# SimKube Trace Validation Checks - -| code | name | description | -|---|---|---| -| W0000 | status_field_populated | Indicates that the status field of a Kubernetes object in the trace is non-empty; status fields are updated by their controlling objects and shouldn't be applied "by hand". This is probably "fine" but it would be better to clean them up (and also they take up a lot of space. | - -This file is auto-generated; to rebuild, run `make validation_rules`. diff --git a/sk-cli/src/validation/annotated_trace.rs b/sk-cli/src/validation/annotated_trace.rs index 816689d0..e071427e 100644 --- a/sk-cli/src/validation/annotated_trace.rs +++ b/sk-cli/src/validation/annotated_trace.rs @@ -123,7 +123,7 @@ impl AnnotatedTrace { for event in self.events.iter_mut() { event.clear_annotations(); for (code, validator) in validators.iter() { - let event_patches = validator.check_next_event(event)?; + 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); diff --git a/sk-cli/src/validation/rules/README.md b/sk-cli/src/validation/rules/README.md new file mode 100644 index 00000000..33fd70fb --- /dev/null +++ b/sk-cli/src/validation/rules/README.md @@ -0,0 +1,8 @@ +# SimKube Trace Validation Checks + +| code | name | description | +|---|---|---| +| W0000 | status_field_populated | Indicates that the status field of a Kubernetes object in the trace is non-empty; status fields are updated by their controlling objects and shouldn't be applied "by hand". This is probably "fine" but it would be better to clean them up (and also they take up a lot of space. | +| E0001 | service_account_missing | A pod needs a service account that is not present in the trace file. The simulation will fail because pods cannot be created if their service account does not exist. | + +This file is auto-generated; to rebuild, run `make validation_rules`. diff --git a/sk-cli/src/validation/rules/mod.rs b/sk-cli/src/validation/rules/mod.rs index c7fd3baa..991266c5 100644 --- a/sk-cli/src/validation/rules/mod.rs +++ b/sk-cli/src/validation/rules/mod.rs @@ -1,3 +1,4 @@ +pub mod service_account_missing; pub mod status_field_populated; #[cfg(test)] diff --git a/sk-cli/src/validation/rules/service_account_missing.rs b/sk-cli/src/validation/rules/service_account_missing.rs new file mode 100644 index 00000000..6970e820 --- /dev/null +++ b/sk-cli/src/validation/rules/service_account_missing.rs @@ -0,0 +1,92 @@ +use std::collections::{ + BTreeMap, + HashSet, +}; +use std::sync::{ + Arc, + RwLock, +}; + +use json_patch_ext::prelude::*; +use sk_core::k8s::GVK; +use sk_core::prelude::*; +use sk_store::TracerConfig; + +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 +if their service account does not exist."#; + +#[derive(Default)] +pub struct ServiceAccountMissing { + pub(crate) seen_service_accounts: HashSet, +} + +impl Diagnostic for ServiceAccountMissing { + 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" { + self.seen_service_accounts.insert(obj.namespaced_name()); + } + } + } + for obj in &event.data.deleted_objs { + if let Some(ref type_meta) = obj.types { + if &type_meta.kind == "ServiceAccount" { + self.seen_service_accounts.remove(&obj.namespaced_name()); + } + } + } + + let mut patches = vec![]; + 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 = [ + // serviceAccount is deprecated but still supported (for now) + 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(BTreeMap::from_iter(patches)) + } + + fn reset(&mut self) { + self.seen_service_accounts.clear(); + } +} + +pub fn validator() -> Validator { + Validator { + type_: ValidatorType::Error, + name: "service_account_missing", + help: HELP, + diagnostic: Arc::new(RwLock::new(ServiceAccountMissing::default())), + } +} diff --git a/sk-cli/src/validation/rules/status_field_populated.rs b/sk-cli/src/validation/rules/status_field_populated.rs index 0332cc83..f9e4f214 100644 --- a/sk-cli/src/validation/rules/status_field_populated.rs +++ b/sk-cli/src/validation/rules/status_field_populated.rs @@ -5,6 +5,7 @@ use std::sync::{ use json_patch_ext::prelude::*; use lazy_static::lazy_static; +use sk_store::TracerConfig; use crate::validation::validator::{ CheckResult, @@ -34,7 +35,7 @@ lazy_static! { } impl Diagnostic for StatusFieldPopulated { - fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent) -> CheckResult { + fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent, _: &TracerConfig) -> CheckResult { Ok(event .data .applied_objs diff --git a/sk-cli/src/validation/rules/tests/mod.rs b/sk-cli/src/validation/rules/tests/mod.rs index 2a4b9609..4c942210 100644 --- a/sk-cli/src/validation/rules/tests/mod.rs +++ b/sk-cli/src/validation/rules/tests/mod.rs @@ -1,7 +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 new file mode 100644 index 00000000..b410aca9 --- /dev/null +++ b/sk-cli/src/validation/rules/tests/service_account_missing_test.rs @@ -0,0 +1,113 @@ +use assertables::*; +use serde_json::json; +use sk_store::{ + TraceEvent, + TracerConfig, +}; + +use super::service_account_missing::ServiceAccountMissing; +use super::*; + +#[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!({"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() + }; + 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_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!({"spec": {"template": {"spec": {"serviceAccountName": TEST_SERVICE_ACCOUNT}}}})), + test_service_account, + ], + deleted_objs: vec![], + }, + ..Default::default() + }; + 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 6b5bb298..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,7 +15,7 @@ fn test_status_field_populated(test_deployment: DynamicObject) { }, ..Default::default() }; - let annotations = v.check_next_event(&mut evt).unwrap(); + let annotations = v.check_next_event(&mut evt, &Default::default()).unwrap(); assert_eq!(annotations.keys().collect::>(), vec![&0]); } @@ -30,6 +30,6 @@ fn test_status_field_not_populated(test_deployment: DynamicObject) { }, ..Default::default() }; - let annotations = v.check_next_event(&mut evt).unwrap(); + let annotations = v.check_next_event(&mut evt, &Default::default()).unwrap(); assert_is_empty!(annotations); } diff --git a/sk-cli/src/validation/tests/mod.rs b/sk-cli/src/validation/tests/mod.rs index 1b6f4dea..d7f7ff12 100644 --- a/sk-cli/src/validation/tests/mod.rs +++ b/sk-cli/src/validation/tests/mod.rs @@ -10,7 +10,10 @@ 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::{ @@ -53,7 +56,7 @@ pub fn annotated_trace() -> AnnotatedTrace { struct TestDiagnostic {} impl Diagnostic for TestDiagnostic { - fn check_next_event(&mut self, evt: &mut AnnotatedTraceEvent) -> CheckResult { + 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() { Ok(BTreeMap::from([( 1, diff --git a/sk-cli/src/validation/validation_store.rs b/sk-cli/src/validation/validation_store.rs index 472c4b1c..69126544 100644 --- a/sk-cli/src/validation/validation_store.rs +++ b/sk-cli/src/validation/validation_store.rs @@ -97,6 +97,7 @@ impl ValidationStore { let mut store = ValidationStore { validators: BTreeMap::new() }; store.register(status_field_populated::validator()); + store.register(service_account_missing::validator()); store } diff --git a/sk-cli/src/validation/validator.rs b/sk-cli/src/validation/validator.rs index 161abfd1..5e8bc4dd 100644 --- a/sk-cli/src/validation/validator.rs +++ b/sk-cli/src/validation/validator.rs @@ -11,6 +11,7 @@ use serde::{ Serialize, Serializer, }; +use sk_store::TracerConfig; use super::annotated_trace::{ AnnotatedTraceEvent, @@ -60,7 +61,7 @@ impl fmt::Display for ValidatorCode { pub type CheckResult = anyhow::Result>>; pub trait Diagnostic { - fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent) -> CheckResult; + fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent, config: &TracerConfig) -> CheckResult; fn reset(&mut self); } @@ -78,8 +79,8 @@ pub struct Validator { } impl Validator { - pub fn check_next_event(&self, event: &mut AnnotatedTraceEvent) -> CheckResult { - self.diagnostic.write().unwrap().check_next_event(event) + pub fn check_next_event(&self, event: &mut AnnotatedTraceEvent, config: &TracerConfig) -> CheckResult { + self.diagnostic.write().unwrap().check_next_event(event, config) } pub fn reset(&self) { 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) +}