Skip to content

Add finalizers to ObjectMetaBuilder #1094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 12, 2025
Merged

Add finalizers to ObjectMetaBuilder #1094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eaa3168
add finalizers to ObjectMetaBuilder
maltesander Sep 12, 2025
ff12cdf
adapt changelog
maltesander Sep 12, 2025
fa658f1
remove empty line
maltesander Sep 12, 2025
730a48e
fix pre commit
maltesander Sep 12, 2025
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
6 changes: 6 additions & 0 deletions crates/stackable-operator/CHANGELOG.md
View file
Open in desktop
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]

### Added

- Extend `ObjectMetaBuilder` with `finalizers` ([#1094]).

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

## [0.97.0] - 2025-09-09

### Added
Expand Down
31 changes: 29 additions & 2 deletions crates/stackable-operator/src/builder/meta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ pub enum Error {
/// It is strongly recommended to always call [`Self::with_recommended_labels()`]!
#[derive(Clone, Default)]
pub struct ObjectMetaBuilder {
ownerreference: Option<OwnerReference>,
annotations: Option<Annotations>,
finalizers: Option<Vec<String>>,
generate_name: Option<String>,
namespace: Option<String>,
labels: Option<Labels>,
namespace: Option<String>,
name: Option<String>,
ownerreference: Option<OwnerReference>,
}

impl ObjectMetaBuilder {
Expand Down Expand Up @@ -163,6 +164,26 @@ impl ObjectMetaBuilder {
Ok(self)
}

/// This adds a single finalizer to the existing finalizers.
pub fn with_finalizer(&mut self, finalizer: impl Into<String>) -> &mut Self {
self.finalizers
.get_or_insert(Vec::new())
.push(finalizer.into());
self
}

/// This adds multiple finalizers to the existing finalizers.
pub fn with_finalizers(&mut self, finalizers: Vec<String>) -> &mut Self {
self.finalizers.get_or_insert(Vec::new()).extend(finalizers);
self
}

/// This will replace all existing finalizers
pub fn finalizers(&mut self, finalizers: Vec<String>) -> &mut Self {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be called replace_finalizers. Could be easy to misuse otherwise.

Also, I think (not 100% sure) finalizers might be a free-for-all (anything with permission can add a finalizer to stop something being deleted too early). SO maybe it isn't a good idea to allow replacing them. You should only be allowed to add or remove "your" finalizer.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally each entity (for lack of a better name... think operator/controller) should have a const for it's finalizer name (its stamp that it puts on things that it depends on being alive until it does some process, then it removes the stamp and the k8s reconciler continues).

Copy link
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, but these are mostly pure builders, and its consistent with other stuff. You could argue anyone could add / remove labels as well?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say only the admin and the owning-operators should change labels.

Finalizers are for a different reason which is to prevent the object from disappearing when things unknown to the owning-operator need to do things.

One example is Ingress... The resource is an internal k8s Resource, but three can be various operators/controllers that can "do things".

For example, An ingress controller creating a cloud load balancer for Ingresses of an applicable ingressClass - it will add a finalizer (to the Ingress) so that when an admin deletes the Ingress resource, it has to wait until the cloud load balancer has finished being deleted.

If K8s just removes the resource while the controller managing the Load Balancer restarts, when it starts up it will not know about the deleted Ingress and therefore won't know that it needed to delete the Load Balancer (which has cost implications and minimum).

Copy link
Member Author

Choose a reason for hiding this comment

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

Im not sure i get this.

This is a framework, among other things, i can create / manipulate k8s resources. If i wanna reset labels, annotations or finalizers i should be able to do so. If this kills a whole cluster, then i would not say this was the frameworks mistake? This probably should have been prevented via rbac permissions?

Its a little bit like the bike and the pipe?

Copy link
Member

@NickLarsenNZ NickLarsenNZ Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What I mean is, imagine someone has an operator that does "something" (we don't know what it does)... and it wants to do "something" when a Pod with specific labels is deleted, but it needs to block the deletion until it has finished its work...

We shouldn't be allowed to clear/replace the finalizer list just because we want all of our stuff gone, because we cannot predict what other finalizers are there and why.

Now that example was for Pods... But that same mysterious operator might want to do things on deletion of a NifiCluster, or KafkaCluster for example. It's not up to us to say "no you can't" because we cannot predict the case where it might be a good idea, or even necessary.

So, finalizers that are not our own (the operator's own) should be respected and left alone.

self.finalizers = Some(finalizers);
self
}

pub fn build(&self) -> ObjectMeta {
// NOTE (Techassi): Shouldn't this take self instead of &self to consume
// the builder and build ObjectMeta without cloning?
Expand All @@ -187,6 +208,7 @@ impl ObjectMetaBuilder {
.map(|ownerreference| vec![ownerreference.clone()]),
labels: self.labels.clone().map(|l| l.into()),
annotations: self.annotations.clone().map(|a| a.into()),
finalizers: self.finalizers.clone(),
..ObjectMeta::default()
}
}
Expand Down Expand Up @@ -339,6 +361,7 @@ mod tests {
})
.unwrap()
.with_annotation(("foo", "bar").try_into().unwrap())
.with_finalizer("finalizer")
.build();

assert_eq!(meta.generate_name, Some("generate_foo".to_string()));
Expand All @@ -352,5 +375,9 @@ mod tests {
meta.annotations.as_ref().unwrap().get(&"foo".to_string()),
Some(&"bar".to_string())
);
assert_eq!(
meta.finalizers.as_ref().unwrap().first(),
Some(&"finalizer".to_string())
);
}
}