fix(openbao): harden the OIDC client check before its first live run - #2085
Merged
Merged
Conversation
store_exists answers false both for a secret that does not exist and for a describe that failed (a throttle, a missing grant). Three callers act on 'absent', and each gave wrong advice on a failed read: - openbao-oidc-check.sh passed a stranded platform as 'not bootstrapped' (exit 0), or warned that the next apply would DESTROY a live mount. - reconcile_openbao_oidc skipped silently under --apply, for a key the sync had just written. - cmd_sync advised deleting a working app in ZITADEL. store_probe returns 0 present, 1 absent, 2 cannot tell, with the CLI's error in STORE_PROBE_ERR. Only the provider's own not-found error means absent. store_exists keeps its boolean, which is what store_write needs. Closes #2082
A deadlocked OpenBao core accepts the connection and never answers. With no ceiling, openbao_req -- and so stage4's reconcile and stage5's check -- would hang until someone killed the deploy. The same holds for ZITADEL on the check's first hop, which also defeats the probe's own retry budget. openbao_req now passes --connect-timeout 10 and --max-time 30 (overridable through OPENBAO_CONNECT_TIMEOUT and OPENBAO_MAX_TIME), and the check's first hop reuses them. 30s covers auth_url, the slowest call: OpenBao fetches ZITADEL's discovery document inside it. Closes #2083
oidc.tf ignores the client id, secret and audience that the deploy's sync rotates (#2045), so the management stack's plan can no longer see a stale client, and stage5 only runs on deploy. A client that goes stale between deploys -- a restore, a hand-run sync, an in-place management update -- went unreported until the next deploy. The management stack's drift detect now runs openbao-oidc-check.sh after the plan, with the CA the job already fetches. Its exit 1 is reported as drift and exits 0, so the walk continues, as global.drift_verdict does for a plan; exit 2 (cannot tell) still fails. A non-aws primary_cloud skips it. The suite runs the rendered step against a stub check exiting 0, 1 and 2. Closes #2084
The 30s ceiling from the previous commit can end the one call the discovery-retry loop exists for. OpenBao's own discovery fetch has no timeout of its own, so against a ZITADEL route that drops packets it used to answer after ~90s with 'error checking oidc discovery URL', and the loop retried. Now curl gives up first with exit 28, whose text the loop did not match, so stage4 failed on the first attempt. A curl timeout is the same cause and gets the same retry; the write is idempotent. Also name the absent-key case in its failure message instead of 'still holds client <none>'.
Contributor
🔍 Rendered manifest diff — this PR vs
|
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.
🔍 fix
📝 Summary
Hardens the OpenBao OIDC client check and reconcile that #2078 (#2045) added, before the rebuild that exercises them for the first time. Closes #2082. Closes #2083. Closes #2084.
The worst of the three was #2082. A throttled or denied secret-store read counted as "absent", so the check could pass a broken platform as "not bootstrapped" (exit 0), and the sync could advise deleting a working ZITADEL app.
🎯 Changes
store_probe: present / absent / cannot tell. Only the provider's own not-found error means absent. It replacesstore_existsat the three sites that give advice on "absent"openbao_reqand the check's first hop to ZITADEL get--connect-timeout 10 --max-time 30drift detectruns the check after its plan. Exit 1 is reported as drift and the walk continues; exit 2 still fails📊 What each site now does on a failed store read
flowchart LR probe["store_probe"]:::new -->|"0 present"| ok["unchanged path"] probe -->|"1 absent"| absent["check: bootstrap / DESTROY warning<br/>reconcile: skip (dry run), retry then fail (apply)<br/>sync: restore-or-recreate advice"] probe -->|"2 cannot tell"| fail["check: exit 2<br/>reconcile and sync: FAILED, with the store's error"]:::modified classDef new fill:#1e3a8a,stroke:#3b82f6,stroke-width:3px,color:#fff classDef modified fill:#c2410c,stroke:#f97316,stroke-width:3px,color:#fffBehaviour changes worth knowing
--apply, a missingopenbao-oidckey now fails the reconcile instead of skipping. The sync wrote that key moments earlier, so absent means eventual consistency or a failed write. It goes through the existing 6-retry read loop first. A dry run still skips.drift detectnow calls OpenBao and ZITADEL. If ZITADEL is unreachable the check exits 2 after its retry budget (about 340s worst case) and, like any failing drift job, stops the walk. OIDC drift is printed as a banner and is not part of the drift status sent to Terramate Cloud. No CI workflow consumes drift today.🧪 Evidence
Branch is on
origin/main@279e5b66(0 behind).task ci:testvectornot installed), 0 failedtest-cloud-secret-store/test-openbao-oidc-check/test-zitadel-oidc-clients-openbao/-convergencetest-terramate-script-refstask ci:links/ci:doc-paths/ci:doc-claimsterramate fmt --check,shellcheck -x -S warningEvery new test failed against the pre-fix code before its fix went in. The drift tests render the step's bash out of
workflows.tm.hcland run it against a stub check exiting 0, 1 and 2. An independent review found one Important issue (the timeout vs. the discovery retry). It is fixed ina2fa70a4, with a test.Nothing here has run against a live platform yet. The owner's rebuild after #2078 exercises it.
📌 Follow-up
The same "failed read taken as absent" pattern exists in
merge_secretandzitadel-pat.sh, outside this PR's three issues. Filed separately.