internal/controller: return stale AuthConfig deletion errors so the controller retries

[SAY-5]

What

AuthConfigsReconciler.Reconcile in internal/controller/authconfigs_reconciler.go sweeps out stale AuthConfig objects but swallows the error:

for _, authConfig := range staleAuthConfigs {
    if err := r.client.Resource(...).Delete(ctx, authConfig.GetName(), metav1.DeleteOptions{}); err != nil {
        logger.Error(err, "failed to delete authconfig object", "authconfig", ...)
        // TODO: handle error
    }
}
return nil

The controller runtime only re-queues a reconcile when Reconcile returns a non-nil error. So a failed delete was a one-shot log line and the stale resource was left in-cluster until some other event happened to trigger another reconcile pass. Under API-server pressure, a transient permission glitch, or a brief conflict, that's long enough for stale AuthConfigs to accumulate, and for Authorino to keep enforcing policy fragments the operator already tried to remove.

Per #1867.

Fix

Collect per-delete failures and return them through errors.Join at the end of the function:

var deleteErrors []error
for _, authConfig := range staleAuthConfigs {
    if err := r.client.Resource(...).Delete(...); err != nil {
        logger.Error(err, "failed to delete authconfig object", "authconfig", ...)
        deleteErrors = append(deleteErrors, fmt.Errorf("failed to delete stale authconfig %s/%s: %w",
            authConfig.GetNamespace(), authConfig.GetName(), err))
    }
}

return errors.Join(deleteErrors...)

• Successful deletes in the same loop still complete; only the failed ones are surfaced.
• The existing per-object logger.Error(...) call is kept so the operator log still has the structured context.
• errors.Is/errors.As in any upstream error handlers continue to work because we wrap the original error with %w.

errors.Join(nil...) returns nil, so the success path is unchanged from the previous return nil.

Fixes #1867

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling during stale configuration cleanup: deletion errors encountered while removing obsolete AuthConfig resources are now accumulated and reported collectively, while not-found cases are ignored. As a result, cleanup failures are surfaced instead of being silently skipped, and the reconciler may now return an error when stale configuration deletions fail for reasons other than not-found.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5755be4e-6cd6-428b-8e0a-481e5d18cae9

📥 Commits

Reviewing files that changed from the base of the PR and between 1ba7d65 and f724ec8.

📒 Files selected for processing (1)

• internal/controller/authconfigs_reconciler.go

💤 Files with no reviewable changes (1)

• internal/controller/authconfigs_reconciler.go

---

📝 Walkthrough

Walkthrough

The reconciler now aggregates deletion errors when removing stale AuthConfig resources, ignores NotFound as idempotent, and returns a combined error via errors.Join(...) after attempting all deletions.

Changes

AuthConfigs reconciler

Layer / File(s)	Summary
Stale AuthConfig deletion and error aggregation internal/controller/authconfigs_reconciler.go	Adds apierrors import; collects non-NotFound deletion errors for stale AuthConfigs into a slice, wraps them, and returns an aggregated error with errors.Join(...) instead of only logging failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I found the deletes that used to hide,
Skipped the gone ones, kept the rest beside,
I gathered errors, stitched them tight,
Joined their voices into one clear bite,
Hop—reconcile—now retries with pride.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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: returning stale AuthConfig deletion errors to enable controller retries.
Linked Issues check	✅ Passed	The changes fully implement the requirements from issue #1867: collect deletion errors with contextual information and return them via errors.Join for controller retry.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to implementing the deletion error handling for stale AuthConfigs as required by issue #1867; no unrelated modifications present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/controller/authconfigs_reconciler.go`:
- Around line 98-102: The delete error aggregation in Reconcile for AuthConfigs
must ignore NotFound errors—import k8s.io/apimachinery/pkg/api/errors as
apierrors and, in the block that calls
r.client.Resource(kuadrantauthorino.AuthConfigsResource).Namespace(authConfig.GetNamespace()).Delete(...),
check if apierrors.IsNotFound(err) and if so treat it as success (optionally log
at V(1) rather than appending to deleteErrors); only append to deleteErrors and
log an error when err is non-nil and not apierrors.IsNotFound(err). Update the
delete handling in authconfigs_reconciler.go around the Delete call (the
deleteErrors slice and logger usage) accordingly.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f69e3ca-9793-4ac5-b0da-d8bc31b9b71e

📥 Commits

Reviewing files that changed from the base of the PR and between a0a0702 and 264ca2c.

📒 Files selected for processing (1)

• internal/controller/authconfigs_reconciler.go

[maksymvavilov]

@SAY-5 up to you to act or ignore the nit. In any case this is good

[maksymvavilov]

Nit: a grumpy side of mine sees line numbers and yells: "This will go out of date in a month!"; It is a fairly well-known pattern, and I see no added benefit of the comments. If this were me, I'd remove them, but no engineer died of having more comments afaik - this should not hold the merger.

[SAY-5]

Fair point, the file references would rot quickly. Dropped the comment block in f724ec8.

[maksymvavilov]

Good work! Thank you

[guicassolato]

The solution proposed here is consistent with the hints left in #1867. However, I think I would favour a more holistic approach to tackle the problem of non-blocking, final errors raised by the API server (#946).

The current State of the World reconciliation has several locations where API server errors are logged but not propagated (marked with // TODO: handle error). The challenge is:

• Returning errors aborts the workflow - If you return an error from a reconciler, the entire workflow stops, preventing other reconcilers from running
• Ignoring errors causes resource leaks - Stale resources accumulate when deletion/update failures are swallowed
• Standard controller retry waits for new events - Without returning an error, there's no automatic retry until another API server event arrives
• Risk of infinite loops - Naive retry mechanisms could create reconciliation loops

I think we should introduce a reconciliation error registry that collects all non-blocking errors during workflow execution and triggers a single deferred reconciliation after the workflow completes successfully.

It could be added to the final steps of the workflow:

func (b *BootOptionsBuilder) finalStepsWorkflow() *controller.Workflow {
	workflow := &controller.Workflow{
		Tasks: []controller.ReconcileFunc{
			// ... existing tasks ...
		},
		Postcondition: // Add error retry reconciler as the final task
			traceReconcileFunc("finalize.error_retry", NewErrorRetryReconciler().Reconcile),
	}
    
	// ... rest of the function
}

Then, in the AuthConfig reconciler (and similar):

func (r *AuthConfigsReconciler) Reconcile(ctx context.Context, _ []controller.ResourceEvent, topology *machinery.Topology, _ error, state *sync.Map) error {
	// ... 

	// Get or create error registry from state
	errorRegistry := GetOrCreateErrorRegistry(state)

	for _, authConfig := range staleAuthConfigs {
		if err := r.client.Resource(kuadrantauthorino.AuthConfigsResource).Namespace(authConfig.GetNamespace()).Delete(ctx, authConfig.GetName(), metav1.DeleteOptions{}); err != nil {
			logger.Error(…)

			// Record the error for deferred retry instead of returning it
			errorRegistry.Record(…)
	}

	// ... 
}

Finally, the implementation of a hypothetical ErrorRetryReconciler can get complex, but it shoould:

1. Check if the registry contains any error
2. Merges the tracked errors into an error tracker struct that lives outside the workflow

The external error tracker can be a wrapper for Policy Machinery's controller that implements controller-runtime's Reconcile. It:

1. Keeps count of the accrued number of occurrences for each type of error
2. Triggers a retry (by returning a controller-runtime's Result)

[SAY-5]

Agreed the registry plus deferred-retry reconciler in #946 is the right long-term shape; this only patches the one stale-AuthConfig delete path that #1867 pointed at and would be redundant once the workflow-wide handling lands. Happy to keep it as an interim stopgap if useful, or close it in favour of #946 if you'd rather not carry the localized version. Your call.

[SAY-5]

Thanks, that framing of the four failure modes is helpful and the registry-plus-deferred-retry approach makes sense as the real fix for the broader set of swallowed errors across the State of the World workflow, not just this one delete.

I'm happy to take that on, but it's a meaningfully larger change than this PR's scope (a new error registry in state, GetOrCreateErrorRegistry, the ErrorRetryReconciler postcondition, and the external counting/backoff tracker), so I'd rather not bolt a half version onto this one. Would you prefer I close this and open a dedicated PR that introduces the registry and converts the existing // TODO: handle error sites to it, or keep this open and grow it into that? Either works, just want to match how you'd like #946 tracked.

[SAY-5]

That makes sense, a deferred error-registry that collects non-blocking API-server errors and runs one retry reconciler at the end of the workflow is a cleaner fit for #946 than patching each TODO site individually. This PR only covers the AuthConfig deletion path, so it would become one consumer of that registry rather than the whole solution. Happy to either rescope this to just wire the AuthConfig path into the registry once the shared piece exists, or close it in favor of a dedicated PR for the holistic approach, whichever you prefer.

[guicassolato]

Closing in favour of #2043.

Thanks for the work, @SAY-5!