From 78d015173c1a8f321e9f22222f46a278203e9481 Mon Sep 17 00:00:00 2001 From: Erick Bourgeois Date: Sat, 3 Oct 2026 18:25:28 -0400 Subject: [PATCH] fix(bind9instance): converge pod volumes and bind9 mounts on existing Deployments The Deployment update path patched only the API container, labels and placement. A Deployment created before DNSSEC signing was enabled therefore never got the dnssec-keys volume, and with key-directory now pointing at it (#518), BIND had nowhere to keep keys and zones stayed unsigned. Seen live after rolling onto #518 and #520. - volumes_missing(): detects a pod volume or bind9 volume mount that the operator renders but the running Deployment lacks. It compares by name and by (name, mountPath), because the API server adds defaults to the stored object. - The reconcile treats missing volumes as drift, so the resource step runs. - build_volumes_patch(): the update patch carries pod volumes and the bind9 container's volumeMounts as strategic-merge `$patch: replace` lists, so stale entries are removed instead of left behind. Tests (volume_convergence): enabling signing on an existing Deployment needs an update and an identical one does not; the patch carries the dnssec-keys volume and the bind9 mount as replace lists. Signed-off-by: Erick Bourgeois --- .claude/CHANGELOG.md | 16 +- src/reconcilers/bind9instance/mod.rs | 17 ++ src/reconcilers/bind9instance/resources.rs | 100 +++++++++++- .../bind9instance/resources_tests.rs | 149 ++++++++++++++++++ 4 files changed, 277 insertions(+), 5 deletions(-) diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index 022eca6a..0cf10265 100644 --- a/.claude/CHANGELOG.md +++ b/.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), diff --git a/src/reconcilers/bind9instance/mod.rs b/src/reconcilers/bind9instance/mod.rs index c85b0e53..d4e1cdcb 100644 --- a/src/reconcilers/bind9instance/mod.rs +++ b/src/reconcilers/bind9instance/mod.rs @@ -436,6 +436,23 @@ pub async fn reconcile_bind9instance(ctx: Arc, 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 { diff --git a/src/reconcilers/bind9instance/resources.rs b/src/reconcilers/bind9instance/resources.rs index 10160cd7..c4c2e650 100644 --- a/src/reconcilers/bind9instance/resources.rs +++ b/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 { + 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| { + 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) diff --git a/src/reconcilers/bind9instance/resources_tests.rs b/src/reconcilers/bind9instance/resources_tests.rs index c2d2dab4..cd62d093 100644 --- a/src/reconcilers/bind9instance/resources_tests.rs +++ b/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(¤t, &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}" + ); + } + } }