Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions crates/stackable-operator/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.

## [Unreleased]

### Changed

- BREAKING: `ClusterResources` now warns about `objectOverrides` entries that did not match any of the objects it created. To enable this, the signatures of `apply_deep_merge` and `ObjectOverrides::apply_to` needed to be adjusted ([#1264]).

[#1264]: https://github.com/stackabletech/operator-rs/pull/1264

## [0.116.0] - 2026-08-14

### Added
Expand Down
58 changes: 56 additions & 2 deletions crates/stackable-operator/src/cluster_resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,10 @@ pub struct ClusterResources<'a> {

/// Arbitrary Kubernetes object overrides specified by the user via the CRD.
object_overrides: &'a ObjectOverrides,

/// The indices of the object_overrides entries that matched at least one of
/// the added resources.
matched_object_overrides: HashSet<usize>,
}

impl<'a> ClusterResources<'a> {
Expand Down Expand Up @@ -499,6 +503,7 @@ impl<'a> ClusterResources<'a> {
resource_ids: HashSet::default(),
apply_strategy,
object_overrides,
matched_object_overrides: HashSet::default(),
})
}

Expand Down Expand Up @@ -570,10 +575,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
Expand Down Expand Up @@ -657,6 +664,16 @@ impl<'a> ClusterResources<'a> {
///
/// * `client` - The client which is used to access Kubernetes
pub async fn delete_orphaned_resources(self, client: &Client) -> Result<()> {
// We warn late about unmatched object overrides, as every override is matched against
// each object individually (by apiVersion, kind, name and namespace). An override that
// e.g. targets the discovery ConfigMap will therefore not match any of the rolegroup
// ConfigMaps, so whether an override matched nothing at all can only be determined once
// all objects have been added.
// As this function consumes `self` and finalizes the cluster creation, it is the last
// point at which we can do so without requiring an extra call in every operator.
// The downside is that the warnings are lost in case reconciliation fails earlier.
self.warn_about_unmatched_object_overrides();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the call. I understand it better now. I'd like to have a comment here, why the decision was to put in delete_orphaned_resources something along the lines:

Did it here because last step as it's only about resources which didn't had a match. Did so to not clutter Snafu warnings directly related to malformed CRDs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 9a2adee

@Maleware I thought a bit more about this - I think you are right that we should warn earlier. My initial thinking was that an object override would not make an apply step fail but that is wrong. When I tried to add it earlier I ran into the issue that you actually only get to match against all possible overrides, once all applies have been done. There are some checks we could do before, but not all of them.

To keep this small, I'd leave it at this, but I think this deserves further improvements - but from what I looked at, that would be more invasive than this change.


// We can only delete Listeners in case the "crds" feature is enabled, otherwise it's a NOP.
#[cfg(feature = "crds")]
let delete_listeners = self
Expand All @@ -681,6 +698,43 @@ 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(("<not set>", "<not set>"), |types| {
(types.api_version.as_str(), types.kind.as_str())
});
let name = object_override
.metadata
.name
.as_deref()
.unwrap_or("<not set>");
let namespace = object_override
.metadata
.namespace
.as_deref()
.unwrap_or("<not set>");

warn!(
index,
api_version,
kind,
metadata.name = name,
metadata.namespace = namespace,
cluster_namespace = self.namespace,
"objectOverride 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 matches the cluster namespace."
);
Comment on lines +724 to +734

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally prefer providing dynamic values as fields/attributes (by mentioning them before the message text in the warn! macro). This allows consumers (like OpenTelemetry collectors) to access those fields as structured data instead of only having access to a message string.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense - thank you! Addressed in c48736c

}
}

/// 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.
///
Expand Down
28 changes: 24 additions & 4 deletions crates/stackable-operator/src/deep_merger/crd.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashSet;

use k8s_openapi::DeepMerge;
use kube::api::DynamicObject;
use schemars::JsonSchema;
Expand Down Expand Up @@ -27,13 +29,31 @@ impl ObjectOverrides {
///
/// Merges are only applied to objects that have the same apiVersion, kind, name
/// and namespace.
pub fn apply_to<R>(&self, base: &mut R) -> Result<(), super::Error>
///
/// Returns the indices of the entries that matched `base` and were therefore merged into it.
pub fn apply_to<R>(&self, base: &mut R) -> Result<Vec<usize>, super::Error>
where
R: kube::Resource<DynamicType = ()> + 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<usize>,
) -> impl Iterator<Item = (usize, &'a DynamicObject)> {
self.0
.iter()
.enumerate()
.filter(move |(index, _)| !matched_indices.contains(index))
}
}
78 changes: 70 additions & 8 deletions crates/stackable-operator/src/deep_merger/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<R>(base: &mut R, merge: &DynamicObject) -> Result<(), Error>
pub fn apply_deep_merge<R>(base: &mut R, merge: &DynamicObject) -> Result<bool, Error>
where
R: kube::Resource<DynamicType = ()> + 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
Expand All @@ -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::{
Expand Down Expand Up @@ -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::<usize>::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::<Vec<_>>();
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();
Expand Down