fix: add do-not-delete finalizer to signal the finalizer shouldn't be removed manually by assafgi · Pull Request #2574 · weka/weka-operator

assafgi · 2026-05-31T12:44:25Z

No description provided.

github-actions · 2026-05-31T12:44:34Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

assafgi · 2026-05-31T12:44:39Z

How to use the Graphite Merge Queue

Add the label main-merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has required the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

claude · 2026-05-31T12:44:56Z

Claude finished @assafgi's task in 2m 8s —— View job

Claude PR Review

Read all changed files
Understand the do-not-delete finalizer pattern
Identify code quality / correctness issues
Post review feedback

Summary

The intent — adding a second, human-readable guard finalizer alongside WekaFinalizer so the name itself signals "do not strip manually" — is reasonable and the diff is small and consistent across the three controllers. However, the rollout has a real correctness issue that I think should be fixed before merge.

Findings (see inline comments for details and suggested fixes):

🐞 Bug — guard finalizer is never persisted on existing resources (wekaclient/client_reconciler_loop.go:265, wekacontainer/funcs_active_state_init.go:83, wekacluster/steps_cluster_creation.go:140-152).
AddFinalizer returns false when the finalizer is already present. In all three "ensure"/"init" sites, the code reaches Update() only when WekaFinalizer was missing — so any resource that already has WekaFinalizer (i.e. every resource that exists today) gets the new guard finalizer added in-memory but never written back. The protection effectively only applies to freshly created resources, which probably isn't the intent.
🎨 Naming — protection.io is not a domain this project owns (internal/consts/consts.go:11). All sibling constants use weka.io. Suggest do-not-delete.weka.io/guard to keep the semantic signal while staying inside an owned domain.
🧹 Consistency — finalizer-remove ordering differs across controllers (wekacluster/steps_deletion.go:216). Not a bug, but wekacluster removes the guard before WekaFinalizer; the other two do the opposite. Pick one order across the codebase.
🧪 No tests. A small unit test asserting that (a) both finalizers are added together on init and (b) both are removed on deletion would lock in the contract this PR is establishing, and would have caught issue Operator sdk #1.

Non-issues I looked at: the HandleDeletion sites correctly skip Update() on early-return so no partial state is persisted; the new constant is used consistently; the protection.io/guard string is syntactically a valid finalizer name.

Overall: nice idea, low-risk diff, but please address #1 before merge — otherwise the guard won't be doing its job on the fleet that actually needs protecting.

· Branch: 05-31-fix_add_do-not-delete_finalizer_to_signal_the_finalizer_shouldn_t_be_removed_manually

graphite-app · 2026-05-31T12:45:03Z

Graphite Automations

"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (05/31/26)

2 reviewers were added to this PR based on Anton Bykov's automation.

claude · 2026-05-31T12:47:17Z

 	logger.Info("Adding Finalizer for weka client")
+	controllerutil.AddFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)
 	if ok := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer); !ok {
 		return nil
 	}


Bug: guard finalizer is not persisted on existing resources.

AddFinalizer returns false when the finalizer is already present. So for a WekaClient created before this PR (which has WekaFinalizer but no DeletionProtectionFinalizer), the flow is:

AddFinalizer(DeletionProtectionFinalizer) — mutates the in-memory object.

AddFinalizer(WekaFinalizer) returns false → early return nil.

Update() is never called → the guard addition is lost.

This means rollout to existing clusters won't backfill the protection finalizer; only freshly created WekaClient resources will get it.

Suggest tracking whether either call mutated the object and only short-circuiting when neither did:

Suggested change

logger.Info("Adding Finalizer for weka client")

controllerutil.AddFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)

if ok := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer); !ok {

return nil

}

logger.Info("Adding Finalizer for weka client")

addedGuard := controllerutil.AddFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)

addedWeka := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer)

if !addedGuard && !addedWeka {

return nil

}

Fix this →

claude · 2026-05-31T12:47:18Z

+	controllerutil.AddFinalizer(container, consts.DeletionProtectionFinalizer)
 	if ok := controllerutil.AddFinalizer(container, consts.WekaFinalizer); !ok {
 		return nil
 	}


Same bug as in wekaclient: if WekaFinalizer is already present, AddFinalizer returns false, the function returns early, and the DeletionProtectionFinalizer addition is never Update()'d. Existing WekaContainer resources will not be backfilled with the guard finalizer.

Suggested change

controllerutil.AddFinalizer(container, consts.DeletionProtectionFinalizer)

if ok := controllerutil.AddFinalizer(container, consts.WekaFinalizer); !ok {

return nil

}

addedGuard := controllerutil.AddFinalizer(container, consts.DeletionProtectionFinalizer)

addedWeka := controllerutil.AddFinalizer(container, consts.WekaFinalizer)

if !addedGuard && !addedWeka {

return nil

}

claude · 2026-05-31T12:47:19Z

+	// DeletionProtectionFinalizer is a guard finalizer with a clear name to prevent
+	// accidental deletion by automation or AI agents. It is added alongside WekaFinalizer
+	// and removed only during intentional operator-driven deletion flows.
+	DeletionProtectionFinalizer = "do-not-delete.protection.io/guard"


Nit (naming): every other finalizer/annotation/resource constant in this file uses the weka.io domain that the project owns (e.g. weka.weka.io/finalizer, weka.io/weka-drives). protection.io is a generic, unowned domain and breaks that convention.

Suggest something like do-not-delete.weka.io/guard so the "do-not-delete" semantic signal is preserved while staying within a domain you control.

Copilot

Pull request overview

This PR adds a second Kubernetes finalizer intended to act as a deletion-protection guard alongside the existing Weka finalizer, and updates WekaContainer, WekaCluster, and WekaClient deletion paths to remove it during operator-managed cleanup.

Changes:

Adds DeletionProtectionFinalizer in internal/consts.
Adds the new finalizer during active/init reconciliation for Weka resources.
Removes the new finalizer during deletion handling.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`internal/consts/consts.go`	Defines the new deletion-protection finalizer constant.
`internal/controllers/wekacontainer/funcs_active_state_init.go`	Adds the protection finalizer during container finalizer reconciliation.
`internal/controllers/wekacontainer/funcs_common_deletion.go`	Removes the protection finalizer during container deletion.
`internal/controllers/wekacluster/steps_cluster_creation.go`	Adds the protection finalizer during cluster initialization.
`internal/controllers/wekacluster/steps_deletion.go`	Removes the protection finalizer during cluster deletion.
`internal/controllers/wekaclient/client_reconciler_loop.go`	Adds/removes the protection finalizer for WekaClient resources.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	controllerutil.AddFinalizer(container, consts.DeletionProtectionFinalizer)
 	if ok := controllerutil.AddFinalizer(container, consts.WekaFinalizer); !ok {
 		return nil


+		controllerutil.AddFinalizer(wekaCluster, consts.DeletionProtectionFinalizer)
 		if updated := controllerutil.AddFinalizer(wekaCluster, consts.WekaFinalizer); updated {


+	controllerutil.AddFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)
 	if ok := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer); !ok {
 		return nil


+	// DeletionProtectionFinalizer is a guard finalizer with a clear name to prevent
+	// accidental deletion by automation or AI agents. It is added alongside WekaFinalizer
+	// and removed only during intentional operator-driven deletion flows.
+	DeletionProtectionFinalizer = "do-not-delete.protection.io/guard"


… removed manually

claude · 2026-05-31T13:24:21Z

Claude finished @assafgi's task in 37s —— View job

Claude PR Review

Compare current PR state against prior review
Exit early — no code changes since last review

Status: no new changes since prior review

The current PR head is still commit 5293e20b with the same six files and same +27/−13 diff that I reviewed at 12:44 UTC. The prior review feedback still stands — re-posting it below for convenience.

Summary (unchanged)

The intent — adding a second, human-readable guard finalizer alongside WekaFinalizer so the name itself signals "do not strip manually" — is reasonable and the diff is small and consistent across the three controllers. However, the rollout has a real correctness issue that I think should be fixed before merge.

Findings (see inline comments on the prior review for details and suggested fixes):

🐞 Bug — guard finalizer is never persisted on existing resources (wekaclient/client_reconciler_loop.go:265, wekacontainer/funcs_active_state_init.go:83, wekacluster/steps_cluster_creation.go:140-152).
AddFinalizer returns false when the finalizer is already present. In all three "ensure"/"init" sites, the code reaches Update() only when WekaFinalizer was missing — so any resource that already has WekaFinalizer (i.e. every resource that exists today) gets the new guard finalizer added in-memory but never written back. The protection effectively only applies to freshly created resources, which probably isn't the intent. Copilot flagged the same issue independently.
🎨 Naming — protection.io is not a domain this project owns (internal/consts/consts.go:11). All sibling constants use weka.io. Suggest do-not-delete.weka.io/guard to keep the semantic signal while staying inside an owned domain.
🧹 Consistency — finalizer-remove ordering differs across controllers (wekacluster/steps_deletion.go:216). Not a bug, but wekacluster removes the guard before WekaFinalizer; the other two do the opposite. Pick one order across the codebase.
🧪 No tests. A small unit test asserting that (a) both finalizers are added together on init and (b) both are removed on deletion would lock in the contract this PR is establishing, and would have caught issue Operator sdk #1.

Issue #1 is the main blocker — please address before merge.

--- · Branch: 05-31-fix_add_do-not-delete_finalizer_to_signal_the_finalizer_shouldn_t_be_removed_manually

assafgi marked this pull request as ready for review May 31, 2026 12:44

assafgi requested a review from a team as a code owner May 31, 2026 12:44

Copilot AI review requested due to automatic review settings May 31, 2026 12:44

graphite-app Bot requested review from kristina-solovyova and tigrawap May 31, 2026 12:44

Copilot started reviewing on behalf of assafgi May 31, 2026 12:44 View session

claude Bot reviewed May 31, 2026

View reviewed changes

Copilot AI reviewed May 31, 2026

View reviewed changes

fix: add do-not-delete finalizer to signal the finalizer shouldn't be…

5293e20

… removed manually

assafgi force-pushed the 05-31-fix_add_do-not-delete_finalizer_to_signal_the_finalizer_shouldn_t_be_removed_manually branch from f21ffdb to 5293e20 Compare May 31, 2026 13:24

assafgi mentioned this pull request May 31, 2026

fix: add admission control to block finalizers removal not made by the operator service account #2575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add do-not-delete finalizer to signal the finalizer shouldn't be removed manually#2574

fix: add do-not-delete finalizer to signal the finalizer shouldn't be removed manually#2574
assafgi wants to merge 1 commit into
mainfrom
05-31-fix_add_do-not-delete_finalizer_to_signal_the_finalizer_shouldn_t_be_removed_manually

assafgi commented May 31, 2026

Uh oh!

github-actions Bot commented May 31, 2026 •

edited

Loading

Uh oh!

assafgi commented May 31, 2026 •

edited

Loading

Uh oh!

claude Bot commented May 31, 2026 •

edited

Loading

Uh oh!

graphite-app Bot commented May 31, 2026

Uh oh!

claude Bot May 31, 2026

Uh oh!

claude Bot May 31, 2026

Uh oh!

claude Bot May 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

claude Bot commented May 31, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		controllerutil.AddFinalizer(wekaCluster, consts.DeletionProtectionFinalizer)
		if updated := controllerutil.AddFinalizer(wekaCluster, consts.WekaFinalizer); updated {

Conversation

assafgi commented May 31, 2026

Uh oh!

github-actions Bot commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

assafgi commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude Bot commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude PR Review

Summary

Uh oh!

graphite-app Bot commented May 31, 2026

Graphite Automations

Uh oh!

claude Bot May 31, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 31, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 31, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

claude Bot commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude PR Review

Status: no new changes since prior review

Summary (unchanged)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented May 31, 2026 •

edited

Loading

assafgi commented May 31, 2026 •

edited

Loading

claude Bot commented May 31, 2026 •

edited

Loading

claude Bot commented May 31, 2026 •

edited

Loading