forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 12 Pull requests 3 Projects Releases Packages Wiki Activity Actions

feat: switch Keycloak admin API calls to cluster-internal URL #150

Merged
forgejo_admin merged 1 commit from 149-keycloak-internal-url into main 2026-03-21 22:22:47 +00:00
Conversation 2 Commits 1 Files changed 8 +18 -15
forgejo_admin commented 2026-03-21 22:19:09 +00:00
Owner
Copy link

Summary

Keycloak admin API calls (account creation, password reset, user lookup) now route through the cluster-internal service URL (http://keycloak.keycloak.svc.cluster.local) instead of the external Tailscale funnel (https://keycloak.tail5b443a.ts.net). This eliminates the ~50% TLS failure rate observed for in-cluster requests through the funnel.

OIDC token validation in auth.py is unchanged -- it must use the external URL to match the token issuer claim in Keycloak-issued JWTs.

Changes

  • src/basketball_api/config.py: Added keycloak_admin_url config field defaulting to the cluster-internal service
  • src/basketball_api/routes/password_reset.py: Switched get_admin_token, get_existing_user, and reset_parent_password calls to use keycloak_admin_url
  • src/basketball_api/routes/register.py: Switched both create_account_for_parent call sites to use keycloak_admin_url
  • src/basketball_api/routes/admin.py: Switched get_admin_token and reset_parent_password in tryout announcement to use keycloak_admin_url
  • src/basketball_api/routes/webhooks.py: Switched create_account_for_parent in Stripe webhook to use keycloak_admin_url
  • k8s/deployment.yaml: Added BASKETBALL_KEYCLOAK_ADMIN_URL env var pointing to internal service
  • tests/test_keycloak_integration.py: Updated mocks and assertions to use keycloak_admin_url
  • tests/test_password_reset.py: Updated mocks and assertions to use keycloak_admin_url

Test Plan

  • All 493 tests pass
  • ruff check and ruff format --check clean on both src/ and tests/
  • After deploy: verify registration creates Keycloak account (no more intermittent TLS failures)
  • After deploy: verify password reset completes successfully
  • After deploy: verify OIDC login still works (auth.py unchanged)

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

## Summary Keycloak admin API calls (account creation, password reset, user lookup) now route through the cluster-internal service URL (`http://keycloak.keycloak.svc.cluster.local`) instead of the external Tailscale funnel (`https://keycloak.tail5b443a.ts.net`). This eliminates the ~50% TLS failure rate observed for in-cluster requests through the funnel. OIDC token validation in `auth.py` is unchanged -- it must use the external URL to match the token issuer claim in Keycloak-issued JWTs. ## Changes - `src/basketball_api/config.py`: Added `keycloak_admin_url` config field defaulting to the cluster-internal service - `src/basketball_api/routes/password_reset.py`: Switched `get_admin_token`, `get_existing_user`, and `reset_parent_password` calls to use `keycloak_admin_url` - `src/basketball_api/routes/register.py`: Switched both `create_account_for_parent` call sites to use `keycloak_admin_url` - `src/basketball_api/routes/admin.py`: Switched `get_admin_token` and `reset_parent_password` in tryout announcement to use `keycloak_admin_url` - `src/basketball_api/routes/webhooks.py`: Switched `create_account_for_parent` in Stripe webhook to use `keycloak_admin_url` - `k8s/deployment.yaml`: Added `BASKETBALL_KEYCLOAK_ADMIN_URL` env var pointing to internal service - `tests/test_keycloak_integration.py`: Updated mocks and assertions to use `keycloak_admin_url` - `tests/test_password_reset.py`: Updated mocks and assertions to use `keycloak_admin_url` ## Test Plan - [x] All 493 tests pass - [x] `ruff check` and `ruff format --check` clean on both `src/` and `tests/` - [ ] After deploy: verify registration creates Keycloak account (no more intermittent TLS failures) - [ ] After deploy: verify password reset completes successfully - [ ] After deploy: verify OIDC login still works (auth.py unchanged) ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/basketball-api#149
feat: switch Keycloak admin API calls to cluster-internal URL
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
67d2432f8d 
Admin API calls (account creation, password reset, user lookup) now use
the cluster-internal service URL instead of the external Tailscale
funnel, eliminating ~50% TLS failure rate for in-cluster requests.

OIDC token validation in auth.py is unchanged -- it must use the
external URL to match the token issuer claim.

Closes #149

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-21 22:19:43 +00:00
Author
Owner
Copy link

Review: LGTM

Diff reviewed: 8 files, +18/-15 lines. All changes are mechanical substitutions of settings.keycloak_base_url to settings.keycloak_admin_url at admin API call sites.

Correctness

  • All 9 call sites in routes/ and webhooks.py that previously used keycloak_base_url for admin API calls now use keycloak_admin_url. Verified via grep -- zero remaining references to keycloak_base_url outside of config.py.
  • auth.py is correctly untouched -- OIDC token validation uses keycloak_realm_url, which is unrelated.
  • keycloak_base_url is retained in config.py for backwards compatibility (no breakage if anything else references it).
  • k8s/deployment.yaml correctly adds BASKETBALL_KEYCLOAK_ADMIN_URL env var matching the pydantic-settings env_prefix = "BASKETBALL_" convention.

Tests

  • 493 tests pass. The 4 test assertions that checked the URL passed to Keycloak mocks are updated to match keycloak_admin_url.
  • Ruff lint and format clean.

Discovered scope (not blocking)

  • scripts/create_keycloak_accounts.py hardcodes KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net" (its own constant, not using settings). This batch script is run manually from outside the cluster, so the external URL is likely correct there. Worth a follow-up issue if it ever runs in-cluster.
## Review: LGTM **Diff reviewed:** 8 files, +18/-15 lines. All changes are mechanical substitutions of `settings.keycloak_base_url` to `settings.keycloak_admin_url` at admin API call sites. ### Correctness - All 9 call sites in `routes/` and `webhooks.py` that previously used `keycloak_base_url` for admin API calls now use `keycloak_admin_url`. Verified via grep -- zero remaining references to `keycloak_base_url` outside of `config.py`. - `auth.py` is correctly untouched -- OIDC token validation uses `keycloak_realm_url`, which is unrelated. - `keycloak_base_url` is retained in `config.py` for backwards compatibility (no breakage if anything else references it). - `k8s/deployment.yaml` correctly adds `BASKETBALL_KEYCLOAK_ADMIN_URL` env var matching the pydantic-settings `env_prefix = "BASKETBALL_"` convention. ### Tests - 493 tests pass. The 4 test assertions that checked the URL passed to Keycloak mocks are updated to match `keycloak_admin_url`. - Ruff lint and format clean. ### Discovered scope (not blocking) - `scripts/create_keycloak_accounts.py` hardcodes `KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net"` (its own constant, not using `settings`). This batch script is run manually from outside the cluster, so the external URL is likely correct there. Worth a follow-up issue if it ever runs in-cluster.
forgejo_admin commented 2026-03-21 22:22:26 +00:00
Author
Owner
Copy link

PR #150 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / Pydantic / SQLAlchemy / httpx / k8s manifests

Config layer (config.py):

  • New keycloak_admin_url field added to Settings with correct default http://keycloak.keycloak.svc.cluster.local. Pydantic BaseSettings with env_prefix = "BASKETBALL_" means it maps to BASKETBALL_KEYCLOAK_ADMIN_URL -- correct.
  • keycloak_base_url retained as a dormant field (no remaining consumers in routes). Acceptable -- removing it would be a separate cleanup.
  • keycloak_realm_url unchanged. Correct.

Route migration (all 4 route files):

  • routes/register.py: Both create_account_for_parent call sites (lines 995, 1323) switched from keycloak_base_url to keycloak_admin_url. Confirmed via grep -- zero remaining keycloak_base_url references in routes.
  • routes/admin.py: get_admin_token (line 530) and reset_parent_password (line 551) switched. Correct.
  • routes/password_reset.py: All three call sites (get_admin_token line 163, get_existing_user line 169, reset_parent_password line 178) switched. Correct.
  • routes/webhooks.py: create_account_for_parent (line 298) switched. Correct.

auth.py (MUST NOT change):

  • Confirmed untouched. Uses settings.keycloak_realm_url for JWKS and issuer validation. Does not reference keycloak_base_url or keycloak_admin_url. OIDC token validation correctly continues to use the external realm URL for issuer claim matching.

services/keycloak.py:

  • Not modified in this PR. All functions (get_admin_token, get_existing_user, create_keycloak_user, get_realm_role, assign_realm_role, reset_parent_password, create_account_for_parent) accept base_url as a parameter -- the callers now pass the internal URL. Clean separation of concerns.

k8s/deployment.yaml:

  • BASKETBALL_KEYCLOAK_ADMIN_URL added with value http://keycloak.keycloak.svc.cluster.local. Correctly uses HTTP (no TLS for cluster-internal traffic). Placement is logical, adjacent to BASKETBALL_KEYCLOAK_REALM_URL.

Test updates:

  • test_keycloak_integration.py: Mock settings updated from keycloak_base_url = "https://keycloak.test" to keycloak_admin_url = "http://keycloak.internal". Assertions on mock_get_token and mock_get_existing call args updated to match. Both TestRegisterAutoKeycloakAccount and TestRegisterDuplicateKeycloakSkip updated.
  • test_password_reset.py: Same pattern -- mock_settings.keycloak_admin_url and assertion on mock_reset_pw base_url kwarg updated. Both test_successful_reset and test_keycloak_failure_returns_error updated.

BLOCKERS

None.

  • No new functionality without tests -- existing tests updated to cover the URL switch.
  • No unvalidated user input -- the admin URL is server config, not user-supplied.
  • No secrets or credentials in code.
  • No DRY violations in auth paths.

NITS

  1. Dormant keycloak_base_url config field (config.py:15): Now has zero consumers in route code. Consider removing it in a follow-up to avoid confusion about which URL to use. Not blocking since it's just a config definition with a default.

  2. scripts/create_keycloak_accounts.py still uses hardcoded external URL (line 52: KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net"). This batch script runs inside the pod via kubectl exec and would benefit from using the internal URL too. This is discovered scope -- should become a Forgejo issue, not part of this PR.

SOP COMPLIANCE

  • Branch named after issue (149-keycloak-internal-url -> issue #149)
  • PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
  • Related references issue (Closes forgejo_admin/basketball-api#149)
  • Related references plan slug -- no plan slug referenced (acceptable for a targeted bug fix)
  • No secrets committed
  • No unnecessary file changes (8 files, 18 additions, 15 deletions -- all scoped)
  • Commit messages are descriptive
  • Tests exist and are updated

PROCESS OBSERVATIONS

  • DORA impact (Deployment Frequency): This unblocks reliable Keycloak admin operations. The 50% TLS failure rate on the external funnel was likely causing intermittent registration and password reset failures in production. Merging this directly improves MTTR for Keycloak-dependent flows.
  • Change failure risk: Low. The change is mechanical (URL substitution) with no logic changes. All existing tests updated. The critical auth.py OIDC path is correctly left untouched.
  • Discovered scope: scripts/create_keycloak_accounts.py should be updated to use the internal URL when run in-cluster. Track as a separate issue.

VERDICT: APPROVED

## PR #150 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / Pydantic / SQLAlchemy / httpx / k8s manifests **Config layer (config.py):** - New `keycloak_admin_url` field added to `Settings` with correct default `http://keycloak.keycloak.svc.cluster.local`. Pydantic `BaseSettings` with `env_prefix = "BASKETBALL_"` means it maps to `BASKETBALL_KEYCLOAK_ADMIN_URL` -- correct. - `keycloak_base_url` retained as a dormant field (no remaining consumers in routes). Acceptable -- removing it would be a separate cleanup. - `keycloak_realm_url` unchanged. Correct. **Route migration (all 4 route files):** - `routes/register.py`: Both `create_account_for_parent` call sites (lines 995, 1323) switched from `keycloak_base_url` to `keycloak_admin_url`. Confirmed via grep -- zero remaining `keycloak_base_url` references in routes. - `routes/admin.py`: `get_admin_token` (line 530) and `reset_parent_password` (line 551) switched. Correct. - `routes/password_reset.py`: All three call sites (`get_admin_token` line 163, `get_existing_user` line 169, `reset_parent_password` line 178) switched. Correct. - `routes/webhooks.py`: `create_account_for_parent` (line 298) switched. Correct. **auth.py (MUST NOT change):** - Confirmed untouched. Uses `settings.keycloak_realm_url` for JWKS and issuer validation. Does not reference `keycloak_base_url` or `keycloak_admin_url`. OIDC token validation correctly continues to use the external realm URL for issuer claim matching. **services/keycloak.py:** - Not modified in this PR. All functions (`get_admin_token`, `get_existing_user`, `create_keycloak_user`, `get_realm_role`, `assign_realm_role`, `reset_parent_password`, `create_account_for_parent`) accept `base_url` as a parameter -- the callers now pass the internal URL. Clean separation of concerns. **k8s/deployment.yaml:** - `BASKETBALL_KEYCLOAK_ADMIN_URL` added with value `http://keycloak.keycloak.svc.cluster.local`. Correctly uses HTTP (no TLS for cluster-internal traffic). Placement is logical, adjacent to `BASKETBALL_KEYCLOAK_REALM_URL`. **Test updates:** - `test_keycloak_integration.py`: Mock settings updated from `keycloak_base_url = "https://keycloak.test"` to `keycloak_admin_url = "http://keycloak.internal"`. Assertions on `mock_get_token` and `mock_get_existing` call args updated to match. Both `TestRegisterAutoKeycloakAccount` and `TestRegisterDuplicateKeycloakSkip` updated. - `test_password_reset.py`: Same pattern -- `mock_settings.keycloak_admin_url` and assertion on `mock_reset_pw` base_url kwarg updated. Both `test_successful_reset` and `test_keycloak_failure_returns_error` updated. ### BLOCKERS None. - No new functionality without tests -- existing tests updated to cover the URL switch. - No unvalidated user input -- the admin URL is server config, not user-supplied. - No secrets or credentials in code. - No DRY violations in auth paths. ### NITS 1. **Dormant `keycloak_base_url` config field** (`config.py:15`): Now has zero consumers in route code. Consider removing it in a follow-up to avoid confusion about which URL to use. Not blocking since it's just a config definition with a default. 2. **`scripts/create_keycloak_accounts.py` still uses hardcoded external URL** (line 52: `KEYCLOAK_BASE_URL = "https://keycloak.tail5b443a.ts.net"`). This batch script runs inside the pod via `kubectl exec` and would benefit from using the internal URL too. This is discovered scope -- should become a Forgejo issue, not part of this PR. ### SOP COMPLIANCE - [x] Branch named after issue (`149-keycloak-internal-url` -> issue #149) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references issue (`Closes forgejo_admin/basketball-api#149`) - [ ] Related references plan slug -- no plan slug referenced (acceptable for a targeted bug fix) - [x] No secrets committed - [x] No unnecessary file changes (8 files, 18 additions, 15 deletions -- all scoped) - [x] Commit messages are descriptive - [x] Tests exist and are updated ### PROCESS OBSERVATIONS - **DORA impact (Deployment Frequency):** This unblocks reliable Keycloak admin operations. The 50% TLS failure rate on the external funnel was likely causing intermittent registration and password reset failures in production. Merging this directly improves MTTR for Keycloak-dependent flows. - **Change failure risk:** Low. The change is mechanical (URL substitution) with no logic changes. All existing tests updated. The critical `auth.py` OIDC path is correctly left untouched. - **Discovered scope:** `scripts/create_keycloak_accounts.py` should be updated to use the internal URL when run in-cluster. Track as a separate issue. ### VERDICT: APPROVED
forgejo_admin deleted branch 149-keycloak-internal-url 2026-03-21 22:22:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/basketball-api!150
No description provided.