From 4ee966b9323934bbe5e5fccced8d219c820f441d Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Tue, 12 Nov 2024 11:42:33 -0600 Subject: [PATCH 1/5] feat: force-delete option for when the cluster is inaccessible --- src/controller.rs | 92 ++++++++++++++++++++++++++++++++--------------- src/resources.rs | 14 ++++++++ 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/src/controller.rs b/src/controller.rs index eb99eb4..262e6ca 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -71,28 +71,38 @@ async fn reconcile_deleted_resource( Ok(Action::await_change()) } Err(kube::Error::Api(err)) if err.code == 404 => { - resource_sync.stop_remote_watches_if_watching(ctx).await; + stop_watches_and_remove_resource_sync_finalizers(resource_sync, name, parent_api, ctx) + .await + } + Err(err) => Err(err.into()), + } +} - let patched_finalizers = resource_sync - .finalizers_clone_or_empty() - .with_item_removed(&FINALIZER.to_string()); +async fn stop_watches_and_remove_resource_sync_finalizers( + resource_sync: Arc, + name: &str, + parent_api: &Api, + ctx: Arc, +) -> Result { + resource_sync.stop_remote_watches_if_watching(ctx).await; - // Target has been deleted, remove the finalizer from the ResourceSync - let patch = Merge(json!({ - "metadata": { - "finalizers": patched_finalizers, - }, - })); + let patched_finalizers = resource_sync + .finalizers_clone_or_empty() + .with_item_removed(&FINALIZER.to_string()); - parent_api - .patch(name, &PatchParams::default(), &patch) - .await?; + // Target has been deleted, remove the finalizer from the ResourceSync + let patch = Merge(json!({ + "metadata": { + "finalizers": patched_finalizers, + }, + })); - // We have removed our finalizer, so nothing more needs to be done - Ok(Action::await_change()) - } - Err(err) => Err(err.into()), - } + parent_api + .patch(name, &PatchParams::default(), &patch) + .await?; + + // We have removed our finalizer, so nothing more needs to be done + Ok(Action::await_change()) } async fn add_target_finalizer( @@ -205,16 +215,23 @@ async fn reconcile(resource_sync: Arc, ctx: Arc) -> Resul debug!(?resource_sync.spec, "got"); let local_ns = resource_sync.namespace().ok_or(Error::NamespaceRequired)?; - let target_api = resource_sync - .spec - .target - .api_for(ctx.client.clone(), &local_ns) - .await?; - let source_api = resource_sync - .spec - .source - .api_for(ctx.client.clone(), &local_ns) - .await?; + let (source_api, target_api) = + match source_and_target_apis(&resource_sync, &ctx, local_ns).await { + Ok(apis) => apis, + Err(_) + if resource_sync.has_force_delete_option_enabled() + && resource_sync.has_been_deleted() => + { + return stop_watches_and_remove_resource_sync_finalizers( + resource_sync, + &name, + &parent_api, + ctx, + ) + .await; + } + Err(err) => return Err(err), + }; match resource_sync { resource_sync if resource_sync.has_been_deleted() => { @@ -258,6 +275,25 @@ async fn reconcile(resource_sync: Arc, ctx: Arc) -> Resul result } +async fn source_and_target_apis( + resource_sync: &Arc, + ctx: &Arc, + local_ns: String, +) -> Result<(NamespacedApi, NamespacedApi)> { + let target_api = resource_sync + .spec + .target + .api_for(ctx.client.clone(), &local_ns) + .await?; + let source_api = resource_sync + .spec + .source + .api_for(ctx.client.clone(), &local_ns) + .await?; + + Ok((source_api, target_api)) +} + fn sync_failing_transition_time(status: &Option) -> Time { let now = Time(Utc::now()); diff --git a/src/resources.rs b/src/resources.rs index e92ae65..433a45a 100644 --- a/src/resources.rs +++ b/src/resources.rs @@ -9,6 +9,20 @@ use kube::{ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +static FORCE_DELETE_ANNOTATION: &str = "sinker.influxdata.io/force-delete"; + +impl ResourceSync { + pub fn has_force_delete_option_enabled(&self) -> bool { + self.metadata + .annotations + .as_ref() + .map(|annotations| annotations[FORCE_DELETE_ANNOTATION].clone()) + .unwrap_or_default() + .parse() + .unwrap_or_default() + } +} + #[derive(CustomResource, Debug, Serialize, Deserialize, Default, Clone, JsonSchema)] #[kube( group = "sinker.influxdata.io", From 8c38a937123a979543bbea006c40a7acb48a662b Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Tue, 12 Nov 2024 12:17:21 -0600 Subject: [PATCH 2/5] feat: test parsing the annotation --- src/resources.rs | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/resources.rs b/src/resources.rs index 433a45a..de8b8a3 100644 --- a/src/resources.rs +++ b/src/resources.rs @@ -16,7 +16,9 @@ impl ResourceSync { self.metadata .annotations .as_ref() - .map(|annotations| annotations[FORCE_DELETE_ANNOTATION].clone()) + .map(|annotations| annotations.get(FORCE_DELETE_ANNOTATION)) + .unwrap_or_default() + .cloned() .unwrap_or_default() .parse() .unwrap_or_default() @@ -156,3 +158,35 @@ impl SinkerContainer { crd } } + +#[cfg(test)] +mod tests { + use super::*; + use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; + use rstest::rstest; + use std::collections::BTreeMap; + + #[rstest] + #[case::no_annotations( + ResourceSync{metadata: Default::default(),spec: Default::default(),status: None,}, false + )] + #[case::force_delete_annotation_not_present( + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::new()), ..Default::default()},spec: Default::default(),status: None,}, false + )] + #[case::force_delete_annotation_is_false( + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "false".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, false + )] + #[case::force_delete_annotation_is_other( + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "other".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, false + )] + #[case::force_delete_annotation_is_true( + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "true".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, true + )] + #[tokio::test] + async fn test_resource_sync_has_force_delete_option_enabled( + #[case] resource_sync: ResourceSync, + #[case] expected: bool, + ) { + assert_eq!(resource_sync.has_force_delete_option_enabled(), expected); + } +} From 1efface3b46ad5dba6552be73f798682bac15c0c Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Tue, 12 Nov 2024 12:23:07 -0600 Subject: [PATCH 3/5] feat: some logging --- src/controller.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/controller.rs b/src/controller.rs index 262e6ca..efa312d 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -222,6 +222,7 @@ async fn reconcile(resource_sync: Arc, ctx: Arc) -> Resul if resource_sync.has_force_delete_option_enabled() && resource_sync.has_been_deleted() => { + debug!(?name, "force-deleting ResourceSync"); return stop_watches_and_remove_resource_sync_finalizers( resource_sync, &name, From 52e28257b03486e7a559c7c75c819e86de2ed835 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Tue, 12 Nov 2024 12:48:26 -0600 Subject: [PATCH 4/5] fix: status handling --- src/controller.rs | 87 +++++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 37 deletions(-) diff --git a/src/controller.rs b/src/controller.rs index efa312d..182c8d7 100644 --- a/src/controller.rs +++ b/src/controller.rs @@ -207,43 +207,13 @@ async fn reconcile(resource_sync: Arc, ctx: Arc) -> Resul .ok_or(Error::NameRequired)?; let parent_api = resource_sync.api(ctx.client.clone()); - let result = { - let resource_sync = Arc::clone(&resource_sync); - - info!(?name, "running reconciler"); - - debug!(?resource_sync.spec, "got"); - let local_ns = resource_sync.namespace().ok_or(Error::NamespaceRequired)?; - - let (source_api, target_api) = - match source_and_target_apis(&resource_sync, &ctx, local_ns).await { - Ok(apis) => apis, - Err(_) - if resource_sync.has_force_delete_option_enabled() - && resource_sync.has_been_deleted() => - { - debug!(?name, "force-deleting ResourceSync"); - return stop_watches_and_remove_resource_sync_finalizers( - resource_sync, - &name, - &parent_api, - ctx, - ) - .await; - } - Err(err) => return Err(err), - }; - - match resource_sync { - resource_sync if resource_sync.has_been_deleted() => { - reconcile_deleted_resource(resource_sync, &name, target_api, &parent_api, ctx).await - } - resource_sync if !resource_sync.has_target_finalizer() => { - add_target_finalizer(resource_sync, &name, &parent_api).await - } - _ => reconcile_normally(resource_sync, &name, source_api, target_api, ctx).await, - } - }; + let result = reconcile_helper( + Arc::clone(&resource_sync), + Arc::clone(&ctx), + &name, + &parent_api, + ) + .await; let status = match &result { Err(err) => { @@ -276,6 +246,49 @@ async fn reconcile(resource_sync: Arc, ctx: Arc) -> Resul result } +async fn reconcile_helper( + resource_sync: Arc, + ctx: Arc, + name: &String, + parent_api: &Api, +) -> Result { + let resource_sync = Arc::clone(&resource_sync); + + info!(?name, "running reconciler"); + + debug!(?resource_sync.spec, "got"); + let local_ns = resource_sync.namespace().ok_or(Error::NamespaceRequired)?; + + let (source_api, target_api) = + match source_and_target_apis(&resource_sync, &ctx, local_ns).await { + Ok(apis) => apis, + Err(_) + if resource_sync.has_force_delete_option_enabled() + && resource_sync.has_been_deleted() => + { + debug!(?name, "force-deleting ResourceSync"); + return stop_watches_and_remove_resource_sync_finalizers( + resource_sync, + name, + parent_api, + ctx, + ) + .await; + } + Err(err) => return Err(err), + }; + + match resource_sync { + resource_sync if resource_sync.has_been_deleted() => { + reconcile_deleted_resource(resource_sync, name, target_api, parent_api, ctx).await + } + resource_sync if !resource_sync.has_target_finalizer() => { + add_target_finalizer(resource_sync, name, parent_api).await + } + _ => reconcile_normally(resource_sync, name, source_api, target_api, ctx).await, + } +} + async fn source_and_target_apis( resource_sync: &Arc, ctx: &Arc, From a8bd7fd88ebb73db1b5c33623e6e98df441691c2 Mon Sep 17 00:00:00 2001 From: Zach Robinson Date: Tue, 12 Nov 2024 13:11:24 -0600 Subject: [PATCH 5/5] chore: cleanup tests slightly --- src/resources.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/resources.rs b/src/resources.rs index de8b8a3..632d08a 100644 --- a/src/resources.rs +++ b/src/resources.rs @@ -174,13 +174,13 @@ mod tests { ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::new()), ..Default::default()},spec: Default::default(),status: None,}, false )] #[case::force_delete_annotation_is_false( - ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "false".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, false + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), false.to_string())])), ..Default::default()},spec: Default::default(),status: None,}, false )] #[case::force_delete_annotation_is_other( ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "other".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, false )] #[case::force_delete_annotation_is_true( - ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), "true".to_string())])), ..Default::default()},spec: Default::default(),status: None,}, true + ResourceSync{metadata: ObjectMeta{annotations: Some(BTreeMap::from([(FORCE_DELETE_ANNOTATION.to_string(), true.to_string())])), ..Default::default()},spec: Default::default(),status: None,}, true )] #[tokio::test] async fn test_resource_sync_has_force_delete_option_enabled(