fix: Reset ResourceSyncFailing condition to False on successful reconcile

src/controller.rs

@@ -27,6 +27,7 @@ use crate::resources::ResourceSyncStatus;
 use crate::{requeue_after, resources::ResourceSync, util, Error, Result, FINALIZER};
 
 const RESOURCE_SYNC_FAILING_CONDITION: &str = "ResourceSyncFailing";
+const RESOURCE_SYNC_SUCCEEDED_REASON: &str = "ResourceSyncSucceeded";
 
 pub struct Context {
     pub client: Client,
@@ -234,23 +235,7 @@ async fn reconcile(resource_sync: Arc<ResourceSync>, ctx: Arc<Context>) -> Resul
     )
     .await;
 
-    let status = match &result {
-        Err(err) => {
-            let sync_failing_condition = Condition {
-                last_transition_time: sync_failing_transition_time(&(resource_sync.status)),
-                message: err.to_string(),
-                observed_generation: resource_sync.metadata.generation,
-                reason: RESOURCE_SYNC_FAILING_CONDITION.to_string(),
-                status: "True".to_string(),
-                type_: RESOURCE_SYNC_FAILING_CONDITION.to_string(),
-            };
-
-            Some(ResourceSyncStatus {
-                conditions: Some(vec![sync_failing_condition]),
-            })
-        }
-        _ => None,
-    };
+    let status = reconcile_status(&resource_sync, &result);
 
     if status != resource_sync.status {
         parent_api
@@ -327,25 +312,67 @@ async fn source_and_target_apis(
     Ok((source_api, target_api))
 }
 
-fn sync_failing_transition_time(status: &Option<ResourceSyncStatus>) -> Time {
-    let now = Time(Utc::now());
+fn reconcile_status(
+    resource_sync: &ResourceSync,
+    result: &Result<Action>,
+) -> Option<ResourceSyncStatus> {
+    match result {
+        Err(err) => Some(ResourceSyncStatus {
+            conditions: Some(vec![sync_failing_condition(
+                resource_sync,
+                "True",
+                RESOURCE_SYNC_FAILING_CONDITION,
+                err.to_string(),
+            )]),
+        }),
+        // A successful reconcile must reset the condition to False rather than leave the last
+        // failure latched. Skip this for deleted resources; their finalizer may already be gone.
+        Ok(_) if !resource_sync.has_been_deleted() => Some(ResourceSyncStatus {
+            conditions: Some(vec![sync_failing_condition(
+                resource_sync,
+                "False",
+                RESOURCE_SYNC_SUCCEEDED_REASON,
+                "Sync succeeded".to_string(),
+            )]),
+        }),
+        Ok(_) => resource_sync.status.clone(),
+    }
+}
 
-    match status {
-        None => now,
-        Some(status) => match &status.conditions {
-            None => now,
-            Some(conditions) => {
-                let sync_failing_condition = conditions
-                    .iter()
-                    .find(|c| c.type_ == RESOURCE_SYNC_FAILING_CONDITION);
-                sync_failing_condition
-                    .map(|c| c.last_transition_time.clone())
-                    .unwrap_or(now)
-            }
-        },
+fn sync_failing_condition(
+    resource_sync: &ResourceSync,
+    status: &str,
+    reason: &str,
+    message: String,
+) -> Condition {
+    Condition {
+        last_transition_time: sync_failing_transition_time(&resource_sync.status, status),
+        message,
+        observed_generation: resource_sync.metadata.generation,
+        reason: reason.to_string(),
+        status: status.to_string(),
+        type_: RESOURCE_SYNC_FAILING_CONDITION.to_string(),
     }
 }
 
+// The transition time is only carried over while the condition value is unchanged; a True<->False
+// flip records a new transition.
+fn sync_failing_transition_time(status: &Option<ResourceSyncStatus>, new_status: &str) -> Time {
+    let now = Time(Utc::now());
+
+    status
+        .as_ref()
+        .and_then(|status| status.conditions.as_ref())
+        .and_then(|conditions| {
+            conditions
+                .iter()
+                .find(|c| c.type_ == RESOURCE_SYNC_FAILING_CONDITION)
+        })
+        .filter(|c| c.status == new_status)
+        .map(|c| c.last_transition_time.clone())
+        .unwrap_or(now)
+}
+
 // TODO: Exponential Backoff using DefaultBackoff for watcher
 fn error_policy(resource_sync: Arc<ResourceSync>, error: &Error, _ctx: Arc<Context>) -> Action {
     let name = resource_sync.name_any();
@@ -391,48 +418,103 @@ pub async fn run(client: Client) -> Result<()> {
 
 #[cfg(test)]
 mod tests {
-    use super::{sync_failing_transition_time, RESOURCE_SYNC_FAILING_CONDITION};
-    use crate::resources::ResourceSyncStatus;
+    use super::{
+        reconcile_status, sync_failing_transition_time, RESOURCE_SYNC_FAILING_CONDITION,
+        RESOURCE_SYNC_SUCCEEDED_REASON,
+    };
+    use crate::resources::{ResourceSync, ResourceSyncStatus};
+    use crate::{Error, Result};
     use chrono::{TimeDelta, TimeZone};
     use k8s_openapi::apimachinery::pkg::apis::meta::v1::Condition;
+    use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta;
     use k8s_openapi::apimachinery::pkg::apis::meta::v1::Time;
+    use kube::runtime::controller::Action;
     use once_cell::sync::Lazy;
     use rstest::rstest;
 
     static NOW: Lazy<Time> = Lazy::new(|| Time(chrono::Utc::now()));
     static EPOCH: Lazy<Time> = Lazy::new(|| Time(chrono::Utc.timestamp_opt(0, 0).unwrap()));
 
+    fn status_with_condition(status: &str) -> Option<ResourceSyncStatus> {
+        Some(ResourceSyncStatus {
+            conditions: Some(vec![Condition {
+                last_transition_time: EPOCH.clone(),
+                type_: RESOURCE_SYNC_FAILING_CONDITION.to_string(),
+                message: "".to_string(),
+                reason: "".to_string(),
+                observed_generation: None,
+                status: status.to_string(),
+            }]),
+        })
+    }
+
     #[rstest]
-    #[case::none(None, &NOW)]
-    #[case::no_conditions(Some(ResourceSyncStatus::default()), &NOW)]
-    #[case::empty_conditions(Some(ResourceSyncStatus{conditions: Some(vec![])}), &NOW)]
-    #[case::condition_already_present(
-        Some(
-            ResourceSyncStatus{
-                conditions: Some(
-                    vec![
-                        Condition{
-                            last_transition_time: EPOCH.clone(),
-                            type_: RESOURCE_SYNC_FAILING_CONDITION.to_string(),
-                            message: "".to_string(),
-                            reason: "".to_string(),
-                            observed_generation: None,
-                            status: "".to_string()
-                        }
-                    ]
-                )
-            }
-        ),
-        &NOW
-    )]
+    #[case::none(None, "True", &NOW)]
+    #[case::no_conditions(Some(ResourceSyncStatus::default()), "True", &NOW)]
+    #[case::empty_conditions(Some(ResourceSyncStatus{conditions: Some(vec![])}), "True", &NOW)]
+    #[case::still_failing_keeps_time(status_with_condition("True"), "True", &EPOCH)]
+    #[case::still_succeeding_keeps_time(status_with_condition("False"), "False", &EPOCH)]
+    #[case::failure_after_success_transitions(status_with_condition("False"), "True", &NOW)]
+    #[case::success_after_failure_transitions(status_with_condition("True"), "False", &NOW)]
     #[tokio::test]
     async fn test_sync_failing_transition_time(
         #[case] status: Option<ResourceSyncStatus>,
+        #[case] new_status: &str,
         #[case] expected: &Time,
     ) {
-        let result = sync_failing_transition_time(&status);
-        let diff = result.0 - expected.0;
+        let result = sync_failing_transition_time(&status, new_status);
+        let diff = (result.0 - expected.0).abs();
 
         assert!(diff.le(&TimeDelta::minutes(1)))
     }
+
+    fn resource_sync(deleted: bool, status: Option<ResourceSyncStatus>) -> ResourceSync {
+        ResourceSync {
+            metadata: ObjectMeta {
+                deletion_timestamp: deleted.then(|| EPOCH.clone()),
+                ..Default::default()
+            },
+            spec: Default::default(),
+            status,
+        }
+    }
+
+    fn single_condition(status: Option<ResourceSyncStatus>) -> Condition {
+        let conditions = status.unwrap().conditions.unwrap();
+        assert_eq!(conditions.len(), 1);
+        conditions.into_iter().next().unwrap()
+    }
+
+    #[test]
+    fn test_reconcile_status_err_sets_failing_true() {
+        let rs = resource_sync(false, None);
+        let result: Result<Action> = Err(Error::NameRequired);
+
+        let condition = single_condition(reconcile_status(&rs, &result));
+
+        assert_eq!(condition.type_, RESOURCE_SYNC_FAILING_CONDITION);
+        assert_eq!(condition.status, "True");
+        assert_eq!(condition.reason, RESOURCE_SYNC_FAILING_CONDITION);
+        assert_eq!(condition.message, Error::NameRequired.to_string());
+    }
+
+    #[test]
+    fn test_reconcile_status_ok_resets_failing_to_false() {
+        let rs = resource_sync(false, status_with_condition("True"));
+        let result: Result<Action> = Ok(Action::await_change());
+
+        let condition = single_condition(reconcile_status(&rs, &result));
+
+        assert_eq!(condition.type_, RESOURCE_SYNC_FAILING_CONDITION);
+        assert_eq!(condition.status, "False");
+        assert_eq!(condition.reason, RESOURCE_SYNC_SUCCEEDED_REASON);
+    }
+
+    #[test]
+    fn test_reconcile_status_ok_deleted_keeps_existing_status() {
+        let rs = resource_sync(true, status_with_condition("True"));
+        let result: Result<Action> = Ok(Action::await_change());
+
+        assert_eq!(reconcile_status(&rs, &result), rs.status);
+    }
 }