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}" + ); + } + } }