fix(janitor): apply ExtRR review feedback (init, dispatch comment, RBAC msg)

* Drop the defensive nil-spec check in Reconcile. With failurePolicy=Fail
  on the validating webhook, a poison-pill object can never land on the
  apiserver — the check was dead code.

* Combine reconcileInitialize's two passes into one. setInitialConditions
  already re-fetches before patching, so the rv bump from the finalizer
  Update doesn't matter; we save a watch round-trip per ExtRR.

* Tighten the dispatch default-case comment: Released=False (terminal
  apply failure) also falls through here, not just Released=True awaiting
  the external system.

* Rename the forbidden-error condition message from "forbidden to patch
  node ..." to "RBAC forbids patching node ..." so the operator-visible
  reason is unambiguous.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: jtschelling

janitor/pkg/controller/externalremediationrequest_controller.go

@@ -121,15 +121,6 @@ func (r *ExternalRemediationRequestReconciler) Reconcile(ctx context.Context, re
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// Belt-and-suspenders: the validating webhook normally guarantees these,
-	// but a webhook outage shouldn't crashloop the controller.
-	if extrr.Spec == nil || extrr.Spec.HealthEvent == nil || extrr.Spec.HealthEvent.NodeName == "" {
-		slog.ErrorContext(ctx, "ExternalRemediationRequest missing required spec fields; webhook bypass?",
-			"name", extrr.Name, "namespace", extrr.Namespace)
-
-		return ctrl.Result{}, nil
-	}
-
 	annotations := extrr.GetAnnotations()
 
 	ctx, span := tracing.StartSpanWithLinkFromTraceContext(
@@ -181,9 +172,9 @@ func (r *ExternalRemediationRequestReconciler) needsInitialization(
 	return false
 }
 
-// reconcileInitialize adds the finalizer then seeds initial Unknown conditions
-// across two passes. Each step writes only what's missing, so a partial
-// initialization recovers cleanly on re-reconcile.
+// reconcileInitialize adds the finalizer and seeds initial Unknown conditions
+// in a single pass. setInitialConditions re-fetches before patching, so it
+// tolerates the rv bump from the finalizer Update.
 func (r *ExternalRemediationRequestReconciler) reconcileInitialize(
 	ctx context.Context, extrrObj *nvsentinelv1.ExternalRemediationRequest,
 ) (ctrl.Result, error) {
@@ -196,8 +187,6 @@ func (r *ExternalRemediationRequestReconciler) reconcileInitialize(
 		}
 
 		slog.InfoContext(ctx, "Added cleanup finalizer to ExternalRemediationRequest", "name", extrrObj.Name)
-		// The own-kind watch re-enqueues on this metadata Update; no requeue needed.
-		return ctrl.Result{}, nil
 	}
 
 	changed, err := r.setInitialConditions(ctx, extrrObj)
@@ -269,7 +258,8 @@ func (r *ExternalRemediationRequestReconciler) dispatch(
 		return r.reconcileNoOpOnFalse(ctx, extrrObj)
 
 	default:
-		// Released, awaiting the external system.
+		// Steady state: Released=True awaiting the external system, or
+		// Released=False after a terminal apply failure.
 		return ctrl.Result{}, nil
 	}
 }
@@ -341,7 +331,7 @@ func (r *ExternalRemediationRequestReconciler) reconcileApply(
 
 	if err := r.Patch(ctx, nodeToUpdate, client.StrategicMergeFrom(&node)); err != nil {
 		if apierrors.IsForbidden(err) {
-			msg := fmt.Sprintf("forbidden to patch node %q: %v", nodeName, err)
+			msg := fmt.Sprintf("RBAC forbids patching node %q: %v", nodeName, err)
 			slog.ErrorContext(ctx, "release taint apply forbidden by RBAC",
 				"extrr", extrrObj.Name, "node", nodeName, "error", err)
 

janitor/pkg/controller/externalremediationrequest_controller_test.go

@@ -203,15 +203,14 @@ var _ = Describe("ExternalRemediationRequest Controller", func() {
 		Expect(r.Client.Create(ctx, extrrObj)).To(Succeed())
 
 		key := ctrlclient.ObjectKey{Name: extrrObj.Name, Namespace: extrrObj.Namespace}
-		// Init-only: the apply path would requeue forever waiting for the Node.
-		for i := 0; i < 2; i++ {
-			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
-			Expect(err).NotTo(HaveOccurred(), "init pass %d", i+1)
-		}
+		// One pass to init; don't drive further or the apply path will requeue
+		// forever waiting for the Node.
+		_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+		Expect(err).NotTo(HaveOccurred())
 
 		Expect(r.Client.Delete(ctx, extrrObj)).To(Succeed())
 
-		_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+		_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
 		Expect(err).NotTo(HaveOccurred())
 
 		var got nvsentinelv1.ExternalRemediationRequest
@@ -774,11 +773,9 @@ var _ = Describe("ExternalRemediationRequest Controller apply path (branch 3)",
 
 		key := ctrlclient.ObjectKey{Name: extrrObj.Name, Namespace: extrrObj.Namespace}
 
-		// Drive the two init passes (finalizer + status) before hitting the apply branch.
-		for i := 0; i < 2; i++ {
-			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
-			Expect(err).NotTo(HaveOccurred(), "init pass %d", i+1)
-		}
+		// First pass inits; second hits the apply branch and requeues.
+		_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+		Expect(err).NotTo(HaveOccurred())
 
 		result, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key})
 		Expect(err).NotTo(HaveOccurred(), "missing Node must not propagate as a reconcile error")