Repository navigation
fix(ci): Keep the Forgejo Access service token across a teardown - #895
Merged
Merged
Conversation
`Lifecycle: Teardown` runs an untargeted `tofu destroy`, and the Cloudflare
Access service token for Forgejo lives in that state. So every teardown
destroyed it and the next spin-up minted a replacement with a new client id
and secret. Anything outside the stack that authenticates to Forgejo with
that pair stopped working at that moment — and stopped working silently,
because Access answers an unauthenticated call with a 302 to its login page
rather than a 401. Measured on the Conductor-Stack after its update to
v0.82.2: two Nexus-Conductor operations failed, retried, failed again, with
nothing wrong except a credential that no longer existed.
The token is a management-plane credential. Its lifetime is the relationship
between the two systems, not the lifetime of a server. So it is taken out of
the state before the destroy and imported back on the next spin-up — the
trick `setup-control-plane.yaml` already uses for the KV namespace.
`.github/scripts/forgejo-service-token.sh` has the three halves:
preserve teardown.yml, before the destroy: `tofu state rm`, so the destroy
cannot reach it. Fails loudly rather than let the destroy run.
adopt spin-up.yml, before the apply: finds the token by the name main.tf
gives it and imports it. Then plans that one resource with
`-detailed-exitcode` and refuses if applying would still change
it — an import that succeeds but does not settle would rotate the
credential anyway, and nothing would say so.
purge destroy-all.yml, after the destroy: removes it by name, because by
then it is exactly the unmanaged object a teardown left behind.
Two refusals are deliberate. `adopt` stops when two tokens carry the name:
importing the wrong one hands the Access policy a credential the caller does
not hold, which is the same silent failure one level deeper. And a listing
that returns 200 with `"success": false` is an error, not an empty list — read
as "no token", it would mint a second one.
What this cannot restore is the client_secret. Cloudflare returns it once, at
creation; the provider's documentation says an imported token "will not have
the client_secret available in the state for use". That is the point rather
than a gap — nobody needs a new one, the external system keeps the pair it
was given. It does mean Infisical shows the id and no secret after the first
teardown, so the adopt step says so in the log and three docs pages say it too.
Only the rebuild lifecycle needed this: `teardown-snapshot.yml` destroys
`-target=hcloud_server.main` and nothing else, so the token was never at risk
there. A test fails if that ever widens.
Guards in tests/unit/test_forgejo_service_token.py, 28 of them. The script
runs for real under bash with `tofu` and `curl` replaced by fakes, so what is
asserted is which subcommands ran with which arguments. Six workflow-wiring
mutations were each caught by the intended test: the preserve step removed,
the preserve step moved after the destroy, the adopt step removed, DOMAIN
dropped from its environment, purge removed, and the snapshot teardown made
untargeted.
One near-miss worth recording: the first version of the ordering tests used
`str.find()`, which returns -1 for a needle that does not match — and -1 sorts
before everything, so `assert preserve < destroy` could never have failed.
The needles did not match, because the step calls the script through
`"$GITHUB_WORKSPACE/..."`. They are regexes now, with an explicit assert that
each one matched.
Closes #892
From the local CodeRabbit round on c402225, and valid for a stronger reason than the one it gave. The purge step copied its neighbours' `if: env.SKIP_TOFU_DESTROY != 'true'`. That flag is set when the OpenTofu backend could not be reached — missing R2 credentials, or a state bucket that is already gone. A preserved service token is not in the state; it is an object in the Cloudflare account, and the step needs only the API, the account id and the domain. So the condition skipped the cleanup in precisely the case where nothing else could ever perform it: a stack torn down, its state bucket deleted, and a live credential left in the account with no policy and no owner. Guard added and mutation-tested: re-adding the condition fails test_the_purge_step_does_not_depend_on_the_opentofu_backend. Refs #892
Reviewer's GuideKeeps the externally consumed Forgejo Cloudflare Access service token stable across rebuild teardowns by taking it out of state before destruction and adopting it before the next apply, with defensive API/state checks, unconditional full-destroy cleanup, executable tests, and corresponding operator documentation. Sequence diagram for preserving and adopting the Forgejo service tokensequenceDiagram
participant Teardown as Rebuild Teardown
participant Script as forgejo-service-token.sh
participant State as OpenTofu State
participant Cloudflare as Cloudflare Access
participant SpinUp as Spin-Up
Teardown->>Script: preserve
Script->>State: tofu state rm RESOURCE
Note over State,Cloudflare: Token remains in Cloudflare and is no longer destroyable
Teardown->>State: tofu destroy
SpinUp->>Script: adopt
Script->>Cloudflare: List service tokens by token_name()
Cloudflare-->>Script: One matching token ID
Script->>State: tofu import RESOURCE
Script->>State: tofu plan -target=RESOURCE -detailed-exitcode
State-->>Script: No changes
SpinUp->>State: tofu apply
Flow diagram for Forgejo service token safety checksflowchart TD
A["Lifecycle action"] --> B{"Operation"}
B -->|preserve| C{"Token in OpenTofu state?"}
C -->|yes| D["tofu state rm RESOURCE"]
C -->|no| E["Continue without preservation"]
D --> F["Run untargeted destroy"]
B -->|adopt| G{"Feature enabled?"}
G -->|no| H["Continue without adoption"]
G -->|yes| I["List matching Cloudflare tokens"]
I --> J{"Exactly one match?"}
J -->|no| K["Refuse and report error"]
J -->|yes| L["tofu import RESOURCE"]
L --> M{"tofu plan -detailed-exitcode is unchanged?"}
M -->|no| K
M -->|yes| N["Proceed with apply"]
B -->|purge| O["Delete all matching tokens from Cloudflare"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: stefanko-ch/Nexus-Stack/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/workflows/destroy-all.yml" line_range="331-336" />
<code_context>
+ # is not in the state; it is an object in the Cloudflare account, and
+ # this step needs only the API. So the case where the flag is set is
+ # precisely the case where nothing else will ever remove it.
+ - name: Remove a preserved Forgejo service token
+ env:
+ CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
+ CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}
+ DOMAIN: ${{ secrets.DOMAIN }}
+ run: bash .github/scripts/forgejo-service-token.sh purge
+
- name: Destroy Control Plane infrastructure
</code_context>
<issue_to_address>
**issue (bug_risk):** The purge step is skipped whenever the preceding destroy step fails, because GitHub Actions applies the default `success()` condition to this step. A failed or cancelled infrastructure destroy therefore leaves the preserved Cloudflare token behind even though `destroy-all` is intended to remove everything.
**Triggers:** When the OpenTofu destroy fails after a teardown has preserved the token.
**Suggested fix:** Run the purge step with `if: always()` (while retaining the required credentials), or otherwise ensure cleanup executes after a failed destroy.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if this is wrong, the teardown/apply orchestration could rotate or fail to remove a Cloudflare Access service token, either breaking the external management plane or leaving a live credential behind after teardown. Reverting the change does not undo tokens already preserved or deleted, so cleanup and possible credential recovery would require manual intervention.
Blocking findings: .github/workflows/destroy-all.yml:336
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address PR review comments on #895. [4052605472] sourcery-ai — Fixed. The purge step carried no `if:`, and a step without one gets an implicit `success()`: "A default status check of success() is applied unless you include one of these functions" (GitHub's expressions reference). So a destroy that failed skipped the cleanup — which is precisely the run in which a preserved token is most likely to be left behind, since nothing else reaches it. Fixed with `if: ${{ !cancelled() }}` rather than the suggested `always()`. The same page advises against `always()` because it runs after a human cancels the workflow, and a cancelled destroy-all should change nothing. A failed one still should: the operator asked for everything to go, and the token is reachable through the API whether or not the state was. The guard for this step now checks all three properties and is mutation-tested on each: no condition at all, `always()`, and the `SKIP_TOFU_DESTROY` gate each fail it.
stefanko-ch
pushed a commit
that referenced
this pull request
Sep 19, 2026
🤖 I have created a release *beep* *boop* --- ## [0.82.3](v0.82.2...v0.82.3) (2026-09-19) ### 🐛 Bug Fixes * **ci:** Keep the Forgejo Access service token across a teardown ([#895](#895)) ([6784086](6784086)) * **deploy:** Copy stacks without rsync when the job image has none ([#899](#899)) ([d083531](d083531)) * **service_env:** reject empty DOMAIN in render_all_env_files ([#863](#863)) ([#867](#867)) ([5c6e93d](5c6e93d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). ## Summary by Sourcery Release version 0.82.3 with fixes for CI token persistence, deployment stack copying, and invalid domain configuration. Bug Fixes: - Preserve the Forgejo Access service token during teardown. - Support copying deployment stacks when rsync is unavailable in the job image. - Reject empty DOMAIN values when rendering service environment files. Chores: - Release version 0.82.3 and update the changelog. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #892.
What broke
Lifecycle: Teardownruns an untargetedtofu destroy, and the Cloudflare Access service token for Forgejo lives in that state. Every teardown destroyed it; the next spin-up minted a replacement with a new client id and secret. Anything outside the stack authenticating to Forgejo with that pair stopped working at that moment — and stopped working silently, because Access answers an unauthenticated call with a 302 to its login page rather than a 401.Measured on the Conductor-Stack after its update to v0.82.2 (teardown → update → spin up):
Two Nexus-Conductor operations failed, retried, failed again. Nothing was wrong with the stack; the credential it had been given no longer existed.
Approach
The token is a management-plane credential: its lifetime is the relationship between the two systems, not the lifetime of a server. So it is taken out of the state before the destroy and imported back on the next spin-up — the trick
setup-control-plane.yaml:950already uses for the KV namespace.The alternative, moving it into
tofu/control-plane, was considered and dropped: the Access application it hangs on lives in the stack state, and the secret would then be created in a layer with no path to Infisical — the server may not exist at Setup Control Plane time. That is a new delivery mechanism for a one-time value, which is the shape of the original problem..github/scripts/forgejo-service-token.sh, three verbs:preserveteardown.yml, before the destroytofu state rm, so the destroy cannot reach it. Fails loudly rather than let the destroy proceed.adoptspin-up.yml, before the applymain.tfgives it, imports it, then plans that one resource with-detailed-exitcodeand refuses if applying would still change it.purgedestroy-all.yml, after the destroyOnly the rebuild lifecycle needed this.
teardown-snapshot.ymldestroys-target=hcloud_server.mainand nothing else, so the token was never at risk there. A test fails if that ever widens.Three refusals, each deliberate
adoptstops. Importing the wrong one hands the Access policy a credential the caller does not hold: the same silent failure, one level deeper."success": false→ an error, not an empty list. Read as "no token", it would mint a second one.-detailed-exitcodeplan catches it. The credential would otherwise rotate on the apply anyway, with nothing saying so.What cannot be restored, and why that is fine
The
client_secret. Cloudflare returns it once, at creation; the provider documentation for v4.49 is explicit that an imported token "will not have the client_secret available in the state for use".That is the point rather than a gap: nobody needs a new one, because the external system keeps the pair it was given. It does mean Infisical shows the id and no secret after the first teardown —
_filter_emptydrops the empty value rather than writing a blank over anything. The adopt step says so in the log, and three docs pages say it too, because unexplained it reads as a defect.Guards
tests/unit/test_forgejo_service_token.py, 29 tests. The script runs for real under bash withtofuandcurlreplaced by fakes, so what is asserted is which subcommands ran with which arguments — not which strings the file contains.jqis the one real dependency, because the name matching is the part worth exercising; locally its absence skips, in CI it fails.Seven mutations, each caught by the intended test:
teardown.ymltest_the_rebuild_teardown_preserves_the_token_before_it_destroysspin-up.ymltest_the_spin_up_adopts_the_token_before_it_appliesDOMAINdropped from the adopt step's environmenttest_the_spin_up_passes_what_the_adopt_step_readsdestroy-all.ymltest_destroy_all_removes_the_preserved_tokentest_the_snapshot_teardown_needs_no_preservationSKIP_TOFU_DESTROYgate re-added to purgetest_the_purge_step_does_not_depend_on_the_opentofu_backendOne near-miss worth recording. The first version of the ordering tests used
str.find(), which returns-1for a needle that does not match — and-1sorts before everything, soassert preserve < destroycould never have failed. The needles did not match, because the step calls the script through"$GITHUB_WORKSPACE/...". They are regexes now, with an explicit assert that each one matched.pytest tests/unit: 3593 passed. Pre-commit (ruff, mypy strict, actionlint): all hooks pass.Docs
docs/stacks/forgejo.md— a new section on the token's lifetime, the once-only secret, and the two refusals.docs/concepts/lifecycle.md— the token joins the "always survives" list.docs/admin-guides/setup-guide.md— one sentence on theENABLE_FORGEJO_SERVICE_TOKENrow.docs/admin-guides/troubleshooting.md— what to do when the adopt step refuses two same-named tokens.Not verified yet
A real teardown → spin-up cycle. The script's logic is exercised by the fakes, but Cloudflare's own import behaviour is not: whether
tofu importof this resource leaves a plan that is genuinely empty is stated by the provider's docs and checked at runtime by the-detailed-exitcodeguard, rather than measured here. The next Conductor lifecycle is what settles it, and the guard is what makes a wrong answer loud instead of silent.Side finding, filed separately
The shellcheck hook matches
^(scripts|stacks)/.*\.sh$, so.github/scripts/is not linted at all — which is how the new script got a "no files to check". Measured the cost of widening it: seven of eight scripts are clean, and the eighth has one false positive (a function reached only throughtrap) and one genuinely deadMIGRATION_SQLvariable that is a stale copy of a schema migration. Filed as #894 rather than folded in here.Local CodeRabbit round
Reviewed
c4022254: 1 finding, valid, fixed in397cece5.if: env.SKIP_TOFU_DESTROY != 'true'. That flag means the OpenTofu backend could not be reached — missing R2 credentials, or a state bucket already gone. A preserved token is not in the state, so the flag says nothing about it, and the condition skipped the cleanup in exactly the case where nothing else could ever perform it.397cece5itself was not reviewed locally; this review covers it.Summary by Sourcery
Keep the Forgejo Access service token stable across rebuild lifecycles while ensuring complete destruction still removes preserved credentials.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: