OCPBUGS-47508: Deny superseded duplicate node CSRs and remove approve-stop

[RadekManak]

Summary

On large Machine scale-ups (hundreds–thousands of nodes), Cluster Machine Approver (CMA) would stop approving CSRs entirely. Kubelet abandons a pending CSR after ~15 minutes and issues a new one for the same node; CMA never denied the abandoned ones, so they piled up. Once "recently pending" CSRs crossed max(machines, nodes) + 100, CMA's safety check stopped reconciling all CSRs — including ones it could otherwise approve — until enough of them aged out of the 1h window or an operator intervened.

This PR fixes CMA's side of that: prune the abandoned duplicates as they appear instead of letting them accumulate, and remove the breaker that turned "some stale CSRs" into "no approvals at all." (A companion MAO/MAPA PR raises AWS machine-controller concurrency to 10, so Machine.status.addresses stops lagging far enough behind instance creation to trigger this in the first place — that's the actual root cause; this PR makes CMA resilient to it regardless.)

Changes

Deny superseded duplicates, every reconcile. For each unsigned node CSR, group by (parsed system:node CN, signerName), keep the newest per group, deny the rest — regardless of whether the newest one is currently approvable. Serving CSRs also require parsed CN == Spec.Username (same identity bar as approval) before they can keep or supersede a group. This runs unconditionally, so a long CMA outage or address-lag window still cleans itself up once CMA resumes. Individual deny failures are aggregated so a partial prune does not skip the named CSR approval attempt.

Remove the approve-stop. reconcileLimits no longer short-circuits reconciliation. CMA keeps approving/denying at any pending count; only alerting reacts to "too many pending."

Separate wake vs unsigned predicates. pendingNodeCertFilter still wakes reconcile for unsigned node CSRs and for recently foreign-approved ones (metrics / catch-up). Supersede selection and recentlyPendingNodeCSRs use isUnsignedPendingNodeCSR (!Approved && !Denied && !Failed plus signer/group checks), so Approved/Denied/Failed CSRs cannot enter the deny pool or inflate the pending gauge.

New metric + alert: mapi_duplicate_csr_denied_total. A counter incremented every time deny() successfully denies a superseded CSR, registered alongside the existing gauges. Backing alert:

- alert: MachineApproverDuplicateCSRDenied
  expr: increase(mapi_duplicate_csr_denied_total[5m]) > 0
  for: 15m

This is now the primary "CMA is falling behind" signal, replacing the old freeze-and-notice behavior. for: 15m tolerates a brief burst of denials during normal address-population lag and only pages on sustained pressure. MachineApproverMaxPendingCSRsReached (mapi_current_pending_csr > mapi_max_pending_csr) is kept as-is for non-duplicate overload — pruning + unsigned-only counting now keeps the raw pending count tracking live demand instead of accumulated garbage, so no new "unique pending" metric was needed.

Metrics simplification this unlocks

The old code called reconcileLimits twice per CSR: once at the top of Reconcile (cached, to decide whether to short-circuit) and once at the end via reconcileLimitsUncached (an extra uncached List, because the just-approved CSR wasn't visible in the cache yet). Removing the short-circuit removes the reason for the first call. And once denySupersededDuplicates/reconcileCSR mutate the CSR objects in place in the csrs slice passed through Reconcile, that local slice already reflects this pass's successful approve/deny by the time metrics are read — so the second call no longer needs to be uncached either:

denyErr := m.denySupersededDuplicates(csrs)
// ... find req.Name and reconcileCSR ...
refreshPendingCSRMetrics(req.Name, machines, nodes, csrs)
return reconcile.Result{}, errors.NewAggregate([]error{denyErr, reconcileErr})

Net effect: one cached metrics update per reconcile (down from a cached check plus an extra uncached List), and it now runs even when deny or approve fails, so the pending count stays accurate instead of waiting for the next reconcile.

Testing

Added table-driven unit tests in pkg/controller/csr_check_test.go: superseded selection per (CN, signer), Approved/Denied/Failed exclusion from supersede candidates, serving CSRs with CN != Username ignored for supersede, Denied/Failed exclusion from the pending count, and counter increment on successful deny (no increment when UpdateApproval fails). No new envtest/controller harness — full verification is the large scale-up run once this, the MAO concurrency PR, and the MAPA flag PR are all merged.

Context & Patterns

Why in-place mutation instead of tracking denied names separately — denySupersededDuplicates and reconcileCSR mutate the certificatesv1.CertificateSigningRequest structs inside the same csrs slice passed to the metrics refresh at the end of Reconcile. An earlier version tracked denied names in a side set and re-listed CSRs uncached to get fresh state for metrics. Since the CSR objects already carry their new conditions in place after a successful API update, there's nothing to look up — the slice is the fresh state, so the extra bookkeeping and extra API call both disappear.

Why deny doesn't need approve's idempotency guard — approve can be called on a CSR that's already approved (e.g. by another controller), so it checks hasSameState before updating. deny is only ever invoked from denySupersededDuplicates on CSRs that just passed isUnsignedPendingNodeCSR in the same pass, so it can't already be in a terminal state — the guard would be dead code.

Why for: 15m on the new alert instead of firing on any denial — a single denied CSR during normal address-population lag is tolerated background noise, not an incident. Requiring 15 minutes of sustained denials (vs. the 5-minute window used for the increase() itself) distinguishes "CMA briefly pruned one duplicate" from "CMA has been unable to keep up for a while," which is the actual page-worthy condition.

Summary by CodeRabbit

• New Features
  • Automatically denies superseded duplicate node CSRs while reconciling only the targeted CSR.
  • Exposes a Prometheus counter for duplicate CSR denials and clarifies the “recently pending” threshold metric description.
• Alerting
  • Adds a warning alert (MachineApproverDuplicateCSRDenied) when duplicate CSR denials persist for 15 minutes; brief one-off denials are ignored, with guidance on where to investigate.
• Documentation
  • Expands the metrics documentation with full Prometheus exposition details and alerting conditions.
• Bug Fixes
  • Ensures denied/failed CSRs are excluded from “recently pending” calculations to prevent incorrect reconciliation decisions.

[openshift-ci-robot]

@RadekManak: This pull request references Jira Issue OCPBUGS-47508, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

On large Machine scale-ups (hundreds–thousands of nodes), Cluster Machine Approver (CMA) would stop approving CSRs entirely. Kubelet abandons a pending CSR after ~15 minutes and issues a new one for the same node; CMA never denied the abandoned ones, so they piled up. Once "recently pending" CSRs crossed max(machines, nodes) + 100, CMA's safety check stopped reconciling all CSRs — including ones it could otherwise approve — until enough of them aged out of the 1h window or an operator intervened.

This PR fixes CMA's side of that: prune the abandoned duplicates as they appear instead of letting them accumulate, and remove the breaker that turned "some stale CSRs" into "no approvals at all." (A companion MAO/MAPA PR raises AWS machine-controller concurrency to 10, so Machine.status.addresses stops lagging far enough behind instance creation to trigger this in the first place — that's the actual root cause; this PR makes CMA resilient to it regardless.)

Changes

Deny superseded duplicates, every reconcile. For each pending node CSR, group by (parsed system:node CN, signerName), keep the newest per group, deny the rest — regardless of whether the newest one is currently approvable. This runs unconditionally, so a long CMA outage or address-lag window still cleans itself up in one pass once CMA resumes.

Remove the approve-stop. reconcileLimits no longer short-circuits reconciliation. CMA keeps approving/denying at any pending count; only alerting reacts to "too many pending."

Exclude Denied/Failed from the pending filter and count. Once a superseded CSR is denied, it needs to drop out of pendingNodeCertFilter and recentlyPendingNodeCSRs immediately — otherwise denied duplicates would keep inflating the pending count they were denied to fix.

New metric + alert: mapi_duplicate_csr_denied_total. A counter incremented every time deny() denies a superseded CSR, registered alongside the existing gauges. Backing alert:

- alert: MachineApproverDuplicateCSRDenied
 expr: increase(mapi_duplicate_csr_denied_total[5m]) > 0
 for: 15m

This is now the primary "CMA is falling behind" signal, replacing the old freeze-and-notice behavior. for: 15m tolerates a brief burst of denials during normal address-population lag and only pages on sustained pressure. MachineApproverMaxPendingCSRsReached (mapi_current_pending_csr > mapi_max_pending_csr) is kept as-is for non-duplicate overload — pruning + Denied/Failed exclusion now keeps the raw pending count tracking live demand instead of accumulated garbage, so no new "unique pending" metric was needed.

Metrics simplification this unlocks

The old code called reconcileLimits twice per CSR: once at the top of Reconcile (cached, to decide whether to short-circuit) and once at the end via reconcileLimitsUncached (an extra uncached List, because the just-approved CSR wasn't visible in the cache yet). Removing the short-circuit removes the reason for the first call. And once denySupersededDuplicates/reconcileCSR mutate the CSR objects in place in the csrs slice passed through Reconcile, that local slice already reflects this pass's approve/deny by the time metrics are read — so the second call no longer needs to be uncached either:

reconcileErr := m.reconcileNamedCSR(req.Name, csrs, parsedPending, machines)
// csrs reflects approve/deny from this pass, so gauges can use this list.
refreshPendingCSRMetrics(req.Name, machines, nodes, csrs)
return reconcile.Result{}, reconcileErr

Net effect: one cached metrics update per reconcile (down from a cached check plus an extra uncached List), and it now runs on the error path too, so a failed approval still gets an accurate pending count instead of waiting for the next reconcile.

Testing

Added table-driven unit tests in pkg/controller/csr_check_test.go: superseded selection per (CN, signer), Denied/Failed exclusion from the pending filter/count, no-short-circuit-when-over-max, and counter increment on deny (including a no-op case for an already-denied CSR). No new envtest/controller harness — full verification is the large scale-up run once this, the MAO concurrency PR, and the MAPA flag PR are all merged.

Context & Patterns

Why in-place mutation instead of tracking denied names separately — denySupersededDuplicates and reconcileCSR mutate the certificatesv1.CertificateSigningRequest structs inside the same csrs slice passed to the metrics refresh at the end of Reconcile. An earlier version tracked denied names in a side set and re-listed CSRs uncached to get fresh state for metrics. Since the CSR objects already carry their new conditions in place, there's nothing to look up — the slice is the fresh state, so the extra bookkeeping and extra API call both disappear.

Why deny doesn't need approve's idempotency guard — approve can be called on a CSR that's already approved (e.g. by another controller), so it checks hasSameState before updating. deny is only ever invoked from denySupersededDuplicates on CSRs that just passed pendingNodeCertFilter in the same pass, so it can't already be in a terminal state — the guard would be dead code.

Why for: 15m on the new alert instead of firing on any denial — a single denied CSR during normal address-population lag is expected background noise, not an incident. Requiring 15 minutes of sustained denials (vs. the 5-minute window used for the increase() itself) distinguishes "CMA briefly pruned one duplicate" from "CMA has been unable to keep up for a while," which is the actual page-worthy condition.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

The controller now detects superseded pending node CSRs, denies older duplicates, excludes terminal CSRs from pending classification, refreshes pending metrics, and exposes duplicate-denial metrics with alerting and documentation.

Changes

CSR denial and observability

Layer / File(s)	Summary
CSR selection and denial flow pkg/controller/csr_check.go, pkg/controller/controller.go	Node CSRs are grouped by common name and signer, superseded requests are denied, terminal CSRs are skipped, pending metrics are refreshed, and reconciliation errors are aggregated.
Metrics, alerting, and documentation pkg/metrics/metrics.go, manifests/.../0000_90_cluster-machine-approver_04_alertrules.yaml, docs/dev/metrics.md, go.mod	The duplicate-denial counter is registered, metric help and documentation are updated, alerting is configured, and the Prometheus client model requirement is made direct.
CSR behavior validation pkg/controller/csr_check_test.go	Tests cover terminal CSR exclusion, superseded selection, denial persistence, and counter behavior after successful or failed approval updates.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: nrb, theobarberbany

Sequence Diagram(s)

sequenceDiagram
  participant Reconcile
  participant CSRSelection
  participant CertificatesAPI
  participant Prometheus
  Reconcile->>CSRSelection: identify superseded pending node CSRs
  Reconcile->>CertificatesAPI: update superseded CSRs with denial conditions
  CertificatesAPI-->>Reconcile: return denial results
  Reconcile->>Prometheus: increment duplicate-denial counter
  Reconcile->>Prometheus: refresh pending CSR gauges

Loading

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: denying superseded duplicate node CSRs and removing the approval stop.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	No Ginkgo titles were added; the new test/subtest names are static strings and contain no dynamic identifiers or timestamps.
Test Structure And Quality	✅ Passed	The added tests are table-driven unit tests with focused subcases, no cluster resources, waits, or cleanup needs, and they include useful failure messages.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the new tests are standard Go unit tests and don't use MicroShift-unsupported OpenShift APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the only test changes are Go unit tests in pkg/controller/csr_check_test.go, with no SNO-relevant assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Changed files only touch CSR/metrics/alerts; no nodeSelector, affinity, spread, PDB, or topology-based replica logic was added.
Ote Binary Stdout Contract	✅ Passed	No changed process-level code writes to stdout; new init()s only register metrics and test fixtures, and searches found no fmt.Print/log.SetOutput/klog stdout calls in touched files.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the new tests are standard Go unit tests, so this check is not applicable.
No-Weak-Crypto	✅ Passed	No weak crypto, custom crypto, or unsafe secret/token comparisons were added; the changes only use standard x509/tls certificate handling.
Container-Privileges	✅ Passed	Changed manifests only add alert rules; no privileged, hostPID/Network/IPC, SYS_ADMIN, or allowPrivilegeEscalation settings were introduced.
No-Sensitive-Data-In-Logs	✅ Passed	No new logs expose secrets/PII; added log lines only mention CSR names and counts, not passwords, tokens, hostnames, or customer data.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: sunzhaohua2.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@RadekManak: This pull request references Jira Issue OCPBUGS-47508, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (5.0.0) matches configured target version for branch (5.0.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Summary

On large Machine scale-ups (hundreds–thousands of nodes), Cluster Machine Approver (CMA) would stop approving CSRs entirely. Kubelet abandons a pending CSR after ~15 minutes and issues a new one for the same node; CMA never denied the abandoned ones, so they piled up. Once "recently pending" CSRs crossed max(machines, nodes) + 100, CMA's safety check stopped reconciling all CSRs — including ones it could otherwise approve — until enough of them aged out of the 1h window or an operator intervened.

This PR fixes CMA's side of that: prune the abandoned duplicates as they appear instead of letting them accumulate, and remove the breaker that turned "some stale CSRs" into "no approvals at all." (A companion MAO/MAPA PR raises AWS machine-controller concurrency to 10, so Machine.status.addresses stops lagging far enough behind instance creation to trigger this in the first place — that's the actual root cause; this PR makes CMA resilient to it regardless.)

Changes

Deny superseded duplicates, every reconcile. For each pending node CSR, group by (parsed system:node CN, signerName), keep the newest per group, deny the rest — regardless of whether the newest one is currently approvable. This runs unconditionally, so a long CMA outage or address-lag window still cleans itself up in one pass once CMA resumes.

Remove the approve-stop. reconcileLimits no longer short-circuits reconciliation. CMA keeps approving/denying at any pending count; only alerting reacts to "too many pending."

Exclude Denied/Failed from the pending filter and count. Once a superseded CSR is denied, it needs to drop out of pendingNodeCertFilter and recentlyPendingNodeCSRs immediately — otherwise denied duplicates would keep inflating the pending count they were denied to fix.

New metric + alert: mapi_duplicate_csr_denied_total. A counter incremented every time deny() denies a superseded CSR, registered alongside the existing gauges. Backing alert:

- alert: MachineApproverDuplicateCSRDenied
 expr: increase(mapi_duplicate_csr_denied_total[5m]) > 0
 for: 15m

This is now the primary "CMA is falling behind" signal, replacing the old freeze-and-notice behavior. for: 15m tolerates a brief burst of denials during normal address-population lag and only pages on sustained pressure. MachineApproverMaxPendingCSRsReached (mapi_current_pending_csr > mapi_max_pending_csr) is kept as-is for non-duplicate overload — pruning + Denied/Failed exclusion now keeps the raw pending count tracking live demand instead of accumulated garbage, so no new "unique pending" metric was needed.

Metrics simplification this unlocks

The old code called reconcileLimits twice per CSR: once at the top of Reconcile (cached, to decide whether to short-circuit) and once at the end via reconcileLimitsUncached (an extra uncached List, because the just-approved CSR wasn't visible in the cache yet). Removing the short-circuit removes the reason for the first call. And once denySupersededDuplicates/reconcileCSR mutate the CSR objects in place in the csrs slice passed through Reconcile, that local slice already reflects this pass's approve/deny by the time metrics are read — so the second call no longer needs to be uncached either:

reconcileErr := m.reconcileNamedCSR(req.Name, csrs, parsedPending, machines)
// csrs reflects approve/deny from this pass, so gauges can use this list.
refreshPendingCSRMetrics(req.Name, machines, nodes, csrs)
return reconcile.Result{}, reconcileErr

Net effect: one cached metrics update per reconcile (down from a cached check plus an extra uncached List), and it now runs on the error path too, so a failed approval still gets an accurate pending count instead of waiting for the next reconcile.

Testing

Added table-driven unit tests in pkg/controller/csr_check_test.go: superseded selection per (CN, signer), Denied/Failed exclusion from the pending filter/count, no-short-circuit-when-over-max, and counter increment on deny (including a no-op case for an already-denied CSR). No new envtest/controller harness — full verification is the large scale-up run once this, the MAO concurrency PR, and the MAPA flag PR are all merged.

Context & Patterns

Why in-place mutation instead of tracking denied names separately — denySupersededDuplicates and reconcileCSR mutate the certificatesv1.CertificateSigningRequest structs inside the same csrs slice passed to the metrics refresh at the end of Reconcile. An earlier version tracked denied names in a side set and re-listed CSRs uncached to get fresh state for metrics. Since the CSR objects already carry their new conditions in place, there's nothing to look up — the slice is the fresh state, so the extra bookkeeping and extra API call both disappear.

Why deny doesn't need approve's idempotency guard — approve can be called on a CSR that's already approved (e.g. by another controller), so it checks hasSameState before updating. deny is only ever invoked from denySupersededDuplicates on CSRs that just passed pendingNodeCertFilter in the same pass, so it can't already be in a terminal state — the guard would be dead code.

Why for: 15m on the new alert instead of firing on any denial — a single denied CSR during normal address-population lag is expected background noise, not an incident. Requiring 15 minutes of sustained denials (vs. the 5-minute window used for the increase() itself) distinguishes "CMA briefly pruned one duplicate" from "CMA has been unable to keep up for a while," which is the actual page-worthy condition.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (5)

docs/dev/metrics.md (2)

11-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a language to the fenced code block.

Static analysis (MD040) flags this fence as missing a language hint.

📝 Proposed fix

-```
+```text
 # HELP mapi_current_pending_csr Count of recently pending node CSRs at the cluster level

🤖 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 `@docs/dev/metrics.md` around lines 11 - 21, Update the fenced metrics example
in the documentation to specify the text language, using the existing metrics
block content unchanged.

Source: Linters/SAST tools

---

18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

HELP text doesn't match the metric's actual Help string.

csr_check.go defines DuplicateCSRDeniedTotal's help as "Total number of pending node CSRs denied because they were superseded by a newer CSR for the same node and signer", but this line reads "Count of pending node CSRs denied because a newer CSR exists for the same node and signer" — unlike the two lines above it, which match their source Desc/CounterOpts verbatim.

📝 Proposed fix

-# HELP mapi_duplicate_csr_denied_total Count of pending node CSRs denied because a newer CSR exists for the same node and signer
+# HELP mapi_duplicate_csr_denied_total Total number of pending node CSRs denied because they were superseded by a newer CSR for the same node and signer

🤖 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 `@docs/dev/metrics.md` at line 18, Update the mapi_duplicate_csr_denied_total
HELP text in the metrics documentation to exactly match the Help string defined
for DuplicateCSRDeniedTotal in csr_check.go, including the wording about CSRs
being superseded by a newer CSR.

pkg/controller/controller.go (2)

389-406: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

deny() uses context.Background() instead of the caller's context.

UpdateApproval is a blocking external API call made with no deadline/cancellation tied to Reconcile's ctx. Threading ctx through denySupersededDuplicates → deny() would let this call be cancelled/timed-out consistently with the rest of the reconcile loop.

As per path instructions, Go security guidance requires "context.Context for cancellation and timeouts."

♻️ Proposed fix

-func deny(certClient certificatesv1client.CertificateSigningRequestInterface, csr *certificatesv1.CertificateSigningRequest) error {
+func deny(ctx context.Context, certClient certificatesv1client.CertificateSigningRequestInterface, csr *certificatesv1.CertificateSigningRequest) error {
 	now := metav1.Now()
 	csr.Status.Conditions = append(csr.Status.Conditions, certificatesv1.CertificateSigningRequestCondition{
 		Type:               certificatesv1.CertificateDenied,
 		Reason:             csrConditionDenyReason,
 		Message:            csrConditionDenyMessage,
 		LastUpdateTime:     now,
 		LastTransitionTime: now,
 		Status:             corev1.ConditionTrue,
 	})

-	if _, err := certClient.UpdateApproval(context.Background(), csr.Name, csr, metav1.UpdateOptions{}); err != nil {
+	if _, err := certClient.UpdateApproval(ctx, csr.Name, csr, metav1.UpdateOptions{}); err != nil {
 		return err
 	}
 	DuplicateCSRDeniedTotal.Inc()
 	return nil
 }

Note: this also requires threading ctx through denySupersededDuplicates and updating the two deny(...) call sites in csr_check_test.go.

🤖 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 `@pkg/controller/controller.go` around lines 389 - 406, Update deny to accept a
context.Context parameter and pass it to UpdateApproval instead of
context.Background(). Thread the caller’s ctx through denySupersededDuplicates
and both deny call sites in csr_check_test.go, preserving existing denial
behavior while enabling cancellation and deadlines.

Source: Path instructions

---

207-217: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Metrics aren't refreshed when denySupersededDuplicates errors, even though the data is already available.

csrs, machines, and nodes are all fetched before this call, so refreshPendingCSRMetrics could still run on this error path (e.g. via defer) instead of skipping it entirely. This is the one place the "updates metrics on error paths" goal isn't fully realized.

♻️ Proposed fix

+	defer func() {
+		// csrs reflects approve/deny from this pass, so gauges can use this list
+		// even if denial or reconciliation returned an error.
+		refreshPendingCSRMetrics(req.Name, machines, nodes, csrs)
+	}()
+
 	parsedPending, err := m.denySupersededDuplicates(csrs)
 	if err != nil {
 		klog.Errorf("%v: failed to deny superseded CSRs: %v", req.Name, err)
 		return reconcile.Result{}, fmt.Errorf("%v: failed to deny superseded CSRs: %w", req.Name, err)
 	}

 	reconcileErr := m.reconcileNamedCSR(req.Name, csrs, parsedPending, machines)
-	// csrs reflects approve/deny from this pass, so gauges can use this list.
-	refreshPendingCSRMetrics(req.Name, machines, nodes, csrs)
 	return reconcile.Result{}, reconcileErr

🤖 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 `@pkg/controller/controller.go` around lines 207 - 217, Ensure
refreshPendingCSRMetrics runs when denySupersededDuplicates returns an error,
using the already fetched csrs, machines, and nodes before returning from the
error path. Update the surrounding reconciliation flow so metrics are refreshed
exactly once for both success and failure outcomes, without changing the
existing error logging or returned error.

pkg/controller/csr_check_test.go (1)

2680-2834: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a case for an approved-but-recently-approved CSR to lock in the fix for the major issue in csr_check.go.

Coverage here is solid, but none of the cases exercise a CSR that already carries an Approved condition within isRecentlyApproved's window alongside a newer duplicate pending CSR — the scenario flagged in csr_check.go's supersededPendingNodeCSRs review comment where such a CSR could wrongly end up in wantDenied.

🤖 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 `@pkg/controller/csr_check_test.go` around lines 2680 - 2834, Add a test case
to TestSupersededPendingNodeCSRs with an older CSR carrying an Approved
condition within the isRecentlyApproved window and a newer duplicate pending
CSR; assert that neither CSR is included in wantDenied. Reuse the existing
pendingClient fixture and condition setup, preserving the test’s current
name-based result comparison.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@pkg/controller/csr_check.go`:
- Around line 93-134: Update supersededPendingNodeCSRs to exclude CSRs with
terminal Approved, Denied, or Failed conditions before appending indexes to
pendingIdx. Preserve pendingNodeCertFilter for other eligibility checks,
ensuring only non-terminal candidates can reach the superseded list and
subsequent deny flow.

---

Nitpick comments:
In `@docs/dev/metrics.md`:
- Around line 11-21: Update the fenced metrics example in the documentation to
specify the text language, using the existing metrics block content unchanged.
- Line 18: Update the mapi_duplicate_csr_denied_total HELP text in the metrics
documentation to exactly match the Help string defined for
DuplicateCSRDeniedTotal in csr_check.go, including the wording about CSRs being
superseded by a newer CSR.

In `@pkg/controller/controller.go`:
- Around line 389-406: Update deny to accept a context.Context parameter and
pass it to UpdateApproval instead of context.Background(). Thread the caller’s
ctx through denySupersededDuplicates and both deny call sites in
csr_check_test.go, preserving existing denial behavior while enabling
cancellation and deadlines.
- Around line 207-217: Ensure refreshPendingCSRMetrics runs when
denySupersededDuplicates returns an error, using the already fetched csrs,
machines, and nodes before returning from the error path. Update the surrounding
reconciliation flow so metrics are refreshed exactly once for both success and
failure outcomes, without changing the existing error logging or returned error.

In `@pkg/controller/csr_check_test.go`:
- Around line 2680-2834: Add a test case to TestSupersededPendingNodeCSRs with
an older CSR carrying an Approved condition within the isRecentlyApproved window
and a newer duplicate pending CSR; assert that neither CSR is included in
wantDenied. Reuse the existing pendingClient fixture and condition setup,
preserving the test’s current name-based result comparison.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ed78e3fb-f48d-4fd3-8c79-df485ad5b164

📥 Commits

Reviewing files that changed from the base of the PR and between 1ae3f15 and 10f47c2.

📒 Files selected for processing (6)

• docs/dev/metrics.md
• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/controller/controller.go
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go
• pkg/metrics/metrics.go

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with 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.

Inline comments:
In `@docs/dev/metrics.md`:
- Around line 11-21: Add the text language identifier to the fenced metrics
block in the documentation by changing its opening fence to use text, while
leaving the metrics contents unchanged.

In `@pkg/controller/controller.go`:
- Around line 389-401: Propagate the reconciliation context through
denySupersededDuplicates into deny, updating both method signatures and their
call sites as needed. In deny, replace context.Background() in the
certClient.UpdateApproval call with the received ctx while preserving the
existing denial behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fc21e132-35e6-463d-b0b0-f5ba8f7f50e9

📥 Commits

Reviewing files that changed from the base of the PR and between 10f47c2 and 0b49426.

📒 Files selected for processing (7)

• docs/dev/metrics.md
• go.mod
• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/controller/controller.go
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go
• pkg/metrics/metrics.go

🚧 Files skipped from review as they are similar to previous changes (4)

• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/metrics/metrics.go
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go

[RadekManak]

/retest

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@pkg/controller/csr_check.go`:
- Around line 100-147: The supersededPendingNodeCSRs path repeatedly parses
unchanged pending CSRs across reconciles, causing excessive x509 work. Add a
cache keyed by CSR name and resourceVersion that stores the parsed CN/signer
result, reuse it from supersededPendingNodeCSRs, and invalidate or replace
entries when the resourceVersion changes while preserving existing handling for
parse failures and non-node CSRs.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e3d640af-9a48-4ad4-999d-4f6adde9fee9

📥 Commits

Reviewing files that changed from the base of the PR and between 0b49426 and e373d08.

📒 Files selected for processing (7)

• docs/dev/metrics.md
• go.mod
• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/controller/controller.go
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go
• pkg/metrics/metrics.go

🚧 Files skipped from review as they are similar to previous changes (5)

• docs/dev/metrics.md
• pkg/metrics/metrics.go
• go.mod
• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/controller/csr_check_test.go

[coderabbitai]

🧹 Nitpick comments (2)

pkg/controller/controller.go (2)

398-416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

deny discards the server-returned updated object; consider naming it for its actual scope.

Two small points:

1. UpdateApproval's return value (which carries the server-assigned ResourceVersion after the write) is discarded, and csr.Status is instead spliced from the locally-built updated object with the pre-update ResourceVersion. Currently harmless since csr isn't written again in this pass, but fragile if a future change reuses this object for another update.
2. deny unconditionally increments DuplicateCSRDeniedTotal, but the name deny is generic; if it's ever reused for a different denial reason, the metric would be mislabeled.

♻️ Optional tweaks

-	if _, err := certClient.UpdateApproval(ctx, updated.Name, updated, metav1.UpdateOptions{}); err != nil {
-		return err
-	}
-	csr.Status = updated.Status
+	result, err := certClient.UpdateApproval(ctx, updated.Name, updated, metav1.UpdateOptions{})
+	if err != nil {
+		return err
+	}
+	*csr = *result

🤖 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 `@pkg/controller/controller.go` around lines 398 - 416, Update deny to retain
the object returned by UpdateApproval and copy its returned Status onto csr,
preserving the server-assigned ResourceVersion. Rename deny to a
duplicate-CSR-specific name, or otherwise ensure DuplicateCSRDeniedTotal is only
incremented for that denial reason. Update all call sites to use the renamed
function.

---

244-268: 🚀 Performance & Scalability | 🔵 Trivial

Duplicate-detection pass re-runs in full on every single CSR reconcile.

denySupersededDuplicates recomputes supersededPendingNodeCSRs over the entire CSR list on every reconcile invocation (triggered per watched CSR event). During the exact large scale-up scenario this PR targets, this repeats O(n) grouping/denial work for every one of the n incoming CSR events, layered on top of the existing full listNodeCSRs per reconcile. Worth confirming this doesn't reintroduce meaningful overhead or excessive API conflict errors (stale watch-cache resourceVersions) during a scale-up burst, e.g., via rate-limiting/batching the duplicate-denial pass instead of running it unconditionally on every event.

🤖 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 `@pkg/controller/controller.go` around lines 244 - 268, Change the
reconciliation flow around denySupersededDuplicates so duplicate detection and
denial are coalesced or rate-limited instead of running for every CSR event.
Ensure a scheduled pass processes the latest CSR state, retains aggregated
denial errors, and avoids repeated API conflicts during scale-up while
preserving eventual denial of superseded CSRs.

🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@pkg/controller/controller.go`:
- Around line 398-416: Update deny to retain the object returned by
UpdateApproval and copy its returned Status onto csr, preserving the
server-assigned ResourceVersion. Rename deny to a duplicate-CSR-specific name,
or otherwise ensure DuplicateCSRDeniedTotal is only incremented for that denial
reason. Update all call sites to use the renamed function.
- Around line 244-268: Change the reconciliation flow around
denySupersededDuplicates so duplicate detection and denial are coalesced or
rate-limited instead of running for every CSR event. Ensure a scheduled pass
processes the latest CSR state, retains aggregated denial errors, and avoids
repeated API conflicts during scale-up while preserving eventual denial of
superseded CSRs.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1c977ab5-cc97-4919-848c-e85306844cfe

📥 Commits

Reviewing files that changed from the base of the PR and between e373d08 and dd3812c.

📒 Files selected for processing (7)

• docs/dev/metrics.md
• go.mod
• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• pkg/controller/controller.go
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go
• pkg/metrics/metrics.go

🚧 Files skipped from review as they are similar to previous changes (6)

• manifests/0000_90_cluster-machine-approver_04_alertrules.yaml
• go.mod
• pkg/metrics/metrics.go
• docs/dev/metrics.md
• pkg/controller/csr_check.go
• pkg/controller/csr_check_test.go

[RadekManak]

/test unit

[openshift-ci]

@RadekManak: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-hypershift-aws	dd3812c	link	true	/test e2e-hypershift-aws
ci/prow/e2e-hypershift-aks	dd3812c	link	true	/test e2e-hypershift-aks

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.