fix(apisix): restart the login flow instead of 500ing on an expired auth session

[blarghmatey]

What are the relevant tickets?

Part of https://github.com/mitodl/hq/issues/12178 — specifically part (1a). Parts (1b) and (2) stay open.

Follows #5413, which added the alerting for these failures.

Description (What does it do?)

Adds oidc_error_callback_recovery_plugin() and attaches it to the shared plugin configs for api.learn.mit.edu, mitxonline.mit.edu, and nb.learn.mit.edu.

• When a Keycloak authentication session expires — user left the login tab open, or followed a stale bookmark — Keycloak redirects back with error=temporarily_unavailable and no code, marking its own event restart_after_timeout="true". Its intent is that the client start over.
• lua-resty-openidc treats any error parameter as fatal, so APISIX serves a 21KB HTTP 500 where the user expected a login form. Nothing retries.
• The new plugin runs as a serverless-pre-function in the rewrite phase, where its priority (10000) outranks openid-connect's (2599) — confirmed by their relative order in APISIX_DEFAULT_PLUGINS_3_17. It redirects back into the authorization flow before the plugin can fail.
• The redirect target is derived from the callback URI rather than configured. The callback always sits at <login prefix>/.apisix/redirect and the route serving it is by construction the unauth_action="auth" one, so one attachment per host covers every route group on it — including mit-learn's /login and /learn/login prefixes.
• Only transient errors are recovered. access_denied (user pressed Cancel) and invalid_request (a real misconfiguration) keep failing, otherwise the browser spins between the gateway and Keycloak.
• A guard cookie caps recovery at one attempt per browser per minute, so a persistently broken IdP surfaces as an error rather than a redirect loop.

Why this cause and not the others. #5413 shipped with the caveat that its 500 rate is a rate over requests, not users, and that real impact was unknown. Keycloak's event stream answers it — unlike the gateway access log, it identifies users and sessions. The 1,251 callback 500s/day split three ways:

Cause	Volume/day	Blocks anyone?
error=temporarily_unavailable (expired auth session)	614	Yes — 530 distinct client addresses, 181 of which never reached a successful callback in the same 24h
Stale-code replay	~465	No — the first callback already succeeded
Genuine code-exchange failure	~171	Yes, but small: 2.0% of api.learn first attempts, ≤6.5% of mitxonline's

Keycloak settles the replay question: 4,762 successful code_to_token exchanges against only 7 invalid_code errors in 24h. If replayed callbacks reached the token endpoint we would see thousands. They die at APISIX's state check instead. That is why the 27.4% / 14.4% callback failure rates in the original audit are not login failure rates, and why only the first row here is worth fixing now.

Per-host the split is exact — for api.learn, 327 + 191 + 55 = 573, the measured total.

How can this be tested?

Three layers, each covering something the one below it cannot. All run locally on this branch; the first two are wired as CI jobs alongside the existing test job.

1. Unit — uv run pytest tests/ol_infrastructure/components/services/ (132 pass)
Config rendering, plus test_apisix_lua.py executing the shipped Lua in a lupa runtime against a stubbed ngx/apisix.core. Fast feedback on the Lua's logic; it does not prove APISIX accepts anything.

2. Integration — uv run pytest tests/apisix_integration -m integration (11 pass, ~2s)
A real APISIX 3.17.0 container in standalone mode, fed the route config built by the actual plugin helper. This is what proves the oidc_error_recovery block survives schema validation and is readable on conf, and that Set-Cookie survives ngx.redirect on the real runtime. Marked integration, so the repo's existing -m 'not integration' default keeps it out of the fast suite.

3. test-nginx — tests/apisix_testnginx/run.sh (19 subtests pass)
APISIX's own harness (t/APISIX.pm) on the same OpenResty build the gateway runs. Notes for anyone touching it:

• It needs etcd. APISIX.pm boots APISIX's real init path, so without etcd every test fails test-nginx's default no [error] in error.log check on connection-refused noise. etcd is in the image rather than filtering those lines out, because filtering would also hide errors worth seeing.
• It builds from the repo root, so the Dockerfile copies the Lua from src/. A second copy beside the .t would be free to drift, and the suite would then pass against Lua we do not ship.
• t/APISIX.pm and the eight cert fixtures it opens come from the APISIX source tarball at build time — one request, not nine from raw.githubusercontent.com, which reliably trips its rate limiter (HTTP 429) and made the build a coin flip.

Mutation-tested, not assumed. Each layer was verified to fail when the behaviour it covers is broken:

Mutation	Effect
Remove the guard-cookie early return	1 integration test fails
Simulate the settings block being stripped	6 integration tests fail
Remove the repeated-error table handling	2 test-nginx subtests fail

Lint: pre-commit clean on the changed set, including hadolint on the new Dockerfile (verified separately with hadolint directly — the repo-wide Lint Dockerfiles failure is pre-existing across other Dockerfiles and is not from this branch).

Config-passing chain, checked at every hop against the pinned versions rather than assumed:

Layer	Why an unknown key survives
ApisixPluginConfig v2 / PluginConfig v1alpha1 CRDs	config marked x-kubernetes-preserve-unknown-fields: true
apisix-ingress-controller	Config apiextensionsv1.JSON — raw bytes
ADC	type Plugins map[string]any
APISIX 3.17.0 serverless/init.lua	schema declares only phase/functions, no additionalProperties
runtime	invoked as func(conf, ctx)

Additional Context

• Correction — the chronic alert will NOT go quiet, and an earlier version of this description wrongly said it would. APISIXOIDCCallbackFailureRateChronic fires at >5% over 6h, and its own annotation says it stays up "until the two known causes are fixed". This PR fixes one. Working the measured numbers through:

  host	callbacks/24h	500 rate now	after this PR	chronic >5%?
  api.learn.mit.edu	3,297	17.4%	7.5%	still fires
  mitxonline.mit.edu	2,633	25.4%	14.6%	still fires

  The replay 500s alone keep both hosts above the threshold. Alert silence is therefore not the deployment check. Verify instead that the temporarily_unavailable callbacks stop returning 500 — they should go to ~0 while the count of callback requests carrying error= stays flat:

  sum by (host, status) (count_over_time(
    {namespace="operations", container="apisix"}
    |= ".apisix/redirect" |= "time_local" |= "error=" | logfmt | __error__="" [24h]))
  

  and that the overall rates land near the "after" column above. No rule change is included here — the rule behaves as designed, it is the claim about it that was wrong. Adding a temporarily_unavailable-specific expression so this cause is independently trackable is a reasonable follow-up if you want it.

• On prioritising the rest of #12178: on this evidence part (1b) (tolerating replayed codes) is log-noise and alert-inflation cleanup, not a user-facing fix, and is better done after 1a has been observed in production since removing 1a changes the remaining mix. Part (2) (per-issuer route scoping, Rootly INC-10) is untouched and still needs design.

• Unrelated finding, filed separately: Keycloak rejects ~12/day of redirect_uri="http://nb.learn.mit.edu/.apisix/redirect" with invalid_redirect_uri — note http, not https. Those fail at the authorization endpoint and never reach the callback, so they are invisible in the numbers above.

• On the dismissed Seer finding: it predicted (HIGH) that ngx.header["Set-Cookie"] would be dropped by ngx.redirect(), breaking the loop guard. It was dismissed last round on source reading (ngx_http_lua_ngx_redirect pushes Location onto the existing r->headers_out.headers list and clears nothing). That is now settled empirically instead: test_guard_cookie_survives_ngx_redirect asserts the header on a response a real APISIX generates, and test_second_failure_falls_through_instead_of_looping asserts the guard actually engages. Its suggested header_filter fix would not have worked — ngx.redirect() terminates the request in rewrite, so that phase never runs for this response.

Checklist:

Not merge blockers. The three test layers now cover the plugin's behaviour and APISIX's acceptance of its config; what they cannot reach is the real cluster's CRD → ingress-controller → ADC path and a live Keycloak. Worth confirming against applications.mitlearn.CI after this deploys there, before promoting to Production:

• The settings block survives the real config pipeline end to end: curl -sSi 'https://api.ci.learn.mit.edu/login/.apisix/redirect?error=temporarily_unavailable&state=x' returns 302 with Location: /login/ and Set-Cookie: apisix_oidc_recovery=1; .... A 500 here means the oidc_error_recovery key was dropped somewhere between the CRD and APISIX — the one hop no test covers.
• A normal login through https://ci.learn.mit.edu still completes end to end against real Keycloak — this plugin runs in the rewrite phase of every route on the host.

[Copilot]

Pull request overview

Adds APISIX recovery for expired OIDC authentication sessions, restarting login instead of returning HTTP 500.

Changes:

• Adds guarded Lua callback recovery.
• Enables recovery for MIT Learn, MITx Online, and JupyterHub.
• Adds configuration-rendering and source-level tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/ol_infrastructure/components/services/apisix.py	Defines the recovery plugin.
src/ol_infrastructure/applications/mit_learn/__main__.py	Enables recovery for MIT Learn.
src/ol_infrastructure/applications/mitxonline/__main__.py	Enables recovery for MITx Online.
src/ol_infrastructure/applications/jupyterhub/deployment.py	Enables recovery for JupyterHub.
tests/ol_infrastructure/components/services/test_apisix.py	Tests plugin generation and rendering.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/ol_infrastructure/components/services/files/oidc_error_callback_recovery.lua:24

• This suffix check also matches non-callback paths such as /login/foo.apisix/redirect, even though APISIX 3.17 constructs and recognizes callbacks with the exact /.apisix/redirect suffix. Because this plugin is attached host-wide, such application requests with error=temporarily_unavailable are now unexpectedly redirected. Include the slash in the match (and add a near-match regression case) so only actual OIDC callbacks are intercepted.

    if not uri or not uri:match("%.apisix/redirect$") then

tests/apisix_integration/conftest.py:35

• APISIX_IT_IMAGE is documented above as an override, but this constant never reads it, so setting the variable silently continues testing 3.17.0. That can produce false confidence when using the suite to validate an image upgrade; either honor the environment variable here or remove the override claim.

APISIX_IMAGE = "apache/apisix:3.17.0-debian"

src/ol_infrastructure/applications/mit_learn/main.py:1350

• The PR description says this attachment should make APISIXOIDCCallbackFailureRateChronic go quiet, but that rule is >5% and explicitly documents silence only after both known causes are fixed (log_rules/apisix_oidc.py:72-76, 183-186). This PR leaves stale replay and exchange failures open; for api.learn alone the stated 191 + 55 remaining failures against the measured callback volume still exceed 5%, even after adding the recovery callbacks. Alert silence therefore cannot be the deployment confirmation claimed in the PR; update that validation expectation (for example, verify the expected rate reduction or query this error specifically).

            oidc_error_callback_recovery_plugin(),

[blarghmatey]

Picked up the three suppressed findings from the latest Copilot review — they had no inline threads, so replying here.

1. %.apisix/redirect$ matched near-miss paths — correct, fixed in 09b730a. It also matched application paths ending in the same characters, e.g. /login/foo.apisix/redirect, and since this is attached host-wide such a request carrying error=temporarily_unavailable would have been redirected. Now anchored on the leading slash. The gsub still strips only .apisix/redirect so the target keeps its trailing slash (/login/, not /login). Regression case added to all three test layers; reverting the anchor fails one test in each.

2. APISIX_IT_IMAGE documented but not read — correct, fixed in 09b730a. The docstring advertised an override the constant never honoured, so setting it silently kept testing 3.17.0. That is exactly the false confidence you would not want when rehearsing a version bump. It reads the environment variable now.

3. The alert-silence claim was wrong — thank you, this one mattered. I checked the arithmetic and Copilot is right. APISIXOIDCCallbackFailureRateChronic fires on a ratio (>5% over 6h) and its own annotation says it stays up until both causes are fixed. This PR fixes one:

host	callbacks/24h	500 rate now	after this PR	chronic >5%?
api.learn.mit.edu	3,297	17.4%	7.5%	still fires
mitxonline.mit.edu	2,633	25.4%	14.6%	still fires

The replay 500s alone hold both hosts above the threshold — removing 49% of the absolute count is not the same as clearing a 5% ratio. I had stated the opposite confidently in the PR description, the tracking task, and a team memory; all three are corrected. The deployment check is now the temporarily_unavailable-specific 500 count going to ~0 plus the overall rates landing near the "after" column, not alert silence.

No rule change included: the rule behaves as designed, it was the claim about it that was wrong. Adding a temporarily_unavailable-specific expression so this cause is independently trackable is a reasonable follow-up if wanted.

[blarghmatey]

Verified on CI

Deployed this branch's shared-plugin config to the CI stack and ran the probes. The hop no test layer reaches — CRD → ingress-controller → ADC, against real Keycloak — works: the oidc_error_recovery block survives to APISIX, so the config-drop failure mode the unit/integration suites can't see is ruled out.

Deploy was scoped with --target to the two mitlearn-ol-shared-plugins resources, so it left #4759's browser resources and the pre-existing Fastly drift alone. pulumi preview on the branch vs. on main differ by exactly those two resources — this PR introduces no other change to CI.

#	probe	result
1	error=temporarily_unavailable	302, location: /login/, set-cookie: apisix_oidc_recovery=1; Path=/; Max-Age=60; Secure; HttpOnly; SameSite=Lax
2	error=access_denied	500 — correctly not recovered
3	retry carrying the guard cookie	500 — guard engages, no redirect loop
4	/login/foo.apisix/redirect	falls through to a normal Keycloak auth redirect, not intercepted — 09b730a's slash anchoring holds
5	/login	authorization redirect well-formed, redirect_uri https; Keycloak serves a real login form (200)
6	/api/v1/courses/	200 — unauthenticated API paths unaffected by the rewrite-phase plugin

Probe 1 matches the expected Set-Cookie byte for byte, which also settles Seer's HIGH prediction that ngx.redirect() would drop a header set via ngx.header[...] — it does not, on a real gateway.

Not covered: a full credentialed login. Probe 5 gets as far as Keycloak rendering the form; completing the code exchange needs credentials I don't have. Worth one manual browser login through https://ci.learn.mit.edu before production.

Merge-order interaction with #4759

#4759 adds a second shared plugin config, mitlearn-ol-browser-shared-plugins, and points the browser-* route rules at it. It carries its own copy of stale_session_cookie_cleanup_plugin but would not carry this PR's recovery plugin, so the two configs drift apart on the /login/* path. On CI right now, with both deployed, that is directly observable:

Origin: https://ci.learn.mit.edu  →  500   (browser-reqauth → browser-shared-plugins, no recovery plugin)
no Origin header                  →  302   (reqauth → ol-shared-plugins, recovered)

In practice this probably does not hit the real callback: the callback is a cross-origin GET top-level navigation from Keycloak, and browsers don't attach Origin to those, so it should keep landing on the non-Origin rules. I'd rather not rest a login path on that reasoning without a real browser confirming it.

Either way it's a latent trap — whichever of these two PRs merges second should attach oidc_error_callback_recovery_plugin() to the browser config as well, so the two configs can't silently disagree about whether an expired auth session is recoverable.

Unrelated: CI stack has pending operations

The CI stack state carries 4 pending creating ops on mitlearn-ci-pre-deploy-job, stamped 2026-08-19T19:30:55Z — from the #4759 deploys, not from this run (my --targeted update never touched that resource, and no such Jobs exist in the cluster now). Clearing them needs an interactive pulumi refresh on the CI stack.

[blarghmatey]

Rebased onto main and resolved the pytest.yml conflict (this branch's apisix-integration/apisix-testnginx jobs vs. main's ci-gate + workflow-scoped permissions in #5567/#5571): kept both, added the two jobs to ci-gate's needs list, applied the contents:read restriction. No review threads were open — the only blocker was the merge-order conflict causing pre-commit.ci's mergeable-check error.