From 124d17c25b73ba564887a991d3443646660a0e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=BCller?= Date: Thu, 13 Aug 2026 20:38:20 +0200 Subject: [PATCH 1/2] chore: reproduce unnecessary statefulset restarts --- .../00-patch-ns.yaml.j2 | 9 +++++ .../10-assert.yaml | 14 +++++++ .../10-statefulset.yaml | 38 +++++++++++++++++++ .../20-assert.yaml | 23 +++++++++++ .../20-content-identical-write.yaml | 19 ++++++++++ tests/test-definition.yaml | 3 ++ 6 files changed, 106 insertions(+) create mode 100644 tests/templates/kuttl/restarter-content-identical-write/00-patch-ns.yaml.j2 create mode 100644 tests/templates/kuttl/restarter-content-identical-write/10-assert.yaml create mode 100644 tests/templates/kuttl/restarter-content-identical-write/10-statefulset.yaml create mode 100644 tests/templates/kuttl/restarter-content-identical-write/20-assert.yaml create mode 100644 tests/templates/kuttl/restarter-content-identical-write/20-content-identical-write.yaml diff --git a/tests/templates/kuttl/restarter-content-identical-write/00-patch-ns.yaml.j2 b/tests/templates/kuttl/restarter-content-identical-write/00-patch-ns.yaml.j2 new file mode 100644 index 00000000..67185acf --- /dev/null +++ b/tests/templates/kuttl/restarter-content-identical-write/00-patch-ns.yaml.j2 @@ -0,0 +1,9 @@ +{% if test_scenario['values']['openshift'] == 'true' %} +# see https://github.com/stackabletech/issues/issues/566 +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: kubectl patch namespace $NAMESPACE -p '{"metadata":{"labels":{"pod-security.kubernetes.io/enforce":"privileged"}}}' + timeout: 120 +{% endif %} diff --git a/tests/templates/kuttl/restarter-content-identical-write/10-assert.yaml b/tests/templates/kuttl/restarter-content-identical-write/10-assert.yaml new file mode 100644 index 00000000..9941c1f5 --- /dev/null +++ b/tests/templates/kuttl/restarter-content-identical-write/10-assert.yaml @@ -0,0 +1,14 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 300 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: test + generation: 1 +status: + readyReplicas: 2 + replicas: 2 + observedGeneration: 1 diff --git a/tests/templates/kuttl/restarter-content-identical-write/10-statefulset.yaml b/tests/templates/kuttl/restarter-content-identical-write/10-statefulset.yaml new file mode 100644 index 00000000..453532be --- /dev/null +++ b/tests/templates/kuttl/restarter-content-identical-write/10-statefulset.yaml @@ -0,0 +1,38 @@ +--- +# A minimal StatefulSet opted in to the restarter, mounting a ConfigMap. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-config +data: + foo: bar +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: test + labels: + restarter.stackable.tech/enabled: "true" +spec: + selector: + matchLabels: + app: test + serviceName: test + replicas: 2 + template: + metadata: + labels: + app: test + spec: + terminationGracePeriodSeconds: 1 + containers: + - name: nginx + image: nginxinc/nginx-unprivileged:1.27 + volumeMounts: + - name: config + mountPath: /config + volumes: + - name: config + configMap: + name: test-config diff --git a/tests/templates/kuttl/restarter-content-identical-write/20-assert.yaml b/tests/templates/kuttl/restarter-content-identical-write/20-assert.yaml new file mode 100644 index 00000000..dbd23104 --- /dev/null +++ b/tests/templates/kuttl/restarter-content-identical-write/20-assert.yaml @@ -0,0 +1,23 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 30 +--- +# The ConfigMap content never changed. +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-config +data: + foo: bar +--- +# This should stay on generation 1 +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: test + generation: 1 +status: + readyReplicas: 2 + replicas: 2 + observedGeneration: 1 diff --git a/tests/templates/kuttl/restarter-content-identical-write/20-content-identical-write.yaml b/tests/templates/kuttl/restarter-content-identical-write/20-content-identical-write.yaml new file mode 100644 index 00000000..be86506e --- /dev/null +++ b/tests/templates/kuttl/restarter-content-identical-write/20-content-identical-write.yaml @@ -0,0 +1,19 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +timeout: 120 +commands: + # Do a change that should (tm) not have an impact on a pod mounting/needing this cm. + - script: | + kubectl apply --server-side --field-manager=third-party-controller --force-conflicts -f - < Date: Thu, 13 Aug 2026 21:09:00 +0200 Subject: [PATCH 2/2] feat: create digest from content to avoid metadata-based restarts --- CHANGELOG.md | 4 + Cargo.lock | 1 + Cargo.nix | 4 + Cargo.toml | 6 +- rust/operator-binary/Cargo.toml | 1 + .../src/restart_controller/statefulset.rs | 345 ++++++++++++++++-- 6 files changed, 329 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0372e363..f4da3874 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,13 @@ All notable changes to this project will be documented in this file. - Bump stackable-operator to 0.114.0 ([#439]). - Bump stackabke-operator to 0.115.0 ([#444]). +- The StatefulSet restarter now keys its annotations on a digest of the mounted ConfigMap/Secret + content instead of the object's `resourceVersion`, so a content-identical write by a third party + no longer triggers a rolling restart of the whole role group ([#449]). [#439]: https://github.com/stackabletech/commons-operator/pull/439 [#444]: https://github.com/stackabletech/commons-operator/pull/444 +[#449]: https://github.com/stackabletech/commons-operator/issues/449 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.lock b/Cargo.lock index 4b5f2c56..42d41b9a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3135,6 +3135,7 @@ dependencies = [ "json-patch", "serde", "serde_json", + "sha2", "snafu 0.9.2", "stackable-operator", "strum", diff --git a/Cargo.nix b/Cargo.nix index afdf2c7f..634cfad5 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -10338,6 +10338,10 @@ rec { name = "serde_json"; packageId = "serde_json"; } + { + name = "sha2"; + packageId = "sha2"; + } { name = "snafu"; packageId = "snafu 0.9.2"; diff --git a/Cargo.toml b/Cargo.toml index dbe71121..5c812d6d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,10 @@ edition = "2024" repository = "https://github.com/stackabletech/commons-operator" [workspace.dependencies] -stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.115.0", features = ["crds", "webhook"] } +stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.115.0", features = [ + "crds", + "webhook", +] } anyhow = "1.0" built = { version = "0.8", features = ["chrono", "git2"] } @@ -21,6 +24,7 @@ http = "1.3" json-patch = "4.1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +sha2 = "0.10" snafu = "0.9" strum = { version = "0.28", features = ["derive"] } tokio = { version = "1.40", features = ["full"] } diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index 12c1a69b..eb416144 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -19,6 +19,7 @@ futures.workspace = true json-patch.workspace = true serde.workspace = true serde_json.workspace = true +sha2.workspace = true snafu.workspace = true strum.workspace = true tokio.workspace = true diff --git a/rust/operator-binary/src/restart_controller/statefulset.rs b/rust/operator-binary/src/restart_controller/statefulset.rs index b7d60ef1..d0c852e0 100644 --- a/rust/operator-binary/src/restart_controller/statefulset.rs +++ b/rust/operator-binary/src/restart_controller/statefulset.rs @@ -1,29 +1,35 @@ use std::{ + borrow::Cow, collections::{BTreeMap, BTreeSet}, future::Future, + marker::PhantomData, sync::Arc, time::Duration, }; -use futures::{Stream, StreamExt, TryStream, stream}; +use futures::{Stream, StreamExt, TryStream, TryStreamExt, stream}; use serde_json::json; +use sha2::{Digest, Sha256}; use snafu::{ResultExt, Snafu}; use stackable_operator::{ client::Client, - k8s_openapi::api::{ - apps::v1::StatefulSet, - core::v1::{ConfigMap, EnvFromSource, EnvVar, PodSpec, Secret, Volume}, + k8s_openapi::{ + api::{ + apps::v1::StatefulSet, + core::v1::{ConfigMap, EnvFromSource, EnvVar, PodSpec, Secret, Volume}, + }, + apimachinery::pkg::apis::meta::v1::ObjectMeta, }, kube::{ self, Resource, ResourceExt, - api::{PartialObjectMeta, Patch, PatchParams}, + api::{Patch, PatchParams}, core::{DeserializeGuard, DynamicObject, error_boundary}, runtime::{ Config, WatchStreamExt, applier, controller::{Action, ReconcileRequest, trigger_self, trigger_with}, events::{Recorder, Reporter}, reflector, - reflector::{ObjectRef, Store}, + reflector::{Lookup, ObjectRef, Store}, watcher::{self, watcher}, }, }, @@ -36,10 +42,72 @@ use crate::utils::delayed_init::{DelayedInit, InitDropped, Initializer}; const FULL_CONTROLLER_NAME: &str = "statefulset.restarter.commons.stackable.tech"; +/// A watched ConfigMap or Secret, reduced to its identity and a digest of its content. +#[derive(Clone, Debug)] +pub struct ContentDigest { + name: Option, + namespace: Option, + uid: Option, + resource_version: Option, + digest: String, + _phantom: PhantomData, +} + +impl ContentDigest { + fn new(metadata: ObjectMeta, digest: String) -> Self { + Self { + name: metadata.name, + namespace: metadata.namespace, + uid: metadata.uid, + resource_version: metadata.resource_version, + digest, + _phantom: PhantomData, + } + } +} + +/// Lets a [`Store`] cache these objects, keyed by name and namespace - so a +/// `ContentDigest` is found under the same key as the ConfigMap it was made from. +impl Lookup for ContentDigest { + type DynamicType = K::DynamicType; + + fn kind(dt: &Self::DynamicType) -> Cow<'_, str> { + K::kind(dt) + } + + fn group(dt: &Self::DynamicType) -> Cow<'_, str> { + K::group(dt) + } + + fn version(dt: &Self::DynamicType) -> Cow<'_, str> { + K::version(dt) + } + + fn plural(dt: &Self::DynamicType) -> Cow<'_, str> { + K::plural(dt) + } + + fn name(&self) -> Option> { + self.name.as_deref().map(Cow::Borrowed) + } + + fn namespace(&self) -> Option> { + self.namespace.as_deref().map(Cow::Borrowed) + } + + fn resource_version(&self) -> Option> { + self.resource_version.as_deref().map(Cow::Borrowed) + } + + fn uid(&self) -> Option> { + self.uid.as_deref().map(Cow::Borrowed) + } +} + pub struct Ctx { client: Client, - cms: DelayedInit>>, - secrets: DelayedInit>>, + cms: DelayedInit>>, + secrets: DelayedInit>>, } #[derive(Snafu, Debug, EnumDiscriminants)] @@ -78,13 +146,76 @@ impl ReconcilerError for Error { } } +/// Adds one key/value pair to `hasher`, length-prefixed. +fn update_entry(hasher: &mut Sha256, key: &str, value: &[u8]) { + hasher.update((key.len() as u64).to_be_bytes()); + hasher.update(key.as_bytes()); + hasher.update((value.len() as u64).to_be_bytes()); + hasher.update(value); +} + +/// Truncates the digest to 128 bits and hex-encodes it. +fn finish_digest(hasher: Sha256) -> String { + hasher.finalize()[..16] + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + +/// Digest over the parts of a ConfigMap that a Pod can read. +fn config_map_content_digest(config_map: &ConfigMap) -> String { + let mut hasher = Sha256::new(); + hasher.update(b"configmap/data"); + for (key, value) in config_map.data.iter().flatten() { + update_entry(&mut hasher, key, value.as_bytes()); + } + hasher.update(b"configmap/binaryData"); + for (key, value) in config_map.binary_data.iter().flatten() { + update_entry(&mut hasher, key, &value.0); + } + finish_digest(hasher) +} + +/// Digest over the parts of a Secret that a Pod can read. +fn secret_content_digest(secret: &Secret) -> String { + let mut hasher = Sha256::new(); + hasher.update(b"secret/data"); + for (key, value) in secret.data.iter().flatten() { + update_entry(&mut hasher, key, &value.0); + } + finish_digest(hasher) +} + +/// Reduces a ConfigMap to what the restarter needs, dropping its content. +fn digest_config_map(config_map: ConfigMap) -> ContentDigest { + let digest = config_map_content_digest(&config_map); + ContentDigest::new(config_map.metadata, digest) +} + +/// Reduces a Secret to what the restarter needs, dropping its content. +fn digest_secret(secret: Secret) -> ContentDigest { + let digest = secret_content_digest(&secret); + ContentDigest::new(secret.metadata, digest) +} + +/// Maps the object contained in a watcher event, changing its type. +fn map_event(event: watcher::Event, f: impl FnOnce(K) -> L) -> watcher::Event { + match event { + watcher::Event::Apply(obj) => watcher::Event::Apply(f(obj)), + watcher::Event::Delete(obj) => watcher::Event::Delete(f(obj)), + watcher::Event::InitApply(obj) => watcher::Event::InitApply(f(obj)), + watcher::Event::Init => watcher::Event::Init, + watcher::Event::InitDone => watcher::Event::InitDone, + } +} + #[allow(clippy::type_complexity)] pub fn create_context( client: Client, ) -> ( Arc, - Initializer>>, - Initializer>>, + Initializer>>, + Initializer>>, ) { let (cm_store_tx, cm_store_delayed) = DelayedInit::new(); let (secret_store_tx, secret_store_delayed) = DelayedInit::new(); @@ -99,19 +230,19 @@ pub fn create_context( pub async fn start( ctx: Arc, - cm_store_tx: Initializer>>, - secret_store_tx: Initializer>>, + cm_store_tx: Initializer>>, + secret_store_tx: Initializer>>, watch_namespace: &WatchNamespace, shutdown_signal: F, ) where F: Future, { let stses = watch_namespace.get_api::>(&ctx.client); - let cms = watch_namespace.get_api::>(&ctx.client); - let secrets = watch_namespace.get_api::>(&ctx.client); + let cms = watch_namespace.get_api::(&ctx.client); + let secrets = watch_namespace.get_api::(&ctx.client); let sts_store = reflector::store::Writer::>::new(()); - let cm_store = reflector::store::Writer::>::new(()); - let secret_store = reflector::store::Writer::>::new(()); + let cm_store = reflector::store::Writer::>::new(()); + let secret_store = reflector::store::Writer::>::new(()); let mut cm_store_tx = Some(cm_store_tx); let mut secret_store_tx = Some(secret_store_tx); let ctx2 = ctx.clone(); @@ -139,7 +270,8 @@ pub async fn start( cms, watcher::Config::default() .labels("restarter.stackable.tech/ignore != true"), - ), + ) + .map_ok(|event| map_event(event, digest_config_map)), ) .inspect(move |_| { if let Some(tx) = cm_store_tx.take() { @@ -159,7 +291,8 @@ pub async fn start( secrets, watcher::Config::default() .labels("restarter.stackable.tech/ignore != true"), - ), + ) + .map_ok(|event| map_event(event, digest_secret)), ) .inspect(move |_| { if let Some(tx) = secret_store_tx.take() { @@ -219,7 +352,7 @@ where }) } -fn find_pod_refs<'a, K: Resource + 'a>( +fn find_pod_refs<'a, K: Lookup + 'a>( pod_spec: &'a PodSpec, volume_ref: impl Fn(&Volume) -> Option> + 'a, env_var_ref: impl Fn(&EnvVar) -> Option> + 'a, @@ -264,12 +397,12 @@ pub async fn get_updated_restarter_annotations( find_pod_refs( pod_spec, |volume| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( &volume.config_map.as_ref()?.name, )) }, |env_var| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( &env_var .value_from .as_ref()? @@ -279,7 +412,7 @@ pub async fn get_updated_restarter_annotations( )) }, |env_from| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( &env_from.config_map_ref.as_ref()?.name, )) }, @@ -304,11 +437,11 @@ pub async fn get_updated_restarter_annotations( ( format!("configmap.restarter.stackable.tech/{cm_name}",), if let Some(cm) = cm - && let Some(uid) = &cm.metadata.uid - && let Some(resource_version) = &cm.metadata.resource_version + && let Some(uid) = &cm.uid && !ignored_cms.contains(&cm_name) { - format!("{uid}/{resource_version}",) + let digest = &cm.digest; + format!("{uid}/{digest}") } else { "changes-ignored".to_owned() }, @@ -321,17 +454,17 @@ pub async fn get_updated_restarter_annotations( find_pod_refs( pod_spec, |volume| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( volume.secret.as_ref()?.secret_name.as_deref()?, )) }, |env_var| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( &env_var.value_from.as_ref()?.secret_key_ref.as_ref()?.name, )) }, |env_from| { - Some(ObjectRef::>::new( + Some(ObjectRef::>::new( &env_from.secret_ref.as_ref()?.name, )) }, @@ -358,11 +491,11 @@ pub async fn get_updated_restarter_annotations( ( format!("secret.restarter.stackable.tech/{secret_name}",), if let Some(secret) = secret - && let Some(uid) = &secret.metadata.uid - && let Some(resource_version) = &secret.metadata.resource_version + && let Some(uid) = &secret.uid && !ignored_secrets.contains(&secret_name) { - format!("{uid}/{resource_version}",) + let digest = &secret.digest; + format!("{uid}/{digest}") } else { "changes-ignored".to_owned() }, @@ -429,3 +562,153 @@ fn error_policy(_obj: Arc>, error: &Error, _ctx: A _ => Action::requeue(Duration::from_secs(5)), } } + +#[cfg(test)] +mod tests { + use stackable_operator::k8s_openapi::{ + ByteString, + apimachinery::pkg::apis::meta::v1::{ManagedFieldsEntry, ObjectMeta}, + }; + + use super::*; + + fn config_map(data: &[(&str, &str)]) -> ConfigMap { + ConfigMap { + data: Some( + data.iter() + .map(|(key, value)| ((*key).to_owned(), (*value).to_owned())) + .collect(), + ), + ..ConfigMap::default() + } + } + + #[test] + fn digest_is_stable_across_releases() { + // Changing the digest function rolls every StatefulSet labelled + // restarter.stackable.tech/enabled=true which we want to avoid + assert_eq!( + config_map_content_digest(&config_map(&[("foo", "bar")])), + "401bdc692ecd7b9bb6b02f186e2976c3" + ); + } + + #[test] + fn digest_ignores_metadata() { + let mut changed = config_map(&[("foo", "bar")]); + changed.metadata = ObjectMeta { + resource_version: Some("12345".to_owned()), + labels: Some([("cost-center".to_owned(), "irrelevant".to_owned())].into()), + annotations: Some([("probe".to_owned(), "1".to_owned())].into()), + managed_fields: Some(vec![ManagedFieldsEntry { + manager: Some("third-party-controller".to_owned()), + ..ManagedFieldsEntry::default() + }]), + ..ObjectMeta::default() + }; + + assert_eq!( + config_map_content_digest(&config_map(&[("foo", "bar")])), + config_map_content_digest(&changed) + ); + } + + #[test] + fn digest_changes_when_content_changes() { + assert_ne!( + config_map_content_digest(&config_map(&[("foo", "bar")])), + config_map_content_digest(&config_map(&[("foo", "baz")])) + ); + } + + #[test] + fn digest_distinguishes_ambiguous_entry_splits() { + // Without length prefixes these would hash identically. + assert_ne!( + config_map_content_digest(&config_map(&[("ab", "c")])), + config_map_content_digest(&config_map(&[("a", "bc")])) + ); + } + + #[test] + fn digest_distinguishes_data_from_binary_data() { + let binary = ConfigMap { + binary_data: Some([("foo".to_owned(), ByteString(b"bar".to_vec()))].into()), + ..ConfigMap::default() + }; + + assert_ne!( + config_map_content_digest(&config_map(&[("foo", "bar")])), + config_map_content_digest(&binary) + ); + } + + fn identifying_metadata() -> ObjectMeta { + ObjectMeta { + name: Some("my-config".to_owned()), + namespace: Some("my-namespace".to_owned()), + uid: Some("f9dc0a8f-5f4b-4f52-9d0f-1a0ba1e0bd2c".to_owned()), + managed_fields: Some(vec![ManagedFieldsEntry { + manager: Some("third-party-controller".to_owned()), + ..ManagedFieldsEntry::default() + }]), + annotations: Some( + [( + "kubectl.kubernetes.io/last-applied-configuration".to_owned(), + r#"{"data":{"password":"aHVudGVyMg=="}}"#.to_owned(), + )] + .into(), + ), + ..ObjectMeta::default() + } + } + + #[test] + fn digesting_keeps_the_identity() { + let config_map = ConfigMap { + metadata: identifying_metadata(), + ..config_map(&[("foo", "bar")]) + }; + let digest = config_map_content_digest(&config_map); + let digested = digest_config_map(config_map); + + assert_eq!(digested.digest, digest); + assert_eq!(digested.name.as_deref(), Some("my-config")); + assert_eq!(digested.namespace.as_deref(), Some("my-namespace")); + assert_eq!( + digested.uid.as_deref(), + Some("f9dc0a8f-5f4b-4f52-9d0f-1a0ba1e0bd2c") + ); + } + + #[test] + fn digesting_a_secret_retains_no_content() { + let secret = Secret { + metadata: identifying_metadata(), + data: Some([("password".to_owned(), ByteString(b"hunter2".to_vec()))].into()), + ..Secret::default() + }; + let digest = secret_content_digest(&secret); + let digested = digest_secret(secret); + + assert_eq!(digested.digest, digest); + assert_eq!(digested.name.as_deref(), Some("my-config")); + + let debug = format!("{digested:?}"); + assert!(!debug.contains("hunter2"), "{debug}"); + assert!(!debug.contains("aHVudGVyMg=="), "{debug}"); + } + + #[test] + fn store_keys_match_the_refs_built_from_a_pod_template() { + let digested = digest_config_map(ConfigMap { + metadata: identifying_metadata(), + ..config_map(&[("foo", "bar")]) + }); + + assert_eq!( + ObjectRef::from_obj(&digested), + ObjectRef::>::new("my-config").within("my-namespace") + ); + } +}