Skip to content

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

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

Open
assafgi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions internal/consts/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ package consts
const (
// WekaFinalizer is the finalizer added to Weka resources to ensure proper cleanup
WekaFinalizer = "weka.weka.io/finalizer"

// 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.weka.io/unsafe"
)

// Node annotation keys for drive management
Expand Down
5 changes: 4 additions & 1 deletion internal/controllers/wekaclient/client_reconciler_loop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ func (c *clientReconcilerLoop) HandleDeletion(ctx context.Context) error {
return err
}

controllerutil.RemoveFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)
controllerutil.RemoveFinalizer(c.wekaClient, consts.WekaFinalizer)
if err := c.Update(ctx, c.wekaClient); err != nil {
logger.Error(err, "Error removing finalizer")
Expand Down Expand Up @@ -260,7 +261,9 @@ func (c *clientReconcilerLoop) ensureFinalizer(ctx context.Context) error {
logger := instrumentation.CurrentSpanLogger(ctx)

logger.Info("Adding Finalizer for weka client")
if ok := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer); !ok {
addedGuard := controllerutil.AddFinalizer(c.wekaClient, consts.DeletionProtectionFinalizer)
addedWeka := controllerutil.AddFinalizer(c.wekaClient, consts.WekaFinalizer)
if !addedGuard && !addedWeka {
return nil
}
Comment on lines 263 to 268

@claude claude Bot May 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

  1. AddFinalizer(DeletionProtectionFinalizer) — mutates the in-memory object.
  2. AddFinalizer(WekaFinalizer) returns false → early return nil.
  3. 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 →


Expand Down
24 changes: 13 additions & 11 deletions internal/controllers/wekacluster/steps_cluster_creation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,22 @@ func (r *wekaClusterReconcilerLoop) InitState(ctx context.Context) error {
if err != nil {
logger.Error(err, "failed to init states")
}
}

if updated := controllerutil.AddFinalizer(wekaCluster, consts.WekaFinalizer); updated {
logger.Info("Adding Finalizer for weka cluster")
if err := r.getClient().Update(ctx, wekaCluster); err != nil {
logger.Error(err, "Failed to update custom resource to add finalizer")
return err
}
addedGuard := controllerutil.AddFinalizer(wekaCluster, consts.DeletionProtectionFinalizer)
addedWeka := controllerutil.AddFinalizer(wekaCluster, consts.WekaFinalizer)
if addedGuard || addedWeka {
logger.Info("Adding Finalizer for weka cluster")
if err := r.getClient().Update(ctx, wekaCluster); err != nil {
logger.Error(err, "Failed to update custom resource to add finalizer")
return err
}

if err := r.getClient().Get(ctx, client.ObjectKey{Namespace: wekaCluster.Namespace, Name: wekaCluster.Name}, r.cluster); err != nil {
logger.Error(err, "Failed to re-fetch data")
return err
}
logger.Info("Finalizer added for wekaCluster", "conditions", len(wekaCluster.Status.Conditions))
if err := r.getClient().Get(ctx, client.ObjectKey{Namespace: wekaCluster.Namespace, Name: wekaCluster.Name}, r.cluster); err != nil {
logger.Error(err, "Failed to re-fetch data")
return err
}
logger.Info("Finalizer added for wekaCluster", "conditions", len(wekaCluster.Status.Conditions))
}

clusterGuid := string(wekaCluster.GetUID())
Expand Down
1 change: 1 addition & 0 deletions internal/controllers/wekacluster/steps_deletion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ func (r *wekaClusterReconcilerLoop) HandleDeletion(ctx context.Context) error {
}

logger.Info("Removing Finalizer for wekaCluster after successfully perform the operations")
controllerutil.RemoveFinalizer(r.cluster, consts.DeletionProtectionFinalizer)
if ok := controllerutil.RemoveFinalizer(r.cluster, consts.WekaFinalizer); !ok {
err := errors.New("Failed to remove finalizer for wekaCluster")
return err
Expand Down
4 changes: 3 additions & 1 deletion internal/controllers/wekacontainer/funcs_active_state_init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ func (r *containerReconcilerLoop) ensureFinalizer(ctx context.Context) error {

container := r.container

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

Expand Down
1 change: 1 addition & 0 deletions internal/controllers/wekacontainer/funcs_common_deletion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func (r *containerReconcilerLoop) HandleDeletion(ctx context.Context) error {
if err != nil {
return err
}
controllerutil.RemoveFinalizer(r.container, consts.DeletionProtectionFinalizer)
controllerutil.RemoveFinalizer(r.container, consts.WekaFinalizer)
err = r.Update(ctx, r.container)
if err != nil {
Expand Down
Loading