pal-e-docs

Review: Service onboarding: Keycloak + Twitch IdP + CNPG + funnel

review-786-2026-04-03-rescope Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after major re-scope from Twitch OAuth spike to full service onboarding feature. Issue body is dramatically improved -- well-structured, complete template, clear file targets. Three issues remain before READY.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Standalone, replaces original spike scope
  • [x] Repo -- forgejo_admin/pal-e-services
  • [x] User Story -- Twitch viewer sign-in via Keycloak
  • [x] Context -- Clear motivation, Twitch OAuth app policy noted
  • [x] File Targets -- 4 files to modify, 2 files NOT to touch
  • [x] Acceptance Criteria -- 8 criteria, specific and testable
  • [x] Test Expectations -- 4 test commands, all concrete
  • [x] Constraints -- References service-onboarding-sop, tofu -lock=false, credential source
  • [x] Checklist -- Standard PR/test/no-unrelated
  • [x] Related -- project, arch notes, SOP, sibling issue

Traceability

  • [x] story:challenger-auth label -- Challenger Twitch Login
  • [x] story note verified -- story-twitch-2k-wager-challenger-auth exists, listed in project-twitch-2k-wager user-stories table
  • [x] arch:deployment label -- arch-deployment-twitch-2k-wager exists
  • [ ] arch:twitch-oauth label -- NO MATCHING NOTE. No arch-twitch-oauth note exists. The OAuth flow is documented in arch-dataflow-twitch-2k-wager. [LABEL] Rename label to arch:dataflow or create arch-twitch-oauth as a redirect.
  • [x] arch-domain-twitch-2k-wager -- exists (domain model with Mermaid ER diagram)
  • [x] arch-dataflow-twitch-2k-wager -- exists (OAuth + payment sequence diagram)
  • [x] arch-deployment-twitch-2k-wager -- exists (deployment topology diagram)
  • [x] Forgejo issue -- forgejo_admin/twitch-2k-wager#1, open

File Targets

  • [x] terraform/k3s.tfvars -- verified: exists, contains keycloak_realms (2 realms), keycloak_clients (5 clients), services (9 entries) maps. Agent adds to each.
  • [x] terraform/keycloak.tf -- verified: exists, has keycloak_realm and keycloak_openid_client for_each resources. Confirmed keycloak_oidc_identity_provider does NOT exist yet (new resource type as stated).
  • [x] terraform/variables.tf -- verified: exists, has keycloak_realms and keycloak_clients variable types. No identity_provider variable type yet (new addition as stated).
  • [ ] terraform/cnpg.tf -- ISSUE: exists but current pattern is a single shared CNPG cluster named pal-e-postgres in the postgres namespace. Issue says "add CNPG cluster for twitch-2k-wager namespace" which is a new per-namespace pattern. [BODY] Clarify: is this a second independent CNPG cluster, or a new database on the existing shared cluster? If new cluster, document as intentional pattern change.
  • [x] terraform/services.tf -- correctly identified as NOT to touch (generic for_each on var.services).

Repo Placement

Correct. Issue filed on forgejo_admin/twitch-2k-wager (consumer repo for tracking) with work targeting forgejo_admin/pal-e-services (the terraform repo). Standard pattern -- onboarding issues track on the consumer, code changes on pal-e-services. Single repo for code changes.

Dependencies

  • [x] Board item #788 (Kustomize overlay in pal-e-deployments) depends on this ticket -- documented in issue Related section as forgejo_admin/twitch-2k-wager#3.
  • [x] No blockers on this ticket -- it can proceed independently.
  • [x] Twitch Developer Console app registration is a manual prerequisite -- documented in Constraints (redirect URI specified).
  • [ ] service-onboarding-sop -- referenced in Constraints but does not exist in pal-e-docs. [BODY] Remove reference or create the SOP first.

Acceptance Criteria

8 criteria, mostly testable. Issues found:
  • [ ] AC5 + AC6 conflict: AC5 says funnel hostname = "playme2k". AC6 says CNPG in "twitch-2k-wager" namespace. But services.tf derives BOTH namespace (line 85: name = each.key) and funnel hostname (line 201: hosts = [each.key]) from the service map key. Cannot satisfy both without either (a) accepting namespace = playme2k, (b) extending var.services with a hostname_override field, or (c) managing the funnel outside var.services. [BODY] Resolve this conflict explicitly in the issue body.
  • [x] AC1-4 -- Keycloak realm, Twitch IdP, OIDC client, service entry -- all testable via tofu plan output inspection.
  • [x] AC7 -- Twitch credentials as Terraform vars -- straightforward.
  • [x] AC8 -- Clean tofu plan -- standard gate.
  • [x] Test Expectations -- all 4 commands are concrete and runnable.

Blast Radius

Low. All changes are additive -- new realm, new client, new service entry, new IdP resource type. No modifications to existing resources. The new keycloak_oidc_identity_provider resource is isolated to the new realm. Risk: if identity_provider variables modify the existing keycloak_realms type signature it could break existing realms. Agent should use a separate variable block for identity providers, not modify the realm type. Rollback is straightforward (tofu destroy targeted resources).

Decomposition Assessment

4 file targets in 1 repo, 8 AC. All targets are tightly coupled Terraform resources provisioned in a single tofu apply. The Keycloak realm, client, IdP, service entry, and CNPG cluster are interdependent -- splitting would create artificial boundaries and require multiple apply cycles. A single agent pass is appropriate. Estimated under 5 minutes. No decomposition needed.

Recommendation

  • [BODY] Resolve namespace vs hostname conflict. services.tf derives both namespace and funnel hostname from the service map key. State which approach: (a) service key = playme2k, accept namespace = playme2k; (b) extend var.services with a hostname field; or (c) manage funnel separately outside var.services.
  • [BODY] Clarify CNPG strategy: new per-namespace cluster or new database on the shared pal-e-postgres cluster in postgres namespace. Current cnpg.tf has one cluster. A second cluster is a pattern change that should be documented as intentional.
  • [BODY] Remove or replace reference to service-onboarding-sop in Constraints -- this note does not exist in pal-e-docs.
  • [LABEL] Rename arch:twitch-oauth label to arch:dataflow (matching arch-dataflow-twitch-2k-wager) or create an arch-twitch-oauth redirect note.