feat(platform): NocoDB and n8n composable deployment migration#15
feat(platform): NocoDB and n8n composable deployment migration#15jestyr27 wants to merge 16 commits into
Conversation
NocoDB template: postgres_password, jwt_secret placeholders n8n template: admin_password, user_password, encryption_key placeholders BATS tests verify template structure and no hardcoded secrets
Remove secret generation (step_generate_secrets) and OpenBao storage (step_store_in_openbao). Ansible manage-secrets.yml now handles both. Bootstrap step reads admin password from config/nocodb.env instead of secrets/ directory. API token printed to stdout for Ansible to capture.
Remove secret generation (step_generate_secrets) and OpenBao storage (step_store_in_openbao). Ansible manage-secrets.yml now handles both. Bootstrap step reads owner password from config/n8n.env instead of secrets/ directory. API key printed to stdout for Ansible to capture.
Replace thin wrapper with full composable deployment: Phase 1: Clone repo, OpenBao secrets, Jinja2 template env files Phase 2: deploy.sh (container lifecycle + bootstrap) Phase 3: Store runtime API token in OpenBao Phase 4: Health check verification
Replace thin wrapper with full composable deployment: Phase 1: Clone repo, OpenBao secrets, Jinja2 template env files Phase 2: deploy.sh (container lifecycle + bootstrap) Phase 3: Store runtime API key in OpenBao Phase 4: Health check verification
…or NocoDB and n8n Clean deploy playbooks use tasks/clean-service.yml to destroy all data then run the full composable deployment. Semaphore templates added for both services with branch override support.
Update OpenBao secrets layout with full secret sets for both services. Add NocoDB and n8n to completed deployments and workflows table. Add clean-deploy playbooks to workflows table.
Use prefixed secret names (nocodb_pg_password, n8n_admin_password, etc.) to match existing secrets on VMs and in OpenBao. Unprefixed names would cause manage-secrets.yml to generate new passwords, breaking running database connections. Also separate N8N_OWNER_PASSWORD from POSTGRES_NON_ROOT_PASSWORD in the n8n template — these are independent secrets for different auth domains.
deploy.sh was reading POSTGRES_PASSWORD as the NocoDB web admin password. These are separate credentials — add NOCODB_ADMIN_PASSWORD to the env template so deploy.sh uses the correct secret for signup/signin. Same pattern as the n8n N8N_OWNER_PASSWORD fix.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughConverted NocoDB and n8n deploys from thin wrappers into explicit multi-phase Ansible playbooks that clone the monorepo, render env templates from Ansible ChangesNocoDB & n8n deployment and orchestration
Sequence DiagramsequenceDiagram
actor Operator
participant Semaphore
participant Agent
participant OpenBao
participant Service
Operator->>Semaphore: trigger deploy/clean-deploy
Semaphore->>Agent: clone/update agent-cloud repo
Agent->>OpenBao: (via manage-secrets.yml) fetch secrets
Agent->>Agent: render config/*.env templates
Agent->>Agent: execute deploy.sh
Agent->>Service: start containers (docker-compose)
Service-->>Agent: become healthy (/healthz)
Agent->>Service: bootstrap credentials (signup/signin or owner setup)
Service-->>Agent: return token on stdout (NOCODB_API_TOKEN / N8N_API_KEY)
Agent->>Semaphore: capture deploy stdout
Semaphore->>OpenBao: AppRole login (delegate to localhost)
Semaphore->>OpenBao: write secret/data/services/{nocodb,n8n} (token + service_url)
OpenBao-->>Semaphore: 200 OK
Semaphore->>Service: final health check
Service-->>Semaphore: 200 OK
Semaphore-->>Operator: deployment complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform/services/n8n/deployment/deploy.sh (2)
58-72: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
trapset inside a function is global — clobbers any pre-existing trap.Bash traps are process-global, so installing
trap 'rm -f "$cookie_jar"' EXIT INT TERMon line 60 silently overwrites whatever trapcommon.shor earlier code may have registered. Low risk today since nothing else is registered, but it's a footgun if anyone later adds cleanup inmain.♻️ Suggested fix — clean up locally with `&&`/explicit rm
local cookie_jar login_payload cookie_jar=$(mktemp) - trap 'rm -f "$cookie_jar"' EXIT INT TERM + # Local cleanup; avoid overriding any global trap installed by common.sh + _cleanup_cookie() { rm -f "$cookie_jar"; }…and call
_cleanup_cookieon each return path, or use a subshell. Skippable if you're sure no other trap will ever land here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/services/n8n/deployment/deploy.sh` around lines 58 - 72, The trap call in the login routine installs a global EXIT/INT/TERM handler and can clobber other traps; remove the trap 'rm -f "$cookie_jar"' usage and instead ensure the temp file is removed locally by either wrapping the login sequence in a subshell or implementing a local cleanup function (e.g., _cleanup_cookie) that calls rm -f "$cookie_jar" and invoking that function on every return path (success and failure) after the curl block; update references around cookie_jar, login_payload and the curl POST to call the local cleanup before returning so no global trap is required.
90-111:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftGuideline violation:
deploy.shis generating a secret in the DB-insert fallback.
openssl rand -hex 20on line 93 generates a runtime credential, and the script then inserts it directly into Postgres. Two coding guidelines call this out explicitly:
- "deploy.sh must NOT generate secrets, interact with OpenBao, or call generate-secrets.sh."
- "deploy.sh must be pure container operations."
This also breaks the stated OpenBao-as-source-of-truth model: a key that exists only in the n8n DB (because the API endpoint was unavailable at deploy time) is now out of band from Phase 3's OpenBao write — except that since you
echoit back, Phase 3 will store it, but only if the regex match still works on stdout. The bigger problem is that this path bypasses any policy/audit OpenBao would apply.Two reasonable options:
- Move the fallback to Ansible: have Phase 2 ask OpenBao for
n8n_api_keyand pass it todeploy.shvia env when present, so the script only ever consumes a key.- If keeping the fallback here for operational simplicity, update the coding guideline +
CLAUDE.mdto carve out "runtime bootstrap credentials" so this doesn't trip future reviews.Either way, the current state contradicts the rules in
**/deploy.shandplatform/services/*/deployment/deploy.sh.As per coding guidelines: "deploy.sh must be pure container operations. It must NOT generate secrets, interact with OpenBao, or call generate-secrets.sh."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/services/n8n/deployment/deploy.sh` around lines 90 - 111, The script currently generates a runtime secret with openssl rand -hex 20, inserts it into Postgres, and echoes it (api_key / insert_result / echo "N8N_API_KEY..."), which violates the "deploy.sh must NOT generate secrets" rule; change the fallback so deploy.sh only consumes a provided secret: remove the openssl rand invocation and the DB-insert echo, read N8N_API_KEY from the environment (or fail fast with a clear warn and exit if unset), and use that value for the psql INSERT (keep the existing insert_result logic), or alternatively move this fallback into Ansible/Phase‑2 so OpenBao supplies the key—do not generate or print secrets in deploy.sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/playbooks/deploy-n8n.yml`:
- Around line 74-91: The debug task "Deploy output" is leaking secrets because
_deploy.stdout_lines includes the printed N8N_API_KEY; remove or disable that
debug output and mark any tasks that handle the secret as no_log: true
(specifically the "Run deploy.sh" shell task that registers _deploy and the
"Extract API key from deploy output" set_fact that writes _n8n_api_key) so the
API key is never logged; keep the regex extraction into _n8n_api_key as-is but
ensure no_log: true is set on the set_fact (and the shell task) so the secret is
suppressed in controller logs.
- Around line 105-135: Replace the two uri-based tasks ("Authenticate to
OpenBao" and "Store API key in OpenBao") with a single
community.hashi_vault.vault_write call that uses auth_method: approle (providing
role_id and secret_id) and writes the KV data for services/n8n (n8n_api_key and
url); this uses community.hashi_vault.vault_write to handle AppRole auth, token
lifecycle, and the KV v2 path semantics, remove the manual _bao_auth register
and the separate POST, keep no_log for sensitive inputs, and ensure the playbook
requires collection version >=6.0.0 so the module is available.
- Around line 148-156: The "Health check" ansible.builtin.uri task currently
relies on retries/delay without an explicit condition; register the task result
(e.g., register: health_check) and add an explicit until condition such as
until: health_check.status == 200 so the task will actually retry the health
endpoint until it returns 200 (keep retries/delay as-is and retain the when:
service_url is defined guard); ensure you reference the uri task named "Health
check" and the registered variable health_check when adding the until clause.
In `@platform/playbooks/deploy-nocodb.yml`:
- Around line 34-61: Replace the inline git clone and symlink tasks with the
composable task include tasks/clone-and-deploy.yml: remove the
ansible.builtin.git block and the ansible.builtin.file (symlink) block and
instead add ansible.builtin.include_tasks: tasks/clone-and-deploy.yml, ensuring
you pass through the same variables used by the inlined tasks (monorepo_repo,
_monorepo_dir, _branch, _deploy_dir, service_name, and any register/failure
semantics expected) so behavior and error handling remain identical; keep the
subsequent include of tasks/manage-secrets.yml unchanged.
- Around line 73-91: The deploy playbook currently leaks NOCODB_API_TOKEN via
the "Run deploy.sh" stdout and the "Deploy output" debug task and also writes
the secret into _nocodb_api_token without no_log; to fix, add no_log: true to
the "Run deploy.sh" task (the ansible.builtin.shell that registers _deploy) so
stdout isn't stored in cleartext, remove or change the "Deploy output" debug
task so it does not print _deploy.stdout_lines (replace with a non-secret
summary message if needed), and add no_log: true to the "Extract API token from
deploy output" set_fact that sets _nocodb_api_token so the extracted token is
not logged.
- Around line 147-155: The "Health check" ansible task is missing a
register/until pair so retries are never used; update the task to register the
response (e.g., register: _health) and add until: _health.status == 200 so the
retries/delay loop actually re-attempts the URI call (keep validate_certs,
timeout, status_code as-is and retain the when: service_url is defined
condition).
In `@platform/services/n8n/deployment/templates/n8n.env.j2`:
- Around line 3-7: The template currently hardcodes usernames
POSTGRES_USER=n8n_admin and POSTGRES_NON_ROOT_USER=n8n_user; change these to
template variables (e.g., POSTGRES_USER={{ postgres_admin_user }} and
POSTGRES_NON_ROOT_USER={{ postgres_non_root_user }}) and wire those variables
into your inventory/secret inputs so they are provided at deploy time instead of
committed; update the corresponding inventory/secret source to define
postgres_admin_user and postgres_non_root_user (or map them from existing secret
names) and ensure any code referencing POSTGRES_USER/POSTGRES_NON_ROOT_USER
still receives the values from the template variables.
- Around line 4-10: Secret templating uses {{ secrets.* }} but must follow the
repo convention; update the secret placeholders in the n8n env template by
replacing each {{ secrets.* }} usage with the vault_* (or _secret_*) form.
Specifically change POSTGRES_PASSWORD, POSTGRES_NON_ROOT_PASSWORD,
DB_POSTGRESDB_PASSWORD, N8N_ENCRYPTION_KEY and N8N_OWNER_PASSWORD to use
vault_n8n_admin_password / vault_n8n_user_password / vault_n8n_user_password /
vault_n8n_encryption_key / vault_n8n_owner_password (or the equivalent _secret_*
names) while leaving non-secret entries like POSTGRES_DB and
POSTGRES_NON_ROOT_USER unchanged.
In `@platform/services/nocodb/deployment/deploy.sh`:
- Around line 12-13: The deploy.sh uses a relative traversal to build LIB_DIR;
change it to prefer the repo root via CLONE_DIR by setting LIB_DIR to use
"${CLONE_DIR:-...}/platform/lib" (i.e. use CLONE_DIR if set, otherwise fall back
to the current dirname traversal), then source "${LIB_DIR}/common.sh" as before;
update the LIB_DIR assignment in deploy.sh (the symbol LIB_DIR and its use when
sourcing common.sh) so the script honors the CLONE_DIR contract.
- Around line 34-37: The bootstrap currently greps CONFIG_DIR/nocodb.env into
admin_pass and, if empty, calls warn and returns success; change this to fail
fast: before or immediately after the admin_pass assignment (the line that reads
admin_pass=$(grep '^NOCODB_ADMIN_PASSWORD=' "${CONFIG_DIR}/nocodb.env" ...)),
detect a missing or unreadable "${CONFIG_DIR}/nocodb.env" or an empty admin_pass
and replace the warn/return 0 behavior with a non-zero exit (e.g., log an error
with the script's error/fatal function or echo to stderr) and exit 1 so the
script aborts when the .env is absent or the password is unset.
- Line 90: The deploy workflow is leaking the NOCODB API token because the shell
task "Run deploy.sh", the debug task "Deploy output", and the set_fact task
"Extract API token from deploy output" all log stdout; update the playbook tasks
named exactly "Run deploy.sh", "Deploy output", and "Extract API token from
deploy output" to include no_log: true so the token echoed by deploy.sh (the
line echo "NOCODB_API_TOKEN=${api_token}" in
platform/services/nocodb/deployment/deploy.sh) is not recorded in Ansible logs;
ensure each task's YAML contains no_log: true (leave the echo in deploy.sh
unchanged).
In `@platform/services/nocodb/deployment/templates/nocodb.env.j2`:
- Around line 4-8: The template uses {{ secrets.nocodb_pg_password }}, {{
secrets.nocodb_jwt_secret }}, and {{ secrets.nocodb_admin_password }} but must
use the mandated secret variable namespace; update the template lines that set
POSTGRES_PASSWORD, NC_DB (the pg URL), NC_AUTH_JWT_SECRET, and
NOCODB_ADMIN_PASSWORD to reference the approved variables (e.g., {{
vault_nocodb_pg_password }} or {{ _secret_nocodb_pg_password }} etc.) and adjust
the corresponding render/vars that supply these values so they match the new
names.
- Around line 3-5: Replace the hardcoded POSTGRES_USER value with a template
variable so credentials/usernames aren't committed: change the literal "nocodb"
to a Jinja variable (e.g., {{ secrets.nocodb_pg_user }} or a similarly named
inventory variable) in nocodb.env.j2, update any inventory/secrets source to
provide that key, and adjust deployment/config docs or templates that reference
POSTGRES_USER to use the new variable (search for POSTGRES_USER and nocodb
references to update callers like your deployment/playbook that loads
nocodb.env.j2).
In `@platform/tests/test_n8n_templates.bats`:
- Around line 11-19: Tests currently assert presence of '{{ secrets.* }}'
placeholders (see grep patterns for '{{ secrets.n8n_admin_password }}', '{{
secrets.n8n_user_password }}', '{{ secrets.n8n_encryption_key }}' in the
n8n.env.j2 tests) but our templates must use the vault_*/_secret_* convention;
update each assertion in the "@test" blocks that grep those secrets to instead
validate the allowed conventions (accept '{{ vault_n8n_* }}' or '{{
_secret_n8n_* }}' placeholders for admin password, user password and encryption
key), and make the same change for the other related assertions referenced in
the comment (the tests around lines 37–43).
In `@platform/tests/test_nocodb_templates.bats`:
- Around line 11-15: The tests are asserting the old secrets.* placeholders;
update the grep expectations in the test cases that check
"$TEMPLATE_DIR/nocodb.env.j2" (both the PG password and JWT secret checks and
the similar checks at lines ~28-34) to look for the mandated service-template
secret names using the vault/_secret convention (e.g., replace checks for '{{
secrets.nocodb_pg_password }}' and '{{ secrets.nocodb_jwt_secret }}' with the
allowed pattern such as '{{ vault_nocodb_pg_password }}' or '{{
_secret_nocodb_pg_password }}' and similarly for the JWT secret) so CI enforces
the vault_*/_secret_* variable family.
---
Outside diff comments:
In `@platform/services/n8n/deployment/deploy.sh`:
- Around line 58-72: The trap call in the login routine installs a global
EXIT/INT/TERM handler and can clobber other traps; remove the trap 'rm -f
"$cookie_jar"' usage and instead ensure the temp file is removed locally by
either wrapping the login sequence in a subshell or implementing a local cleanup
function (e.g., _cleanup_cookie) that calls rm -f "$cookie_jar" and invoking
that function on every return path (success and failure) after the curl block;
update references around cookie_jar, login_payload and the curl POST to call the
local cleanup before returning so no global trap is required.
- Around line 90-111: The script currently generates a runtime secret with
openssl rand -hex 20, inserts it into Postgres, and echoes it (api_key /
insert_result / echo "N8N_API_KEY..."), which violates the "deploy.sh must NOT
generate secrets" rule; change the fallback so deploy.sh only consumes a
provided secret: remove the openssl rand invocation and the DB-insert echo, read
N8N_API_KEY from the environment (or fail fast with a clear warn and exit if
unset), and use that value for the psql INSERT (keep the existing insert_result
logic), or alternatively move this fallback into Ansible/Phase‑2 so OpenBao
supplies the key—do not generate or print secrets in deploy.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1fabdce-b249-403f-8296-b00855032427
📒 Files selected for processing (12)
CLAUDE.mdplatform/playbooks/clean-deploy-n8n.ymlplatform/playbooks/clean-deploy-nocodb.ymlplatform/playbooks/deploy-n8n.ymlplatform/playbooks/deploy-nocodb.ymlplatform/semaphore/templates.ymlplatform/services/n8n/deployment/deploy.shplatform/services/n8n/deployment/templates/n8n.env.j2platform/services/nocodb/deployment/deploy.shplatform/services/nocodb/deployment/templates/nocodb.env.j2platform/tests/test_n8n_templates.batsplatform/tests/test_nocodb_templates.bats
| - name: "Authenticate to OpenBao" | ||
| ansible.builtin.uri: | ||
| url: "{{ _bao_url }}/v1/auth/approle/login" | ||
| method: POST | ||
| body_format: json | ||
| body: | ||
| role_id: "{{ _bao_role_id }}" | ||
| secret_id: "{{ _bao_secret_id }}" | ||
| status_code: [200] | ||
| register: _bao_auth | ||
| delegate_to: localhost | ||
| run_once: true | ||
| no_log: true | ||
| when: _n8n_api_key | default('') | length > 0 | ||
|
|
||
| - name: "Store API key in OpenBao" | ||
| ansible.builtin.uri: | ||
| url: "{{ _bao_url }}/v1/secret/data/services/n8n" | ||
| method: POST | ||
| headers: | ||
| X-Vault-Token: "{{ _bao_auth.json.auth.client_token }}" | ||
| body_format: json | ||
| body: | ||
| data: | ||
| n8n_api_key: "{{ _n8n_api_key }}" | ||
| url: "{{ service_url | default('http://localhost:5678') }}" | ||
| status_code: [200] | ||
| delegate_to: localhost | ||
| run_once: true | ||
| no_log: true | ||
| when: _n8n_api_key | default('') | length > 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
🧩 Analysis chain
🌐 Web query:
community.hashi_vault vault_write module AppRole authentication parameters
💡 Result:
For the Ansible collection community.hashi_vault.vault_write module, AppRole authentication is done via these module parameters: 1) auth_method - Set auth_method to "approle" to use AppRole authentication. [1][2] 2) role_id - role_id is the Vault AppRole role ID (or role name) used for approle auth. [1][2] 3) secret_id - secret_id is the AppRole SecretID used for approle authentication. [1][2] These parameters are used together with the module’s url and path to perform the write. For example, generating a SecretID is a write to auth/approle/role//secret-id, and the module can be used to retrieve/generate RoleID and SecretID as part of a workflow. [1][2] Separately (if you’re looking at Vault/Terraform-native AppRole login rather than Ansible), Vault’s AppRole login uses the auth/approle/role//login endpoint and the role can be configured with parameters such as token_type, secret_id_ttl, token_ttl, token_max_ttl, and secret_id_num_uses (among others). [3] If you meant “community.hashi_vault.vault_write module (Ansible) authentication parameters” specifically: the AppRole-relevant ones are exactly auth_method, role_id, and secret_id as documented above. [1][2]
Citations:
- 1: https://docs.ansible.com/ansible/latest/collections/community/hashi_vault/vault_write_module.html
- 2: https://ansible-collections.github.io/community.hashi_vault/tag/3.0.0/collections/community/hashi_vault/vault_write_module.html
- 3: https://developer.hashicorp.com/vault/docs/auth/approle
🏁 Script executed:
# First, verify the file exists and check the exact code
head -150 platform/playbooks/deploy-n8n.yml | tail -50Repository: uhstray-io/agent-cloud
Length of output: 1709
🏁 Script executed:
# Search for any existing community.hashi_vault usage in the codebase
rg "community\.hashi_vault|vault_write|vault_read" --type yml -B 2 -A 5Repository: uhstray-io/agent-cloud
Length of output: 94
🏁 Script executed:
# Check if there's any AppRole-related task library usage
find platform -name "*.yml" -type f | xargs grep -l "manage-approle\|approle" | head -10Repository: uhstray-io/agent-cloud
Length of output: 463
🏁 Script executed:
# Check for manage-approle.yml task mentioned in guidelines
find platform -name "manage-approle.yml" -o -name "*approle*"Repository: uhstray-io/agent-cloud
Length of output: 110
🏁 Script executed:
# Search for any Vault/Bao interaction patterns in the codebase
rg "vault_write|vault_read|hashi_vault|OpenBao|secret/data" --type yaml -B 2 -A 3 | head -80Repository: uhstray-io/agent-cloud
Length of output: 5252
🏁 Script executed:
# Check the actual vault_write module documentation to understand if it handles AppRole auth
# Let's look at what the module expects
rg "auth_method.*approle|role_id.*secret_id" --type yamlRepository: uhstray-io/agent-cloud
Length of output: 2076
🏁 Script executed:
# Look at how vault_read is actually used in distribute-ssh-keys.yml
sed -n '1,100p' platform/playbooks/distribute-ssh-keys.yml | grep -A 10 -B 5 "vault_read"Repository: uhstray-io/agent-cloud
Length of output: 48
🏁 Script executed:
# Get broader context of vault_read usage
rg "vault_read|vault_write" --type yaml -B 3 -A 8 | head -120Repository: uhstray-io/agent-cloud
Length of output: 48
🏁 Script executed:
# Check the manage-approle.yml task
cat platform/playbooks/tasks/manage-approle.ymlRepository: uhstray-io/agent-cloud
Length of output: 3541
🏁 Script executed:
# Look at the exact context where vault_read is used in distribute-ssh-keys.yml
sed -n '1,150p' platform/playbooks/distribute-ssh-keys.ymlRepository: uhstray-io/agent-cloud
Length of output: 4603
🏁 Script executed:
# Search for all instances of vault_read to see the exact pattern
rg "vault_read" --type yaml -B 5 -A 2Repository: uhstray-io/agent-cloud
Length of output: 48
🏁 Script executed:
# Search for any use of vault_write or vault_read as modules (not lookups)
rg "community.hashi_vault.vault_write|community.hashi_vault.vault_read" --type yamlRepository: uhstray-io/agent-cloud
Length of output: 48
🏁 Script executed:
# Check what version of community.hashi_vault is required
cat platform/services/semaphore/deployment/collections/requirements.ymlRepository: uhstray-io/agent-cloud
Length of output: 138
🏁 Script executed:
# Look at the coding guidelines section again to understand the exact requirement
cat pyproject.toml | grep -A 20 "hashi_vault\|OpenBao" || echo "not in pyproject"Repository: uhstray-io/agent-cloud
Length of output: 83
🏁 Script executed:
# Check if there's any documentation about vault_write usage expectations
find platform -name "*.md" -o -name "*.txt" | xargs grep -l "vault_write\|hashi_vault" 2>/dev/null | head -5Repository: uhstray-io/agent-cloud
Length of output: 95
🏁 Script executed:
# Check the playbooks README for guidance on vault usage
cat platform/playbooks/README.mdRepository: uhstray-io/agent-cloud
Length of output: 5417
🏁 Script executed:
# Check if there are any examples of vault_write being used anywhere in the repo
find . -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.md" \) | xargs grep -l "vault_write" 2>/dev/nullRepository: uhstray-io/agent-cloud
Length of output: 50
Migrate to community.hashi_vault.vault_write for secret storage.
The guideline states "Use community.hashi_vault for OpenBao access," but this playbook uses raw uri calls for AppRole auth and secret writes. The vault_write module (available in the required >=6.0.0 collection) natively handles AppRole authentication, token management, and cleanup, eliminating the manual shuffle of client_token between tasks and reducing token exposure in register dicts.
Not blocking — uri functions — but brings consistency with the stated guideline and removes a couple of potential failure modes.
🧰 Tools
🪛 Checkov (3.2.526)
[medium] 105-121: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🪛 GitHub Check: Static Analysis
[failure] 120-120: run-once[task]
Using run_once may behave differently if strategy is set to free.
[failure] 105-105: run-once[task]
Using run_once may behave differently if strategy is set to free.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/deploy-n8n.yml` around lines 105 - 135, Replace the two
uri-based tasks ("Authenticate to OpenBao" and "Store API key in OpenBao") with
a single community.hashi_vault.vault_write call that uses auth_method: approle
(providing role_id and secret_id) and writes the KV data for services/n8n
(n8n_api_key and url); this uses community.hashi_vault.vault_write to handle
AppRole auth, token lifecycle, and the KV v2 path semantics, remove the manual
_bao_auth register and the separate POST, keep no_log for sensitive inputs, and
ensure the playbook requires collection version >=6.0.0 so the module is
available.
| tasks: | ||
| - name: "Install git if needed" | ||
| ansible.builtin.apt: | ||
| name: git | ||
| state: present | ||
| become: true | ||
|
|
||
| - name: "Clone or update agent-cloud monorepo" | ||
| ansible.builtin.git: | ||
| repo: "{{ monorepo_repo }}" | ||
| dest: "{{ _monorepo_dir }}" | ||
| version: "{{ _branch }}" | ||
| force: true | ||
|
|
||
| - name: "Create convenience symlink" | ||
| ansible.builtin.file: | ||
| src: "{{ _deploy_dir }}" | ||
| dest: "/home/{{ ansible_user }}/{{ service_name }}" | ||
| state: link | ||
| force: true | ||
| register: _symlink_result | ||
| failed_when: | ||
| - _symlink_result is failed | ||
| - _symlink_result.msg is defined | ||
| - "'permission denied' in (_symlink_result.msg | lower)" | ||
|
|
||
| - name: "Manage secrets and template env files" | ||
| ansible.builtin.include_tasks: tasks/manage-secrets.yml |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Reuse tasks/clone-and-deploy.yml instead of inlining clone + symlink (same as deploy-n8n.yml).
Phase 1 duplicates the clone/symlink logic that already lives in the composable task library. Consolidating both NocoDB and n8n on clone-and-deploy.yml removes drift risk and keeps the deployment surface idiomatic per the composability guideline.
As per coding guidelines: "Use composable Ansible tasks from the task library (manage-secrets.yml, manage-diode-credentials.yml, manage-approle.yml, deploy-orb-agent.yml, clean-service.yml, clone-and-deploy.yml) as building blocks for deployment playbooks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/deploy-nocodb.yml` around lines 34 - 61, Replace the
inline git clone and symlink tasks with the composable task include
tasks/clone-and-deploy.yml: remove the ansible.builtin.git block and the
ansible.builtin.file (symlink) block and instead add
ansible.builtin.include_tasks: tasks/clone-and-deploy.yml, ensuring you pass
through the same variables used by the inlined tasks (monorepo_repo,
_monorepo_dir, _branch, _deploy_dir, service_name, and any register/failure
semantics expected) so behavior and error handling remain identical; keep the
subsequent include of tasks/manage-secrets.yml unchanged.
| POSTGRES_USER=nocodb | ||
| POSTGRES_PASSWORD={{ secrets.nocodb_pg_password }} | ||
| POSTGRES_DB=nocodb |
There was a problem hiding this comment.
Avoid hardcoded usernames in committed env templates.
POSTGRES_USER=nocodb is a committed username literal. Move it to a template variable sourced from inventory/secrets to keep committed files free of credentials/usernames.
As per coding guidelines, **/* must not commit credentials, IPs, or usernames directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/services/nocodb/deployment/templates/nocodb.env.j2` around lines 3 -
5, Replace the hardcoded POSTGRES_USER value with a template variable so
credentials/usernames aren't committed: change the literal "nocodb" to a Jinja
variable (e.g., {{ secrets.nocodb_pg_user }} or a similarly named inventory
variable) in nocodb.env.j2, update any inventory/secrets source to provide that
key, and adjust deployment/config docs or templates that reference POSTGRES_USER
to use the new variable (search for POSTGRES_USER and nocodb references to
update callers like your deployment/playbook that loads nocodb.env.j2).
| POSTGRES_PASSWORD={{ secrets.nocodb_pg_password }} | ||
| POSTGRES_DB=nocodb | ||
| NC_DB=pg://workflow-nocodb-postgres:5432?u=nocodb&p={{ secrets.nocodb_pg_password }}&d=nocodb | ||
| NC_AUTH_JWT_SECRET={{ secrets.nocodb_jwt_secret }} | ||
| NOCODB_ADMIN_PASSWORD={{ secrets.nocodb_admin_password }} |
There was a problem hiding this comment.
Use the mandated secret variable namespace in service templates.
This template uses {{ secrets.* }} for secret values, but service deployment templates are required to use {{ vault_* }} or {{ _secret_* }} variables. Please align this file (and corresponding render vars) to that contract.
As per coding guidelines, platform/services/*/deployment/templates/*.j2 must use {{ vault_* }} or {{ _secret_* }} for secret values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/services/nocodb/deployment/templates/nocodb.env.j2` around lines 4 -
8, The template uses {{ secrets.nocodb_pg_password }}, {{
secrets.nocodb_jwt_secret }}, and {{ secrets.nocodb_admin_password }} but must
use the mandated secret variable namespace; update the template lines that set
POSTGRES_PASSWORD, NC_DB (the pg URL), NC_AUTH_JWT_SECRET, and
NOCODB_ADMIN_PASSWORD to reference the approved variables (e.g., {{
vault_nocodb_pg_password }} or {{ _secret_nocodb_pg_password }} etc.) and adjust
the corresponding render/vars that supply these values so they match the new
names.
| grep -q '{{ secrets.n8n_admin_password }}' "$TEMPLATE_DIR/n8n.env.j2" | ||
| } | ||
|
|
||
| @test "n8n.env.j2 contains non-root user password placeholder" { | ||
| grep -q '{{ secrets.n8n_user_password }}' "$TEMPLATE_DIR/n8n.env.j2" | ||
| } | ||
|
|
||
| @test "n8n.env.j2 contains N8N_ENCRYPTION_KEY placeholder" { | ||
| grep -q '{{ secrets.n8n_encryption_key }}' "$TEMPLATE_DIR/n8n.env.j2" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
n8n template tests enforce the wrong secret variable convention.
The test suite requires secrets.* placeholders, but service templates are expected to use vault_*/_secret_*. Please update assertions so CI validates the intended convention.
As per coding guidelines, platform/services/*/deployment/templates/*.j2 secret values must use {{ vault_* }} or {{ _secret_* }} variables.
Also applies to: 37-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/tests/test_n8n_templates.bats` around lines 11 - 19, Tests currently
assert presence of '{{ secrets.* }}' placeholders (see grep patterns for '{{
secrets.n8n_admin_password }}', '{{ secrets.n8n_user_password }}', '{{
secrets.n8n_encryption_key }}' in the n8n.env.j2 tests) but our templates must
use the vault_*/_secret_* convention; update each assertion in the "@test"
blocks that grep those secrets to instead validate the allowed conventions
(accept '{{ vault_n8n_* }}' or '{{ _secret_n8n_* }}' placeholders for admin
password, user password and encryption key), and make the same change for the
other related assertions referenced in the comment (the tests around lines
37–43).
| grep -q '{{ secrets.nocodb_pg_password }}' "$TEMPLATE_DIR/nocodb.env.j2" | ||
| } | ||
|
|
||
| @test "nocodb.env.j2 contains NC_AUTH_JWT_SECRET placeholder" { | ||
| grep -q '{{ secrets.nocodb_jwt_secret }}' "$TEMPLATE_DIR/nocodb.env.j2" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Template tests currently enforce a non-compliant placeholder namespace.
These assertions hardcode secrets.*, which conflicts with the required vault_*/_secret_* service-template convention. Update test expectations to the mandated variable family so CI enforces the correct contract.
As per coding guidelines, service env templates must use {{ vault_* }} or {{ _secret_* }} variables for secret values.
Also applies to: 28-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/tests/test_nocodb_templates.bats` around lines 11 - 15, The tests
are asserting the old secrets.* placeholders; update the grep expectations in
the test cases that check "$TEMPLATE_DIR/nocodb.env.j2" (both the PG password
and JWT secret checks and the similar checks at lines ~28-34) to look for the
mandated service-template secret names using the vault/_secret convention (e.g.,
replace checks for '{{ secrets.nocodb_pg_password }}' and '{{
secrets.nocodb_jwt_secret }}' with the allowed pattern such as '{{
vault_nocodb_pg_password }}' or '{{ _secret_nocodb_pg_password }}' and similarly
for the JWT secret) so CI enforces the vault_*/_secret_* variable family.
NocoDB: - Container names: nocodb, postgres (matching running VMs) - Volume names: nocodb_nc_data, nocodb_db_data (reuse existing data) - Port 8080 (matching VM deployment) - NC_DB connection string points to postgres service - Add NC_INVITE_ONLY_SIGNUP and NC_DISABLE_TELE from pre-monorepo config n8n: - Service names: postgres, redis (podman-compose produces n8n_postgres_1) - Volume names: n8n_db_storage, n8n_redis_storage, n8n_n8n_storage (reuse) - ENCRYPTION_KEY in env file, compose maps to N8N_ENCRYPTION_KEY - Add local-files mount for n8n file storage - Dynamic postgres container detection in DB fallback Both VMs use Podman 4.9.3 with podman-compose 1.0.6.
Validates that secret files in site-config have no trailing newlines, carriage returns, non-printable characters, or empty files. These cause JSON parsing failures when passed to OpenBao or Semaphore APIs. Tests skip gracefully when site-config is not available (CI).
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (5)
platform/services/nocodb/deployment/deploy.sh (1)
33-37:⚠️ Potential issue | 🟠 MajorThis still needs to fail fast when
config/nocodb.envorNOCODB_ADMIN_PASSWORDis missing.Returning success here lets the deploy continue without the bootstrap/token phase ever running, which masks a broken Ansible templating phase on first deploy.
As per coding guidelines,
platform/services/*/deployment/deploy.shshould verify required.envfiles exist and fail early if Ansible hasn’t run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/services/nocodb/deployment/deploy.sh` around lines 33 - 37, The script currently returns success when the config or NOCODB_ADMIN_PASSWORD is missing; instead, verify the file exists and fail fast: check for existence of "${CONFIG_DIR}/nocodb.env" before reading it and if missing call an error log and exit 1, then read admin_pass with admin_pass=$(grep ...) and if [ -z "$admin_pass" ]; then call an error logging function (or stderr) and exit 1 — replace the current warn + return 0 path with a non-zero exit so deploy.sh fails early when CONFIG_DIR/nocodb.env or NOCODB_ADMIN_PASSWORD is absent.platform/tests/test_nocodb_templates.bats (1)
10-40:⚠️ Potential issue | 🟠 MajorThese assertions are still locking CI to
secrets.*.The template contract has moved to
vault_*/_secret_*, but this suite still requires{{ secrets.* }}and rejects any compliant replacement. That will block the template fix rather than protect it.As per coding guidelines,
platform/services/*/deployment/templates/*.j2secret values must use{{ vault_* }}or{{ _secret_* }}variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/tests/test_nocodb_templates.bats` around lines 10 - 40, The tests in test_nocodb_templates.bats still require Jinja placeholders to be secrets.* which is outdated; update the placeholder checks (tests like "nocodb.env.j2 contains POSTGRES_PASSWORD placeholder", "nocodb.env.j2 contains NC_AUTH_JWT_SECRET placeholder", and the final "uses only secrets.* Jinja2 placeholders" block) to accept the new template contract by matching either secrets.*, vault_* or _secret_* prefixes; change the grep/regex patterns that look for '\{\{[^}]+\}\}' and specific 'secrets.' occurrences to a single regex allowing '{{ (secrets\.|vault_[^}\s]+|_secret_[^}\s]+)[^}]* }}' (or equivalent) so valid vault_*/_secret_* placeholders pass while still rejecting hardcoded secrets.platform/services/nocodb/deployment/templates/nocodb.env.j2 (1)
3-10:⚠️ Potential issue | 🟠 MajorStill unresolved: this template is using deprecated placeholders and committed usernames.
POSTGRES_USER/ theu=nocodbfragment inNC_DBare still hardcoded, and the secret-backed fields still use{{ secrets.* }}instead of the requiredvault_*/_secret_*names.As per coding guidelines,
platform/services/*/deployment/templates/*.j2secret values must use{{ vault_* }}or{{ _secret_* }}variables, and**/*.j2must not commit usernames or credentials directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/services/nocodb/deployment/templates/nocodb.env.j2` around lines 3 - 10, Replace hardcoded credentials and deprecated {{ secrets.* }} placeholders in this template: remove the committed username "nocodb" and the literal u=nocodb fragment in NC_DB and instead source POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB, NC_DB credentials from vault-backed variables (e.g., use {{ vault_postgres_user }} or {{ _secret_postgres_user }} for POSTGRES_USER and the equivalent for POSTGRES_PASSWORD, POSTGRES_DB, NC_AUTH_JWT_SECRET, NC_INVITE_ONLY_SIGNUP, and NOCODB_ADMIN_PASSWORD), and rebuild NC_DB to interpolate those vault-backed user/password/db values rather than a hardcoded user fragment; ensure no usernames or credentials remain literal in POSTGRES_USER, NC_DB, or NOCODB_ADMIN_PASSWORD.platform/tests/test_n8n_templates.bats (1)
10-38:⚠️ Potential issue | 🟠 MajorThese tests are still enforcing the deprecated
secrets.*placeholder contract.Once the template is corrected, this suite will fail because it both requires
{{ secrets.* }}and rejects any other Jinja placeholder family. CI should be validating the mandatedvault_*/_secret_*convention instead.As per coding guidelines,
platform/services/*/deployment/templates/*.j2secret values must use{{ vault_* }}or{{ _secret_* }}variables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/tests/test_n8n_templates.bats` around lines 10 - 38, The tests are enforcing the deprecated {{ secrets.* }} placeholders; update them to require the mandated vault/_secret convention instead: change assertions that look for '{{ secrets.n8n_admin_password }}', '{{ secrets.n8n_user_password }}', '{{ secrets.n8n_encryption_key }}', and '{{ secrets.n8n_owner_password }}' to accept either '{{ vault_n8n_admin_password }}' or '{{ _secret_n8n_admin_password }}' (and likewise for user_password, n8n_encryption_key, n8n_owner_password) by updating the grep patterns in the corresponding test blocks, and replace the "n8n.env.j2 uses only secrets.* Jinja2 placeholders" test (and its bad_placeholders extraction) to allow only placeholders matching 'vault_' or '_secret_' while rejecting 'secrets.'; keep the hardcoded-secrets negative regex and other checks intact.platform/services/n8n/deployment/templates/n8n.env.j2 (1)
3-9:⚠️ Potential issue | 🟠 MajorStill unresolved: this template does not follow the repo’s variable contract.
POSTGRES_USER/POSTGRES_NON_ROOT_USERare still committed literals, and the secret fields still use{{ secrets.* }}instead of the requiredvault_*/_secret_*namespace. That keeps the deployment on the deprecated template interface.As per coding guidelines,
platform/services/*/deployment/templates/*.j2secret values must use{{ vault_* }}or{{ _secret_* }}variables, and**/*.j2must not commit usernames or credentials directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform/services/n8n/deployment/templates/n8n.env.j2` around lines 3 - 9, The template commits hard-coded usernames and uses the wrong secret namespace; replace literal POSTGRES_USER and POSTGRES_NON_ROOT_USER values with variable references and switch all secret placeholders to the repo contract (use vault_* or _secret_*), e.g. update POSTGRES_USER, POSTGRES_NON_ROOT_USER, POSTGRES_PASSWORD, POSTGRES_NON_ROOT_PASSWORD, ENCRYPTION_KEY and N8N_OWNER_PASSWORD to reference the appropriate vault/_secret variables instead of literal names or {{ secrets.* }} so the file conforms to platform/services/*/deployment/templates/*.j2 conventions and avoids committing credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/playbooks/check-vm-secrets.yml`:
- Around line 48-53: The "Check running containers" task defaults
container_engine to 'docker', which can misreport on Podman hosts; change the
logic in that task to default to 'podman' instead of 'docker' or implement the
same runtime detection used by the deploy scripts (use the same detection helper
or playbook variable) so the shell command uses the correct engine
(container_engine) when checking {{ service_name }} and registering _containers;
ensure changed_when remains false.
- Around line 30-37: Remove the task named "List secrets directory" that uses
ansible.builtin.find to inspect "{{ _deploy_dir }}/secrets" and the registered
variable _secret_files; this check must be deleted so the playbook no longer
looks for an on-VM secrets/ directory. Also remove or update any downstream
references/conditions that use the _secret_files variable (or its when
conditions) so they instead rely on the generated config/*.env flow (Jinja2
templating of env files) and do not create or expect intermediary secrets files
on the VM.
In `@platform/playbooks/deploy-nocodb.yml`:
- Around line 147-155: The "Health check" task using ansible.builtin.uri
currently skips when service_url is undefined; remove the "when: service_url is
defined" clause so the task always runs (the task named "Health check" that hits
"{{ service_url | default('http://localhost:8080') }}/api/v1/health"); apply the
same change to the equivalent "Health check" tasks in deploy-netbox.yml,
deploy-n8n.yml and sync-secrets-to-openbao.yml so Phase 4 (verify-health)
executes against the default URL when service_url is not provided.
- Around line 104-133: The "Store API token in OpenBao" task currently does a
POST via ansible.builtin.uri which overwrites the KV v2 secret (losing keys
written earlier by Phase 1); replace this task with the
community.hashi_vault.vault_kv2_write module (or implement an explicit
read-merge-write using community.hashi_vault.vault_kv2_read followed by
vault_kv2_write) to merge the new nocodb_api_token and url into the existing
secret at services/nocodb, or alternatively perform a PATCH with Content-Type:
application/merge-patch+json if you must use HTTP; also change this and related
secret operations (e.g., the "Authenticate to OpenBao" flow that provides the
token) to use community.hashi_vault modules per coding guidelines instead of
ansible.builtin.uri so existing keys (pg_password, jwt_secret, admin_password)
are preserved.
In `@platform/services/n8n/deployment/compose.yml`:
- Around line 42-59: Update the n8n and n8n-worker service blocks: replace
restart: always with restart: unless-stopped, change image:
docker.n8n.io/n8nio/n8n:latest to the pinned form using the VERSION variable
(e.g., image: docker.n8n.io/n8nio/n8n:${VERSION:-tag}), and add a healthcheck
block for each service that probes the service readiness (for example an HTTP
check against the n8n port/health endpoint or a TCP check) with sensible
interval/retries/start-period settings so Compose can distinguish started vs
ready; apply these exact changes to the n8n and n8n-worker service definitions
referenced in the compose file.
- Around line 42-50: Remove the service-level YAML merge for n8n (the "<<:
*n8n-env" entry) so only the explicit environment: block defines variables;
change restart: always to restart: unless-stopped for both n8n and n8n-worker
services; replace image: docker.n8n.io/n8nio/n8n:latest with a pinned tag using
a variable like docker.n8n.io/n8nio/n8n:${VERSION:-1.0.0} for both the n8n and
n8n-worker service definitions; and add healthcheck blocks to both n8n and
n8n-worker services (matching the existing postgres/redis healthcheck pattern)
immediately after their environment sections to ensure health monitoring.
In `@platform/services/n8n/deployment/deploy.sh`:
- Around line 33-37: The script currently silently skips bootstrap when
N8N_OWNER_PASSWORD is missing; change it to fail fast by first verifying the
config file exists and then aborting with an error if the owner password is
absent: add a check for the file (test -f "${CONFIG_DIR}/n8n.env") and if it is
missing call the error handler (or echo to stderr) and exit non‑zero, and
replace the warn + return 0 branch that handles an empty owner_pass with an
error log and non‑zero exit (reference owner_pass, CONFIG_DIR, n8n.env,
warn/return 0 in deploy.sh).
- Around line 99-110: The fallback DB insert uses a fragile container picker and
a hardcoded owner id; update the logic so pg_container is resolved by compose
project/service (e.g. prefer containers whose name includes "-postgres-" or
filter ps by the compose label "com.docker.compose.project" / service name
instead of grep -E 'postgres' | head -1), and replace the literal user_id '1' in
the INSERT SQL with a subquery that looks up the real owner id by role (e.g.
SELECT id FROM "user" WHERE role = 'global:owner' OR WHERE settings->>'role' =
'global:owner'), validate that a single owner id was found and abort the
fallback if not, and keep the existing guard that suppresses the fallback or
remove the fallback entirely if you prefer to force manual API-key creation;
update references to pg_container, CONTAINER_ENGINE, and insert_result
accordingly and ensure a clear warn/error is emitted when owner lookup or
container resolution fails.
In `@platform/services/nocodb/deployment/compose.yml`:
- Around line 25-26: The compose file is publishing Postgres to the host via the
ports mapping ("5432:5432") which is unnecessary for inter-service
communication; remove the "5432:5432" entry from the postgres service's ports
block (the ports key under the postgres service) so nocodb communicates over the
compose network, or if host-only access is truly needed change it to bind to
localhost ("127.0.0.1:5432:5432") instead; update the postgres service's ports
setting and keep nocodb unchanged.
In `@platform/services/nocodb/deployment/templates/nocodb.env.j2`:
- Line 6: The NC_DB environment line embeds the raw secret
(secrets.nocodb_pg_password) into a URI and can break if the password contains
special characters; update the template in nocodb.env.j2 to URL-encode the
password before interpolation (e.g. use the Jinja2 urlencode filter: replace {{
secrets.nocodb_pg_password }} with {{ secrets.nocodb_pg_password | urlencode
}}), or alternatively switch NC_DB to a connection form that supplies
user/password separately rather than embedding them in the URI.
---
Duplicate comments:
In `@platform/services/n8n/deployment/templates/n8n.env.j2`:
- Around line 3-9: The template commits hard-coded usernames and uses the wrong
secret namespace; replace literal POSTGRES_USER and POSTGRES_NON_ROOT_USER
values with variable references and switch all secret placeholders to the repo
contract (use vault_* or _secret_*), e.g. update POSTGRES_USER,
POSTGRES_NON_ROOT_USER, POSTGRES_PASSWORD, POSTGRES_NON_ROOT_PASSWORD,
ENCRYPTION_KEY and N8N_OWNER_PASSWORD to reference the appropriate vault/_secret
variables instead of literal names or {{ secrets.* }} so the file conforms to
platform/services/*/deployment/templates/*.j2 conventions and avoids committing
credentials.
In `@platform/services/nocodb/deployment/deploy.sh`:
- Around line 33-37: The script currently returns success when the config or
NOCODB_ADMIN_PASSWORD is missing; instead, verify the file exists and fail fast:
check for existence of "${CONFIG_DIR}/nocodb.env" before reading it and if
missing call an error log and exit 1, then read admin_pass with
admin_pass=$(grep ...) and if [ -z "$admin_pass" ]; then call an error logging
function (or stderr) and exit 1 — replace the current warn + return 0 path with
a non-zero exit so deploy.sh fails early when CONFIG_DIR/nocodb.env or
NOCODB_ADMIN_PASSWORD is absent.
In `@platform/services/nocodb/deployment/templates/nocodb.env.j2`:
- Around line 3-10: Replace hardcoded credentials and deprecated {{ secrets.* }}
placeholders in this template: remove the committed username "nocodb" and the
literal u=nocodb fragment in NC_DB and instead source POSTGRES_USER,
POSTGRES_PASSWORD, POSTGRES_DB, NC_DB credentials from vault-backed variables
(e.g., use {{ vault_postgres_user }} or {{ _secret_postgres_user }} for
POSTGRES_USER and the equivalent for POSTGRES_PASSWORD, POSTGRES_DB,
NC_AUTH_JWT_SECRET, NC_INVITE_ONLY_SIGNUP, and NOCODB_ADMIN_PASSWORD), and
rebuild NC_DB to interpolate those vault-backed user/password/db values rather
than a hardcoded user fragment; ensure no usernames or credentials remain
literal in POSTGRES_USER, NC_DB, or NOCODB_ADMIN_PASSWORD.
In `@platform/tests/test_n8n_templates.bats`:
- Around line 10-38: The tests are enforcing the deprecated {{ secrets.* }}
placeholders; update them to require the mandated vault/_secret convention
instead: change assertions that look for '{{ secrets.n8n_admin_password }}', '{{
secrets.n8n_user_password }}', '{{ secrets.n8n_encryption_key }}', and '{{
secrets.n8n_owner_password }}' to accept either '{{ vault_n8n_admin_password }}'
or '{{ _secret_n8n_admin_password }}' (and likewise for user_password,
n8n_encryption_key, n8n_owner_password) by updating the grep patterns in the
corresponding test blocks, and replace the "n8n.env.j2 uses only secrets.*
Jinja2 placeholders" test (and its bad_placeholders extraction) to allow only
placeholders matching 'vault_' or '_secret_' while rejecting 'secrets.'; keep
the hardcoded-secrets negative regex and other checks intact.
In `@platform/tests/test_nocodb_templates.bats`:
- Around line 10-40: The tests in test_nocodb_templates.bats still require Jinja
placeholders to be secrets.* which is outdated; update the placeholder checks
(tests like "nocodb.env.j2 contains POSTGRES_PASSWORD placeholder",
"nocodb.env.j2 contains NC_AUTH_JWT_SECRET placeholder", and the final "uses
only secrets.* Jinja2 placeholders" block) to accept the new template contract
by matching either secrets.*, vault_* or _secret_* prefixes; change the
grep/regex patterns that look for '\{\{[^}]+\}\}' and specific 'secrets.'
occurrences to a single regex allowing '{{
(secrets\.|vault_[^}\s]+|_secret_[^}\s]+)[^}]* }}' (or equivalent) so valid
vault_*/_secret_* placeholders pass while still rejecting hardcoded secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48c0fe1f-c52a-463a-8db0-4407fdec631a
📒 Files selected for processing (11)
platform/playbooks/check-vm-secrets.ymlplatform/playbooks/deploy-nocodb.ymlplatform/services/n8n/deployment/compose.ymlplatform/services/n8n/deployment/deploy.shplatform/services/n8n/deployment/local-files/.gitkeepplatform/services/n8n/deployment/templates/n8n.env.j2platform/services/nocodb/deployment/compose.ymlplatform/services/nocodb/deployment/deploy.shplatform/services/nocodb/deployment/templates/nocodb.env.j2platform/tests/test_n8n_templates.batsplatform/tests/test_nocodb_templates.bats
| - name: "List secrets directory" | ||
| ansible.builtin.find: | ||
| paths: "{{ _deploy_dir }}/secrets" | ||
| patterns: "*" | ||
| file_type: file | ||
| register: _secret_files | ||
| when: _deploy_stat.stat.exists | default(false) | ||
| ignore_errors: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Stop checking for a secrets/ directory in the composable path.
This playbook is normalizing an artifact the new flow explicitly avoids. For NocoDB/n8n, runtime secrets are supposed to exist only as generated config/*.env, not as on-VM secrets/ files.
Based on learnings, “Secrets flow: OpenBao → Ansible memory → Jinja2 templates → .env files (compose-readable, gitignored). Never create intermediary secret files or secrets/ directory on VMs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/check-vm-secrets.yml` around lines 30 - 37, Remove the
task named "List secrets directory" that uses ansible.builtin.find to inspect
"{{ _deploy_dir }}/secrets" and the registered variable _secret_files; this
check must be deleted so the playbook no longer looks for an on-VM secrets/
directory. Also remove or update any downstream references/conditions that use
the _secret_files variable (or its when conditions) so they instead rely on the
generated config/*.env flow (Jinja2 templating of env files) and do not create
or expect intermediary secrets files on the VM.
| - name: "Check running containers" | ||
| ansible.builtin.shell: | | ||
| set -o pipefail | ||
| {{ container_engine | default('docker') }} ps --format '{{ '{{' }}.Names{{ '}}' }} {{ '{{' }}.Status{{ '}}' }}' 2>/dev/null | grep -i '{{ service_name }}' || echo "No {{ service_name }} containers" | ||
| register: _containers | ||
| changed_when: false |
There was a problem hiding this comment.
Defaulting container inspection to Docker will misreport these hosts.
If container_engine is unset, this task will happily print “No … containers” on a Podman VM even when the service is running. Default to Podman here, or detect the runtime the same way the deploy scripts do.
Based on learnings, “Use Podman for all other services (rootless, security-focused).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/check-vm-secrets.yml` around lines 48 - 53, The "Check
running containers" task defaults container_engine to 'docker', which can
misreport on Podman hosts; change the logic in that task to default to 'podman'
instead of 'docker' or implement the same runtime detection used by the deploy
scripts (use the same detection helper or playbook variable) so the shell
command uses the correct engine (container_engine) when checking {{ service_name
}} and registering _containers; ensure changed_when remains false.
| - name: "Authenticate to OpenBao" | ||
| ansible.builtin.uri: | ||
| url: "{{ _bao_url }}/v1/auth/approle/login" | ||
| method: POST | ||
| body_format: json | ||
| body: | ||
| role_id: "{{ _bao_role_id }}" | ||
| secret_id: "{{ _bao_secret_id }}" | ||
| status_code: [200] | ||
| register: _bao_auth | ||
| delegate_to: localhost | ||
| run_once: true | ||
| no_log: true | ||
| when: _nocodb_api_token | default('') | length > 0 | ||
|
|
||
| - name: "Store API token in OpenBao" | ||
| ansible.builtin.uri: | ||
| url: "{{ _bao_url }}/v1/secret/data/services/nocodb" | ||
| method: POST | ||
| headers: | ||
| X-Vault-Token: "{{ _bao_auth.json.auth.client_token }}" | ||
| body_format: json | ||
| body: | ||
| data: | ||
| nocodb_api_token: "{{ _nocodb_api_token }}" | ||
| url: "{{ service_url | default('http://localhost:8080') }}" | ||
| status_code: [200] | ||
| delegate_to: localhost | ||
| run_once: true | ||
| no_log: true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check file exists and get line count
wc -l platform/playbooks/deploy-nocodb.ymlRepository: uhstray-io/agent-cloud
Length of output: 107
🏁 Script executed:
# Read lines 90-140 to see context around lines 104-133
sed -n '90,140p' platform/playbooks/deploy-nocodb.ymlRepository: uhstray-io/agent-cloud
Length of output: 1830
🏁 Script executed:
# Search for Phase 1 and initial secret setup in the file
grep -n "Phase" platform/playbooks/deploy-nocodb.ymlRepository: uhstray-io/agent-cloud
Length of output: 876
🏁 Script executed:
# Check if community.hashi_vault is used elsewhere in the playbook
grep -n "community.hashi_vault" platform/playbooks/deploy-nocodb.ymlRepository: uhstray-io/agent-cloud
Length of output: 48
🏁 Script executed:
# Look at the full playbook to understand structure
head -150 platform/playbooks/deploy-nocodb.ymlRepository: uhstray-io/agent-cloud
Length of output: 5357
🏁 Script executed:
# Find manage-secrets.yml
find . -name "manage-secrets.yml" -type fRepository: uhstray-io/agent-cloud
Length of output: 112
🏁 Script executed:
# Check if manage-secrets.yml exists and read it
cat -n platform/playbooks/tasks/manage-secrets.ymlRepository: uhstray-io/agent-cloud
Length of output: 4316
🏁 Script executed:
# Also check the directory structure of tasks
ls -la platform/playbooks/tasks/Repository: uhstray-io/agent-cloud
Length of output: 824
🌐 Web query:
OpenBao KV v2 secret engine POST partial data overwrites entire secret version
💡 Result:
Yes, in OpenBao's KV v2 secrets engine, sending a POST request to the /data/:path endpoint with partial data will overwrite the entire secret version, replacing all existing data with only the provided keys [1][2][3]. The POST method performs a full replacement of the 'data' map, discarding any unspecified fields [3]. To perform partial updates without overwriting the entire secret: - Use HTTP PATCH on the same /:mount/data/:path endpoint with Content-Type: application/merge-patch+json. This applies a JSON merge patch to the existing data, preserving unspecified fields, and requires the 'patch' ACL capability [3][4]. - Example curl: curl --header "X-Vault-Token:..." --header "Content-Type: application/merge-patch+json" --request PATCH --data '{"newkey": "value"}' https://127.0.0.1:8200/v1/secret/data/my-secret [3]. - Alternatively, use the CLI 'bao kv patch' command, which prefers PATCH but falls back to read-then-write (requiring 'read' and 'update' capabilities) if 'patch' is unavailable [1][5]. This behavior matches HashiCorp Vault KV v2, from which OpenBao is forked [6][7].
Citations:
- 1: https://openbao.org/docs/secrets/kv/kv-v2/
- 2: https://openbao.org/api-docs/secret/kv/kv-v2/
- 3: https://github.com/openbao/openbao/blob/main/website/content/api-docs/secret/kv/kv-v2.mdx
- 4: https://openbao.org/docs/secrets/kv/kv-v2
- 5: https://openbao.org/docs/2.4.x/commands/kv/patch/
- 6: https://developer.hashicorp.com/vault/docs/commands/kv/patch
- 7: https://developer.hashicorp.com/vault/docs/v1.16.x/secrets/kv/kv-v2
🌐 Web query:
community.hashi_vault Ansible collection KV v2 PUT write support
💡 Result:
community.hashi_vault DOES support KV v2 PUT/write via the module community.hashi_vault.vault_kv2_write. It is a “Perform a write operation against a KVv2 secret in HashiCorp Vault” module, and its parameters include path (KVv2 path to write to) and data (KVv2 secret data to write) [1][2]. Example usage (write/create a secret): community.hashi_vault.vault_kv2_write: url: https://vault:8200 path: hello data: foo: bar [1] Notes: - The module supports CAS via a cas parameter, including “cas: 0” to require that the secret must not exist, and CAS values to update an existing secret version [2]. - If you instead use the more generic module community.hashi_vault.vault_write, it may not automatically handle KV v2 path/data formats; the community discussion states kv1 and kv2 API paths differ and you must include the correct /data/ formatting for kv2 when using vault_write (because it’s not kv-specific) [3]. Conclusion: For KV v2 PUT/write support in the community.hashi_vault Ansible collection, use community.hashi_vault.vault_kv2_write (not vault_write) [1][3].
Citations:
- 1: https://docs.ansible.com/ansible/latest/collections/community/hashi_vault/vault_kv2_write_module.html
- 2: https://ansible-collections.github.io/community.hashi_vault/branch/main/collections/community/hashi_vault/vault_kv2_write_module.html
- 3: `vault_write` module not working with kv v2 ansible-collections/community.hashi_vault#241
🏁 Script executed:
# Check if there are any examples of community.hashi_vault usage in the repo
rg "community\.hashi_vault" --type yml --type yamlRepository: uhstray-io/agent-cloud
Length of output: 94
🏁 Script executed:
# Search for community.hashi_vault usage without file type filter
rg "community\.hashi_vault"Repository: uhstray-io/agent-cloud
Length of output: 5042
Use community.hashi_vault.vault_kv2_write in Phase 3 to preserve full secret payload.
Lines 119-129 POST only nocodb_api_token and url to /v1/secret/data/services/nocodb, which replaces the entire KV v2 secret version. Phase 1 writes pg_password, jwt_secret, and admin_password to the same path; these are lost on the Phase 3 POST.
Replace the "Store API token in OpenBao" task with community.hashi_vault.vault_kv2_write using a read-merge-write pattern, or use PATCH with Content-Type: application/merge-patch+json to preserve existing keys. Additionally, both manage-secrets.yml and Phase 3 should use community.hashi_vault modules instead of ansible.builtin.uri per the coding guidelines.
🧰 Tools
🪛 Checkov (3.2.526)
[medium] 104-120: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🪛 GitHub Check: Static Analysis
[failure] 119-119: run-once[task]
Using run_once may behave differently if strategy is set to free.
[failure] 104-104: run-once[task]
Using run_once may behave differently if strategy is set to free.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/deploy-nocodb.yml` around lines 104 - 133, The "Store API
token in OpenBao" task currently does a POST via ansible.builtin.uri which
overwrites the KV v2 secret (losing keys written earlier by Phase 1); replace
this task with the community.hashi_vault.vault_kv2_write module (or implement an
explicit read-merge-write using community.hashi_vault.vault_kv2_read followed by
vault_kv2_write) to merge the new nocodb_api_token and url into the existing
secret at services/nocodb, or alternatively perform a PATCH with Content-Type:
application/merge-patch+json if you must use HTTP; also change this and related
secret operations (e.g., the "Authenticate to OpenBao" flow that provides the
token) to use community.hashi_vault modules per coding guidelines instead of
ansible.builtin.uri so existing keys (pg_password, jwt_secret, admin_password)
are preserved.
| - name: "Health check" | ||
| ansible.builtin.uri: | ||
| url: "{{ service_url | default('http://localhost:8080') }}/api/v1/health" | ||
| status_code: [200] | ||
| validate_certs: false | ||
| timeout: 30 | ||
| retries: 5 | ||
| delay: 10 | ||
| when: service_url is defined |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
cat -n platform/playbooks/deploy-nocodb.yml | sed -n '140,160p'Repository: uhstray-io/agent-cloud
Length of output: 796
🏁 Script executed:
# Also check the overall structure of the playbook to understand the 4-phase pattern
head -100 platform/playbooks/deploy-nocodb.yml | tail -50Repository: uhstray-io/agent-cloud
Length of output: 1795
🏁 Script executed:
# Search for similar health check patterns in other deploy playbooks
fd -e yml platform/playbooks/deploy-*.yml -x grep -l "Health check"Repository: uhstray-io/agent-cloud
Length of output: 778
🏁 Script executed:
# Check other deploy playbooks for similar conditional health check patterns
rg "when: service_url is defined" platform/playbooks/Repository: uhstray-io/agent-cloud
Length of output: 361
🏁 Script executed:
# Check if other deploy playbooks have health checks and how they handle defaults
rg -A 3 "Health check" platform/playbooks/deploy-*.ymlRepository: uhstray-io/agent-cloud
Length of output: 958
Remove the when: service_url is defined condition to enforce mandatory Phase 4 verification.
This playbook has a default URL fallback on line 149, but line 155 skips the entire health check when service_url is unset. The 4-phase composable pattern requires Phase 4 (verify-health) to always execute. Removing this condition ensures the readiness check runs against the default URL, preventing false-positive deployment success.
Suggested fix
- name: "Health check"
ansible.builtin.uri:
url: "{{ service_url | default('http://localhost:8080') }}/api/v1/health"
status_code: [200]
validate_certs: false
timeout: 30
retries: 5
delay: 10
- when: service_url is definedThis same pattern occurs in deploy-netbox.yml, deploy-n8n.yml, and sync-secrets-to-openbao.yml — address all instances.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: "Health check" | |
| ansible.builtin.uri: | |
| url: "{{ service_url | default('http://localhost:8080') }}/api/v1/health" | |
| status_code: [200] | |
| validate_certs: false | |
| timeout: 30 | |
| retries: 5 | |
| delay: 10 | |
| when: service_url is defined | |
| - name: "Health check" | |
| ansible.builtin.uri: | |
| url: "{{ service_url | default('http://localhost:8080') }}/api/v1/health" | |
| status_code: [200] | |
| validate_certs: false | |
| timeout: 30 | |
| retries: 5 | |
| delay: 10 |
🧰 Tools
🪛 Checkov (3.2.526)
[medium] 147-158: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[medium] 147-158: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/playbooks/deploy-nocodb.yml` around lines 147 - 155, The "Health
check" task using ansible.builtin.uri currently skips when service_url is
undefined; remove the "when: service_url is defined" clause so the task always
runs (the task named "Health check" that hits "{{ service_url |
default('http://localhost:8080') }}/api/v1/health"); apply the same change to
the equivalent "Health check" tasks in deploy-netbox.yml, deploy-n8n.yml and
sync-secrets-to-openbao.yml so Phase 4 (verify-health) executes against the
default URL when service_url is not provided.
| n8n: | ||
| container_name: workflow-n8n | ||
| <<: *n8n-env | ||
| image: docker.n8n.io/n8nio/n8n:latest | ||
| restart: always | ||
| ports: | ||
| - "${N8N_LISTEN:-127.0.0.1}:5678:5678" | ||
| - "5678:5678" | ||
| env_file: ./config/n8n.env | ||
| environment: | ||
| <<: *n8n-env | ||
| volumes: | ||
| - n8n_data:/home/node/.n8n | ||
| - n8n_storage:/home/node/.n8n | ||
| - ./local-files:/files | ||
| depends_on: | ||
| n8n-redis: | ||
| redis: | ||
| condition: service_healthy | ||
| n8n-postgres: | ||
| postgres: | ||
| condition: service_healthy | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat platform/services/n8n/deployment/compose.ymlRepository: uhstray-io/agent-cloud
Length of output: 2045
Add healthchecks to n8n and n8n-worker services; pin image versions and correct restart policies.
The n8n and n8n-worker services lack healthcheck blocks, preventing Compose from distinguishing started vs. ready states. Additionally, images use :latest instead of the required ${VERSION:-tag} syntax, and restart policies are always instead of unless-stopped.
Per coding guidelines:
- "Services must define healthcheck blocks"
- "Compose files should pin image versions using
${VERSION:-tag}syntax" - "Restart policy should be 'unless-stopped'"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/services/n8n/deployment/compose.yml` around lines 42 - 59, Update
the n8n and n8n-worker service blocks: replace restart: always with restart:
unless-stopped, change image: docker.n8n.io/n8nio/n8n:latest to the pinned form
using the VERSION variable (e.g., image:
docker.n8n.io/n8nio/n8n:${VERSION:-tag}), and add a healthcheck block for each
service that probes the service readiness (for example an HTTP check against the
n8n port/health endpoint or a TCP check) with sensible
interval/retries/start-period settings so Compose can distinguish started vs
ready; apply these exact changes to the n8n and n8n-worker service definitions
referenced in the compose file.
| local owner_pass | ||
| owner_pass=$(grep '^N8N_OWNER_PASSWORD=' "${CONFIG_DIR}/n8n.env" 2>/dev/null | cut -d= -f2-) | ||
| if [ -z "$owner_pass" ]; then | ||
| warn " No N8N_OWNER_PASSWORD found in config/n8n.env — skipping bootstrap." | ||
| return 0 |
There was a problem hiding this comment.
Missing config/n8n.env should abort this phase, not silently skip it.
A first deploy can report success here without ever creating the owner or API key if Ansible did not render N8N_OWNER_PASSWORD. That breaks the expected secrets → bootstrap flow.
As per coding guidelines, platform/services/*/deployment/deploy.sh should verify required .env files exist and fail early if Ansible hasn’t run.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/services/n8n/deployment/deploy.sh` around lines 33 - 37, The script
currently silently skips bootstrap when N8N_OWNER_PASSWORD is missing; change it
to fail fast by first verifying the config file exists and then aborting with an
error if the owner password is absent: add a check for the file (test -f
"${CONFIG_DIR}/n8n.env") and if it is missing call the error handler (or echo to
stderr) and exit non‑zero, and replace the warn + return 0 branch that handles
an empty owner_pass with an error log and non‑zero exit (reference owner_pass,
CONFIG_DIR, n8n.env, warn/return 0 in deploy.sh).
| local pg_container | ||
| pg_container=$($CONTAINER_ENGINE ps --format '{{.Names}}' 2>/dev/null | grep -E 'postgres' | head -1) | ||
| if [ -z "$pg_container" ]; then | ||
| warn " No postgres container found — aborting DB insert." | ||
| return 0 | ||
| fi | ||
| insert_result=$($CONTAINER_ENGINE exec "$pg_container" \ | ||
| psql -U n8n_user -d n8n -t -A -c \ | ||
| "INSERT INTO api_key (user_id, label, api_key, created_at, updated_at) | ||
| SELECT '1', 'nemoclaw-agent', '${api_key}', NOW(), NOW() | ||
| WHERE NOT EXISTS (SELECT 1 FROM api_key WHERE label = 'nemoclaw-agent') | ||
| RETURNING api_key;" 2>/dev/null) || insert_result="" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the n8n version used by this deployment, is direct SQL insertion into the api_keytable with columns(user_id, label, api_key, created_at, updated_at) supported, and is the owner account guaranteed to have id 1?
💡 Result:
Direct SQL insertion into a table named api_key with columns (user_id, label, api_key, created_at, updated_at) is not something the n8n docs or codebase generally guarantee. 1) Evidence about the database schema not matching your proposed table/column names - n8n’s database structure is managed by n8n migrations/TypeORM (not a stable public SQL contract) [1]. - n8n uses an ApiKey entity/table managed in code, and there is ongoing refactoring of API key persistence (“Move API keys into their own table”) [2]. This indicates the physical table/column layout (including exact column names like api_key, created_at, updated_at) can change across versions. Given that neither the n8n “database structure” documentation nor the referenced repo changes confirm an api_key table with exactly those column names, you should assume direct insertion with that exact column set is unsupported/fragile for the deployment’s specific n8n version. 2) Is the owner account guaranteed to have id 1? - n8n identifies the instance owner by role (global:owner), and will create a new default owner if no owner exists; the docs do not state that the owner’s database id is always 1 [3]. - Therefore, ownership by id = 1 is not guaranteed. Practical guidance (based on the cited sources) - Don’t hard-code id = 1 for “owner”; instead, query for the user with the global:owner role [3]. - Don’t rely on direct INSERT into a guessed api_key table/columns; use n8n’s API (“Settings > n8n API” / API key endpoints) or adapt to the exact table/columns created by the running n8n version’s migrations [1][4].
Citations:
- 1: https://docs.n8n.io/hosting/architecture/database-structure/
- 2: refactor: Move API keys into their own table (no-changelog) n8n-io/n8n#10629
- 3: https://n8n-io-n8n.mintlify.app/cli/user-management
- 4: https://docs.n8n.io/api/authentication/
🏁 Script executed:
# Check the file size and structure
wc -l platform/services/n8n/deployment/deploy.sh
# Look at the beginning of the file to understand its structure and n8n version
head -50 platform/services/n8n/deployment/deploy.sh
# Check for n8n version specification
grep -i "n8n" platform/services/n8n/deployment/deploy.sh | head -20Repository: uhstray-io/agent-cloud
Length of output: 2801
🏁 Script executed:
# Look at the problematic section in context (lines 80-120)
sed -n '80,120p' platform/services/n8n/deployment/deploy.shRepository: uhstray-io/agent-cloud
Length of output: 1492
🏁 Script executed:
# Check if there's a way to get the n8n version from the deployment
find platform/services/n8n -type f \( -name "*.yml" -o -name "*.yaml" -o -name "*.env*" \) | head -20Repository: uhstray-io/agent-cloud
Length of output: 165
🏁 Script executed:
# Check the compose file structure
head -80 platform/services/n8n/deployment/compose.ymlRepository: uhstray-io/agent-cloud
Length of output: 2045
🏁 Script executed:
# Check the n8n.env template to understand version and configuration
cat platform/services/n8n/deployment/templates/n8n.env.j2Repository: uhstray-io/agent-cloud
Length of output: 468
Scoped Postgres container selection and role-based owner lookup required for DB fallback.
The fallback DB insertion has two critical issues:
-
Container selection —
grep -E 'postgres' | head -1can match the wrong container on multi-service hosts. Use the service name from compose (postgres→ container name<project>-postgres-1) or filter by compose project. -
Hardcoded owner ID —
user_id = '1'is unsafe. N8n owner is identified by role (global:owner), not by a fixed ID. On restored or migrated data, this will fail or create the key under the wrong user. Query the user with roleglobal:ownerinstead.
Additionally, the web search results confirm that direct SQL insertion into the api_key table is unsupported across n8n versions — the table structure is managed by n8n's TypeORM migrations and can change. While the REST API approach (primary path) is correct, the fallback should either:
- Query the correct owner by role and inspect the actual
api_keytable schema at runtime, or - Remove the fallback entirely and require manual API key creation if the endpoint is unavailable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/services/n8n/deployment/deploy.sh` around lines 99 - 110, The
fallback DB insert uses a fragile container picker and a hardcoded owner id;
update the logic so pg_container is resolved by compose project/service (e.g.
prefer containers whose name includes "-postgres-" or filter ps by the compose
label "com.docker.compose.project" / service name instead of grep -E 'postgres'
| head -1), and replace the literal user_id '1' in the INSERT SQL with a
subquery that looks up the real owner id by role (e.g. SELECT id FROM "user"
WHERE role = 'global:owner' OR WHERE settings->>'role' = 'global:owner'),
validate that a single owner id was found and abort the fallback if not, and
keep the existing guard that suppresses the fallback or remove the fallback
entirely if you prefer to force manual API-key creation; update references to
pg_container, CONTAINER_ENGINE, and insert_result accordingly and ensure a clear
warn/error is emitted when owner lookup or container resolution fails.
Special characters (!@#$%^&* etc.) are valid for password complexity. Only reject null bytes, control characters (0x01-0x1F except newline), and DEL — these break shell parsing and JSON encoding.
Security: - Add no_log to deploy.sh execution tasks (prevents API token/key leak) - Filter API credentials from Ansible stdout output - URL-encode password in NocoDB NC_DB connection string - Remove exposed Postgres port from NocoDB compose (internal-only) Correctness: - Add until condition to health check retries (required by Ansible) - Fail fast if config/*.env missing instead of silently skipping - Set compose project name (-p nocodb/-p n8n) for correct volume naming - Remove invalid YAML anchor merge at n8n service level - Create config/ directory before templating env files
|
Saw your post about feat(platform): NocoDB and n8n composable deployme. I deploy custom AI assistants — local models, persistent memory, no subscriptions. Your data stays yours. Price: $600, 48h. Free 15-min scope call first if you want. — Lyn Reply or email: lynchatta@gmail.com |
2 participants
Summary
Breaking Change Mitigation
nocodb_pg_password,n8n_admin_password) matching existingsecrets/files on VMsNOCODB_ADMIN_PASSWORDandN8N_OWNER_PASSWORDseparated from database credentialssync-secrets-to-openbao.ymlfor both services before first composable deployCredential Flow (new)
deploy.sh no longer reads from
secrets/or talks to OpenBao directly.Test plan
sync-secrets-to-openbao.ymlfornocodb_svcandn8n_svcservice_branch: feat/nocodb-n8n-composable)service_branch: feat/nocodb-n8n-composable)Automation Layer
Playbooks & Tasks:
deploy-nocodb.ymlanddeploy-n8n.ymlas self-contained 4-phase composable playbooks (replacing thin wrapper imports) with phases for: repo cloning/secrets initialization, container lifecycle bootstrap, OpenBao credential persistence via AppRole login, and health verificationclean-deploy-nocodb.ymlandclean-deploy-n8n.ymlplaybooks that run cleanup tasks before redeployingcheck-vm-secrets.ymlplaybook for pre-deploy validation of monorepo clones and filesystem state on target VMsDeploy Scripts:
deploy.shfor both services from secrets-generation/OpenBao-persistence workflows to container-lifecycle-only: scripts now read runtime values from generatedconfig/*.envvia grep and no longer invoke OpenBao or generate/store secrets locallydeploy.sh: Bootstraps via admin password fromconfig/nocodb.env, creates API token via REST endpoint, outputsNOCODB_API_TOKEN=...deploy.sh: Bootstraps viaN8N_OWNER_PASSWORDfromconfig/n8n.env, creates API key via REST endpoint with PostgreSQL fallback, outputsN8N_API_KEY=...Environment Templates:
nocodb.env.j2Jinja2 template with PostgreSQL connection params, NC_DB URI, JWT secret, invite-only signup flag, telemetry disable, and admin passwordn8n.env.j2Jinja2 template with PostgreSQL non-root credentials, encryption key, and owner passwordSemaphore Templates:
service_branchsurvey variableTests:
test_nocodb_templates.bats(9 test cases validating template existence, required secrets.* placeholders, NC_DB URI, invite/telemetry flags, and absence of hardcoded values)test_n8n_templates.bats(5 test cases validating template structure, secrets placeholders, Postgres non-root variables, and encryption key format)Platform Layer
Docker Compose Files:
nocodb-postgresservice topostgres, changed port mapping from${NOCODB_LISTEN:-127.0.0.1}:8181:8080to8080:8080, switched to named volumes (nocodb_nc_data,nocodb_db_data)n8n-postgres/n8n-redistopostgres/redis, replaced inline env values with shared YAML anchors, updated to use variable defaults (e.g.,${POSTGRES_DB}), added local-files bind mount for n8n storage, changed port exposure to5678:5678Documentation Layer
CLAUDE.md:
secret/services/nocodbandsecret/services/n8nentriesSecurity Impact
This PR changes the secrets credential flow from direct deploy.sh ↔ OpenBao integration to a managed pattern where OpenBao stores extracted runtime API keys via Ansible AppRole authentication, with deploy.sh reading from Jinja2-templated config files only—reducing attack surface by removing OpenBao client invocations from deployed containers and enforcing centralized secret initialization through Ansible.