diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index c306526c1..58969b6fe 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,6 +4,15 @@ 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. +- `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 + ## [0.116.0] - 2026-08-14 ### Added 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 ef457bbfc..06610dfab 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,27 @@ 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 + // 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) .context(ApplyObjectOverridesSnafu)?; + self.matched_object_overrides + .extend(matched_object_overrides); let patched_resource = self .apply_strategy @@ -657,6 +680,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 +708,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 either not set or 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..f469345fa 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; @@ -12,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. @@ -25,15 +30,36 @@ 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. - pub fn apply_to(&self, base: &mut R) -> Result<(), super::Error> + /// 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 + /// [`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`. + 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..94466cd1d 100644 --- a/crates/stackable-operator/src/deep_merger/mod.rs +++ b/crates/stackable-operator/src/deep_merger/mod.rs @@ -20,35 +20,41 @@ 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. /// /// 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(()); + // 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); } let deserialized_merge = merge @@ -61,12 +67,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 +239,102 @@ mod tests { assert_eq!(sa, original, "The merge shouldn't have changed anything"); } + #[test] + 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 defaults to the namespace of the base object + labels: + app.kubernetes.io/name: overwritten + foo: bar + "}) + .expect("test YAML is valid"); + + 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_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()); + } + + #[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();