search

subspace / infra

7 stars 4 forks source link

add auto domain to infra PT.1 #311

Open DaMandal0rian opened 2 months ago

DaMandal0rian commented 2 months ago

User description

The PR adds auto domain to the terraform infra. The changes will be applied to devnet first.

PR Type

enhancement

Description

Changes walkthrough 📝

Relevant files
Configuration changes
variables.tf
Extend domain configuration in variables                                 
resources/devnet/variables.tf
  • Added additional domain ID and label to support 'auto' domain
    alongside 'evm'.
    		•  +2/-2     
    Enhancement
    bootstrap_node_evm_provisioner.tf
    Update bootstrap node provisioner for multiple domains     
    templates/terraform/hetzner/bootstrap_node_evm_provisioner.tf
  • Updated environment variable setup to differentiate between 'EVM' and
    'AUTO' domain configurations.
    		•  +4/-4     
    domain_node_provisioner.tf
    Enhance domain node provisioner for multi-domain support 
    templates/terraform/hetzner/domain_node_provisioner.tf - Modified domain node provisioner to handle multiple domains.
    		+4/-5     
    bootstrap_node_evm_provisioner.tf
    Refactor bootstrap node provisioner for domain clarity     
    templates/terraform/network-primitives/bootstrap_node_evm_provisioner.tf
  • Adjusted domain-related environment variables for clarity and
    separation.
    		•  +4/-4     
    domain_node_provisioner.tf
    Update domain node provisioner for multiple domains           
    templates/terraform/network-primitives/domain_node_provisioner.tf
  • Updated domain node provisioner scripts to support multiple domain IDs
    and labels.
    		•  +4/-4     
    create_bootstrap_node_evm_compose_file.sh
    Add 'auto' domain support in Docker compose script             
    templates/scripts/create_bootstrap_node_evm_compose_file.sh - Added support for 'auto' domain in Docker compose configuration.
    		+14/-2   
    create_domain_node_compose_file.sh
    Extend Docker compose configuration for multiple domains 
    templates/scripts/create_domain_node_compose_file.sh - Extended Docker compose configuration to handle multiple domains.
    		+20/-2   

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    github-actions[bot] commented 2 months ago

    PR Description updated to latest commit (https://github.com/subspace/infra/commit/3d73464781b4e7bdd7f58ffb88a6ee849666ed24)

    github-actions[bot] commented 2 months ago

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5] 3, because the PR involves multiple configuration changes across various files, including Terraform and shell scripts. The changes are not overly complex but require careful review to ensure that the new domain configurations are correctly implemented and that existing configurations are not adversely affected.
    🧪 Relevant tests No
    ⚡ Possible issues Possible Bug: The PR changes the path where environment variables are written from `/root/subspace/.env` to `/home/${var.ssh_user}/subspace/.env` in multiple scripts. This could potentially break scripts or processes that expect these variables to be in the original location unless corresponding changes are made elsewhere to accommodate this new path.
    🔒 Security concerns No
    Code feedback:
    relevant filetemplates/terraform/hetzner/bootstrap_node_evm_provisioner.tf
    suggestion       Consider using a loop or a more dynamic method to handle the environment variable assignments for different domains to reduce redundancy and improve maintainability. This approach can help manage additional domains more efficiently in the future. [important]
    relevant line"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env",
    relevant filetemplates/scripts/create_bootstrap_node_evm_compose_file.sh
    suggestion       Ensure that the port numbers and other unique configurations do not conflict when setting up multiple domains. It might be beneficial to dynamically assign or manage these settings to avoid hard-coded values that could lead to conflicts or misconfigurations. [important]
    relevant lineecho ' "--rpc-listen-on", "0.0.0.0:7944",'
    relevant filetemplates/scripts/create_domain_node_compose_file.sh
    suggestion       For the Traefik configuration, consider abstracting common settings into a separate configuration or script to avoid duplication and facilitate easier updates across multiple domain configurations. [medium]
    relevant line- "traefik.http.routers.archival-node-auto.rule=Host(\`\${DOMAIN_PREFIX}-\${DOMAIN_ID_AUTO}.\${DOMAIN_LABEL_AUTO}.\${NETWORK_NAME}.subspace.network\`) && Path(\`/ws\`)"
    github-actions[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Improve robustness by using lookup instead of hard-coded indices for domain configurations. ___ **Replace the hard-coded array indices with a more robust method of accessing domain
    configuration to prevent potential index out of bounds errors if the configuration
    changes.** [templates/terraform/hetzner/bootstrap_node_evm_provisioner.tf [143-146]](https://github.com/subspace/infra/pull/311/files#diff-fc6db2f194f6b9987d12a71ca29a199035f8e7003588cc724989293da84f52f0R143-R146) ```diff -"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env", -"echo DOMAIN_ID_EVM=${var.domain-node-config.domain-id[0]} >> /home/${var.ssh_user}/subspace/.env", -"echo DOMAIN_LABEL_AUTO=${var.domain-node-config.domain-labels[1]} >> /home/${var.ssh_user}/subspace/.env", -"echo DOMAIN_ID_AUTO=${var.domain-node-config.domain-id[1]} >> /home/${var.ssh_user}/subspace/.env", +"echo DOMAIN_LABEL_EVM=${lookup(var.domain-node-config.domain-labels, "EVM", "")} >> /home/${var.ssh_user}/subspace/.env", +"echo DOMAIN_ID_EVM=${lookup(var.domain-node-config.domain-id, "EVM", 0)} >> /home/${var.ssh_user}/subspace/.env", +"echo DOMAIN_LABEL_AUTO=${lookup(var.domain-node-config.domain-labels, "AUTO", "")} >> /home/${var.ssh_user}/subspace/.env", +"echo DOMAIN_ID_AUTO=${lookup(var.domain-node-config.domain-id, "AUTO", 0)} >> /home/${var.ssh_user}/subspace/.env", ```
    Best practice
    Add validation for domain_labels to ensure they contain only expected values. ___ **Consider validating the domain_labels to ensure they match expected patterns or values,
    enhancing configuration safety and predictability.** [resources/devnet/variables.tf [15]](https://github.com/subspace/infra/pull/311/files#diff-ae828b66c19dd29479737a7a40937ac48a79fdd36e4c3695b65a884a6c129692R15-R15) ```diff default = ["evm", "auto"] +validation { + condition = contains(["evm", "auto", "other"], each.value) + error_message = "The domain label must be one of 'evm', 'auto', or 'other'." +} ```
    Maintainability
    Refactor domain configuration echoing into a function to reduce duplication. ___ **Encapsulate the domain configuration echoing into a function to reduce code duplication
    and improve maintainability.** [templates/scripts/create_bootstrap_node_evm_compose_file.sh [184-187]](https://github.com/subspace/infra/pull/311/files#diff-579c50457274d21a7b133283ca6ad7754890686962990edfeb597b93b725b762R184-R187) ```diff -echo ' "--domain-id", "${DOMAIN_ID_EVM}",' -echo ' "--state-pruning", "archive",' -echo ' "--blocks-pruning", "archive",' -echo ' "--listen-on", "/ip4/0.0.0.0/tcp/${OPERATOR_PORT}",' +function echo_domain_config { + local domain_id=$1 + echo " \"--domain-id\", \"${domain_id}\"," + echo ' "--state-pruning", "archive",' + echo ' "--blocks-pruning", "archive",' + echo ' "--listen-on", "/ip4/0.0.0.0/tcp/${OPERATOR_PORT}",' +} +echo_domain_config "${DOMAIN_ID_EVM}" ```
    Use a loop for domain configuration echoing to simplify code and enhance scalability. ___ **Use a loop to handle the echoing of domain configurations to avoid code repetition and
    facilitate future expansions.** [templates/scripts/create_domain_node_compose_file.sh [184-187]](https://github.com/subspace/infra/pull/311/files#diff-57a235dd3c69f6e6cffe833934963a7f6d5e2e5115dcd1ee0afffe95f3f6ec6aR184-R187) ```diff -echo ' "--domain-id", "${DOMAIN_ID_EVM}",' -echo ' "--state-pruning", "archive",' -echo ' "--blocks-pruning", "archive",' -echo ' "--listen-on", "/ip4/0.0.0.0/tcp/${OPERATOR_PORT}",' +domains=("EVM" "AUTO") +for domain in "${domains[@]}"; do + echo " \"--domain-id\", \"${!domain}_ID\"," + echo ' "--state-pruning", "archive",' + echo ' "--blocks-pruning", "archive",' + echo ' "--listen-on", "/ip4/0.0.0.0/tcp/${OPERATOR_PORT}",' +done ```
    Security
    Sanitize the ssh_user variable to prevent shell command injection. ___ **Ensure the use of secure shell user variables by validating or sanitizing them to prevent
    potential security vulnerabilities.** [templates/terraform/hetzner/domain_node_provisioner.tf [157-158]](https://github.com/subspace/infra/pull/311/files#diff-5706b6156e87e7ce6632980202e39ed0637b1d2afb0cf1253b0cd76fffa2e44cR157-R158) ```diff -"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/${var.ssh_user}/subspace/.env", -"echo DOMAIN_ID_EVM=${var.domain-node-config.domain-id[0]} >> /home/${var.ssh_user}/subspace/.env", +"echo DOMAIN_LABEL_EVM=${var.domain-node-config.domain-labels[0]} >> /home/$(echo ${var.ssh_user} | tr -d ';|&')/subspace/.env", +"echo DOMAIN_ID_EVM=${var.domain-node-config.domain-id[0]} >> /home/$(echo ${var.ssh_user} | tr -d ';|&')/subspace/.env", ```