Skip to content

[controllers] Fix maxUnhealthyCount bypass #4420

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

Draft
jrvaldes wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[controllers] Fix maxUnhealthyCount bypass #4420

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
66 changes: 47 additions & 19 deletions controllers/windowsmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net"
"strings"
"time"

oconfig "github.com/openshift/api/config/v1"
mapi "github.com/openshift/api/machine/v1beta1"
Expand Down Expand Up @@ -54,13 +55,20 @@ const (
WindowsMachineController = "windowsmachine"
// IgnoreLabel is a label that will cause machines to be ignored by the Windows Machine controller
IgnoreLabel = "windowsmachineconfig.openshift.io/ignore"
// machineDeletionRestrictedRequeueInterval is the amount of time to wait before re-checking whether a Machine
// whose deletion was restricted by maxUnhealthyCount is now allowed to be deleted. Using a backoff here (instead
// of an immediate requeue) avoids a tight reconcile loop and event/log spam while a sibling Machine's
// remediation is still in progress.
machineDeletionRestrictedRequeueInterval = 30 * time.Second
)

// WindowsMachineReconciler is used to create a controller which manages Windows Machine objects
type WindowsMachineReconciler struct {
instanceReconciler
// machineClient holds the information for machine client
machineClient *mclient.MachineV1beta1Client
// This is typed as the interface (rather than the concrete *mclient.MachineV1beta1Client) so that it can be
// substituted with a fake implementation in unit tests.
machineClient mclient.MachineV1beta1Interface
}

// NewWindowsMachineReconciler returns a pointer to a WindowsMachineReconciler
Expand Down Expand Up @@ -281,18 +289,7 @@ func (r *WindowsMachineReconciler) Reconcile(ctx context.Context,
if node.Annotations[nodeconfig.PubKeyHashAnnotation] !=
nodeconfig.CreatePubKeyHashAnnotation(r.signer.PublicKey()) {
log.Info("deleting machine")
deletionAllowed, err := r.isAllowedDeletion(ctx, machine)
if err != nil {
return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err)
}
if !deletionAllowed {
log.Info("machine deletion restricted", "maxUnhealthyCount", maxUnhealthyCount)
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted",
"Machine %v deletion restricted as the maximum unhealthy machines can`t exceed %v count",
machine.Name, maxUnhealthyCount)
return ctrl.Result{Requeue: true}, nil
}
return ctrl.Result{}, r.deleteMachine(ctx, machine)
return r.deleteMachineIfAllowed(ctx, machine, "private key out of date")
}
if node.Annotations[metadata.VersionAnnotation] == version.Get() {
// version annotation exists with a valid value, node is fully configured.
Expand Down Expand Up @@ -339,7 +336,7 @@ func (r *WindowsMachineReconciler) Reconcile(ctx context.Context,
// re-provisioned.
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineSetupFailure",
"Machine %s authentication failure", machine.Name)
return ctrl.Result{}, r.deleteMachine(ctx, machine)
return r.deleteMachineIfAllowed(ctx, machine, "authentication failure")

@coderabbitai coderabbitai Bot Aug 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Evaluate the deletion budget after deletion.

The authentication-failure path can run for a Provisioned Machine. That target is already unhealthy. With two replicas, one healthy sibling, and maxUnhealthyCount == 1, the gate returns RequeueAfter instead of deleting the failed target. The target then cannot be replaced.

Calculate the post-deletion unhealthy count. Exclude the target from the sibling health count, then allow deletion when the post-deletion count is <= maxUnhealthyCount. Add a regression case in TestDeleteMachineIfAllowed for a Provisioned target and reason "authentication failure".

Also applies to: 355-371

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/windowsmachine_controller.go` at line 339, Update
deleteMachineIfAllowed and its authentication-failure caller to evaluate the
unhealthy budget after removing the target: exclude the target from the sibling
health count and permit deletion when the resulting count is <=
maxUnhealthyCount, including Provisioned targets. Add a regression case to
TestDeleteMachineIfAllowed covering a Provisioned target with reason
"authentication failure".

@jrvaldes jrvaldes Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@coderabbitai create a commit; verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @controllers/windowsmachine_controller.go at line 339, Update
deleteMachineIfAllowed and its authentication-failure caller to evaluate the
unhealthy budget after removing the target: exclude the target from the sibling
health count and permit deletion when the resulting count is <=
maxUnhealthyCount, including Provisioned targets. Add a regression case to
TestDeleteMachineIfAllowed covering a Provisioned target with reason
"authentication failure".

@coderabbitai coderabbitai Bot Aug 4, 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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline controllers/windowsmachine_controller.go --items all --type function --match 'deleteMachineIfAllowed|isAllowedDeletion|isWindowsMachineHealthy|Reconcile'
ast-grep outline controllers/windowsmachine_controller_deletion_test.go --items all --type function --match 'TestDeleteMachineIfAllowed|TestIsAllowedDeletion|new|fixture|event'

rg -n -C 8 'func \(r \*WindowsMachineReconciler\) (deleteMachineIfAllowed|isAllowedDeletion)|authentication failure' \
  controllers/windowsmachine_controller.go

rg -n -C 12 'func TestDeleteMachineIfAllowed|authentication failure|Provisioned|MachineDeletionRestricted' \
  controllers/windowsmachine_controller_deletion_test.go

Length of output: 9277

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '36,138p' controllers/windowsmachine_controller_deletion_test.go
sed -n '139,282p' controllers/windowsmachine_controller_deletion_test.go
sed -n '463,525p' controllers/windowsmachine_controller.go

rg -n -C 4 'OwnerReferences\[0\]|UID|ObjectKey|newTestMachine\(' \
  controllers/windowsmachine_controller_deletion_test.go controllers/windowsmachine_controller.go

Length of output: 31207

@jrvaldes, I verified the finding. It remains valid.

The authentication-failure caller at Line 339 already calls deleteMachineIfAllowed. Do not change that caller.

The minimal fix is in isAllowedDeletion:

  • Exclude the target from the healthy sibling count.
  • Check ma.OwnerReferences before reading ma.OwnerReferences[0].
  • Calculate the post-deletion unhealthy count as MachineSet replicas - healthy siblings.
  • Allow deletion when that count is <= maxUnhealthyCount.

This preserves the existing restricted case where a healthy target has an unhealthy sibling. Deleting that target produces two unhealthy Machines. It allows the requested Provisioned target case because the healthy sibling remains and the post-deletion unhealthy count is one.

I cannot create a commit directly. The verified patch should also add one focused TestDeleteMachineIfAllowed case:

  • reason: "authentication failure"
  • target phase: Provisioned
  • sibling: healthy and Running
  • MachineSet replicas: 2
  • expected result: no requeue and target deletion
  • expected events: no MachineDeletionRestricted event

You are interacting with an AI system.

}
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineSetupFailure",
"Machine %s configuration failure", machine.Name)
Expand All @@ -350,6 +347,30 @@ func (r *WindowsMachineReconciler) Reconcile(ctx context.Context,
return ctrl.Result{}, nil
}

// deleteMachineIfAllowed deletes the given Machine if doing so would not cause the number of unhealthy Machines
// in its MachineSet to reach or exceed maxUnhealthyCount. This is the single safety gate used by every Machine
// remediation path (e.g. authentication failure, stale private key), ensuring they are all bound by the same
// disruption budget rather than some paths being gated and others deleting unconditionally.
// reason is a short human-readable description of why deletion is being attempted, used for logging/events.
func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine,
reason string) (ctrl.Result, error) {
deletionAllowed, err := r.isAllowedDeletion(ctx, machine)
if err != nil {
return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err)
}
if !deletionAllowed {
r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason,
"maxUnhealthyCount", maxUnhealthyCount)
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted",
"Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count",
machine.Name, reason, maxUnhealthyCount)
// Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while
// waiting for a sibling Machine's remediation to complete.
return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil
}
return ctrl.Result{}, r.deleteMachine(ctx, machine)
Comment on lines +355 to +371

@coderabbitai coderabbitai Bot Aug 4, 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.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Preserve the no-op for a Machine already deleting.

deleteMachine returns successfully when DeletionTimestamp is set. This wrapper checks the budget first. If the budget is full, an already-deleting Machine emits MachineDeletionRestricted and requeues instead of returning success.

Return before isAllowedDeletion when deletion has already started. Add a wrapper-level test for this case.

Proposed fix 
 func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine,
 	reason string) (ctrl.Result, error) {
+	if !machine.GetDeletionTimestamp().IsZero() {
+		return ctrl.Result{}, nil
+	}
 	deletionAllowed, err := r.isAllowedDeletion(ctx, machine)

As per path instructions, “Check reconciliation loop logic and idempotency.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine,
reason string) (ctrl.Result, error) {
deletionAllowed, err := r.isAllowedDeletion(ctx, machine)
if err != nil {
return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err)
}
if !deletionAllowed {
r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason,
"maxUnhealthyCount", maxUnhealthyCount)
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted",
"Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count",
machine.Name, reason, maxUnhealthyCount)
// Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while
// waiting for a sibling Machine's remediation to complete.
return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil
}
return ctrl.Result{}, r.deleteMachine(ctx, machine)
func (r *WindowsMachineReconciler) deleteMachineIfAllowed(ctx context.Context, machine *mapi.Machine,
reason string) (ctrl.Result, error) {
if !machine.GetDeletionTimestamp().IsZero() {
return ctrl.Result{}, nil
}
deletionAllowed, err := r.isAllowedDeletion(ctx, machine)
if err != nil {
return ctrl.Result{}, fmt.Errorf("unable to determine if Machine can be deleted: %w", err)
}
if !deletionAllowed {
r.log.Info("machine deletion restricted", "name", machine.GetName(), "reason", reason,
"maxUnhealthyCount", maxUnhealthyCount)
r.recorder.Eventf(machine, core.EventTypeWarning, "MachineDeletionRestricted",
"Machine %v deletion restricted (%s) as the maximum unhealthy machines can`t exceed %v count",
machine.Name, reason, maxUnhealthyCount)
// Requeue with a backoff rather than immediately, to avoid a tight reconcile loop and event spam while
// waiting for a sibling Machine's remediation to complete.
return ctrl.Result{RequeueAfter: machineDeletionRestrictedRequeueInterval}, nil
}
return ctrl.Result{}, r.deleteMachine(ctx, machine)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/windowsmachine_controller.go` around lines 355 - 371, Update
deleteMachineIfAllowed to return successfully before calling isAllowedDeletion
when machine.DeletionTimestamp is already set, preserving deleteMachine’s no-op
behavior for in-progress deletion. Add a wrapper-level test covering an
already-deleting Machine and verifying it neither checks the deletion budget nor
emits a restriction event or requeues.

Source: Path instructions

@coderabbitai coderabbitai Bot Aug 4, 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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Wrap the delete error with operation context.

Line 371 returns the raw deleteMachine error. Wrap it so the reconciliation error identifies the safety-gated delete operation.

Proposed fix 
-	return ctrl.Result{}, r.deleteMachine(ctx, machine)
+	if err := r.deleteMachine(ctx, machine); err != nil {
+		return ctrl.Result{}, fmt.Errorf("delete machine %q: %w", machine.Name, err)
+	}
+	return ctrl.Result{}, nil

As per coding guidelines, “In Go code, wrap errors with context using fmt.Errorf with %w.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return ctrl.Result{}, r.deleteMachine(ctx, machine)
if err := r.deleteMachine(ctx, machine); err != nil {
return ctrl.Result{}, fmt.Errorf("delete machine %q: %w", machine.Name, err)
}
return ctrl.Result{}, nil
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/windowsmachine_controller.go` at line 371, Update the
reconciliation return around deleteMachine to wrap its error with fmt.Errorf
using %w, including context that identifies the safety-gated machine deletion
operation. Preserve the existing ctrl.Result{} return and deleteMachine(ctx,
machine) behavior.

Source: Coding guidelines

}

// deleteMachine deletes the specified Machine
func (r *WindowsMachineReconciler) deleteMachine(ctx context.Context, machine *mapi.Machine) error {
if !machine.GetDeletionTimestamp().IsZero() {
Expand Down Expand Up @@ -470,6 +491,9 @@ func (r *WindowsMachineReconciler) isAllowedDeletion(ctx context.Context, machin
for _, ma := range machines.Items {
// Increment the count if the machine is identified as healthy and is a part of given Windows MachineSet and
// on which deletion is not already initiated.
// Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated
// non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for
// clarity/safety in case this function is refactored again in the future.
if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&
r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() {
Comment on lines +494 to 498

@coderabbitai coderabbitai Bot Aug 4, 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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard the loop item's owner references.

The condition validates machine.OwnerReferences, but it indexes ma.OwnerReferences[0]. A Windows-labeled Machine without owner references panics the controller during deletion evaluation.

Check len(ma.OwnerReferences) before indexing. Add a regression fixture for an ownerless sibling Machine.

Proposed fix 
-		if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&
+		if len(ma.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&

As per path instructions, “Verify ... owner references.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated
// non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for
// clarity/safety in case this function is refactored again in the future.
if len(machine.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&
r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() {
// Note: `len(machine.OwnerReferences) != 0` here refers to the outer `machine` parameter (already validated
// non-empty at the top of this function), not the loop variable `ma`. It is redundant but kept for
// clarity/safety in case this function is refactored again in the future.
if len(ma.OwnerReferences) != 0 && ma.OwnerReferences[0].Name == machinesetName &&
r.isWindowsMachineHealthy(ctx, &ma) && ma.DeletionTimestamp.IsZero() {
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/windowsmachine_controller.go` around lines 494 - 498, Update the
condition in the loop using ma so it checks len(ma.OwnerReferences) before
accessing ma.OwnerReferences[0], replacing the redundant machine.OwnerReferences
guard while preserving the existing owner-name, health, and deletion checks. Add
a regression fixture covering an ownerless sibling Windows-labeled Machine
during deletion evaluation.

Source: Path instructions

totalHealthy += 1
Expand All @@ -489,14 +513,18 @@ func (r *WindowsMachineReconciler) isAllowedDeletion(ctx context.Context, machin
// 2. Machine is not associated with a Node object
// 3. Associated Node object doesn't have a Version annotation
func (r *WindowsMachineReconciler) isWindowsMachineHealthy(ctx context.Context, machine *mapi.Machine) bool {
if (machine.Status.Phase == nil || *machine.Status.Phase != "Running") &&
machine.Status.NodeRef == nil {
// Note: these conditions must be OR'd, not AND'd. A previous version of this check used `&&`, which meant a
// Machine reporting phase "Running" with a nil NodeRef would fall through to the NodeRef.Name dereference
// below and panic, instead of correctly being treated as unhealthy.
if machine.Status.Phase == nil || *machine.Status.Phase != "Running" || machine.Status.NodeRef == nil {
return false
}

// Get node associated with the machine
node, err := r.k8sclientset.CoreV1().Nodes().Get(ctx, machine.Status.NodeRef.Name, meta.GetOptions{})
if err != nil {
// Get node associated with the machine. Use the cached controller-runtime client (consistent with how the
// Node is fetched in Reconcile) rather than k8sclientset, so this function can be exercised in unit tests
// against a fake client.
node := &core.Node{}
if err := r.client.Get(ctx, kubeTypes.NamespacedName{Name: machine.Status.NodeRef.Name}, node); err != nil {
return false
}
_, present := node.Annotations[metadata.VersionAnnotation]
Expand Down
Loading