feat(terraform): add BYOIP support for all devnet instances#734
Merged
ktechmidas merged 1 commit intov1.0-devfrom Mar 2, 2026
Merged
feat(terraform): add BYOIP support for all devnet instances#734ktechmidas merged 1 commit intov1.0-devfrom
ktechmidas merged 1 commit intov1.0-devfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces BYOIP (Bring Your Own IP) support to the Terraform AWS infrastructure. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terraform/aws/services_output.tf (1)
248-252:⚠️ Potential issue | 🟡 MinorCopy-paste bug: ARM masternode incorrectly named "amd".
The
hp_masternodes_armblock uses"hp-masternode-amd-${n + 1}"instead of"hp-masternode-arm-${n + 1}".🐛 Proposed fix
hp_masternodes_arm = [ for n in range(length(aws_instance.hp_masternode_arm)) : templatefile( "${path.module}/templates/services/node.tpl", { - name = "hp-masternode-amd-${n + 1}" + name = "hp-masternode-arm-${n + 1}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/aws/services_output.tf` around lines 248 - 252, The hp_masternodes_arm list comprehension incorrectly sets the generated name string to "hp-masternode-amd-${n + 1}"; update the templatefile argument in the hp_masternodes_arm block so the name value reads "hp-masternode-arm-${n + 1}" (i.e., replace "amd" with "arm") to match the block identifier hp_masternodes_arm and ensure correct naming for ARM masternodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/deploy.yml`:
- Line 338: Change the HP masternode play's top-level setting from gather_facts:
true back to gather_facts: false (restore fast-deploy behavior), and if specific
facts are needed move those collects into pre_tasks; also ensure the play
includes the dashmate_deploy tag and sets strategy: free to allow parallel fast
deployments (look for the play block labeled for the HP masternode or the
gather_facts entry in ansible/deploy.yml to update these keys).
---
Outside diff comments:
In `@terraform/aws/services_output.tf`:
- Around line 248-252: The hp_masternodes_arm list comprehension incorrectly
sets the generated name string to "hp-masternode-amd-${n + 1}"; update the
templatefile argument in the hp_masternodes_arm block so the name value reads
"hp-masternode-arm-${n + 1}" (i.e., replace "amd" with "arm") to match the block
identifier hp_masternodes_arm and ensure correct naming for ARM masternodes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
INFRA.MDansible/deploy.ymlterraform/aws/ansible_inventory_output.tfterraform/aws/instances.tfterraform/aws/main.tfterraform/aws/services_output.tfterraform/aws/variables.tf
ansible/deploy.yml
Outdated
| hosts: hp_masternodes | ||
| become: true | ||
| gather_facts: false | ||
| gather_facts: true |
There was a problem hiding this comment.
Revert HP masternode play to gather_facts: false
Line 338 currently enables full fact gathering, which breaks the fast-deploy contract for ansible/deploy.yml. Keep gather_facts: false and gather only minimal required facts in pre_tasks if needed.
Suggested patch
- gather_facts: true
+ gather_facts: false
@@
pre_tasks:
+ - name: Gather minimal OS facts needed by roles
+ ansible.builtin.setup:
+ gather_subset:
+ - min
+ filter:
+ - ansible_os_family
- name: Check inventory for HP masternodes
ansible.builtin.set_fact:
node: "{{ hp_masternodes[inventory_hostname] }}"As per coding guidelines, "ansible/deploy.yml: Add dashmate_deploy tag, set gather_facts: false, and use strategy: free in ansible/deploy.yml to enable fast, parallel deployments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/deploy.yml` at line 338, Change the HP masternode play's top-level
setting from gather_facts: true back to gather_facts: false (restore fast-deploy
behavior), and if specific facts are needed move those collects into pre_tasks;
also ensure the play includes the dashmate_deploy tag and sets strategy: free to
allow parallel fast deployments (look for the play block labeled for the HP
masternode or the gather_facts entry in ansible/deploy.yml to update these
keys).
When byoip_pool_id is set, all instances (masternodes, HP masternodes, web, wallet, seed, miner, VPN) get EIPs allocated from the specified IPAM pool instead of auto-assigned AWS public IPs. This gives devnets stable, predictable IPs from our own address space. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ktechmidas
force-pushed
the
infra/byoip
branch
from
0c219e7 to
c1a0c58
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terraform/aws/services_output.tf (1)
248-252:⚠️ Potential issue | 🟡 MinorBug: Incorrect name for ARM HP masternode in services output.
Line 252 names ARM HP masternodes as
"hp-masternode-amd-${n + 1}"instead of"hp-masternode-arm-${n + 1}". This appears to be a pre-existing bug, but worth fixing while touching this file.Proposed fix
hp_masternodes_arm = [ for n in range(length(aws_instance.hp_masternode_arm)) : templatefile( "${path.module}/templates/services/node.tpl", { - name = "hp-masternode-amd-${n + 1}" + name = "hp-masternode-arm-${n + 1}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/aws/services_output.tf` around lines 248 - 252, In hp_masternodes_arm output generation change the mistaken instance name string: update the name passed into templatefile for the hp_masternodes_arm list (currently "hp-masternode-amd-${n + 1}") to the correct "hp-masternode-arm-${n + 1}" so ARM nodes are labeled correctly; modify the name literal in the hp_masternodes_arm for-expression that calls templatefile.
🧹 Nitpick comments (1)
terraform/aws/main.tf (1)
358-374: Consider extracting the repeated concat expression into a local.The same concat expression appears three times. Extracting it to a local variable would improve readability and maintainability.
Suggested refactor
+locals { + hpmn_ips = concat( + (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_arm_eip.*.public_ip : aws_instance.hp_masternode_arm.*.public_ip, + (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_amd_eip.*.public_ip : aws_instance.hp_masternode_amd.*.public_ip + ) +} + resource "random_shuffle" "dns_ips" { - input = concat( - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_arm_eip.*.public_ip : aws_instance.hp_masternode_arm.*.public_ip, - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_amd_eip.*.public_ip : aws_instance.hp_masternode_amd.*.public_ip - ) - result_count = length( - concat( - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_arm_eip.*.public_ip : aws_instance.hp_masternode_arm.*.public_ip, - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_amd_eip.*.public_ip : aws_instance.hp_masternode_amd.*.public_ip - ) - ) > local.dns_record_length ? local.dns_record_length : length( - concat( - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_arm_eip.*.public_ip : aws_instance.hp_masternode_arm.*.public_ip, - (var.create_eip || var.byoip_pool_id != "") ? aws_eip.hpmn_amd_eip.*.public_ip : aws_instance.hp_masternode_amd.*.public_ip - ) - ) + input = local.hpmn_ips + result_count = min(length(local.hpmn_ips), local.dns_record_length) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/aws/main.tf` around lines 358 - 374, The repeated concat(...) expression used inside resource random_shuffle.dns_ips should be extracted into a local variable (e.g., local.dns_inputs) to avoid duplication; update the resource to reference that local for input and for computing result_count (replace the three concat(...) uses with local.dns_inputs and use length(local.dns_inputs) where needed), and ensure the ternary conditions remain identical to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@terraform/aws/services_output.tf`:
- Around line 248-252: In hp_masternodes_arm output generation change the
mistaken instance name string: update the name passed into templatefile for the
hp_masternodes_arm list (currently "hp-masternode-amd-${n + 1}") to the correct
"hp-masternode-arm-${n + 1}" so ARM nodes are labeled correctly; modify the name
literal in the hp_masternodes_arm for-expression that calls templatefile.
---
Nitpick comments:
In `@terraform/aws/main.tf`:
- Around line 358-374: The repeated concat(...) expression used inside resource
random_shuffle.dns_ips should be extracted into a local variable (e.g.,
local.dns_inputs) to avoid duplication; update the resource to reference that
local for input and for computing result_count (replace the three concat(...)
uses with local.dns_inputs and use length(local.dns_inputs) where needed), and
ensure the ternary conditions remain identical to preserve behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
terraform/aws/ansible_inventory_output.tfterraform/aws/instances.tfterraform/aws/main.tfterraform/aws/services_output.tfterraform/aws/variables.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- terraform/aws/ansible_inventory_output.tf
- terraform/aws/variables.tf
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
byoip_pool_idterraform variable — when set, all instance types get EIPs allocated from the specified IPAM poolUsage
In a devnet tfvars file:
When not set (default
""), behavior is unchanged.Test plan
byoip_pool_idset and verify all instances get 68.67.122.x addressesbyoip_pool_idand verify existing behavior is unchanged🤖 Generated with Claude Code
Summary by CodeRabbit