The Big Bastard#746
Open
infraclaw-dash wants to merge 10 commits into
Open
Conversation
added 5 commits
May 28, 2026 19:50
- use ipam_pool_id for AWS EIP allocation from IPAM pools - make masternode roles compatible with strict boolean conditionals - handle empty regular masternode lists and HPMN-only networks - default dashd wallet enablement safely and add start-at-task support - add required Dashmate Tenderdash p2p allowlistOnly setting - harden ZeroSSL SSM restore for missing values and suppress key logging
|
Warning Review limit reached
More reviews will be available in 3 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR implements end-to-end support for building and deploying pre-baked base AMIs. It adds a GitHub Actions workflow and Packer template to build AMIs with common setup pre-installed, introduces Ansible configuration flags to conditionally skip setup during deployment, refactors the Ansible role suite to support fast mode, and switches Terraform from dynamic AMI lookups to pre-configured AMI IDs. ChangesPre-baked AMI Build and Deployment
Sequence DiagramsequenceDiagram
participant Developer
participant GitHub
participant Packer
participant AWS_EC2 as AWS EC2
participant Ansible as Ansible Provisioner
participant Terraform
Developer->>GitHub: trigger manual AMI build or schedule
GitHub->>Packer: packer build with region/architecture
Packer->>AWS_EC2: launch temporary EC2 instance
Packer->>Ansible: run ansible/prebake-common.yml
Ansible->>AWS_EC2: install docker, packages, utilities
Packer->>AWS_EC2: snapshot instance as AMI
GitHub->>GitHub: append summary with AMI IDs to step
Developer->>Terraform: configure base_ami_amd64_id, base_ami_arm64_id
Terraform->>AWS_EC2: launch instances using pre-configured AMI IDs
Developer->>Terraform: deploy with -p --prebaked-common-setup
Terraform->>Ansible: skip common setup phases
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ansible/deploy.yml (1)
378-399: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep the
dashmate_deployplay fact-free.This play still opts into
gather_facts: true, which keeps extra setup on the hot path of the fast parallel deploy flow this PR is adding. Please align it with the deploy playbook contract and fetch only explicit facts if a role here truly needs them.As per coding guidelines, "Add
dashmate_deploytag, setgather_facts: false, and usestrategy: freeinansible/deploy.ymlto enable fast, parallel deployments".🤖 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 `@ansible/deploy.yml` around lines 378 - 399, The play "Set up core and platform on HP masternodes" currently has gather_facts: true; change it to gather_facts: false and add the dashmate_deploy tag to the play declaration so it follows the deploy playbook contract; keep strategy: free as-is and, if any role (e.g., dash_cli, dashmate, mn_status_report, metricbeat) truly needs specific facts, call ansible.builtin.setup with an explicit gather_subset or invoke targeted ansible.builtin.set_fact in those roles rather than enabling global gather_facts for the play.ansible/roles/dashmate/tasks/ssl/zerossl.yml (1)
45-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck both ZeroSSL artifacts before skipping the restore path.
This only stats
private.key, so a host withprivate.keypresent butcsr.pemmissing will skip the SSM restore even though the certificate state is incomplete.Proposed fix
-- name: Check that ZeroSSL CSR and private key files exist +- name: Check that ZeroSSL private key file exists ansible.builtin.stat: path: '{{ dashmate_zerossl_keys_path }}/{{ dashmate_zerossl_private_key_file_name }}' - register: zero_ssl_files + register: zero_ssl_private_key_file + +- name: Check that ZeroSSL CSR file exists + ansible.builtin.stat: + path: '{{ dashmate_zerossl_keys_path }}/{{ dashmate_zerossl_csr_file_name }}' + register: zero_ssl_csr_file - name: Get ZeroSSL CSR and private key from SSM no_log: true ansible.builtin.copy: dest: '{{ dashmate_zerossl_keys_path }}/{{ item }}' @@ loop: - '{{ dashmate_zerossl_private_key_file_name }}' - '{{ dashmate_zerossl_csr_file_name }}' when: > - not zero_ssl_files.stat.exists and + (not zero_ssl_private_key_file.stat.exists or not zero_ssl_csr_file.stat.exists) and (dashmate_zerossl_ssm_certificate_id | default('', true) | length) > 0🤖 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 `@ansible/roles/dashmate/tasks/ssl/zerossl.yml` around lines 45 - 63, The current check only stats the private key file (task "Check that ZeroSSL CSR and private key files exist" registering zero_ssl_files) so a host with the private key but missing the CSR will skip the SSM restore; update the task to verify existence of both dashmate_zerossl_private_key_file_name and dashmate_zerossl_csr_file_name (e.g. stat both files or loop the stat and register a combined result) and change the when condition (used by the "Get ZeroSSL CSR and private key from SSM" task) to only skip restore when both files exist, referencing the same registered variable(s) (zero_ssl_files or new registration) and the same variable names dashmate_zerossl_private_key_file_name and dashmate_zerossl_csr_file_name so the SSM copy runs when either artifact is missing.
🧹 Nitpick comments (2)
packer/dash-network-base.pkr.hcl (1)
2-10: ⚡ Quick winPin tested Packer plugin versions instead of using open-ended lower bounds.
>=lets futureamazon/ansibleplugin releases change this build pipeline under your feet. For a scheduled AMI bake job, that makes failures and behavior changes hard to reproduce. Please pin to the tested version or at least a bounded compatible range.🤖 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 `@packer/dash-network-base.pkr.hcl` around lines 2 - 10, The required_plugins block currently uses open-ended lower-bound constraints for the amazon and ansible plugins (symbols: required_plugins, amazon, ansible, version), which risks unexpected changes; update the version values to either a pinned tested version (e.g., "1.3.0") or a bounded compatible range (e.g., ">= 1.3.0, < 2.0.0" or "~> 1.3.0") for both amazon and ansible to ensure reproducible builds and stable behavior.ansible/prebake-common.yml (1)
7-17: ⚡ Quick winExtract the shared pip package pins.
This list is duplicated in
ansible/deploy.ymlLines 61-68. Keeping the prebake and regular deploy paths in sync by hand will drift on the next version bump, leaving AMI-based hosts and fresh installs on different Python dependencies.♻️ One way to centralize the shared package pins
+# ansible/vars/common_python_packages.yml +pip_package: python3-pip +pip_install_packages: + - name: docker + version: "6.0.1" + - name: docker-compose + version: "1.29.2" + - name: requests + version: "2.31.0"- name: Build Dash Network common base image hosts: all become: true gather_facts: true - vars: - swap_space: 2G - pip_package: python3-pip - pip_install_packages: - - name: docker - version: "6.0.1" - - name: docker-compose - version: "1.29.2" - - name: requests - version: "2.31.0" + vars_files: + - vars/common_python_packages.yml + vars: + swap_space: 2G# Apply the same vars_files entry in ansible/deploy.yml for the matching play.🤖 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 `@ansible/prebake-common.yml` around lines 7 - 17, The pip package pin list (pip_install_packages) is duplicated between prebake and deploy; extract the shared pins into a single vars file (e.g., create a vars file holding pip_package and pip_install_packages) and update both prebake and the matching play in deploy to load that file via vars_files so both paths use the same source of truth; ensure you update references to pip_package and pip_install_packages in the playbooks to use the centralized vars file.
🤖 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 @.github/workflows/build-base-images.yml:
- Around line 52-56: Update the GitHub Actions steps that currently use tag
pins: change the "Checkout dash-network-deploy" step (uses: actions/checkout@v4)
to pin to the specific commit SHA and add a with: persist-credentials: false key
to prevent the workflow from retaining the repo token, and change the "Set up
Packer" step (uses: hashicorp/setup-packer@v3) to pin to a specific commit SHA
instead of the tag; keep the step names ("Checkout dash-network-deploy" and "Set
up Packer") and only modify the uses values to SHAs and add the
persist-credentials: false under the checkout step.
In `@ansible/roles/dashmate/tasks/main.yml`:
- Around line 234-239: The "Configure logs" task (import_tasks: ./logs.yml)
currently only checks skip_observability_setup and related flags, so logs.yml
still runs during fast deploys; modify its when clause to also skip when fast
mode is enabled by adding a check such as "and not (fast_deploy | default(false)
| bool)" (or your project’s fast-mode variable name) so the full when becomes:
not (skip_observability_setup | default(false) | bool) and not (fast_deploy |
default(false) | bool) and not (logrotate_config_stat.stat.exists |
default(false)) or force_logs_config | default(false) | bool and
dashmate_user_exists | bool, ensuring the import_tasks "./logs.yml" is bypassed
in fast mode.
In `@ansible/roles/dashmate/tasks/ssl/zerossl.yml`:
- Around line 50-57: The task "Get ZeroSSL CSR and private key from SSM" writes
keys with mode "0644" making the private key world-readable; change the mode so
private.key is created with tighter permissions (e.g. "0600") while keeping
CSR/public files at "0644". Update the ansible.builtin.copy invocation that uses
dest '{{ dashmate_zerossl_keys_path }}/{{ item }}' and owner/group '{{
dashmate_user }}'/'{{ dashmate_group }}' to set mode conditionally (for example
using a Jinja2 conditional on item == 'private.key' such as mode: "{{ '0600' if
item == 'private.key' else '0644' }}") so only the private key is restricted.
Ensure no other parts of the task change.
In `@bin/build-base-image`:
- Around line 48-57: The script currently fails fast for missing packer but not
for Ansible; add a similar check for the ansible-playbook binary before running
packer init so builds that use the Ansible provisioner fail early. Specifically,
after the existing packer check (the command -v packer block) add a check using
command -v ansible-playbook >/dev/null 2>&1 and if it is missing, print an error
like "Error: ansible-playbook is required to build base AMIs" to stderr and exit
1; keep the rest of the flow (PROFILE -> AWS_PROFILE export and packer init
packer/dash-network-base.pkr.hcl) unchanged.
In `@terraform/aws/main.tf`:
- Around line 46-50: The locals currently reference data.aws_ami.ubuntu_amd and
ubuntu_arm unconditionally which forces those Canonical lookups into the graph
even when overrides are supplied; change the data "aws_ami" resources to be
conditional (use count = var.base_ami_amd64_id == "" ? 1 : 0 and count =
var.base_ami_arm64_id == "" ? 1 : 0) and update the locals (ubuntu_amd_ami_id,
ubuntu_arm_ami_id, main_host_ami_id) to index the conditional data (e.g.
data.aws_ami.ubuntu_amd[0].id) only in the fallback path so the ternary uses
var.base_ami_*_id when provided and only references data.aws_ami.*[0].id when
count == 1, preventing unnecessary Canonical lookups from being added to the
dependency graph.
In `@terraform/aws/variables.tf`:
- Around line 4-14: The variables base_ami_amd64_id and base_ami_arm64_id accept
any string today; add a Terraform validation block for each variable that allows
either an empty string or a valid AMI identifier (match e.g. regex
^ami-[0-9a-fA-F]+$ or startswith "ami-") so mistakes fail at plan time; update
the variable blocks for base_ami_amd64_id and base_ami_arm64_id to include a
validation { condition = ... error_message = "..." } enforcing "" or an ami-...
value.
---
Outside diff comments:
In `@ansible/deploy.yml`:
- Around line 378-399: The play "Set up core and platform on HP masternodes"
currently has gather_facts: true; change it to gather_facts: false and add the
dashmate_deploy tag to the play declaration so it follows the deploy playbook
contract; keep strategy: free as-is and, if any role (e.g., dash_cli, dashmate,
mn_status_report, metricbeat) truly needs specific facts, call
ansible.builtin.setup with an explicit gather_subset or invoke targeted
ansible.builtin.set_fact in those roles rather than enabling global gather_facts
for the play.
In `@ansible/roles/dashmate/tasks/ssl/zerossl.yml`:
- Around line 45-63: The current check only stats the private key file (task
"Check that ZeroSSL CSR and private key files exist" registering zero_ssl_files)
so a host with the private key but missing the CSR will skip the SSM restore;
update the task to verify existence of both
dashmate_zerossl_private_key_file_name and dashmate_zerossl_csr_file_name (e.g.
stat both files or loop the stat and register a combined result) and change the
when condition (used by the "Get ZeroSSL CSR and private key from SSM" task) to
only skip restore when both files exist, referencing the same registered
variable(s) (zero_ssl_files or new registration) and the same variable names
dashmate_zerossl_private_key_file_name and dashmate_zerossl_csr_file_name so the
SSM copy runs when either artifact is missing.
---
Nitpick comments:
In `@ansible/prebake-common.yml`:
- Around line 7-17: The pip package pin list (pip_install_packages) is
duplicated between prebake and deploy; extract the shared pins into a single
vars file (e.g., create a vars file holding pip_package and
pip_install_packages) and update both prebake and the matching play in deploy to
load that file via vars_files so both paths use the same source of truth; ensure
you update references to pip_package and pip_install_packages in the playbooks
to use the centralized vars file.
In `@packer/dash-network-base.pkr.hcl`:
- Around line 2-10: The required_plugins block currently uses open-ended
lower-bound constraints for the amazon and ansible plugins (symbols:
required_plugins, amazon, ansible, version), which risks unexpected changes;
update the version values to either a pinned tested version (e.g., "1.3.0") or a
bounded compatible range (e.g., ">= 1.3.0, < 2.0.0" or "~> 1.3.0") for both
amazon and ansible to ensure reproducible builds and stable behavior.
🪄 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: ae25224d-0a6f-42f0-bb89-17cb620a87eb
📒 Files selected for processing (32)
.github/workflows/build-base-images.yml.gitignoreREADME.mdansible/deploy.ymlansible/group_vars/allansible/prebake-common.ymlansible/roles/dashd/tasks/main.ymlansible/roles/dashmate/defaults/main.ymlansible/roles/dashmate/tasks/build.ymlansible/roles/dashmate/tasks/logs.ymlansible/roles/dashmate/tasks/main.ymlansible/roles/dashmate/tasks/rolling_restart.ymlansible/roles/dashmate/tasks/ssl/zerossl.ymlansible/roles/dashmate/templates/dashmate.json.j2ansible/roles/load_tool/tasks/main.ymlansible/roles/metricbeat/tasks/main.ymlansible/roles/mn_createprotx/tasks/main.ymlansible/roles/mn_find_collateral/tasks/main.ymlansible/roles/mn_init/tasks/main.ymlansible/roles/mn_protx_config/tasks/main.ymlansible/roles/mn_unban/tasks/main.ymlansible/roles/status_dashboard/tasks/main.ymlbin/build-base-imagebin/deployflake.nixlib/cli/ansible.shlib/configGenerator/generateTerraformConfig.jspacker/dash-network-base.pkr.hclterraform/aws/.terraform.lock.hclterraform/aws/instances.tfterraform/aws/main.tfterraform/aws/variables.tf
Contributor
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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
2 participants
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
Validation
ansible-playbook --syntax-checkfor regular deploy pathansible-playbook --syntax-checkwith fast-mode varspacker fmt -checkpacker validateSummary by CodeRabbit
New Features
Documentation
Chores
Improvements