fix(bind9instance): converge pod volumes and bind9 mounts on existing Deployments

.claude/CHANGELOG.md

@@ -30,8 +30,22 @@
   operator renders, or the Deployment's pods carry another config hash. A
   render error counts as drift, so the reconcile runs and reports it.
 
+- `src/reconcilers/bind9instance/resources.rs` (`volumes_missing`,
+  `build_volumes_patch`) and `mod.rs`: the Deployment update path now
+  converges pod `volumes` and the bind9 container's `volumeMounts`
+  (`$patch: replace`, so stale entries are removed). It patched only the API
+  container, labels and placement, so a Deployment created before signing
+  was enabled never got the DNSSEC key volume; with `key-directory` now
+  pointing at it, BIND had nowhere to keep keys and the zone went unsigned
+  (seen live after rolling onto the fixes above). Missing volumes or mounts
+  are compared by name and path (the API server adds defaults to the stored
+  object) and count as drift, so the resource step runs.
+
 ### Tests
-- `reconcilers/bind9instance/resources_tests.rs` (`config_rollout`): drift
+- `reconcilers/bind9instance/resources_tests.rs` (`volume_convergence`):
+  enabling signing on an existing Deployment needs an update, an identical
+  one does not; the patch carries the `dnssec-keys` volume and the bind9
+  mount as replace lists. (`config_rollout`): drift
   from stale content, from a missing or older hash, and none when nothing is
   generated; hash
   determinism and sensitivity (a byte moving between files counts),

src/reconcilers/bind9instance/mod.rs

@@ -436,6 +436,23 @@ pub async fn reconcile_bind9instance(ctx: Arc<Context>, instance: Bind9Instance)
             Err(_) => true,
         };
 
+        // Volume drift: the running Deployment lacks a pod volume or bind9
+        // mount this operator renders (the DNSSEC key volume, once signing is
+        // enabled on an existing cluster). Compared by name only, so the RNDC
+        // Secret name passed here does not matter.
+        let volumes_drifted = current_deployment.as_ref().is_some_and(|deployment| {
+            let desired = crate::bind9_resources::build_deployment(
+                &name,
+                &namespace,
+                &instance,
+                cluster.as_ref(),
+                cluster_provider.as_ref(),
+                &format!("{name}-rndc-key"),
+            );
+            resources::volumes_missing(deployment, &desired)
+        });
+        let config_drifted = config_drifted || volumes_drifted;
+
         // Check Secret existence AND rotation status
         let secret_name = format!("{name}-rndc-key");
         let (secret_exists, needs_rotation) = match secret_api.get(&secret_name).await {

src/reconcilers/bind9instance/resources.rs

@@ -1013,6 +1013,75 @@ fn config_hash_of(deployment: &Deployment) -> Option<&str> {
         .map(String::as_str)
 }
 
+/// The bind9 container of a Deployment, if present.
+fn bind9_container(deployment: &Deployment) -> Option<&k8s_openapi::api::core::v1::Container> {
+    deployment
+        .spec
+        .as_ref()?
+        .template
+        .spec
+        .as_ref()?
+        .containers
+        .iter()
+        .find(|c| c.name == crate::constants::CONTAINER_NAME_BIND9)
+}
+
+/// Whether `desired` has a pod volume, or a bind9 volume mount, that
+/// `current` lacks. Compared by name and by (name, mountPath): the API server
+/// adds defaults to the stored object, so whole-value equality would report
+/// a change on every reconcile.
+pub(super) fn volumes_missing(current: &Deployment, desired: &Deployment) -> bool {
+    let volume_names = |d: &Deployment| -> std::collections::BTreeSet<String> {
+        d.spec
+            .as_ref()
+            .and_then(|s| s.template.spec.as_ref())
+            .and_then(|p| p.volumes.as_ref())
+            .map(|v| v.iter().map(|vol| vol.name.clone()).collect())
+            .unwrap_or_default()
+    };
+    let mounts = |d: &Deployment| -> std::collections::BTreeSet<(String, String)> {
+        bind9_container(d)
+            .and_then(|c| c.volume_mounts.as_ref())
+            .map(|m| {
+                m.iter()
+                    .map(|vm| (vm.name.clone(), vm.mount_path.clone()))
+                    .collect()
+            })
+            .unwrap_or_default()
+    };
+    !volume_names(desired).is_subset(&volume_names(current))
+        || !mounts(desired).is_subset(&mounts(current))
+}
+
+/// The pod `volumes` and bind9 `volumeMounts` patch fragments for `desired`,
+/// each a strategic-merge `$patch: replace` list, so a volume or mount bindy
+/// no longer renders is removed rather than left behind.
+///
+/// # Returns
+///
+/// `(volumes, bind9 volume mounts)`, either `null` when `desired` has none.
+fn build_volumes_patch(desired: &Deployment) -> (serde_json::Value, serde_json::Value) {
+    let replace_list = |items: Vec<serde_json::Value>| {
+        let mut list = vec![json!({"$patch": "replace"})];
+        list.extend(items);
+        json!(list)
+    };
+    let volumes = desired
+        .spec
+        .as_ref()
+        .and_then(|s| s.template.spec.as_ref())
+        .and_then(|p| p.volumes.as_ref())
+        .map_or(json!(null), |v| {
+            replace_list(v.iter().map(|vol| json!(vol)).collect())
+        });
+    let mounts = bind9_container(desired)
+        .and_then(|c| c.volume_mounts.as_ref())
+        .map_or(json!(null), |m| {
+            replace_list(m.iter().map(|vm| json!(vm)).collect())
+        });
+    (volumes, mounts)
+}
+
 /// Check if a deployment needs updating by comparing current and desired state.
 ///
 /// Returns true if any of the following have changed:
@@ -1032,6 +1101,13 @@ fn deployment_needs_update(current: &Deployment, desired: &Deployment) -> bool {
         }
     }
 
+    // A volume or bind9 mount bindy now renders (e.g. the DNSSEC key volume
+    // after signing was enabled) is missing from the running Deployment.
+    if volumes_missing(current, desired) {
+        debug!("Pod volumes or bind9 volume mounts changed");
+        return true;
+    }
+
     // Compare desired replicas with current replicas
     let desired_replicas = desired.spec.as_ref().and_then(|s| s.replicas);
     let current_replicas = current.spec.as_ref().and_then(|s| s.replicas);
@@ -1233,10 +1309,14 @@ async fn create_or_update_deployment(
 
     let mut patch_containers = vec![];
 
-    // Add bind9 container name to preserve ordering (strategic merge needs this)
-    patch_containers.push(json!({
-        "name": crate::constants::CONTAINER_NAME_BIND9
-    }));
+    // bind9: name for ordering (strategic merge needs it) and its volume
+    // mounts, so a key volume added after creation reaches the container.
+    let (volumes_patch, bind9_mounts_patch) = build_volumes_patch(&deployment);
+    let mut bind9_patch = json!({ "name": crate::constants::CONTAINER_NAME_BIND9 });
+    if !bind9_mounts_patch.is_null() {
+        bind9_patch["volumeMounts"] = bind9_mounts_patch;
+    }
+    patch_containers.push(bind9_patch);
 
     // Add api container with only the fields we want to update
     if let Some(api) = api_container {
@@ -1293,6 +1373,11 @@ async fn create_or_update_deployment(
         }
     });
 
+    // The pod's volumes, in the same Pod spec fragment as the scheduling fields.
+    if !volumes_patch.is_null() {
+        patch["spec"]["template"]["spec"]["volumes"] = volumes_patch;
+    }
+
     // Merge the scheduling fields into the same Pod spec fragment.
     if let Some(pod_spec) = patch["spec"]["template"]["spec"].as_object_mut() {
         if let Some(placement) = build_placement_patch(&deployment).as_object() {
@@ -1513,6 +1598,13 @@ pub(super) async fn delete_resources(client: &Client, namespace: &str, name: &st
 /// whether a live Deployment gets a scheduling change and how that change is
 /// expressed as a patch, so they need direct coverage — but they are not part
 /// of the production API surface.
+#[cfg(test)]
+pub(super) fn build_volumes_patch_for_test(
+    desired: &Deployment,
+) -> (serde_json::Value, serde_json::Value) {
+    build_volumes_patch(desired)
+}
+
 #[cfg(test)]
 pub(super) fn deployment_needs_update_for_test(current: &Deployment, desired: &Deployment) -> bool {
     deployment_needs_update(current, desired)

src/reconcilers/bind9instance/resources_tests.rs

@@ -1287,4 +1287,153 @@ mod tests {
             );
         }
     }
+
+    // ------------------------------------------------------------------
+    // Volume convergence: a Deployment created before signing was enabled
+    // must gain the DNSSEC key volume when it is. The update path patched
+    // only the API container, labels and placement, so it never did, and
+    // BIND (now pointed at the key directory) had nowhere to keep keys.
+    // ------------------------------------------------------------------
+    mod volume_convergence {
+        use crate::bind9_resources::build_deployment;
+        use crate::crd::{
+            Bind9Cluster, Bind9ClusterCommonSpec, Bind9ClusterSpec, Bind9Config, Bind9Instance,
+            Bind9InstanceSpec, DNSSECConfig, DNSSECSigningConfig, ServerRole,
+        };
+        use crate::reconcilers::bind9instance::resources::{
+            build_volumes_patch_for_test as build_volumes_patch,
+            deployment_needs_update_for_test as deployment_needs_update,
+        };
+        use kube::api::ObjectMeta;
+
+        fn instance() -> Bind9Instance {
+            #[allow(deprecated)]
+            Bind9Instance {
+                metadata: ObjectMeta {
+                    name: Some("my-dns-primary-0".into()),
+                    namespace: Some("dns".into()),
+                    ..Default::default()
+                },
+                spec: Bind9InstanceSpec {
+                    cluster_ref: "my-dns".to_string(),
+                    role: ServerRole::Primary,
+                    replicas: Some(1),
+                    version: Some("9.18".into()),
+                    image: None,
+                    config_map_refs: None,
+                    config: None,
+                    primary_servers: None,
+                    volumes: None,
+                    volume_mounts: None,
+                    rndc_secret_ref: None,
+                    rndc_key: None,
+                    storage: None,
+                    placement: None,
+                    bindcar_config: None,
+                },
+                status: None,
+            }
+        }
+
+        fn cluster(signing: bool) -> Bind9Cluster {
+            let global = signing.then(|| Bind9Config {
+                rate_limit: None,
+                recursion: None,
+                allow_query: None,
+                allow_transfer: None,
+                dnssec: Some(DNSSECConfig {
+                    validation: Some(true),
+                    signing: Some(DNSSECSigningConfig {
+                        enabled: true,
+                        policy: Some("core-dns".into()),
+                        algorithm: None,
+                        ksk_lifetime: None,
+                        zsk_lifetime: None,
+                        nsec3: None,
+                        nsec3_salt: None,
+                        nsec3_iterations: None,
+                        keys_from: None,
+                        auto_generate: None,
+                        export_to_secret: None,
+                    }),
+                }),
+                forwarders: None,
+                listen_on: None,
+                listen_on_v6: None,
+                rndc_secret_ref: None,
+                bindcar_config: None,
+            });
+            Bind9Cluster::new(
+                "my-dns",
+                Bind9ClusterSpec {
+                    common: Bind9ClusterCommonSpec {
+                        version: Some("9.18".into()),
+                        primary: None,
+                        secondary: None,
+                        image: None,
+                        config_map_refs: None,
+                        global,
+                        rndc_secret_refs: None,
+                        acls: None,
+                        volumes: None,
+                        volume_mounts: None,
+                    },
+                },
+            )
+        }
+
+        #[test]
+        fn enabling_signing_on_an_existing_deployment_needs_an_update() {
+            let inst = instance();
+            let current = build_deployment(
+                "my-dns-primary-0",
+                "dns",
+                &inst,
+                Some(&cluster(false)),
+                None,
+                "k",
+            );
+            let desired = build_deployment(
+                "my-dns-primary-0",
+                "dns",
+                &inst,
+                Some(&cluster(true)),
+                None,
+                "k",
+            );
+            assert!(
+                deployment_needs_update(&current, &desired),
+                "a missing DNSSEC key volume must count as a change"
+            );
+            assert!(
+                !deployment_needs_update(&desired, &desired),
+                "an identical Deployment needs nothing"
+            );
+        }
+
+        #[test]
+        fn the_patch_carries_the_key_volume_and_the_bind9_mount() {
+            let desired = build_deployment(
+                "my-dns-primary-0",
+                "dns",
+                &instance(),
+                Some(&cluster(true)),
+                None,
+                "k",
+            );
+            let (volumes, mounts) = build_volumes_patch(&desired);
+            let volumes = volumes.to_string();
+            let mounts = mounts.to_string();
+            assert!(volumes.contains("\"dnssec-keys\""), "volumes: {volumes}");
+            assert!(
+                volumes.contains("\"$patch\":\"replace\""),
+                "volumes replace, so stale ones go: {volumes}"
+            );
+            assert!(mounts.contains("/var/cache/bind/keys"), "mounts: {mounts}");
+            assert!(
+                mounts.contains("\"$patch\":\"replace\""),
+                "mounts: {mounts}"
+            );
+        }
+    }
 }