forgejo role: enable Actions — pinned runner on the guest (issue #3) #5
No reviewers
Labels
No labels
needs-info
needs-triage
ready-for-agent
ready-for-human
wontfix
needs-info
needs-triage
ready-for-agent
ready-for-human
review/merge-ready
review/needs-fix
review/needs-human
review/needs-review
wayfinder:grilling
wayfinder:map
wayfinder:prototype
wayfinder:research
wayfinder:task
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
olympus/infra-forge!5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "forgejo-actions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Runner executes jobs in containers (Docker inside the guest, nesting enabled) — verified live on PRE: a pushed smoke workflow ran green, its job in a container (
Linux 7.0.14-19-pve ... 7a0fae0482a3), logs stored by the instance. Container-only labels: nohostlabel is registered, so no workflow can bypass the boundary by asking for it. Decision + RCE mitigations indocs/adr/0001.Registration is Forgejo 15's shared-secret flow: a vaulted 40-hex secret is upserted server-side (
forgejo-cli actions register, idempotent — same row keyed on uuid=secret[:16]) and the daemon authenticates viaserver.connections. This deliberately supersedes the issue's sketched registration-token mechanism (deprecated in runner v13); reasoning in the ADR.Per-repo
has_actionsis ensured via API with a transient admin token minted on the guest (create → PATCH only the repos that lack it → self-DELETE; nothing long-lived left behind), guarded by a read-onlyrepo_unitSELECT so re-runs staychanged=0.The PRE rehearsal rig lives on the
forgejo-prebranch (a593463), not here — a disposable guest in the main tree would make every plan show its creation. README documents the checkout → apply -target → playbook → destroy flow.Evidence
PRE rehearsal (LXC 142, built from the parked rig, since destroyed):
unauthenticated: unregistered runner— config template got the PROD URL because inventory host-line vars lose to playbook group_vars.After (run 2):
ok=26 changed=5 failed=0; runner declared,last_onlineheartbeating.changed=0(verified after every review fix — 5 consecutive convergent runs total).changed=7, all converged.pit/smoke.forgejo/workflows/smoke.ymlonruns-on: docker→ run 1success;hello from the runnerin the instance-stored log; job executed in a container.tofu planon this branch =No changes(no phantom container in every future plan).Review
Two-axis review (standards + spec) run pre-merge; material findings fixed in
3b6147e+a593463: ADR renumbered to 0001, README restates its own gates (plan review, PBS snapshot), magicrepo_unittype hoisted to a named default,hostlabel removed from registration, PATCH loop narrowed to the repos that lack the unit,query_resultempty-dict normalization (live bug found on PRE), handler-order comment corrected. PR review (0800332): PRE rig parked off-main, ADR mention dropped from the tasks banner,ADR 0001:prefixed to the ADR title.Merge Danger
Door: two-way (role change; rollback = revert + playbook re-run, runner removal is
systemctl disable+ delete binary).Blast Radius: prod-guest service.
Adds Docker + a runner daemon to the prod guest (RAM headroom for job containers; the runner is RCE-by-design — container labels + single-user, registration-disabled instance are the mitigations, ADR-0001). The app.ini change restarts forgejo once at rollout. No data migrations; forgejo 15.0.9 ships actions GA. The
vault.ymlre-encryption adds the runner secret (same values otherwise).@ -119,3 +119,3 @@# ============================================================# 4. create accounts# 4. enable Forgejo Actions + the runner (docs/adr/0001)remove adr mention
@ -0,0 +1,51 @@# Enable Forgejo Actions: runner on the guest, jobs in containersAdd
ADR 0001:to the title@ -0,0 +1,48 @@# PRE guest: disposable Forgejo rehearsal container (LXC 142). Managed in the# same tofu state as the prod guest; created/destroyed with -target as the# role's test rig (issue #3: "the PRE run is the test rig").resource "proxmox_virtual_environment_container" "forgejo_pre" {Do we want to commit the PRE resource? Doesn't this make every plan show the creation of the container?
All three comments addressed in
0800332:# 4. enable Forgejo Actions + the runner.# ADR 0001: Enable Forgejo Actions: runner on the guest, jobs in containers.+1 to addfor it. The rig (tofu/pre.tf, host_vars/forgejo-pre.yml, the pre inventory host line) is parked on theforgejo-prebranch ata593463— the same pattern the README already documented for PRE config.guest_prestays in variables.tf (documented as a parked consumer) so a rehearsal needs no variable re-adds;tofu validatepasses without pre.tf. The PRE guest is destroyed and a fresh plan on this branch readsNo changes. Your infrastructure matches the configuration.The README PRE section now documents the checkout-rehearsal-destroy flow.Two-axis code review — PR #5
Fixed point:
main@4255193(merge-base =main; diff non-empty — 13 files / 670 lines across 3 commits:a95f6e5,3b6147e,a593463). Spec source: issue #3. Both axes ran as parallel sub-agents; findings below are kept separate, not merged or reranked. Material findings verified against the working tree.Standards
(a) Documented-standard breaches
ansible/group_vars/forgejo/vault.ymlstill carries the comment# Forgejo Actions runner shared secret (ADR-0002), but the ADR isdocs/adr/0001-forgejo-actions-runner-on-guest.mdand every other new reference (defaults, tasks §4,runner.yaml.j2,README.md:85) says 0001. Commit3b6147erenumbered the code but left this comment stale — invisible to grep because it sits inside the encrypted blob (verified by decrypting).pre.tf/variables.tf/host_varsuse only10.12.0.x(conv 7); the runner secret sits in an encrypted committed file (conv 5); the runner version is pinned indefaults/main.yml(conv 4); branch+PR (conv 1). No hard violations.(b) Baseline smells (all judgement calls)
variables.tf39–68:guest_precopiesguest's entire 12-fieldobject({…})verbatim, andpre.tfduplicatescontainer.tf's resource body. Oneguestsmap would change the shape once.ssh_keys = list(string)is declared in bothguestandguest_pre, set[], and never read: both resources uselocal.operator_pubkey. The diff propagates a dead field.{{ forgejo_home }}/data/act-cacheholds job working dirs (workdir_parent), not a cache. Rename (e.g.runner-work).forgejo_actions_unit_type: 10, a bare int for the Actions repo-unit concept, explained only by a comment.tasks/main.ymlnow carries six unrelated sections; runner provisioning could live in its own file.become_user: postgres/become_method: su; runner CLI tasks repeat therunuser -u … forgejo … --configprefix.Spec
(a) Missing / partial
dockerandubuntu-latestexist, both aliased tonode:22-bookworm. The consumer (wiki-sync, #2) is Out of Scope, so nothing confirms these are the labels its workflows request; any otherruns-onqueues forever.13.1.0) but sits in the Actions block, not adjacent toforgejo_version. Cosmetic; convention 4 satisfied.(b) Scope creep (behaviour not asked for)
runner.yaml.j2sets jobtimeout: 1h— deferred story 20, not in scope.tofu/pre.tf+guest_preadd a managed PRE container. Out of Scope: "PRE Guest work beyond using it as the rehearsal target it already is." Defensible as the test rig, but it is new provisioning work..gitignore.worktrees/— unrelated to the spec.(c) Implemented but looks wrong
changed_when: falseunconditionally (tasks/main.yml:180), so genuine server-side registration drift is invisible to Ansible. PRE-verified, but the guard is weaker than "changes nothing" implies.Everything else maps cleanly: stories 1–6, 8–9, 11, 13–17, the container-only-label ADR claim (no
hostlabel registered), the per-repo PATCH loop narrowed to repos lacking the unit (block-levelwhen+ guarded SELECT), and PRE overrides in host_vars.Summary — Standards: 1 soft documented-standard breach (stale ADR-0002 in the vault comment) plus 6 judgement-call smells, worst being the verbatim
guest_pre/pre.tfduplication. Spec: 2 partial requirements, 3 scope-creep items, 2 implemented-but-questionable; worst is the long-lived credential contradicting the spec's stated transient-secrets posture (deliberate and ADR-recorded, acceptable if the deviation is signed off).All findings from the two-axis review addressed in
6c6dd6a(+ 1843511/3779e36 cleanup of an accidental sweep):Standards
guest_premirrorsguest's type and pre.tf mirrors container.tf (the repo's existing PRE pattern, now parked off-main anyway);forgejo_actions_unit_typestays an int — it's a forgejo DB enum value, a named constant would just alias it; the six-section tasks file follows the role's existing banner structure.Spec
docker:docker://…strings server-side breaks job matching: the daemon's Declare persists only bare label names inagent_labels, andActionRunJob.ItRunsOncomparesruns-on:against exactly those names. A playbook re-run re-upserted the full strings behind a running daemon → new jobs satwaitingforever. Register now derives bare names (docker,ubuntu-latest); the name→backend mapping stays in the daemon config where it's consumed. PRE-verified with the exact regression: push with daemon up → green run in a container. ADR updated with the measured semantics.changed_whenfor registration is now driven by a read-onlyaction_runnerpre-check — a genuinely-deleted runner row reports changed and is repaired; steady-state re-runs staychanged=0. (The earlier daemon-recreated-the-row race in my drift test was the daemon re-declaring within its 2s poll; with the daemon stopped the guard reports changed correctly.)forgejo_version(story 7 cosmetic).dockerandubuntu-latestare the labels this instance's workflows will use; the wiki-sync workflow (#2) will pinruns-on: docker. Any other label intentionally queues forever (no runner serves it).timeout: 1hkept (it protects the single-capacity queue; noted as deferred-scope in the template comment); PRE provisioning now parked off-main;.worktrees/gitignore dropped with the sweep cleanup... actually it remains from the earlier commit — flagging for your call, happy to drop it.Also fixed en route:
act-cache→forgejo_runner_workdir_parent(it holds job workspaces, not the cache), andlogin_dbinstead of the deprecateddb:alias.Full PRE evidence: converge →
changed=0→ row-deletion drift reports changed and repairs → clean-workflow pushes (two, incl. one after the daemon had been up through a re-register) → green runs withhello from the runnerin the instance-stored logs, job in a container (Linux … 7a0fae0482a3-style hostname).Two-axis code review — PR #5 (re-run)
Fixed point:
main@4255193(merge-base =main; diff non-empty — 10 files / 496 insertions / 21 deletions across 8 commits). Spec source: issue #3. Standards sources:README.md"Conventions (mandatory reading)" +AGENTS.md. Both axes ran as parallel sub-agents; findings kept separate, not merged or reranked. Material claims verified against the working tree — one Standards finding was a false positive and is corrected inline.Standards
(a) Documented-standard breaches
data.forgejo.org.app.ini.j2:36addsDEFAULT_ACTIONS_URL = https://data.forgejo.org, echoed inREADME:88and in the default label image refs (defaults/main.yml:49-50,docker://data.forgejo.org/oci/node:22-bookworm). The rule's target is the operator's own public IP/DDNS; this is a third-party software mirror — so judgement on severity, not a clear hard breach. Either the convention gains a "public mirror FQDNs allowed" carve-out or the URL moves to a var.README:72-81 (PRE pattern) vs— verified FALSE.tofu/variables.tf:39-64git ls-files --error-unmatch tofu/pre.tf→ not tracked (parked, as README states). README's parked list is "tofu resource, host_vars overrides, inventory host line";variables.tfis not in it, and the committedguest_predescription documents it is kept deliberately "so a rehearsal needs no variable re-adds." No contradiction. Its unused-on-main status is covered by the Speculative Generality smell below.(AGENTS.md → domain.md says read
GLOSSARY.md; absent, but domain.md says "proceed silently; don't flag absence." No breach. ADR correctly atdocs/adr/0001-…. Version pins in one place — convention 4 satisfied.)(b) Baseline smells (all judgement calls)
tasks/main.yml:157-164,:253-261,:326-341— samebecome_user: postgres+login_db+changed_when: false+registerscaffold. Extract one reusable task/include.variables.tf:9-22and:39-52carry the identical 12-field clump (vmid, hostname, ip, gateway, cores, memory, swap, datastore, disk_gb, network_interface, network_bridge, ssh_keys), duplicate default maps too — anlxc_guesttype wants to be born.guest_pre(variables.tf:39) has no consumer on this branch (its only caller,pre.tf, is parked).forgejo_actions_unit_type: 10(defaults/main.yml:27) — a magic integer for the "Actions unit" concept.forgejo_runner_uuidderives inline fromvault_forgejo_runner_secret(defaults/main.yml:71); the same credential appears astoken,--secret, and a deriveduuid— three names for one value.Spec
(a) Missing / partial
forgejo-cli actions register --secret); a CLI-generated registration token is never produced or consumed. ADR-0001 records the deviation, but the asked mechanism is absent.(b) Scope creep
tofu/variables.tfgainsguest_pre(vm 142,forgejo-pre). Spec Out of Scope: "PRE Guest work beyond using it as the rehearsal target it already is."pre.tfis correctly parked off-branch, but its variable is committed with no consumer.runner.yaml.j2:15setstimeout: 1h, i.e. story 20, listed under "(agent follow-ups, deferred)".(c) Implemented but looks wrong
has_actionsinferred, not read. Story 4: "I wanthas_actionsenabled on this repo." The guard checks existence of arepo_unit.type = 10row and only PATCHes when absent; a row that exists but is disabled would skip the PATCH, leaving CI silently off — the "half-enabled" state story 5 warns against. Low-confidence; the author says it was checked against v15.0.9's handler.(Stories 14/15/16 adequately covered by ADR-0001: fallback recorded, RCE mitigations noted.)
Summary — Standards: 2 documented-standard findings (1 downgraded to judgement on verification, 1 false positive) plus 5 judgement-call smells; worst surviving documented-standard item is the
data.forgejo.org-vs-convention-7 question, worst smell the Postgres probe scaffold duplicated three times. Spec: 7 findings (2 missing/partial, 3 scope creep, 2 implemented-but-questionable); worst is the long-lived credential overwriting the spec's stated transient-secrets posture — deliberate and ADR-recorded, so it needs sign-off rather than a code change.All findings from the re-run review addressed in
forgejo-actions@e5b1f5f(pushed; PR now e16531d..e5b1f5f). Every claims-bearing fix was verified against forgejo v15.0.9 source and re-run live on the PRE guest.Standards
data.forgejo.org. Took the carve-out route and moved the value: README convention 7 now says third-party software mirrors are allowed (the rule targets the operator's own IP/DDNS — that's the actual wording), and the host lives inforgejo_actions_mirror(role defaults). app.ini takes it with the scheme added; the label image refs take the bare host. Verified on PRE: app.ini rendersDEFAULT_ACTIONS_URL = https://data.forgejo.org, the labels renderdocker:docker://data.forgejo.org/oci/node:22-bookworm.&forgejo_pg_probe) on the three read-onlypostgresql_queryprobes. Two things worth recording: (a)module_defaults— the obvious tool — does not work here;community.postgresql.postgresql_queryrejectsbecome/become_user/become_methodas module params (they're task keywords). Confirmed live, both in isolation and via the actual playbook run. (b) I did not use an include: it either losesregisteror needs a fragileset_factindirection because include-leveluntil/retriesis rejected by TaskInclude. The anchor is honest about what it is.forgejo_actions_unit_typecomment now cites v15.0.9models/unit/unit.go:35and the handler lines, and states it's a DB enum value, not a tunable.Spec
has_actionsinferred, not read — you were right, and it's worse than "low confidence": it's actually correct, and my comment was the misleading part. v15.0.9 stores an enabled unit as arepo_unitrow and a disabled one as a deleted row —services/repository/setting.go:45doesDELETE ... WHERE type IN (deleteUnitTypes), and both readers are "does the row exist":UnitEnabled(models/repo/repo.go:399-409) and the API'shas_actions(services/convert/repository.go:137-140,GetUnit(...TypeActions) == nil). So existence is the flag; there is no "row present but blanked" state to miss. Foldingconfiginto the test would flag the working row (empty config,default_permissions 0— queried on PRE) as broken and PATCH every run. Rewrote the §6 comment to record all of that, plus the explicit posture: the role is authoritative, a UI disable is repaired on the next run (that's story 5), and to keep a repo's Actions off on purpose you drop it fromforgejo_actions_repos.data.forgejo.orgscope — folded into the convention-7 item above.guest_pre/ README. Keptguest_pre(bare variable, no consumer, creates nothing — parkedpre.tfis off-main as README says), and made the README's parked list explicit about it. The false-positive half I left alone.timeout: 1h(deferred story 20). Kept, deliberately — an anti-wedge guard on a capacity-1 queue. Comment now names the real backstop: forgejo's ownENDLESS_TASK_TIMEOUTdefault, 3h (modules/setting/actions.go:116).Caught while verifying (not raised in the review)
docker://host/...→docker://https://host/...in the label list. The daemon crash-loopedunauthenticated: unregistered runneron the live guest. Fixed to a bare host (thedocker://registry/imageform takes no scheme) and re-verified green — so the pushed run below did exercise the new templates.git checkout forgejo-pre --command spanned lines mid-token, its file list omitted the one file that actually differs (inventories/pre/hosts), and the branch's inventory line isforgejo-pre, not main'sforgejo— so a rehearsal run against main's inventory silently loses thehost_varsoverride and points the runner at the prod URL. That last one is exactly the failure mode called out in the earlier review; the how-to now reproduces it if followed with main's tree. Rewritten as a fenced block with the branch copy used as-is, and I verified the branch host line and the cleanup step..worktrees/gitignore: dropped ine16531d(earlier commit), as flagged in #34.Evidence (live, on PRE LXC 142, this round)
ok=26 changed=4 failed=0on the fix run, thenchanged=0on re-run (runner daemon active,last_onlineheartbeating ~2s ago,agent_labels = ["docker","ubuntu-latest"]— bare names, so job matching holds).pit/smokeafter the mirror fix: run 5status=success(1), job landed on runner id 4,action_run_job.runs_on = ["docker"]. Its log is inactions_log/pit/smoke/02/2.log.zstwithhello from the runner; the runner's own journal showstask 3 repo is pit/smoke https://data.forgejo.org http://10.12.0.142:3080/.ADR-0002comment still readsADR-0001(decrypted to check).tofu plan→No changes; prod guest 141 untouched.Must-fix from the re-run review — PR #5 @
e5b1f5fOnly one item from the re-run review is a required change; the rest are judgement calls or sign-offs (listed at the bottom).
HARD —
code.forgejo.orghardcoded:ansible/roles/forgejo/tasks/main.yml:149,154This PR introduces convention 7's new single-home clause and then breaks it in the same diff:
README.md:13(added by this PR) now reads: "The mirror host itself lives inforgejo_actions_mirror(role defaults), not scattered across templates.":149—url: "https://code.forgejo.org/forgejo/runner/releases/download/v{{ forgejo_runner_version }}/forgejo-runner-…-linux-amd64":154—checksum: "sha256:https://code.forgejo.org/forgejo/runner/releases/download/v{{ forgejo_runner_version }}/…-linux-amd64.sha256"git grep code.forgejo.org main→ no hits, so both the rule and the violation are new on this branch.It also falsifies the README's own claim at
:105-108— "To move off Forgejo's mirror, change that one value." That holds for the label image refs (which use{{ forgejo_actions_mirror }}) but not for the runner fetch.Not runtime-breaking — a consistency breach. Fix: hoist the runner-download host into
forgejo_actions_mirror(or a sibling role-default var, since the runner fetch has no scheme/scheme-mixed usage) and use it at both lines.Open call — not a code fix, needs sign-off
Judgement calls kept deliberately (no action needed)
guest_preunused-on-main var,forgejo_actions_unit_type: 10primitive,forgejo_runner_uuidname,timeout: 1h(deferred story 20), and the residualchanged_when: falserepeat on the Postgres probes — all reviewed, all with stated reasoning.Fix in
6753880(pushed; PR nowe5b1f5f..6753880).The HARD item is closed as suggested — with one correction to the suggestion. I did not reuse
forgejo_actions_mirror; I hoisted the host into a sibling role default:defaults/main.yml— newforgejo_runner_download_host: "code.forgejo.org".tasks/main.yml:149,154—urland thesha256:…checksum both takehttps://{{ forgejo_runner_download_host }}/….Why a sibling var rather than the mirror var: they are not the same host, and can never be made one.
data.forgejo.orgis the Actions package mirror (it servesuses:and the job images); the runner release lives on Forgejo's source forge, and the mirror does not serve it —data.forgejo.org/forgejo/runner/releases/download/v13.1.0/forgejo-runner-13.1.0-linux-amd64.sha256returns 404 (checked live, just now). Pointing both at one value would have been a one-liner that silently breaks the runner install. Two hosts, two vars, each a one-line change — which is what the README claim needed to become true for both reads.Doc truth fixed in the same commit:
README.md:105-110— the "change that one value" sentence now covers both fetches: the mirror var foruses:/job images,forgejo_runner_download_hostfor the runner release.ADR-0001carve-out entry now names both hosts and records that they are deliberately two variables (with the 404 as the reason).Verified:
ansible-playbook -i inventories/prod/hosts site.yml --syntax-check→ clean (only the pre-existing group/host same-name warning).checksum: sha256:https://{{ … }}validation — resolved to…/v13.1.0/forgejo-runner-13.1.0-linux-amd64[.sha256], download green, sha25629dae21e…4bc376matched. Scratch artifacts removed.git grep code.forgejo.orginansible/now hits only the new default; no hardcoded host left in tasks.Still open, unchanged: the secrets-posture sign-off (spec transient vs ADR long-lived) — no code change, your call. Everything else from comment 41 was the judgement-call list, left as reviewed.