feat: use per-devnet GPG-encrypted SSH keys in CI workflows#739
Open
vivekgsharma wants to merge 5 commits intov1.0-devfrom
Open
feat: use per-devnet GPG-encrypted SSH keys in CI workflows#739vivekgsharma wants to merge 5 commits intov1.0-devfrom
vivekgsharma wants to merge 5 commits intov1.0-devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflows now generate, encrypt, store, and decrypt devnet RSA SSH keys as Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions runner
participant Repo as Configs repo (networks/)
participant GPG as GnuPG
participant SSH as SSH (~/.ssh)
participant Ansible as Ansible playbooks
GH->>Repo: clone/list `networks/$NETWORK.*`
GH->>GPG: import GPG private key from secrets (if devnet)
alt devnet and `networks/$NETWORK.ssh.asc` present
GPG->>Repo: decrypt `networks/$NETWORK.ssh.asc`
Repo-->>SSH: write `~/.ssh/id_rsa` and `~/.ssh/id_rsa.pub`
else non-devnet or direct key
GH->>SSH: write `DEPLOY_SERVER_KEY` to `~/.ssh/id_rsa`
end
GH->>SSH: write GitHub deploy key `id_ed25519` and update `~/.ssh/config`
GH->>Ansible: invoke playbooks with `--private-key="$HOME/.ssh/id_rsa"`
Ansible-->>Repo: read `networks/.env` (PRIVATE_KEY_PATH, PUBLIC_KEY_PATH)
GH->>Repo: push/update `networks/*.ssh.asc` and other artifacts (fail if partial)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (5)
.github/workflows/create-devnet.yml (2)
231-240: GPG recipient extraction is clever but fragile.The awk parsing relies on specific GPG output format with
pubfollowed byfprrecords. This works but is tightly coupled to GPG's--with-colonsoutput format.Consider adding a comment explaining the expected GPG output structure for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/create-devnet.yml around lines 231 - 240, Add a short clarifying comment above the mapfile/gpg/awk block explaining the assumed --with-colons output structure (that a "pub" record is followed by an "fpr" record whose 10th colon field is the fingerprint) and why the awk rule ('$1 == "pub" {want=1; next} want && $1 == "fpr" {print $10; want=0}') extracts recipients; reference the variables/commands involved (mapfile, recipients array, gpg --batch --with-colons, and the awk expression) so future maintainers understand the fragile coupling to GPG's output format and can update it safely if the format changes.
94-101: Redundant validation: the final regex check is always satisfied if the earlier checks pass.If
$NAMEmatches^[a-z0-9][a-z0-9-]*$(line 90), thendevnet-$NAMEwill always match^devnet-[a-z0-9][a-z0-9-]*$. The check at lines 98-101 is unreachable as a failure path.Consider removing redundant check
if [[ "$NAME" == "testnet" || "$NAME" == "mainnet" || "$NAME" == mainnet-* ]]; then echo "Error: reserved network names are not allowed in this workflow" exit 1 fi - if [[ ! "devnet-$NAME" =~ ^devnet-[a-z0-9][a-z0-9-]*$ ]]; then - echo "Error: resulting network name is not a valid devnet name" - exit 1 - fi echo "Will create: devnet-$NAME"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/create-devnet.yml around lines 94 - 101, The final regex check ([[ ! "devnet-$NAME" =~ ^devnet-[a-z0-9][a-z0-9-]*$ ]]) is redundant because the prior validation of NAME (^[a-z0-9][a-z0-9-]*$) plus the reserved-name guard already guarantees devnet-$NAME will match; remove the unreachable block (the entire if that echoes "Error: resulting network name is not a valid devnet name" and exits) to avoid duplicated validation, leaving the earlier NAME regex and reserved-name checks intact..github/workflows/destroy-devnet.yml (1)
54-65: Same redundant validation pattern as increate-devnet.yml.The check at lines 62-65 is unreachable since lines 54-57 already ensure
$NAMEmatches^[a-z0-9][a-z0-9-]*$.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/destroy-devnet.yml around lines 54 - 65, The final regex check that validates "devnet-$NAME" is redundant and unreachable because the earlier validation on NAME already enforces ^[a-z0-9][a-z0-9-]*$; remove the unreachable if block that tests [[ ! "devnet-$NAME" =~ ^devnet-[a-z0-9][a-z0-9-]*$ ]] (the block that echoes "Error: resulting network name is not a valid devnet name" and exits) to avoid duplicate validation; keep the initial NAME validation and the reserved-name check (the if checks referencing NAME and the reserved string comparisons) intact..github/workflows/platform-deploy.yml (1)
58-72: SSH config created here is overwritten later, same pattern asdeploy.yml.The config at lines 66-70 is replaced in the "Prepare server SSH key" step. Consider consolidating.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/platform-deploy.yml around lines 58 - 72, The SSH config created in the "Set up GitHub SSH key" step is later overwritten by the "Prepare server SSH key" step; consolidate by creating a single SSH config that contains both Host entries (github.com and the server host) or append the server block instead of clobbering the file. Update the "Set up GitHub SSH key" and "Prepare server SSH key" steps so they either write distinct Host blocks to ~/.ssh/config using >> (append) or a shared helper step that ensures both IdentityFile entries and StrictHostKeyChecking settings coexist, and ensure correct permissions with chmod 600 on the final config..github/workflows/deploy.yml (1)
55-69: SSH config created here is immediately overwritten in the next SSH step.The
~/.ssh/configwritten here (lines 63-67) is replaced in the "Prepare server SSH key" step (lines 117-127). This duplication is harmless but redundant.Consider deferring config creation to the later step
- name: Set up GitHub SSH key env: EVO_APP_DEPLOY_KEY: ${{ secrets.EVO_APP_DEPLOY_KEY }} run: | mkdir -p ~/.ssh printf '%s\n' "$EVO_APP_DEPLOY_KEY" > ~/.ssh/id_ed25519 chmod 600 ~/.ssh/id_ed25519 - - cat > ~/.ssh/config << 'EOL' - Host github.com - IdentityFile ~/.ssh/id_ed25519 - StrictHostKeyChecking no - EOL - - chmod 600 ~/.ssh/config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 55 - 69, The SSH config written in the "Set up GitHub SSH key" step is redundant because the later "Prepare server SSH key" step overwrites ~/.ssh/config; remove or defer creation of ~/.ssh/config from the "Set up GitHub SSH key" step (keep creating ~/.ssh/id_ed25519 and setting permissions) and ensure the actual SSH config is only written in the "Prepare server SSH key" step, or consolidate the two steps so only one writes ~/.ssh/config to avoid duplication; refer to the step names "Set up GitHub SSH key" and "Prepare server SSH key" and the file ~/.ssh/config when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/create-devnet.yml:
- Around line 373-374: The workflow step named "Push configs to
dash-network-configs" uses if: always(), causing it to run even after failed
deployments; change its condition to only run on successful deploys (e.g.,
replace if: always() with if: success() or use the deploy job outcome like if:
needs.deploy.outcome == 'success') so configs aren't pushed after a
partial/failed deploy; update the step condition in the "Push configs to
dash-network-configs" step accordingly.
---
Nitpick comments:
In @.github/workflows/create-devnet.yml:
- Around line 231-240: Add a short clarifying comment above the mapfile/gpg/awk
block explaining the assumed --with-colons output structure (that a "pub" record
is followed by an "fpr" record whose 10th colon field is the fingerprint) and
why the awk rule ('$1 == "pub" {want=1; next} want && $1 == "fpr" {print $10;
want=0}') extracts recipients; reference the variables/commands involved
(mapfile, recipients array, gpg --batch --with-colons, and the awk expression)
so future maintainers understand the fragile coupling to GPG's output format and
can update it safely if the format changes.
- Around line 94-101: The final regex check ([[ ! "devnet-$NAME" =~
^devnet-[a-z0-9][a-z0-9-]*$ ]]) is redundant because the prior validation of
NAME (^[a-z0-9][a-z0-9-]*$) plus the reserved-name guard already guarantees
devnet-$NAME will match; remove the unreachable block (the entire if that echoes
"Error: resulting network name is not a valid devnet name" and exits) to avoid
duplicated validation, leaving the earlier NAME regex and reserved-name checks
intact.
In @.github/workflows/deploy.yml:
- Around line 55-69: The SSH config written in the "Set up GitHub SSH key" step
is redundant because the later "Prepare server SSH key" step overwrites
~/.ssh/config; remove or defer creation of ~/.ssh/config from the "Set up GitHub
SSH key" step (keep creating ~/.ssh/id_ed25519 and setting permissions) and
ensure the actual SSH config is only written in the "Prepare server SSH key"
step, or consolidate the two steps so only one writes ~/.ssh/config to avoid
duplication; refer to the step names "Set up GitHub SSH key" and "Prepare server
SSH key" and the file ~/.ssh/config when making the change.
In @.github/workflows/destroy-devnet.yml:
- Around line 54-65: The final regex check that validates "devnet-$NAME" is
redundant and unreachable because the earlier validation on NAME already
enforces ^[a-z0-9][a-z0-9-]*$; remove the unreachable if block that tests [[ !
"devnet-$NAME" =~ ^devnet-[a-z0-9][a-z0-9-]*$ ]] (the block that echoes "Error:
resulting network name is not a valid devnet name" and exits) to avoid duplicate
validation; keep the initial NAME validation and the reserved-name check (the if
checks referencing NAME and the reserved string comparisons) intact.
In @.github/workflows/platform-deploy.yml:
- Around line 58-72: The SSH config created in the "Set up GitHub SSH key" step
is later overwritten by the "Prepare server SSH key" step; consolidate by
creating a single SSH config that contains both Host entries (github.com and the
server host) or append the server block instead of clobbering the file. Update
the "Set up GitHub SSH key" and "Prepare server SSH key" steps so they either
write distinct Host blocks to ~/.ssh/config using >> (append) or a shared helper
step that ensures both IdentityFile entries and StrictHostKeyChecking settings
coexist, and ensure correct permissions with chmod 600 on the final config.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e404721-f3c2-4c17-8d2f-19751a94944b
📒 Files selected for processing (4)
.github/workflows/create-devnet.yml.github/workflows/deploy.yml.github/workflows/destroy-devnet.yml.github/workflows/platform-deploy.yml
Comment on lines
373
to
+374
| - name: Push configs to dash-network-configs | ||
| if: always() |
There was a problem hiding this comment.
Potential issue: if: always() may push configs after failed deployments.
This step runs even when the deploy step fails. If Terraform succeeded but Ansible failed, you might push config files for a partially deployed devnet.
Consider changing to if: success() or adding logic to skip the push if the deploy step failed.
Suggested fix
- name: Push configs to dash-network-configs
- if: always()
+ if: success()
env:
EVO_APP_DEPLOY_WRITE_KEY: ${{ secrets.EVO_APP_DEPLOY_WRITE_KEY }}📝 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.
Suggested change
| - name: Push configs to dash-network-configs | |
| if: always() | |
| - name: Push configs to dash-network-configs | |
| if: success() | |
| env: | |
| EVO_APP_DEPLOY_WRITE_KEY: ${{ secrets.EVO_APP_DEPLOY_WRITE_KEY }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/create-devnet.yml around lines 373 - 374, The workflow
step named "Push configs to dash-network-configs" uses if: always(), causing it
to run even after failed deployments; change its condition to only run on
successful deploys (e.g., replace if: always() with if: success() or use the
deploy job outcome like if: needs.deploy.outcome == 'success') so configs aren't
pushed after a partial/failed deploy; update the step condition in the "Push
configs to dash-network-configs" step accordingly.
vivekgsharma
force-pushed
the
vivek/devent-gpg
branch
from
c6d503d to
1cb683a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/create-devnet.yml (1)
372-416:⚠️ Potential issue | 🟠 MajorMake config persistence transactional.
deploy.ymlanddestroy-devnet.ymlnow treatyml/tfvars/inventory/ssh.ascas the control-plane state. Because this step still runs withif: always()and downgradesgit pushfailures to warnings, a failed deploy can publish a false resume set, and a successful create can finish without persisting the only recoverable SSH key. Please gate persistence on a known-good infra state and fail the run when a fresh create cannot publish all four artifacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/create-devnet.yml around lines 372 - 416, The "Push configs to dash-network-configs" step currently runs unconditionally (if: always()) and downgrades missing artifacts or push failures to warnings (COPIED != 4 exits 0 and push failures echo warnings), which allows inconsistent control-plane state to be published; change this step to only run on success (replace if: always() with if: success()), require all four artifacts by making the COPIED != 4 check fail the job (exit 1) instead of warning, and make git push failures return non-zero (remove the warning+exit 0 branches and instead exit 1 on push failure). Also ensure the write-key branch (checking EVO_APP_DEPLOY_WRITE_KEY and ~/.ssh/id_ed25519_write) treats missing write key as a hard failure when COPIED == 4 so a fresh create cannot finish without persisting the SSH key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/create-devnet.yml:
- Around line 372-416: The "Push configs to dash-network-configs" step currently
runs unconditionally (if: always()) and downgrades missing artifacts or push
failures to warnings (COPIED != 4 exits 0 and push failures echo warnings),
which allows inconsistent control-plane state to be published; change this step
to only run on success (replace if: always() with if: success()), require all
four artifacts by making the COPIED != 4 check fail the job (exit 1) instead of
warning, and make git push failures return non-zero (remove the warning+exit 0
branches and instead exit 1 on push failure). Also ensure the write-key branch
(checking EVO_APP_DEPLOY_WRITE_KEY and ~/.ssh/id_ed25519_write) treats missing
write key as a hard failure when COPIED == 4 so a fresh create cannot finish
without persisting the SSH key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c2d104b-6dc0-47ea-a58f-29e943717c6a
📒 Files selected for processing (4)
.github/workflows/create-devnet.yml.github/workflows/deploy.yml.github/workflows/destroy-devnet.yml.github/workflows/platform-deploy.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/platform-deploy.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ansible/deploy.yml (1)
404-419: Consider adding a filter to preserve recently-used images during cleanup.The Docker prune removes all unused images (
dangling: false), which could delete images that may be needed for quick rollbacks. If this is intentional for CI environments where disk space is constrained, this is fine. Otherwise, consider adding animages_filterswith a time-based threshold.💡 Optional: Add time-based filter to preserve recent images
- name: Prune unused Docker images community.docker.docker_prune: containers: true images: true images_filters: dangling: false + until: "24h" networks: true volumes: false builder_cache: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/deploy.yml` around lines 404 - 419, The Docker prune task "Prune unused Docker images" currently sets images_filters: dangling: false and will remove unused but potentially recently-used images; update that task (community.docker.docker_prune) to add a time-based filter (e.g., images_filters: { dangling: false, until: "{{ docker_prune_image_age | default('72h') }}" }) so images newer than the threshold are preserved, and make docker_prune_image_age a configurable variable (defaults to 72h) so CI vs prod behavior can be tuned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ansible/deploy.yml`:
- Around line 404-419: The Docker prune task "Prune unused Docker images"
currently sets images_filters: dangling: false and will remove unused but
potentially recently-used images; update that task
(community.docker.docker_prune) to add a time-based filter (e.g.,
images_filters: { dangling: false, until: "{{ docker_prune_image_age |
default('72h') }}" }) so images newer than the threshold are preserved, and make
docker_prune_image_age a configurable variable (defaults to 72h) so CI vs prod
behavior can be tuned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces per-devnet SSH access in CI, replacing the previous shared server key approach with devnet-specific credentials.
Key Changes
Unique SSH keypair per devnet
Create Devnetnow generates a dedicated SSH keypair for each devnet.devnet-<name>.ssh.ascSecure key reuse across workflows
Create Resume,Manual Deploy,Platform Deploy,Destroy) decrypt the stored key in CI using the configured GPG key.Manual access for developers
devnet-<name>.ssh.ascfromdash-network-configs.Fixes
Dashmate (fresh-node) issues in Ansible
dashmateuser to thedockergroup.Impact