Perform ResourceDiff compare before patch (#317)

* Fmt fixes

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

* Implement custom resource comparison logic

Introduces a `ResourceDiff` trait and implements it for various Fleet API resources. This allows controllers to perform more granular comparisons between the desired state and the current state of resources, avoiding unnecessary updates and reconciliations.

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

* Improve readability of the wait conditions individually

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

---------

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

Author: Danil-Grigorev

Cargo.lock

@@ -63,6 +63,7 @@ fleet-api-rs = "0.12.3"
 async-broadcast = "0.7.2"
 pin-project = "1.1.10"
 async-stream = "0.3.6"
+educe = { version = "0.6.0", features = ["PartialEq"] }
 
 [dev-dependencies]
 assert-json-diff = "2.0.2"

Cargo.toml

@@ -45,7 +45,7 @@ test-unit:
   cargo test
 
 # run clippy
-clippy:
+clippy: fmt
   cargo clippy --all-targets --all-features --fix --allow-dirty -- -W clippy::pedantic
 
 # compile for musl (for docker image)
@@ -94,7 +94,7 @@ start-dev: _cleanup-out-dir _create-out-dir _download-kubectl
     kind delete cluster --name dev || true
     kind create cluster --image=kindest/node:v{{KUBE_VERSION}} --config testdata/kind-config.yaml
     just install-capi
-    kubectl wait pods --for=condition=Ready --timeout=300s --all --all-namespaces
+    kubectl wait pods --for=condition=Ready --timeout=500s --all --all-namespaces
 
 # Stop the local dev environment
 stop-dev:
@@ -171,13 +171,13 @@ release-manifests: _create-out-dir _download-kustomize
 test-import: start-dev deploy deploy-child-cluster deploy-kindnet deploy-app && collect-test-import
     kubectl wait pods --for=condition=Ready --timeout=150s --all --all-namespaces
     kubectl wait cluster --timeout=500s --for=condition=ControlPlaneReady=true docker-demo
-    kubectl wait clusters.fleet.cattle.io --timeout=300s --for=condition=Ready=true docker-demo
+    kubectl wait clusters.fleet.cattle.io --timeout=500s --for=condition=Ready=true docker-demo
 
 # Full e2e test of importing cluster in fleet
 test-import-rke2: start-dev deploy deploy-child-rke2-cluster deploy-calico-gitrepo deploy-app
     kubectl wait pods --for=condition=Ready --timeout=150s --all --all-namespaces
     kubectl wait cluster --timeout=500s --for=condition=ControlPlaneReady=true docker-demo
-    kubectl wait clusters.fleet.cattle.io --timeout=300s --for=condition=Ready=true docker-demo
+    kubectl wait clusters.fleet.cattle.io --timeout=500s --for=condition=Ready=true docker-demo
 
 collect-test-import:
     -just collect-artifacts dev
@@ -206,11 +206,15 @@ collect-artifacts cluster:
 # Full e2e test of importing cluster and ClusterClass in fleet
 [private]
 _test-import-all:
-    kubectl wait clustergroups.fleet.cattle.io -n clusterclass --timeout=300s --for=create --for=condition=Ready=true quick-start
+    kubectl wait clustergroups.fleet.cattle.io -n clusterclass --timeout=500s --for=create quick-start
+    kubectl wait clustergroups.fleet.cattle.io -n clusterclass --timeout=500s --for=condition=Ready=true quick-start
     # Verify that cluster group created for cluster referencing clusterclass in a different namespace
-    kubectl wait bundlenamespacemappings.fleet.cattle.io --timeout=300s --for=create -n clusterclass default
-    kubectl wait clustergroups.fleet.cattle.io --timeout=300s --for=create --for=jsonpath='{.status.clusterCount}=1' --for=condition=Ready=true quick-start.clusterclass
-    kubectl wait clusters.fleet.cattle.io --timeout=300s --for=create --for=condition=Ready=true capi-quickstart
+    kubectl wait bundlenamespacemappings.fleet.cattle.io --timeout=500s --for=create -n clusterclass default
+    kubectl wait clustergroups.fleet.cattle.io --timeout=500s --for=create quick-start.clusterclass
+    kubectl wait clustergroups.fleet.cattle.io --timeout=500s --for=jsonpath='{.status.clusterCount}=1' quick-start.clusterclass
+    kubectl wait clustergroups.fleet.cattle.io --timeout=500s --for=condition=Ready=true quick-start.clusterclass
+    kubectl wait clusters.fleet.cattle.io --timeout=500s --for=create capi-quickstart
+    kubectl wait clusters.fleet.cattle.io --timeout=500s --for=condition=Ready=true capi-quickstart
 
 [private]
 _test-delete-all:

justfile

@@ -2,11 +2,13 @@ use fleet_api_rs::fleet_bundle_namespace_mapping::{
     BundleNamespaceMappingBundleSelector, BundleNamespaceMappingNamespaceSelector,
 };
 use kube::{
-    api::{ObjectMeta, TypeMeta},
     Resource,
+    api::{ObjectMeta, TypeMeta},
 };
 use serde::{Deserialize, Serialize};
 
+use crate::api::comparable::ResourceDiff;
+
 mod mapping {
     use kube::CustomResource;
     use schemars::JsonSchema;
@@ -32,3 +34,9 @@ pub struct BundleNamespaceMapping {
     pub bundle_selector: BundleNamespaceMappingBundleSelector,
     pub namespace_selector: BundleNamespaceMappingNamespaceSelector,
 }
+
+impl ResourceDiff for BundleNamespaceMapping {
+    fn diff(&self, other: &Self) -> bool {
+        self != other
+    }
+}

src/api/bundle_namespace_mapping.rs

@@ -8,8 +8,8 @@ use fleet_api_rs::{
     fleet_clustergroup::{ClusterGroupSelector, ClusterGroupSpec},
 };
 use kube::{
-    api::{ObjectMeta, TypeMeta},
     CustomResource, Resource, ResourceExt as _,
+    api::{ObjectMeta, TypeMeta},
 };
 #[cfg(feature = "agent-initiated")]
 use rand::distr::{Alphanumeric, SampleString as _};
@@ -20,7 +20,7 @@ use super::{
     bundle_namespace_mapping::BundleNamespaceMapping,
     fleet_addon_config::ClusterConfig,
     fleet_cluster,
-    fleet_clustergroup::{ClusterGroup, CLUSTER_CLASS_LABEL, CLUSTER_CLASS_NAMESPACE_LABEL},
+    fleet_clustergroup::{CLUSTER_CLASS_LABEL, CLUSTER_CLASS_NAMESPACE_LABEL, ClusterGroup},
 };
 
 #[cfg(feature = "agent-initiated")]

src/api/capi_cluster.rs

@@ -0,0 +1,6 @@
+// Trait for resources that can be compared
+pub(crate) trait ResourceDiff: kube::ResourceExt {
+    fn diff(&self, other: &Self) -> bool {
+        self.meta() != other.meta()
+    }
+}

src/api/comparable.rs

@@ -1,26 +1,28 @@
 use std::{fmt::Display, str::FromStr};
 
+use crate::api::comparable::ResourceDiff;
+use educe::Educe;
 use fleet_api_rs::fleet_cluster::{ClusterAgentEnvVars, ClusterAgentTolerations};
 use k8s_openapi::{
     api::core::v1::{ConfigMap, ObjectReference},
     apimachinery::pkg::apis::meta::v1::{Condition, LabelSelector},
 };
 use kube::{
+    CustomResource, KubeSchema, Resource,
     api::{ObjectMeta, TypeMeta},
     core::{ParseExpressionError, Selector},
-    CustomResource, KubeSchema, Resource,
 };
 use schemars::JsonSchema;
-use serde::{ser, Deserialize, Serialize};
-use serde_with::{serde_as, DisplayFromStr};
+use serde::{Deserialize, Serialize, ser};
+use serde_with::{DisplayFromStr, serde_as};
 use serde_yaml::Value;
 
 pub const AGENT_NAMESPACE: &str = "fleet-addon-agent";
 pub const EXPERIMENTAL_OCI_STORAGE: &str = "EXPERIMENTAL_OCI_STORAGE";
 pub const EXPERIMENTAL_HELM_OPS: &str = "EXPERIMENTAL_HELM_OPS";
 
 /// This provides a config for fleet addon functionality
-#[derive(CustomResource, Deserialize, Serialize, Clone, Default, Debug, KubeSchema)]
+#[derive(CustomResource, Deserialize, Serialize, Clone, Default, Debug, KubeSchema, PartialEq)]
 #[kube(
     kind = "FleetAddonConfig",
     group = "addons.cluster.x-k8s.io",
@@ -63,6 +65,11 @@ impl Default for FleetAddonConfig {
         }
     }
 }
+impl ResourceDiff for FleetAddonConfig {
+    fn diff(&self, _: &Self) -> bool {
+        true
+    }
+}
 
 #[derive(Deserialize, Serialize, Clone, Default, Debug, JsonSchema)]
 #[serde(rename_all = "camelCase")]
@@ -73,7 +80,7 @@ pub struct FleetAddonConfigStatus {
     pub conditions: Vec<Condition>,
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema)]
+#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct ClusterClassConfig {
     /// Setting to disable setting owner references on the created resources
@@ -95,7 +102,7 @@ impl Default for ClusterClassConfig {
     }
 }
 
-#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema)]
+#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct ClusterConfig {
     /// Apply a `ClusterGroup` for a `ClusterClass` referenced from a different namespace.
@@ -152,13 +159,21 @@ pub struct FleetSettings {
     pub data: Option<FleetSettingsSpec>,
 }
 
+impl ResourceDiff for FleetSettings {
+    fn diff(&self, other: &Self) -> bool {
+        self.data != other.data
+    }
+}
+
 #[serde_as]
-#[derive(Serialize, Deserialize, Default, Clone, Debug)]
+#[derive(Serialize, Deserialize, Default, Clone, Debug, Educe)]
+#[educe(PartialEq)]
 pub struct FleetSettingsSpec {
     #[serde(default)]
     #[serde_as(as = "DisplayFromStr")]
     pub fleet: FleetChartValues,
 
+    #[educe(PartialEq(ignore))]
     #[serde(flatten)]
     pub other: Value,
 }
@@ -238,7 +253,7 @@ impl ClusterConfig {
 }
 
 /// `NamingStrategy` is controlling Fleet cluster naming
-#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, Default)]
+#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, Default, PartialEq)]
 pub struct NamingStrategy {
     /// Specify a prefix for the Cluster name, applied to created Fleet cluster
     pub prefix: Option<String>,
@@ -264,7 +279,7 @@ impl Default for ClusterConfig {
     }
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct FleetConfig {
     /// fleet server url configuration options
@@ -290,7 +305,7 @@ impl Default for FleetConfig {
 
 /// Feature toggles for enabling or disabling experimental functionality.
 /// This struct controls access to specific experimental features.
-#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct FeatureGates {
     /// Enables experimental OCI  storage support.
@@ -360,7 +375,7 @@ impl Default for FeatureGates {
 
 /// `FeaturesConfigMap` references a `ConfigMap` where to apply feature flags.
 /// If a `ConfigMap` is referenced, the controller will update it instead of upgrading the Fleet chart.
-#[derive(Clone, Default, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Default, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct FeaturesConfigMap {
     // The reference to a ConfigMap resource
@@ -369,7 +384,7 @@ pub struct FeaturesConfigMap {
 }
 
 /// `FleetChartValues` represents Fleet chart values.
-#[derive(Clone, Default, Debug, Serialize, Deserialize)]
+#[derive(Clone, Default, Debug, Serialize, Deserialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct FleetChartValues {
     pub extra_env: Option<Vec<EnvironmentVariable>>,
@@ -378,14 +393,14 @@ pub struct FleetChartValues {
 }
 
 /// `EnvironmentVariable` is a simple name/value pair.
-#[derive(Clone, Default, Debug, Serialize, Deserialize)]
+#[derive(Clone, Default, Debug, Serialize, Deserialize, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct EnvironmentVariable {
     pub name: String,
     pub value: String,
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct FleetInstall {
     /// Chart version to install
@@ -421,14 +436,14 @@ impl Default for Install {
     }
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub enum Server {
     InferLocal(bool),
     Custom(InstallOptions),
 }
 
-#[derive(Clone, Default, Debug, Serialize, Deserialize, JsonSchema)]
+#[derive(Clone, Default, Debug, Serialize, Deserialize, JsonSchema, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct InstallOptions {
     pub api_server_ca_config_ref: Option<ObjectReference>,
@@ -450,7 +465,7 @@ impl NamingStrategy {
 }
 
 /// Selectors is controlling Fleet import strategy settings.
-#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, Default)]
+#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, Default, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct Selectors {
     /// Namespace label selector. If set, only clusters in the namespace matching label selector will be imported.

src/api/fleet_addon_config.rs

@@ -1,10 +1,13 @@
 use fleet_api_rs::fleet_cluster::{ClusterSpec, ClusterStatus};
 use kube::{
+    Resource, ResourceExt,
     api::{ObjectMeta, TypeMeta},
-    Resource,
 };
 use serde::{Deserialize, Serialize};
 
+use crate::api::comparable::ResourceDiff;
+use std::collections::HashSet;
+
 #[derive(Resource, Serialize, Deserialize, Clone, Debug, Default, PartialEq)]
 #[resource(inherit = fleet_api_rs::fleet_cluster::Cluster)]
 pub struct Cluster {
@@ -14,3 +17,60 @@ pub struct Cluster {
     pub spec: ClusterSpec,
     pub status: Option<ClusterStatus>,
 }
+
+impl ResourceDiff for Cluster {
+    fn diff(&self, other: &Self) -> bool {
+        // Resource was just created
+        if other.status.is_none() {
+            return true;
+        }
+
+        let template_values_equal = self
+            .spec
+            .template_values
+            .as_ref()
+            .unwrap_or(&std::collections::BTreeMap::new())
+            .iter()
+            .all(|(k, v)| {
+                other
+                    .spec
+                    .template_values
+                    .as_ref()
+                    .unwrap_or(&std::collections::BTreeMap::new())
+                    .get(k)
+                    == Some(v)
+            });
+
+        let spec_equal = template_values_equal
+            && self.spec.agent_namespace == other.spec.agent_namespace
+            && self.spec.host_network == other.spec.host_network
+            && self.spec.agent_env_vars == other.spec.agent_env_vars
+            && self.spec.agent_tolerations == other.spec.agent_tolerations;
+
+        if !spec_equal {
+            return true;
+        }
+
+        let annotations_equal = self
+            .annotations()
+            .iter()
+            .all(|(k, v)| other.annotations().get(k) == Some(v));
+        let labels_equal = self
+            .labels()
+            .iter()
+            .all(|(k, v)| other.labels().get(k) == Some(v));
+
+        let owner_uids: HashSet<String> = other
+            .owner_references()
+            .iter()
+            .map(|r| &r.uid)
+            .cloned()
+            .collect();
+        let owner_references_equal = self
+            .owner_references()
+            .iter()
+            .all(|self_ref| owner_uids.contains(&self_ref.uid));
+
+        !annotations_equal || !labels_equal || !owner_references_equal
+    }
+}

src/api/fleet_cluster.rs

@@ -2,11 +2,13 @@ use fleet_api_rs::fleet_cluster_registration_token::{
     ClusterRegistrationTokenSpec, ClusterRegistrationTokenStatus,
 };
 use kube::{
-    api::{ObjectMeta, TypeMeta},
     Resource,
+    api::{ObjectMeta, TypeMeta},
 };
 use serde::{Deserialize, Serialize};
 
+use crate::api::comparable::ResourceDiff;
+
 #[derive(Resource, Serialize, Deserialize, Clone, Debug, Default, PartialEq)]
 #[resource(inherit = fleet_api_rs::fleet_cluster_registration_token::ClusterRegistrationToken)]
 pub struct ClusterRegistrationToken {
@@ -16,3 +18,9 @@ pub struct ClusterRegistrationToken {
     pub spec: ClusterRegistrationTokenSpec,
     pub status: Option<ClusterRegistrationTokenStatus>,
 }
+
+impl ResourceDiff for ClusterRegistrationToken {
+    fn diff(&self, other: &Self) -> bool {
+        self.spec != other.spec
+    }
+}