Add node maintenance window controller - #7
Conversation
Adds a kube-controller-manager controller that cordons and drains nodes during administrator-declared recurring maintenance windows (annotated per-node) and automatically returns them to service once the window closes. Cluster-wide behavior (max concurrent nodes in maintenance, drain timeout, eviction grace period) is tunable via a ConfigMap so it doesn't require a manager restart to change.
|
| Severity | File | Description |
|---|---|---|
| 🔴 Critical | …/nodemaintenance/drain.go |
WaitGroup misuse: Add called inside goroutine while Wait may |
⬇️ High (4)
| Severity | File | Description |
|---|---|---|
| 🟠 High | …/nodemaintenance/controller.go |
Concurrency slot leaked when CordonNode fails (Acquire witho |
| 🟠 High | …/nodemaintenance/drain.go |
Data race: concurrent goroutines append to shared errs slice |
| 🟠 High | …/nodemaintenance/cordon.go |
RetryOnConflict will keep retrying with stale node object (n |
| 🟠 High | …/nodemaintenance/schedule.go |
NextTransition can skip/never find the correct close time fo |
⬇️ Medium (2)
| Severity | File | Description |
|---|---|---|
| 🟡 Medium | …/app/controller_descriptor.go |
NodeMaintenance controller is added to the always-registered |
| 🟡 Medium | …/nodemaintenance/drain.go |
Compliance Violation 🔒 |
📖 Walkthrough
This change introduces a new NodeMaintenance controller to kube-controller-manager. It loads a maintenance policy from a ConfigMap, evaluates time-window schedules (including timezone handling), then cordons/uncordons nodes and drains evictable pods via eviction with configurable timeouts/behavior. It adds metrics for observability, comprehensive validation/defaulting for policy inputs, and a concurrency limiter fix to avoid races during resize. Extensive unit tests cover scheduling, policy reloads, cordon/drain logic, validation, and concurrency semantics.
🔀 Sequence
sequenceDiagram
participant KCM as kube-controller-manager
participant NMC as NodeMaintenanceController
participant CM as ConfigMap (Policy)
participant SCH as Scheduler (Windows/Timezone)
participant API as Kube API (Nodes/Pods)
participant EV as Eviction API
participant MET as Metrics
KCM->>NMC: Register & start controller loop
NMC->>CM: Load/parse policy (defaults + validation)
NMC->>SCH: Evaluate maintenance window for "now"
SCH-->>NMC: In-window / out-of-window decision
alt In maintenance window
NMC->>API: Cordon node(s)
NMC->>EV: Evict evictable pods (bounded concurrency)
NMC->>API: Wait for drain completion (timeouts)
NMC->>MET: Record outcomes/latencies
else Out of maintenance window
NMC->>API: Uncordon node(s) (if previously cordoned)
NMC->>MET: Record state
end
CM-->>NMC: Policy reload on change
📂 File Changes
📊 Changes by Category (6 categories)
📊 NodeMaintenance controller implementation (cordon/drain/metrics)
Implements the core NodeMaintenance controller behavior, including reconciliation logic, cordon/uncordon and drain workflows, controller metrics, and package-level documentation.
| Files | Summary |
|---|---|
pkg/controller/nodemaintenance/controller.gopkg/controller/nodemaintenance/cordon.gopkg/controller/nodemaintenance/drain.gopkg/controller/nodemaintenance/metrics.gopkg/controller/nodemaintenance/doc.go |
Introduces the nodemaintenance controller package implementing a policy-driven maintenance window controller with metrics and cordon/uncordon plus draining of evictable pods with configurable timeouts/eviction behavior, along with package documentation |
🔧 Maintenance policy + scheduling (ConfigMap-driven windows, validation)
Adds the policy model and parsing from ConfigMaps, defaulting/validation rules, and the scheduling/window evaluation logic (including timezone and transition handling).
| Files | Summary |
|---|---|
pkg/controller/nodemaintenance/policy.gopkg/controller/nodemaintenance/schedule.gopkg/controller/nodemaintenance/types.gopkg/controller/nodemaintenance/validation.go |
Adds ConfigMap-driven maintenance policy parsing (with defaults/validation), maintenance window scheduling logic (timezone and transition calculations), core types/annotations/default policy, and validation for Window/Policy objects (time formats and non-negative fields) |
🔧 Wire NodeMaintenance into kube-controller-manager
Registers the new controller with kube-controller-manager via controller name constants, descriptor registration, and runtime wiring so it can be started and managed like other controllers.
| Files | Summary |
|---|---|
cmd/kube-controller-manager/app/controller_descriptor.gocmd/kube-controller-manager/app/core.gocmd/kube-controller-manager/names/controller_names.go |
Registers and wires up the new NodeMaintenance controller in kube-controller-manager (adds controller name constant, descriptor registration, and controller loop wiring) |
🐛 Fix concurrency limiter race in NodeMaintenance internals
Resolves a high-severity race condition in the concurrency limiter by eliminating unsafe channel swapping and making Acquire/Release safe during in-flight operations.
| Files | Summary |
|---|---|
pkg/controller/nodemaintenance/concurrency.go |
Fixes a high-severity concurrency limiter race by preventing unsafe channel swapping during in-flight Acquire/Release and making Acquire/Release operate under the mutex |
🧪 NodeMaintenance controller behavior tests (incl. drain and policy reload)
Adds tests covering end-to-end controller behaviors, cordon/uncordon logic, draining/eviction behavior and wait semantics, and policy reload scenarios.
| Files | Summary |
|---|---|
pkg/controller/nodemaintenance/controller_test.gopkg/controller/nodemaintenance/cordon_test.go |
Adds tests covering node maintenance controller behaviors, cordon/uncordon logic, and policy reloading |
pkg/controller/nodemaintenance/drain_test.go |
Adds tests for eviction/draining behavior and wait logic in the node maintenance controller |
🧪 Unit tests for policy/scheduling/validation and concurrency limiter
Adds focused unit tests for policy parsing, schedule/window evaluation across timezones/transitions, validation/defaulting rules, and the concurrency limiter’s capacity/resize semantics.
| Files | Summary |
|---|---|
pkg/controller/nodemaintenance/policy_test.gopkg/controller/nodemaintenance/schedule_test.gopkg/controller/nodemaintenance/validation_test.go |
Adds unit tests for policy parsing from ConfigMap, maintenance window parsing/evaluation (timezone/transitions), and window/policy validation (defaults, overrides, and input validation) |
pkg/controller/nodemaintenance/concurrency_test.go |
Adds unit tests for the concurrency limiter (capacity semantics, zero/negative handling, and resize behavior) |
🔒 Compliance Review✅ Compliant — no findings detectedSummary
Impacted Standards
Compliance Status (Scanner)
Files Reviewed: 20
Violations by Framework
🧩 SECURE-CODING-STANDARDS, OWASP TOP 10, NIST SP 800-53, ISO 27001, PCI DSS — 1Secure Logging Practices (MEDIUM) —
Fixability
Recommended fix Reduce sensitive identifiers in error logs and/or ensure structured logging with
redaction. For example, log only a stable hash or UID, and avoid embedding raw error
strings that may include internal endpoints. If detailed identifiers are needed, gate
them behind a debug log level and ensure log access controls/retention are enforced.
Example change: log pod UID and a sanitized error category instead of namespace/name and
full error text.Standards
|
| Scanned commit | Branch | Scan time | Engine version | Scan completed |
|---|---|---|---|---|
fa34df7e5a3f01a29c51ca5af547e750e9524b68 |
master |
345s | v2.1.0 | Sat, 18 Jul 2026 08:16:37 GMT |
Compliance Engine: 85 rules across 6 frameworks
This review ensures compliance with regulatory and security standards.
![devzy-zero-index[bot]](https://avatars.githubusercontent.com/u/211546431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review complete
DevzyAi finished this review for this commit. Feedback is in the inline review comments on this diff.
Files selected (20)
- cmd/kube-controller-manager/app/controller_descriptor.go (1)
- cmd/kube-controller-manager/app/core.go (2)
- cmd/kube-controller-manager/names/controller_names.go (1)
- pkg/controller/nodemaintenance/concurrency.go (1)
- pkg/controller/nodemaintenance/concurrency_test.go (1)
- pkg/controller/nodemaintenance/controller.go (1)
- pkg/controller/nodemaintenance/controller_test.go (1)
- pkg/controller/nodemaintenance/cordon.go (1)
- pkg/controller/nodemaintenance/cordon_test.go (1)
- pkg/controller/nodemaintenance/doc.go (1)
- pkg/controller/nodemaintenance/drain.go (1)
- pkg/controller/nodemaintenance/drain_test.go (1)
- pkg/controller/nodemaintenance/metrics.go (1)
- pkg/controller/nodemaintenance/policy.go (1)
- pkg/controller/nodemaintenance/policy_test.go (1)
- pkg/controller/nodemaintenance/schedule.go (1)
- pkg/controller/nodemaintenance/schedule_test.go (1)
- pkg/controller/nodemaintenance/types.go (1)
- pkg/controller/nodemaintenance/validation.go (1)
- pkg/controller/nodemaintenance/validation_test.go (1)
Review comments generated (7)
- Review: 7
- LGTM: 0
Tips
Chat with DevzyAi Bot (@DevzyAi)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@DevzyAiin a reply.
See More
Interact with @DevzyAi in any bot review thread (Files changed tab):
| Command | Description |
|---|---|
@DevzyAi explain |
Get a detailed explanation of the code or issue |
@DevzyAi fix |
Generate a code fix suggestion |
@DevzyAi suggest |
Get alternative implementations |
@DevzyAi ignore |
Mark this as a false positive |
@DevzyAi review |
Trigger a full PR review (overrides ignore) |
@DevzyAi test |
Generate unit tests for file(s) |
@DevzyAi help |
Show this help message |
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@DevzyAi: ignoreanywhere in the PR description to pause further reviews from the bot.
Models: code-review → gpt-5.2 · summary → gpt-5-nano
| register(newEndpointSliceMirroringControllerDescriptor()) | ||
| register(newReplicationControllerDescriptor()) | ||
| register(newPodGarbageCollectorControllerDescriptor()) | ||
| register(newNodeMaintenanceControllerDescriptor()) |
There was a problem hiding this comment.
🟡 Medium
NodeMaintenance controller is added to the always-registered set without feature-gate or disabled-by-default
The new controller is registered alongside long-established controllers (outside the // feature gated block) and its descriptor does not set requiredFeatureGates or isDisabledByDefault. In kube-controller-manager, this typically means it will run by default when --controllers=* (or default controller set) is used.
Given this controller cordons/drains nodes, default-enabling is a high-impact behavior change if clusters have (or later add) the triggering annotations/configmap. Consider marking it feature-gated or disabled-by-default until the feature is GA.
Concrete mitigation (minimum): disable by default in the descriptor:
🛠️ Code Suggestions
func newNodeMaintenanceControllerDescriptor() *ControllerDescriptor {
return &ControllerDescriptor{
name: names.NodeMaintenanceController,
- aliases: []string{"node-maintenance"},
+ // Consider gating/alpha rollout; at minimum don't auto-enable.
+ isDisabledByDefault: true,
+ aliases: []string{"node-maintenance"},
constructor: newNodeMaintenanceController,
}
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open cmd/kube-controller-manager/app/controller_descriptor.go and confirm NodeMaintenance is registered in the always-on controller descriptor list (not inside a feature-gated section) and that newNodeMaintenanceControllerDescriptor() does not set requiredFeatureGates or isDisabledByDefault. If it’s already gated/disabled elsewhere (or intentionally always-on with explicit docs/release note), don’t change behavior.
2. PROBLEM & LOCATION
File: cmd/kube-controller-manager/app/controller_descriptor.go
Locate the NodeMaintenance controller descriptor (function newNodeMaintenanceControllerDescriptor) and the place where controller descriptors are added to the “always registered” set (outside the “// feature gated” block). The descriptor currently only sets name/aliases/constructor (pattern like name: names.NodeMaintenanceController, aliases: ..., constructor: ...) which means it will be enabled by default when the default controller set is used (e.g., --controllers=*), causing a high-impact behavioral change since this controller cordons/drains nodes.
3. FIX
Prefer feature-gating if a feature gate already exists for this controller:
- Search the codebase for an existing feature gate constant related to NodeMaintenance (look under pkg/features, staging/src/k8s.io/*/features, or similar).
- If found, add requiredFeatureGates: []featuregate.Feature{<that gate>} to the ControllerDescriptor returned by newNodeMaintenanceControllerDescriptor(), and ensure the controller is listed in the feature-gated controllers section if that’s the existing pattern in this file.
If no feature gate exists (minimum mitigation requested):
- Update newNodeMaintenanceControllerDescriptor() to set isDisabledByDefault: true in the returned ControllerDescriptor, keeping existing fields intact.
- Do not move the controller unless this file already has a standard pattern for “disabled-by-default” controllers; follow the existing pattern in this same file for other disabled-by-default descriptors (search for isDisabledByDefault usage and mirror it).
4. VERIFY
Run unit tests that cover controller descriptor registration / default controller sets (any tests under cmd/kube-controller-manager/app/ or related packages). Also sanity-check that kube-controller-manager still starts and that NodeMaintenance is not included in the default enabled controller list unless explicitly enabled (e.g., via --controllers=..., or by turning on the feature gate if you chose that path).
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| } | ||
|
|
||
| if err := CordonNode(ctx, c.kubeClient, node); err != nil { | ||
| cordonErrorsTotal.Inc() | ||
| return err |
There was a problem hiding this comment.
🟠 High
Concurrency slot leaked when CordonNode fails (Acquire without Release)
After successfully acquiring a limiter slot, the code returns early on cordon failure without releasing the slot. That will permanently reduce capacity (eventually to 0), causing the controller to requeue forever at capacity.
Fix: release the slot via a defer immediately after successful acquisition (and remove the manual Release after DrainNode, or keep it but guard against double-release).
🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open pkg/controller/nodemaintenance/controller.go and find where the controller acquires a concurrency/limiter slot before calling CordonNode. Confirm there is an early return/error path on CordonNode failure after a successful Acquire, and that Release is not executed on that path.
2. PROBLEM & LOCATION
File: pkg/controller/nodemaintenance/controller.go
Region: the reconcile/handler logic that does “Acquire limiter slot” then calls something like CordonNode(...) and later DrainNode(...).
Problem pattern: Acquire succeeds, then on cordon failure the code returns (e.g., “if err := CordonNode(...); err != nil { return ... }”) without releasing the acquired slot. This leaks capacity permanently and eventually deadlocks the controller at “at capacity” requeues.
3. FIX
Immediately after a successful Acquire, add a defer that always releases the slot on all exit paths from the function.
Then remove the existing manual Release that happens after DrainNode (or keep it only if you can guarantee it won’t double-release; prefer removing to avoid double-release bugs). The goal is: exactly one Release per successful Acquire, regardless of whether cordon/drain succeeds or fails.
4. VERIFY
Check any other reconcile paths in pkg/controller/nodemaintenance/controller.go that return early after Acquire (cordon, drain, patch/status update errors) and ensure the deferred Release covers them.
Run the controller unit/integration tests (or the package tests for pkg/controller/nodemaintenance) and, if available, an e2e that triggers a cordon failure and confirms subsequent reconciles still proceed with full limiter capacity.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| defer wg.Done() | ||
| if err := evictWithRetry(ctx, client, pod, policy, deadline); err != nil { | ||
| errs = append(errs, fmt.Errorf("evicting pod %s/%s: %w", pod.Namespace, pod.Name, err)) | ||
| } | ||
| }(pod) |
There was a problem hiding this comment.
🟠 High
Data race: concurrent goroutines append to shared errs slice without synchronization
Multiple goroutines append to errs concurrently, which is a Go data race and can corrupt the slice (lost writes/panic under race detector).
Fix: collect errors via a buffered channel, or guard errs = append(...) with a mutex.
🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open pkg/controller/nodemaintenance/drain.go and confirm errs is declared as a slice (e.g., var errs []error) and multiple goroutines append to it (errs = append(errs, ...)) without a mutex/channel. If errors are already collected via a channel or mutex, skip the fix.
2. PROBLEM & LOCATION
File: pkg/controller/nodemaintenance/drain.go
In the drain logic where goroutines are launched (likely inside a loop with a WaitGroup) and each goroutine does errs = append(errs, err) on failure, errs is shared across goroutines. Concurrent appends to a slice are a Go data race and can corrupt the slice (lost writes or panic), especially under the race detector.
3. FIX
Replace shared-slice appends with channel-based collection (preferred) or protect the slice with a mutex.
Channel approach:
- Before starting goroutines, create errCh := make(chan error, <number of goroutines or max expected errors>).
- In each goroutine, on error do: errCh <- err (do not append to errs).
- After wg.Wait(), close(errCh), then range over errCh and build errs sequentially in the main goroutine (or aggregate into a single error as the function expects).
- Ensure no goroutine can block forever on send: make the channel sufficiently buffered, or use a non-blocking send with a fallback (but buffered sized to goroutine count is simplest).
Mutex approach (if minimal change is required):
- Declare a sync.Mutex near errs.
- Replace each errs = append(errs, err) inside goroutines with mu.Lock(); errs = append(errs, err); mu.Unlock().
4. VERIFY
Run go test ./... and specifically any tests covering node maintenance/drain behavior. Also run with the race detector: go test -race ./... to confirm the data race is gone.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| deleter := &podForceDeleter{client: client} | ||
| for _, pod := range remaining { | ||
| go deleter.run(ctx, pod) | ||
| } | ||
| deleter.wait() |
There was a problem hiding this comment.
🔴 Critical
WaitGroup misuse: Add called inside goroutine while Wait may already be running
In waitForPodsGone(), goroutines are started with go deleter.run(...), but run() calls d.wg.Add(1) inside the goroutine. Since deleter.wait() (wg.Wait) is called immediately after launching the goroutines, Wait() can race with Add(), leading to sync: WaitGroup misuse: Add called concurrently with Wait panic or Wait returning before all deletes are accounted for.
Fix by incrementing the WaitGroup counter before starting each goroutine (or avoid the helper and inline the goroutine with a pre-Add).
🛠️ Code Suggestions
- deleter := &podForceDeleter{client: client}
- for _, pod := range remaining {
- go deleter.run(ctx, pod)
- }
- deleter.wait()
+ deleter := &podForceDeleter{client: client}
+ for _, pod := range remaining {
+ deleter.wg.Add(1)
+ go func(p *v1.Pod) {
+ defer deleter.wg.Done()
+ forceDeletePod(ctx, deleter.client, p)
+ }(pod)
+ }
+ deleter.wait()🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open pkg/controller/nodemaintenance/drain.go and inspect waitForPodsGone() and podForceDeleter.run()/wait(). Confirm whether wg.Add(1) happens inside the goroutine while wg.Wait() can already be running; if Add is already done before launching goroutines, skip.
2. PROBLEM & LOCATION
File: pkg/controller/nodemaintenance/drain.go
In waitForPodsGone(), remaining pods are processed with a loop that does `go deleter.run(ctx, pod)` and then calls `deleter.wait()` immediately after. In podForceDeleter.run(), the pattern is effectively “wg.Add(1) inside run()” (i.e., inside the goroutine), which can race with deleter.wait() (wg.Wait). This can panic with “sync: WaitGroup misuse: Add called concurrently with Wait” or allow Wait to return before all deletes are tracked, causing premature exit and leaving pods undeleted.
3. FIX
Move the WaitGroup increment to the caller, before starting each goroutine, so Add always happens before any possible Wait.
Option A (keep run): change podForceDeleter.run() to NOT call wg.Add(1); make it only do the work and ensure it calls wg.Done() via defer. Then in waitForPodsGone(), do deleter.wg.Add(1) immediately before each `go deleter.run(...)`.
Option B (inline goroutine): in waitForPodsGone(), replace `go deleter.run(ctx, pod)` with an inline goroutine that does `defer deleter.wg.Done()` and calls the same force-delete logic, and add `deleter.wg.Add(1)` before launching it.
Ensure there is no code path where run() can be called without a matching pre-Add, and that Done is always executed exactly once per Add (use defer at the top of the goroutine).
4. VERIFY
Run unit tests for the nodemaintenance controller package and any drain-related tests.
Also do a quick local stress check (if available) by simulating multiple remaining pods to ensure no panic occurs and waitForPodsGone() blocks until all delete attempts complete.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| updated := node.DeepCopy() | ||
| updated.Spec.Unschedulable = false | ||
| delete(updated.Annotations, AnnotationCordonedBy) | ||
| _, updateErr := client.CoreV1().Nodes().Update(ctx, updated, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
🟠 High
RetryOnConflict will keep retrying with stale node object (no refetch)
retry.RetryOnConflict expects the closure to re-read the latest object on each retry. Here it always starts from the same node passed into UncordonNode. If the first Update hits a conflict, subsequent retries will likely conflict again because the resourceVersion is never refreshed.
Fix: inside the retry closure, Get() the latest node, mutate it, then Update().
🤖 AI Fix Prompt - Copy this into your AI coding agent
VALIDATION
Open pkg/controller/nodemaintenance/cordon.go and inspect UncordonNode (and any similar CordonNode logic). Confirm it uses retry.RetryOnConflict with a closure that updates a Node object captured from outside the closure (not re-fetched), so resourceVersion is stale on retries.
PROBLEM & LOCATION
File: pkg/controller/nodemaintenance/cordon.go
In UncordonNode, locate the retry.RetryOnConflict(...) call where the closure does something like: take the node argument, mutate node.Spec.Unschedulable / labels / annotations, then call client.CoreV1().Nodes().Update(ctx, node, ...).
This is wrong because RetryOnConflict expects each retry attempt to re-read the latest Node; otherwise the same stale resourceVersion is reused and conflicts will repeat, causing unnecessary retries or failure.
FIX
Change the RetryOnConflict closure to re-fetch the Node every attempt, then apply the uncordon mutation to that freshly read object, then Update it.
Use the node’s name from the original argument (or a separate nodeName parameter) to Get() the latest object inside the closure, e.g. latest := client.CoreV1().Nodes().Get(ctx, node.Name, ...), mutate latest, then Update(ctx, latest, ...). Ensure you return errors from Get/Update so RetryOnConflict can retry on conflicts but surface non-conflict errors.
If there’s a cordon implementation elsewhere that already does “Get inside retry, mutate, Update”, mirror that pattern for consistency.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| for offset := 0; offset < 7; offset++ { | ||
| day := local.AddDate(0, 0, offset) | ||
| if !w.dayMatches(day.Weekday()) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟠 High
NextTransition can skip/never find the correct close time for overnight windows (especially with restricted Days)
NextTransition() picks targetMin := endMin when active == true, then searches for the next candidate only on days that satisfy w.dayMatches(day.Weekday()). For overnight windows with restricted Days, the close boundary typically occurs on the next calendar day (which may not be in Days), so the loop can skip the real close time and potentially return the next open time (or error).
This is visible in:
- Filtering close candidates by
dayMatches(day.Weekday())even though the close boundary’s calendar day is not necessarily the recurrence day. - The candidate day selection doesn’t account for "close is next day when end < start".
Given controller.go uses next.Sub(now) to requeue, this can cause incorrect requeue delays (missing the close boundary).
Fix: when active and startMin > endMin, compute the close boundary on the correct day without applying dayMatches() to the close day. One minimal approach is to explicitly compute the close timestamp based on whether we’re in the late (>= startMin) or early (< endMin) portion.
🛠️ Code Suggestions
func (w *Window) NextTransition(now time.Time) (time.Time, error) {
loc := w.location()
local := now.In(loc)
@@
active, err := w.IsActive(now)
if err != nil {
return time.Time{}, err
}
- targetMin := startMin
- if active {
- targetMin = endMin
- }
+ nowMin := local.Hour()*60 + local.Minute()
+
+ // If active, return the close boundary directly (handles overnight correctly).
+ if active {
+ closeDay := local
+ if startMin > endMin {
+ // If we're in the late portion (>= start), close is next day at endMin.
+ if nowMin >= startMin {
+ closeDay = local.AddDate(0, 0, 1)
+ }
+ // If we're in the early portion (< end), close is today at endMin.
+ }
+ candidate := time.Date(closeDay.Year(), closeDay.Month(), closeDay.Day(), endMin/60, endMin%60, 0, 0, loc)
+ if candidate.After(local) {
+ return candidate, nil
+ }
+ // fall through to search if somehow equal/past
+ }
+
+ targetMin := startMin
@@
- if !w.dayMatches(day.Weekday()) {
+ if !w.dayMatches(day.Weekday()) {
continue
}
candidate := time.Date(day.Year(), day.Month(), day.Day(), targetMin/60, targetMin%60, 0, 0, loc)
if candidate.After(local) {
return candidate, nil
}
}(Alternative: refactor to compute next open/close candidates separately and pick the earliest.)
🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open pkg/controller/nodemaintenance/schedule.go and read Window.NextTransition and Window.IsActive. Reproduce mentally (or with a quick unit test) an overnight window where startMin > endMin and Days is restricted (e.g., only Monday) and confirm NextTransition returns the next open (or errors) instead of the imminent close boundary.
2. PROBLEM & LOCATION
pkg/controller/nodemaintenance/schedule.go, function (*Window) NextTransition(now time.Time).
The code sets targetMin := endMin when active == true, then iterates day-by-day but skips any day where !w.dayMatches(day.Weekday()). For overnight windows with restricted Days, the close boundary can occur on the next calendar day (which may not match Days), so filtering close candidates by dayMatches causes the loop to skip the real close time and potentially return the next open boundary or fail. This breaks controller requeue timing because callers use next.Sub(now).
3. FIX
In NextTransition, when active == true, compute and return the close timestamp directly without applying dayMatches to the close calendar day.
Implement minimal logic:
- Compute nowMin from local time.
- If startMin > endMin (overnight):
- If nowMin >= startMin (late portion), close is next calendar day at endMin.
- Else (early portion), close is today at endMin.
- If startMin <= endMin (non-overnight), close is today at endMin.
Create candidate := time.Date(closeDay.Y/M/D, endMin/60, endMin%60, 0, 0, loc) and if candidate.After(local) return it.
If candidate is not after local (edge cases like exact boundary), fall through to existing logic to find the next open boundary (which should still use dayMatches).
Keep dayMatches filtering for computing the next open (startMin) only.
4. VERIFY
Run any existing tests covering Window.NextTransition / scheduling behavior.
Add/adjust a focused test (in the existing schedule test file, wherever other Window tests live) for:
- Overnight window (start 23:00, end 02:00), Days = [Monday], now = Monday 23:30 local -> NextTransition should be Tuesday 02:00 local (even though Tuesday isn’t in Days).
- Overnight window, Days = [Monday], now = Tuesday 01:00 local -> NextTransition should be Tuesday 02:00 local.
Then run the controller/unit test suite that depends on scheduling/requeue timing.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
| zero := int64(0) | ||
| err := client.CoreV1().Pods(pod.Namespace).Delete(ctx, pod.Name, metav1.DeleteOptions{GracePeriodSeconds: &zero}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| utilruntime.HandleError(fmt.Errorf("force-deleting pod %s/%s: %w", pod.Namespace, pod.Name, err)) |
There was a problem hiding this comment.
🟡 Medium
Compliance Violation 🔒
🟡 Medium Priority Issues
- Secure Logging Practices (secure-coding-standards,owasp-top10,nist-sp800-53,iso-27001,pci-dss): The error path logs full pod namespace/name and the underlying error for force-deletes. In Kubernetes, object names and errors can contain sensitive workload-identifying information (e.g., tenant/customer identifiers embedded in names/labels, internal resource details), and this message is emitted via utilruntime.HandleError which commonly routes to centralized logs. This is an actual confidentiality risk if logs are broadly accessible or retained without proper controls (common in multi-tenant clusters).
- Remediation: Reduce sensitive identifiers in error logs and/or ensure structured logging with redaction. For example, log only a stable hash or UID, and avoid embedding raw error strings that may include internal endpoints. If detailed identifiers are needed, gate them behind a debug log level and ensure log access controls/retention are enforced.
Example change: log pod UID and a sanitized error category instead of namespace/name and full error text.
This review was performed by the Compliance Engine to ensure adherence to regulatory and security standards.
🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open pkg/controller/nodemaintenance/drain.go and find the force-delete error handling that calls utilruntime.HandleError. Confirm it logs full pod namespace/name and the raw underlying error; if it’s already redacted or only emitted at debug level, skip the change.
2. PROBLEM & LOCATION
File: pkg/controller/nodemaintenance/drain.go
Location: the error path for force-deleting pods during drain (look for utilruntime.HandleError(fmt.Errorf(...)) or similar that includes pod.Namespace/pod.Name and “force delete” wording).
What’s wrong: the log message includes full pod namespace/name plus the full error string. Pod names/namespaces can embed tenant/customer identifiers, and raw errors can include internal endpoints or cluster details; utilruntime.HandleError often ends up in centralized logs, creating a confidentiality risk in shared or multi-tenant environments.
3. FIX
Change the utilruntime.HandleError message to avoid printing pod namespace/name and avoid embedding the full err string.
Log only a non-identifying reference like pod UID (pod.UID) or a short stable hash derived from UID, and replace the raw error text with a sanitized category (e.g., “forbidden”, “notfound”, “timeout”, “conflict”, “unknown”) based on Kubernetes API error helpers (k8s.io/apimachinery/pkg/api/errors) and context timeouts.
If detailed identifiers are needed for troubleshooting, gate the detailed message behind an existing verbose/debug logging mechanism used elsewhere in this controller (search in this package for klog.V(...) or similar) and keep the default HandleError path redacted.
4. VERIFY
Run unit tests for this controller package (and any drain-related tests) and build the binary to ensure imports and error classification compile cleanly.
Search for downstream log-parsing expectations (grep for “force delete” log text) and update any tests that assert exact log output.
🔧 Fix in IDE — opens your editor with the fix prompt ready:
1 participant
Adds a kube-controller-manager controller that cordons and drains nodes during administrator-declared recurring maintenance windows (annotated per-node) and automatically returns them to service once the window closes. Cluster-wide behavior (max concurrent nodes in maintenance, drain timeout, eviction grace period) is tunable via a ConfigMap so it doesn't require a manager restart to change.
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Summary by DevzyAi