fix(nvue-client): Don't apply an unchanged config

crates/agent/src/ethernet_virtualization.rs

@@ -231,9 +231,9 @@ impl NvueClientContext {
     }
 
     // Wrap the inner nvue_client's `push_config()` and try to avoid re-applying
-    // a configuration we're already using. Returns Ok(Some(revision_id)) on
-    // a change, Ok(None) if the config was unchanged, and otherwise passes
-    // through errors from the inner client.
+    // a configuration we're already using. Returns Ok(Some(revision_id)) when
+    // a revision was applied, Ok(None) if the config was unchanged, and
+    // otherwise passes through errors from the inner client.
     async fn update_config(
         &mut self,
         config: &NvueConfig,
@@ -245,13 +245,9 @@ impl NvueClientContext {
         {
             Ok(None)
         } else {
-            self.nvue_client
-                .push_config(config)
-                .await
-                .map(|revision_id| {
-                    self.last_applied_hash.replace(new_hash);
-                    Some(revision_id)
-                })
+            let revision_id = self.nvue_client.push_config(config).await?;
+            self.last_applied_hash.replace(new_hash);
+            Ok(revision_id)
         }
     }
 }

crates/nvue-client/src/client.rs

@@ -26,7 +26,9 @@ pub use serde_json::Value as JsonValue;
 
 use crate::config::{NvueConfig, NvueConfigWithHeader, NvueRevision};
 use crate::types::bgp::{BgpNeighbors, BgpVrfInfo};
-use crate::types::revision::{RevisionApplyStatus, RevisionData, RevisionIssueSummary};
+use crate::types::revision::{
+    RevisionApplyStatus, RevisionConfigDiff, RevisionData, RevisionIssueSummary,
+};
 
 /// Repeated NVUE field-selection query parameters.
 ///
@@ -199,6 +201,27 @@ impl NvueClient {
         Ok(nvue_config)
     }
 
+    /// Get the config diff between two NVUE revisions. The order of the
+    /// arguments is significant; NVUE will return the set of changes necessary
+    /// to turn `base_revision` into `target_revision`.
+    pub async fn get_revision_config_diff(
+        &self,
+        base_revision: &str,
+        target_revision: &str,
+    ) -> Result<RevisionConfigDiff, NvueClientError> {
+        let mut request = self.request(Method::GET, "/nvue_v1/")?.build()?;
+        request
+            .url_mut()
+            .query_pairs_mut()
+            .append_pair("diff", base_revision)
+            .append_pair("rev", target_revision)
+            .append_pair("filled", "false");
+
+        let response = self.execute("get_revision_config_diff", request).await?;
+        let diff = response.json().await?;
+        Ok(diff)
+    }
+
     /// Return BGP data for a VRF.
     ///
     /// This calls `GET /nvue_v1/vrf/{vrf-id}/router/bgp` without field filters.
@@ -360,15 +383,34 @@ impl NvueClient {
         }
     }
 
-    /// Create a new configuration using the values from `config`, then  apply
-    /// it, returning the revision ID. This is a convenience method that
-    /// creates, replaces, and then applies the configuration (which a caller
-    /// could do manually if more control is desired).
-    pub async fn push_config(&self, config: &NvueConfig) -> Result<String, NvueClientError> {
+    /// Perform all of the NVUE operations required to apply a new config. Under
+    /// the hood, we create a new revision, patch it with this config, diff it
+    /// against the "applied" revision, and then apply it if it differed.
+    ///
+    /// Returns a `Some(revision_id)` if we applied the new revision, and `None`
+    /// if NVUE indicated no change compared to the "applied" revision.
+    pub async fn push_config(
+        &self,
+        config: &NvueConfig,
+    ) -> Result<Option<String>, NvueClientError> {
         let revision_id = self.create_config_revision().await?;
         self.replace_config_revision(&revision_id, config).await?;
+
+        // NVUE will not actually apply a revision if it's unchanged from what's
+        // currently applied, we need to check this before we try.
+        let diff = self
+            .get_revision_config_diff("applied", &revision_id)
+            .await?;
+        if diff.is_empty() {
+            // Note that we're leaving the old revision sitting around unused!
+            // NVUE provides a `DELETE` operation on a revision ID, but HBN
+            // patches this out in its OpenAPI spec For Some Reason(tm) so it's
+            // unclear if it's safe to try it.
+            return Ok(None);
+        }
+
         self.apply_config_revision(&revision_id).await?;
-        Ok(revision_id)
+        Ok(Some(revision_id))
     }
 
     // Retrieve the system information from the NVUE server. The fields returned

crates/nvue-client/src/types/revision.rs

@@ -2,6 +2,23 @@ use std::collections::BTreeMap;
 
 use serde_json::Value as JsonValue;
 
+/// The configuration difference returned by NVUE's config diff operation.
+#[derive(Clone, Debug, serde::Deserialize)]
+#[serde(transparent)]
+pub struct RevisionConfigDiff(
+    // We're intentionally typing this fairly loosely for code brevity, since the
+    // sole user of this is concerned only with whether the diff is empty.
+    serde_json::Map<String, serde_json::Value>,
+);
+
+impl RevisionConfigDiff {
+    /// Return `true` for an empty diff, which means a no-op change between the
+    /// revisions.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
 #[derive(Clone, Debug, serde::Deserialize)]
 #[serde(rename_all = "kebab-case")]
 pub struct RevisionData {
@@ -97,6 +114,74 @@ pub enum RevisionIssueSeverity {
 mod tests {
     use super::*;
 
+    #[test]
+    fn revision_config_diff_requires_object_root_and_reports_empty_root() {
+        struct Case {
+            name: &'static str,
+            json: &'static str,
+            expected: Option<(bool, JsonValue)>,
+        }
+
+        let cases = [
+            Case {
+                name: "empty root object means no difference",
+                json: r#"{}"#,
+                expected: Some((true, serde_json::json!({}))),
+            },
+            Case {
+                name: "nested null remains a significant removal",
+                json: r#"{"interface":{"swp1":{"link":{"state":null}}}}"#,
+                expected: Some((
+                    false,
+                    serde_json::json!({
+                        "interface": {
+                            "swp1": {
+                                "link": {
+                                    "state": null
+                                }
+                            }
+                        }
+                    }),
+                )),
+            },
+            Case {
+                name: "null root is invalid",
+                json: "null",
+                expected: None,
+            },
+            Case {
+                name: "array root is invalid",
+                json: "[]",
+                expected: None,
+            },
+            Case {
+                name: "scalar root is invalid",
+                json: r#""value""#,
+                expected: None,
+            },
+        ];
+
+        for case in cases {
+            match (
+                serde_json::from_str::<RevisionConfigDiff>(case.json),
+                case.expected,
+            ) {
+                (Ok(diff), Some((expected_empty, expected_value))) => {
+                    assert_eq!(diff.is_empty(), expected_empty, "{}", case.name);
+                    assert_eq!(JsonValue::Object(diff.0), expected_value, "{}", case.name);
+                }
+                (Err(_), None) => {}
+                (Ok(_), None) => panic!("{}: expected deserialization to fail", case.name),
+                (Err(error), Some(_)) => {
+                    panic!(
+                        "{}: expected deserialization to succeed: {error}",
+                        case.name
+                    )
+                }
+            }
+        }
+    }
+
     #[test]
     fn classifies_revision_apply_status() {
         struct Case {