Skip to content

feat(auth): no default Dex issuer; unset DEX_ISSUER_URL turns Dex off - #521

Merged
mashkovd merged 1 commit into
mainfrom
feat/dex-off-without-issuer
Oct 10, 2026
Merged

mashkovd merged 1 commit into
mainfrom
feat/dex-off-without-issuer

Conversation

@mashkovd

Copy link
Copy Markdown
Member

Summary

Phase 4 of mctlhq/mctl-gitops#1500 (retire Dex), step 1 of 3.

DEX_ISSUER_URL defaulted to https://ops.mctl.ai/api/dex, so the only way to turn Dex off was a code change: removing the variable from the deployment would silently bring the default back. With the default in place, the Dex verifier, the legacy dex federation shim and the Dex /readyz check (cmd/api/main.go, dexReady) keep pointing at Dex. If Dex were removed first, /readyz would fail and every pod would go unready, as in the 2026-10-09 outage.

  • DexIssuerURL reads DEX_ISSUER_URL with no default. Unset means: no DexVerifier, no dex shim (BuildFederationRegistry already handles dexIssuer == ""), no Dex readiness check.
  • README row updated.
  • TestDexIssuerURLHasNoDefault; mutation-checked (the old envOr(..., "https://ops.mctl.ai/api/dex") fails it).

No production change on merge: gitops sets DEX_ISSUER_URL explicitly today (mctl-api.yaml).

Order

  1. This PR, then a release.
  2. gitops: drop DEX_ISSUER_URL / DEX_CLIENT_ID from mctl-api.yaml; confirm /readyz no longer lists Dex.
  3. gitops: remove Dex (argocd/templates/dex.yaml, dex-config.yaml, the /api/dex proxy) and the Dex rollback clients of Argo Workflows and Grafana.

Before step 2: confirm no Dex-issued token is still in use (provider="dex" verifications; 0 in 30 days as of 2026-10-04).

Prepared as part of the loop; merge only with the owner's explicit OK.

🤖 Generated with Claude Code

https://claude.ai/code/session_012TfVcukEh7ry8MiLNN89N4

Dex is being retired (mctl-gitops#1500 phase 4). The plan is to stop
setting DEX_ISSUER_URL in the deployment and then remove Dex, but an
unset variable fell back to https://ops.mctl.ai/api/dex, so the Dex
verifier, the legacy "dex" federation shim and the Dex /readyz check
would all keep pointing at an issuer that is about to disappear. With
/readyz failing on it, every pod would go unready: the shape of the
2026-10-09 outage.

Unset now means Dex off. Production is unaffected until gitops drops
the variable, because it sets DEX_ISSUER_URL explicitly today.

Refs mctlhq/mctl-gitops#1500

Claude-Session: https://claude.ai/code/session_012TfVcukEh7ry8MiLNN89N4
Copilot AI balanced review requested due to automatic review settings October 10, 2026 00:16
@mashkovd mashkovd added the steward:escalated Held for a human: the PR shepherd must not merge it label Oct 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 1m 8s —— View job


Review complete

  • Fetch pinned diff (git diff 6bfe92b6...e9f6e269d8)
  • Trace DexIssuerURL through cmd/api/main.go (verifier init, /readyz gate, federation registry)
  • Confirm internal/auth/federation_config.go handles dexIssuer == ""
  • Review new test cmd/api/dex_default_test.go
  • Submit verdict

Findings: none (0 P1, 0 P2, 0 P3).

The change removes the hardcoded https://ops.mctl.ai/api/dex default from DexIssuerURL (cmd/api/main.go:1228) so an unset DEX_ISSUER_URL now means "Dex off" instead of silently falling back to the old issuer. Traced every consumer to confirm the empty-string case is handled correctly:

  • cmd/api/main.go:120 — dexVerifier is only built if cfg.DexIssuerURL != "".
  • cmd/api/main.go:847-850 — dexReady (the /readyz Dex check) is only wired up if dexVerifier != nil, so it disappears along with the verifier.
  • internal/auth/federation_config.go:230 — BuildFederationRegistry already has a case dexIssuer == "": branch that skips synthesizing the legacy dex shim, as the PR description claims.

New test cmd/api/dex_default_test.go (TestDexIssuerURLHasNoDefault) covers both the unset case (expects "") and the explicitly-set case, and is a real mutation check — reverting to envOr("DEX_ISSUER_URL", "https://ops.mctl.ai/api/dex") fails it. README row update accurately reflects the new behavior.

No logic gaps found. Approved via gh api .../reviews bound to commit e9f6e269d84a714791b2b257142bd208b97e2280, per the PR's own note that merge should wait on the owner's explicit OK.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit e9f6e26.

No significant issues found.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P1/P2/P3 findings. Default removal is gated correctly everywhere it's consumed: dexVerifier/dexReady in cmd/api/main.go (lines 120, 848) and the dexIssuer == "" branch already in internal/auth/federation_config.go:230. Regression test cmd/api/dex_default_test.go covers both unset and set cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

steward:escalated Held for a human: the PR shepherd must not merge it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants