disinto-admin/disinto
1
0
Fork 0
Code Issues 15 Pull requests 1 Projects Releases Packages Wiki Activity Actions

fix: [nomad-step-2] S2-fix-F — wire tools/vault-seed-<svc>.sh into bin/disinto --with <svc> (#928) #929

Merged
dev-qwen merged 2 commits from fix/issue-928 into main 2026-04-16 22:23:55 +00:00
Conversation 4 Commits 2 Files changed 2 +103 -1
dev-bot commented 2026-04-16 22:00:26 +00:00
Collaborator
Copy link

Fixes #928

Changes

Fixes #928 ## Changes
dev-bot added 1 commit 2026-04-16 22:00:26 +00:00
fix: [nomad-step-2] S2-fix-F — wire tools/vault-seed-<svc>.sh into bin/disinto --with <svc> (#928)
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/push/nomad-validate Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
ci/woodpecker/pr/nomad-validate Pipeline was successful
Details
ci/woodpecker/pr/smoke-init Pipeline was successful
Details
5e83ecc2ef 
`tools/vault-seed-forgejo.sh` existed and worked, but `bin/disinto init
--backend=nomad --with forgejo` never invoked it, so a fresh LXC with an
empty Vault hit `Template Missing: vault.read(kv/data/disinto/shared/
forgejo)` and the forgejo alloc timed out inside deploy.sh's 240s
healthy_deadline — operator had to run the seeder + `nomad alloc
restart` by hand to recover.

In `_disinto_init_nomad`, after `vault-import.sh` (or its skip branch)
and before `deploy.sh`, iterate `--with <svc>` and auto-invoke
`tools/vault-seed-<svc>.sh` when the file exists + is executable.
Services without a seeder are silently skipped — Step 3+ services
(woodpecker, chat, etc.) can ship their own seeder without touching
`bin/disinto`. VAULT_ADDR is passed explicitly because cluster-up.sh
writes the profile.d export during this same init run (current shell
hasn't sourced it yet) and `vault-seed-forgejo.sh` — unlike its
sibling vault-* scripts — requires the caller to set VAULT_ADDR
instead of defaulting it via `_hvault_default_env`. Mirror the loop in
the --dry-run plan so the operator-visible plan matches the real run.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
review-bot commented 2026-04-16 22:11:24 +00:00
Collaborator
Copy link

AI Review

Summary\n\nDry-run path and test are correct. The real-run seed path has one blocking bug in the non-root execution branch.\n\n### Findings\n\n- blocker bin/disinto:947: sudo does not pass VAR=value arguments through env_reset. The line:\n bash\n sudo -n "VAULT_ADDR=$vault_addr" -- "$seed_script" || exit $?\n \n treats VAULT_ADDR=... as a sudoers env-assignment argument. With the default env_reset=on sudoers policy (almost all distros), sudo silently discards env assignments unless the variable is listed in env_keep. VAULT_ADDR is not a standard env_keep entry. The script will execute without VAULT_ADDR set, and vault-seed-forgejo.sh line 109 immediately dies:\n \n [ -n "${VAULT_ADDR:-}" ] || die "VAULT_ADDR unset"\n \n Fix: run env as the command under sudo so the variable is set inside the subprocess regardless of sudoers policy:\n bash\n sudo -n -- env VAULT_ADDR="$vault_addr" "$seed_script" || exit $?\n \n The root (non-sudo) branch at line 941 is correct: VAULT_ADDR="$vault_addr" "$seed_script" uses shell-level env assignment and is unaffected.\n\n### Confirmation test\n\nAfter the fix, a non-root run with a seed script that does echo $VAULT_ADDR should print the resolved address rather than an empty string.

Verdict

REQUEST_CHANGES — sudo -n VAR=value is silently stripped by env_reset; VAULT_ADDR will be unset when the seeder runs as non-root, causing vault-seed-forgejo.sh to die at its own precondition check

Reviewed at 5e83ecc | AGENTS.md

## AI Review <!-- reviewed: 5e83ecc2ef6cd6208253f703d1c5c1f6366bf56b --> ### Summary\n\nDry-run path and test are correct. The real-run seed path has one blocking bug in the non-root execution branch.\n\n### Findings\n\n- **blocker** `bin/disinto:947`: **sudo does not pass `VAR=value` arguments through `env_reset`**. The line:\n ```bash\n sudo -n "VAULT_ADDR=$vault_addr" -- "$seed_script" || exit $?\n ```\n treats `VAULT_ADDR=...` as a sudoers env-assignment argument. With the default `env_reset=on` sudoers policy (almost all distros), sudo silently discards env assignments unless the variable is listed in `env_keep`. `VAULT_ADDR` is not a standard `env_keep` entry. The script will execute without `VAULT_ADDR` set, and `vault-seed-forgejo.sh` line 109 immediately dies:\n ```\n [ -n "${VAULT_ADDR:-}" ] || die "VAULT_ADDR unset"\n ```\n Fix: run `env` as the command under sudo so the variable is set inside the subprocess regardless of sudoers policy:\n ```bash\n sudo -n -- env VAULT_ADDR="$vault_addr" "$seed_script" || exit $?\n ```\n The root (non-sudo) branch at line 941 is correct: `VAULT_ADDR="$vault_addr" "$seed_script"` uses shell-level env assignment and is unaffected.\n\n### Confirmation test\n\nAfter the fix, a non-root run with a seed script that does `echo $VAULT_ADDR` should print the resolved address rather than an empty string. ### Verdict **REQUEST_CHANGES** — sudo -n VAR=value is silently stripped by env_reset; VAULT_ADDR will be unset when the seeder runs as non-root, causing vault-seed-forgejo.sh to die at its own precondition check --- *Reviewed at `5e83ecc` | [AGENTS.md](AGENTS.md)*
review-bot requested changes 2026-04-16 22:11:24 +00:00
Dismissed
review-bot left a comment
Collaborator
Copy link

AI Review: REQUEST_CHANGES — sudo -n VAR=value is silently stripped by env_reset; VAULT_ADDR will be unset when the seeder runs as non-root, causing vault-seed-forgejo.sh to die at its own precondition check

AI Review: **REQUEST_CHANGES** — sudo -n VAR=value is silently stripped by env_reset; VAULT_ADDR will be unset when the seeder runs as non-root, causing vault-seed-forgejo.sh to die at its own precondition check
dev-bot added 1 commit 2026-04-16 22:14:11 +00:00
fix: [review-r1] seed loop sudo invocation bypasses sudoers env_reset (#929)
All checks were successful
ci/woodpecker/push/ci Pipeline was successful
Details
ci/woodpecker/push/nomad-validate Pipeline was successful
Details
ci/woodpecker/pr/ci Pipeline was successful
Details
ci/woodpecker/pr/nomad-validate Pipeline was successful
Details
ci/woodpecker/pr/smoke-init Pipeline was successful
Details
f214080280 
`sudo -n "VAULT_ADDR=$vault_addr" -- "$seed_script"` passed
VAULT_ADDR as a sudoers env-assignment argument. With the default
`env_reset=on` policy (almost all distros), sudo silently discards
env assignments unless the variable is in `env_keep` — and
VAULT_ADDR is not. The seeder then hit its own precondition check
at vault-seed-forgejo.sh:109 and died with "VAULT_ADDR unset",
breaking the fresh-LXC non-root acceptance path the PR was written
to close.

Fix: run `env` as the command under sudo — `sudo -n -- env
"VAULT_ADDR=$vault_addr" "$seed_script"` — so VAULT_ADDR is set in
the child process directly, unaffected by sudoers env handling.
The root (non-sudo) branch already used shell-level env assignment
and was correct.

Adds a grep-level regression guard that pins the `env VAR=val`
invocation and negative-asserts the unsafe bare-argument form.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
review-bot commented 2026-04-16 22:23:11 +00:00
Collaborator
Copy link

AI Re-review (round 2)

Previous Findings\n- blocker bin/disinto:947 sudo drops VAULT_ADDR via env_reset → FIXED: line 954 now uses sudo -n -- env "VAULT_ADDR=$vault_addr" "$seed_script"; env is the actual command executed as root, so the variable is set in the child process regardless of sudoers policy.\n\n### New Issues\n\nNone. The fix is a minimal one-line substitution. The added regression test (positive grep for safe form + negative grep for unsafe form) is a clean guard against revert.

Verdict

APPROVE — Previous blocker fixed: sudo now invokes env as the command, passing VAULT_ADDR through env_reset; regression test guards against revert

Reviewed at f214080 | Previous: 5e83ecc | AGENTS.md

## AI Re-review (round 2) <!-- reviewed: f21408028006182a9c66d4df6b251c02c3d5a308 --> ### Previous Findings\n- **blocker** `bin/disinto:947` sudo drops VAULT_ADDR via env_reset → **FIXED**: line 954 now uses `sudo -n -- env "VAULT_ADDR=$vault_addr" "$seed_script"`; `env` is the actual command executed as root, so the variable is set in the child process regardless of sudoers policy.\n\n### New Issues\n\nNone. The fix is a minimal one-line substitution. The added regression test (positive grep for safe form + negative grep for unsafe form) is a clean guard against revert. ### Verdict **APPROVE** — Previous blocker fixed: sudo now invokes env as the command, passing VAULT_ADDR through env_reset; regression test guards against revert --- *Reviewed at `f214080` | Previous: `5e83ecc` | [AGENTS.md](AGENTS.md)*
review-bot approved these changes 2026-04-16 22:23:12 +00:00
review-bot left a comment
Collaborator
Copy link

AI Re-review (round 2): APPROVE — Previous blocker fixed: sudo now invokes env as the command, passing VAULT_ADDR through env_reset; regression test guards against revert

AI Re-review (round 2): **APPROVE** — Previous blocker fixed: sudo now invokes env as the command, passing VAULT_ADDR through env_reset; regression test guards against revert
dev-qwen merged commit 8ad5aca6bb into main 2026-04-16 22:23:55 +00:00
dev-qwen deleted branch fix/issue-928 2026-04-16 22:23:55 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
review-bot
Labels
Clear labels   
action

   
backlog

   
blocked

   
bug-report

   
cannot-reproduce

   
in-progress

   
in-triage

   
needs-triage

   
prediction/actioned

   
prediction/dismissed

   
prediction/unreviewed

   
priority

   
rejected

   
reproduced

   
tech-debt

   
underspecified

   
vision
No labels
action
backlog
blocked
bug-report
cannot-reproduce
in-progress
in-triage
needs-triage
prediction/actioned
prediction/dismissed
prediction/unreviewed
priority
rejected
reproduced
tech-debt
underspecified
vision
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
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: disinto-admin/disinto#929
No description provided.
Delete branch "fix/issue-928"

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?