From 5e5d619bbf737cc5389555da52d6873a26a51ca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=BCller?= Date: Fri, 14 Aug 2026 09:30:00 +0200 Subject: [PATCH 1/2] chore: log when object overrides are not applied --- crates/stackable-operator/CHANGELOG.md | 8 ++ .../src/cluster_resources.rs | 50 +++++++++++- .../stackable-operator/src/deep_merger/crd.rs | 34 +++++++- .../stackable-operator/src/deep_merger/mod.rs | 78 +++++++++++++++++-- 4 files changed, 156 insertions(+), 14 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index c306526c1..06ad7e83b 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- `ClusterResources` now warns about `objectOverrides` entries that did not match any of the objects it created ([#1264]). +- BREAKING: To enable this, `apply_deep_merge` now returns whether the merge matched the base object and `ObjectOverrides::apply_to` + returns the indices of the entries that matched. + +[#1264]: https://github.com/stackabletech/operator-rs/pull/1264 + ## [0.116.0] - 2026-08-14 ### Added diff --git a/crates/stackable-operator/src/cluster_resources.rs b/crates/stackable-operator/src/cluster_resources.rs index ef457bbfc..34342bc61 100644 --- a/crates/stackable-operator/src/cluster_resources.rs +++ b/crates/stackable-operator/src/cluster_resources.rs @@ -445,6 +445,11 @@ pub struct ClusterResources<'a> { /// Arbitrary Kubernetes object overrides specified by the user via the CRD. object_overrides: &'a ObjectOverrides, + + /// The indices of the [`ObjectOverrides`] entries that matched at least one of the added + /// resources. Entries that never matched anything are warned about in + /// [`ClusterResources::delete_orphaned_resources`]. + matched_object_overrides: HashSet, } impl<'a> ClusterResources<'a> { @@ -499,6 +504,7 @@ impl<'a> ClusterResources<'a> { resource_ids: HashSet::default(), apply_strategy, object_overrides, + matched_object_overrides: HashSet::default(), }) } @@ -570,10 +576,12 @@ impl<'a> ClusterResources<'a> { let mut mutated = resource.maybe_mutate(&self.apply_strategy); - // We apply the object overrides of the user at the very end to offer maximum flexibility. - self.object_overrides + let matched_object_overrides = self + .object_overrides .apply_to(&mut mutated) .context(ApplyObjectOverridesSnafu)?; + self.matched_object_overrides + .extend(matched_object_overrides); let patched_resource = self .apply_strategy @@ -657,6 +665,10 @@ impl<'a> ClusterResources<'a> { /// /// * `client` - The client which is used to access Kubernetes pub async fn delete_orphaned_resources(self, client: &Client) -> Result<()> { + // All resources of this cluster have been added at this point, so we now know which object + // overrides did not match anything. + self.warn_about_unmatched_object_overrides(); + // We can only delete Listeners in case the "crds" feature is enabled, otherwise it's a NOP. #[cfg(feature = "crds")] let delete_listeners = self @@ -681,6 +693,40 @@ impl<'a> ClusterResources<'a> { Ok(()) } + /// Warns about every object override that did not match any of the added resources. + fn warn_about_unmatched_object_overrides(&self) { + for (index, object_override) in self + .object_overrides + .unmatched(&self.matched_object_overrides) + { + let (api_version, kind) = object_override + .types + .as_ref() + .map_or(("", ""), |types| { + (types.api_version.as_str(), types.kind.as_str()) + }); + let name = object_override + .metadata + .name + .as_deref() + .unwrap_or(""); + let namespace = object_override + .metadata + .namespace + .as_deref() + .unwrap_or(""); + + warn!( + "The objectOverride at index {index} (apiVersion: {api_version:?}, kind: \ + {kind:?}, metadata.name: {name:?}, metadata.namespace: {namespace:?}) did not \ + match any object created for this cluster and therefore had no effect. Please \ + check that apiVersion, kind and metadata.name are correct and that \ + metadata.namespace is set to {cluster_namespace:?}.", + cluster_namespace = self.namespace, + ); + } + } + /// Deletes all deployed resources of the given kind which are labelled as if they belong to /// this cluster instance but are not contained in the given list. /// diff --git a/crates/stackable-operator/src/deep_merger/crd.rs b/crates/stackable-operator/src/deep_merger/crd.rs index d0099d835..7db0edde7 100644 --- a/crates/stackable-operator/src/deep_merger/crd.rs +++ b/crates/stackable-operator/src/deep_merger/crd.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use k8s_openapi::DeepMerge; use kube::api::DynamicObject; use schemars::JsonSchema; @@ -27,13 +29,37 @@ impl ObjectOverrides { /// /// Merges are only applied to objects that have the same apiVersion, kind, name /// and namespace. - pub fn apply_to(&self, base: &mut R) -> Result<(), super::Error> + /// + /// Returns the indices of the entries that matched `base` and were therefore merged into it. + /// Callers that apply the overrides can collect these indices and pass them to + /// [`ObjectOverrides::unmatched`] afterwards, to warn about entries that never matched anything. + pub fn apply_to(&self, base: &mut R) -> Result, super::Error> where R: kube::Resource + DeepMerge + DeserializeOwned, { - for object_override in &self.0 { - apply_deep_merge(base, object_override)?; + let mut matched_indices = Vec::new(); + + for (index, object_override) in self.0.iter().enumerate() { + if apply_deep_merge(base, object_override)? { + matched_indices.push(index); + } } - Ok(()) + + Ok(matched_indices) + } + + /// Returns all entries (and their index) that are not contained in `matched_indices`. + /// + /// These entries did not match any of the objects they were applied to and therefore had no + /// effect at all. Common causes are a missing or wrong `metadata.namespace`, a typo in + /// `metadata.name` or a wrong `apiVersion` or `kind`. + pub fn unmatched<'a>( + &'a self, + matched_indices: &'a HashSet, + ) -> impl Iterator { + self.0 + .iter() + .enumerate() + .filter(move |(index, _)| !matched_indices.contains(index)) } } diff --git a/crates/stackable-operator/src/deep_merger/mod.rs b/crates/stackable-operator/src/deep_merger/mod.rs index f167f3a4e..3863bcb02 100644 --- a/crates/stackable-operator/src/deep_merger/mod.rs +++ b/crates/stackable-operator/src/deep_merger/mod.rs @@ -23,32 +23,34 @@ pub enum Error { /// Merges are only applied to objects that have the same apiVersion, kind, name /// and namespace. /// +/// Returns whether the merge matched the base object and was therefore applied. +/// /// In case the merge matches the base object, it will get cloned prior to merging. /// We modeled it this way, as most of the time it won't match, so we don't need to proactively /// clone. -pub fn apply_deep_merge(base: &mut R, merge: &DynamicObject) -> Result<(), Error> +pub fn apply_deep_merge(base: &mut R, merge: &DynamicObject) -> Result where R: kube::Resource + DeepMerge + DeserializeOwned, { let Some(merge_type) = &merge.types else { - return Ok(()); + return Ok(false); }; if merge_type.api_version != R::api_version(&()) || merge_type.kind != R::kind(&()) { - return Ok(()); + return Ok(false); } let Some(merge_name) = &merge.metadata.name else { - return Ok(()); + return Ok(false); }; // The name always needs to match if &base.name_any() != merge_name { - return Ok(()); + return Ok(false); } // If there is a namespace on the base object, it needs to match as well // Note that it is not set for cluster-scoped objects. if base.namespace() != merge.metadata.namespace { - return Ok(()); + return Ok(false); } let deserialized_merge = merge @@ -61,12 +63,15 @@ where })?; base.merge_from(deserialized_merge); - Ok(()) + Ok(true) } #[cfg(test)] mod tests { - use std::{collections::BTreeMap, vec}; + use std::{ + collections::{BTreeMap, HashSet}, + vec, + }; use indoc::indoc; use k8s_openapi::{ @@ -230,6 +235,63 @@ mod tests { assert_eq!(sa, original, "The merge shouldn't have changed anything"); } + #[test] + fn service_account_not_merged_as_namespace_missing() { + let mut sa = generate_service_account(); + let object_overrides: ObjectOverrides = serde_yaml::from_str(indoc! {" + - apiVersion: v1 + kind: ServiceAccount + metadata: + name: trino-serviceaccount + # namespace omitted, so it does not match the namespaced base object + labels: + app.kubernetes.io/name: overwritten + foo: bar + "}) + .expect("test YAML is valid"); + + let original = sa.clone(); + let matched_indices = object_overrides + .apply_to(&mut sa) + .expect("merging onto test object works"); + assert_eq!(sa, original, "The merge shouldn't have changed anything"); + assert_eq!(matched_indices, Vec::::new()); + } + + #[test] + fn unmatched_overrides_are_reported() { + let mut sa = generate_service_account(); + let object_overrides: ObjectOverrides = serde_yaml::from_str(indoc! {" + - apiVersion: v1 + kind: ServiceAccount + metadata: + name: trino-serviceaccount + namespace: default + labels: + foo: bar + - apiVersion: v1 + kind: ServiceAccount + metadata: + name: trino-serviceaccount-typo # name mismatch + namespace: default + "}) + .expect("test YAML is valid"); + + let matched_indices = object_overrides + .apply_to(&mut sa) + .expect("merging onto test object works"); + assert_eq!(matched_indices, vec![0]); + + let unmatched = object_overrides + .unmatched(&HashSet::from_iter(matched_indices)) + .map(|(index, object_override)| (index, object_override.metadata.name.clone())) + .collect::>(); + assert_eq!( + unmatched, + vec![(1, Some("trino-serviceaccount-typo".to_owned()))] + ); + } + #[test] fn service_account_not_merged_as_different_api_version() { let mut sa = generate_service_account(); From d9924bac737e90b008ef39fa25e7786f03dc83f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=BCller?= Date: Fri, 14 Aug 2026 11:14:34 +0200 Subject: [PATCH 2/2] feat: take base namespace if not provided in object overrides --- crates/stackable-operator/CHANGELOG.md | 1 + .../stackable-operator/crds/DummyCluster.yaml | 3 + crates/stackable-operator/crds/Listener.yaml | 3 + .../src/cluster_resources.rs | 17 +++++- .../stackable-operator/src/deep_merger/crd.rs | 12 ++-- .../stackable-operator/src/deep_merger/mod.rs | 61 ++++++++++++++++--- 6 files changed, 81 insertions(+), 16 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 06ad7e83b..58969b6fe 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -9,6 +9,7 @@ All notable changes to this project will be documented in this file. - `ClusterResources` now warns about `objectOverrides` entries that did not match any of the objects it created ([#1264]). - BREAKING: To enable this, `apply_deep_merge` now returns whether the merge matched the base object and `ObjectOverrides::apply_to` returns the indices of the entries that matched. +- `metadata.namespace` on an `objectOverrides` entry is now optional and defaults to the namespace of the object it is merged into ([#1264]). [#1264]: https://github.com/stackabletech/operator-rs/pull/1264 diff --git a/crates/stackable-operator/crds/DummyCluster.yaml b/crates/stackable-operator/crds/DummyCluster.yaml index b8e616b6a..efef20c34 100644 --- a/crates/stackable-operator/crds/DummyCluster.yaml +++ b/crates/stackable-operator/crds/DummyCluster.yaml @@ -1776,6 +1776,9 @@ spec: creates. List entries are arbitrary YAML objects, which need to be valid Kubernetes objects. + An entry is merged into every object with the same apiVersion, kind and name. The + `metadata.namespace` field is optional, it defaults to the namespace of the object the + entry is merged into. Read the [Object overrides documentation](https://docs.stackable.tech/home/nightly/concepts/overrides#object-overrides) for more information. diff --git a/crates/stackable-operator/crds/Listener.yaml b/crates/stackable-operator/crds/Listener.yaml index 9030fc053..2f29425fd 100644 --- a/crates/stackable-operator/crds/Listener.yaml +++ b/crates/stackable-operator/crds/Listener.yaml @@ -45,6 +45,9 @@ spec: creates. List entries are arbitrary YAML objects, which need to be valid Kubernetes objects. + An entry is merged into every object with the same apiVersion, kind and name. The + `metadata.namespace` field is optional, it defaults to the namespace of the object the + entry is merged into. Read the [Object overrides documentation](https://docs.stackable.tech/home/nightly/concepts/overrides#object-overrides) for more information. diff --git a/crates/stackable-operator/src/cluster_resources.rs b/crates/stackable-operator/src/cluster_resources.rs index 34342bc61..06610dfab 100644 --- a/crates/stackable-operator/src/cluster_resources.rs +++ b/crates/stackable-operator/src/cluster_resources.rs @@ -576,6 +576,21 @@ impl<'a> ClusterResources<'a> { let mut mutated = resource.maybe_mutate(&self.apply_strategy); + // Every object is expected to be created in the namespace of the cluster. Object overrides + // without a `metadata.namespace` as well as the deletion of orphaned resources rely on this. + if mutated.namespace().as_deref() != Some(self.namespace.as_str()) { + warn!( + "The {kind} {name:?} is created in namespace {object_namespace:?} instead of the \ + namespace of the cluster ({cluster_namespace:?}). This is a bug in the operator: \ + objectOverrides without a metadata.namespace can match it unintentionally and it \ + is never deleted once it becomes orphaned.", + kind = T::kind(&()), + name = mutated.name_any(), + object_namespace = mutated.namespace(), + cluster_namespace = self.namespace, + ); + } + let matched_object_overrides = self .object_overrides .apply_to(&mut mutated) @@ -721,7 +736,7 @@ impl<'a> ClusterResources<'a> { {kind:?}, metadata.name: {name:?}, metadata.namespace: {namespace:?}) did not \ match any object created for this cluster and therefore had no effect. Please \ check that apiVersion, kind and metadata.name are correct and that \ - metadata.namespace is set to {cluster_namespace:?}.", + metadata.namespace is either not set or set to {cluster_namespace:?}.", cluster_namespace = self.namespace, ); } diff --git a/crates/stackable-operator/src/deep_merger/crd.rs b/crates/stackable-operator/src/deep_merger/crd.rs index 7db0edde7..f469345fa 100644 --- a/crates/stackable-operator/src/deep_merger/crd.rs +++ b/crates/stackable-operator/src/deep_merger/crd.rs @@ -14,6 +14,9 @@ pub struct ObjectOverrides( /// creates. /// /// List entries are arbitrary YAML objects, which need to be valid Kubernetes objects. + /// An entry is merged into every object with the same apiVersion, kind and name. The + /// `metadata.namespace` field is optional, it defaults to the namespace of the object the + /// entry is merged into. /// /// Read the [Object overrides documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/overrides#object-overrides) /// for more information. @@ -27,8 +30,9 @@ impl ObjectOverrides { /// Takes an arbitrary Kubernetes object (`base`) and applies the configured list of deep merges /// to it. /// - /// Merges are only applied to objects that have the same apiVersion, kind, name - /// and namespace. + /// Merges are only applied to objects that have the same apiVersion, kind and name. A merge + /// with a namespace additionally needs to match the namespace of `base`, an omitted namespace + /// matches any namespace. /// /// Returns the indices of the entries that matched `base` and were therefore merged into it. /// Callers that apply the overrides can collect these indices and pass them to @@ -49,10 +53,6 @@ impl ObjectOverrides { } /// Returns all entries (and their index) that are not contained in `matched_indices`. - /// - /// These entries did not match any of the objects they were applied to and therefore had no - /// effect at all. Common causes are a missing or wrong `metadata.namespace`, a typo in - /// `metadata.name` or a wrong `apiVersion` or `kind`. pub fn unmatched<'a>( &'a self, matched_indices: &'a HashSet, diff --git a/crates/stackable-operator/src/deep_merger/mod.rs b/crates/stackable-operator/src/deep_merger/mod.rs index 3863bcb02..94466cd1d 100644 --- a/crates/stackable-operator/src/deep_merger/mod.rs +++ b/crates/stackable-operator/src/deep_merger/mod.rs @@ -20,8 +20,9 @@ pub enum Error { /// Takes an arbitrary Kubernetes object (`base`) and applies the deep merge. /// -/// Merges are only applied to objects that have the same apiVersion, kind, name -/// and namespace. +/// Merges are only applied to objects that have the same apiVersion, kind and name. A merge with a +/// namespace additionally needs to match the namespace of `base`, an omitted namespace matches any +/// namespace. /// /// Returns whether the merge matched the base object and was therefore applied. /// @@ -47,9 +48,12 @@ where return Ok(false); } - // If there is a namespace on the base object, it needs to match as well - // Note that it is not set for cluster-scoped objects. - if base.namespace() != merge.metadata.namespace { + // If the merge has a namespace, it needs to match as well. An omitted namespace matches any + // namespace, this function does not know which namespace to expect. + // + // Note that the base namespace is not set for cluster-scoped objects, in which case a merge + // with a namespace never matches. + if merge.metadata.namespace.is_some() && base.namespace() != merge.metadata.namespace { return Ok(false); } @@ -236,25 +240,64 @@ mod tests { } #[test] - fn service_account_not_merged_as_namespace_missing() { + fn service_account_merged_as_namespace_defaulted() { let mut sa = generate_service_account(); let object_overrides: ObjectOverrides = serde_yaml::from_str(indoc! {" - apiVersion: v1 kind: ServiceAccount metadata: name: trino-serviceaccount - # namespace omitted, so it does not match the namespaced base object + # namespace omitted, so it defaults to the namespace of the base object labels: app.kubernetes.io/name: overwritten foo: bar "}) .expect("test YAML is valid"); - let original = sa.clone(); + assert_has_label(&sa, "app.kubernetes.io/name", "trino"); let matched_indices = object_overrides .apply_to(&mut sa) .expect("merging onto test object works"); - assert_eq!(sa, original, "The merge shouldn't have changed anything"); + assert_has_label(&sa, "app.kubernetes.io/name", "overwritten"); + assert_eq!(matched_indices, vec![0]); + assert_eq!( + sa.metadata.namespace.as_deref(), + Some("default"), + "The namespace of the base object shouldn't have been removed" + ); + } + + #[test] + fn cluster_scoped_object_not_merged_as_namespace_set() { + let mut storage_class: StorageClass = serde_yaml::from_str(indoc! {" + apiVersion: storage.k8s.io/v1 + kind: StorageClass + metadata: + name: low-latency + labels: + foo: original + provisioner: csi-driver.example-vendor.example + "}) + .expect("test YAML is valid"); + let object_overrides: ObjectOverrides = serde_yaml::from_str(indoc! {" + - apiVersion: storage.k8s.io/v1 + kind: StorageClass + metadata: + name: low-latency + namespace: default # the base object is cluster-scoped, so this never matches + labels: + foo: overwritten + "}) + .expect("test YAML is valid"); + + let original = storage_class.clone(); + let matched_indices = object_overrides + .apply_to(&mut storage_class) + .expect("merging onto test object works"); + assert_eq!( + storage_class, original, + "The merge shouldn't have changed anything" + ); assert_eq!(matched_indices, Vec::::new()); }