Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add auto domain to infra PT.1 #311

Merged
merged 5 commits into from
Aug 27, 2024
Merged

add auto domain to infra PT.1 #311

merged 5 commits into from
Aug 27, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented May 7, 2024

User description

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


PR Type

enhancement


Description

  • Added support for an 'auto' domain alongside the existing 'evm' domain in various Terraform and shell script configurations.
  • Updated environment variable and Docker compose setups to differentiate and handle multiple domain configurations.
  • Enhanced domain node provisioner scripts to support the new domain setup.

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 github-actions bot added the enhancement New feature or request label May 7, 2024
    Copy link

    github-actions bot commented May 7, 2024

    PR Description updated to latest commit (3d73464)

    Copy link

    github-actions bot commented May 7, 2024

    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\`)"

    Copy link

    github-actions bot commented May 7, 2024

    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]

    -"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]

     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]

    -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]

    -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]

    -"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",
     

    @@ -192,7 +192,19 @@ if [ "${enable_domains}" == "true" ]; then
    echo " \"--reserved-nodes\", \"${addr}\"," >> ~/subspace/docker-compose.yml
    echo " \"--bootstrap-nodes\", \"${addr}\"," >> ~/subspace/docker-compose.yml
    done

    # auto domain
    echo ' "--",'
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This is not correct unfotunately

    We would need to create two compose file. One for EVM and one for AutoID
    Our node currently does not support running multiple domains in the single binary

    @DaMandal0rian DaMandal0rian force-pushed the extend-auto-domain branch 2 times, most recently from 8aa7653 to d581e85 Compare May 23, 2024 18:01
    add auto
    
    add DNS records
    
    minor fixes domain label and ID
    
    add operator ID
    
    decouple 2 separate domains for evm and autoid
    
    add more changes to domains
    
    Fix
    
    Fix traefik labels and remove auto extra chains in configs
    
    fix merge conflict
    Copy link
    Contributor

    @vedhavyas vedhavyas left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This PR changes can be much smaller if we merged the nova and autoID config. This will pave way for much more composable infra for future domains too

    @@ -37,6 +37,23 @@ module "devnet" {
    disk-volume-type = var.disk_volume_type
    }

    bootstrap-node-autoid-config = {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I was actually hoping we can merge the nova and autoID in to a single type. There is not much difference except domain-id

    @@ -0,0 +1,176 @@
    #!/bin/bash
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Why not use the EVM script ?
    There is no difference in them

    @DaMandal0rian DaMandal0rian merged commit c190aac into main Aug 27, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the extend-auto-domain branch August 27, 2024 13:45
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants