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 docker compose manifest for ansible PT.3 #314

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented May 7, 2024

User description

Extend docker compose manifests to include auto domain. This continues on #311 and #312


PR Type

enhancement, configuration changes


Description

  • Added and updated Docker Compose configurations to support new domain-specific settings for both EVM and AUTO domains.
  • Configured state and blocks pruning as 'archive' for enhanced node management.
  • Updated Traefik routing and service configurations to handle new domain-specific endpoints securely.

Changes walkthrough 📝

Relevant files
Configuration changes
docker-compose-bootstrap-domain.yml
Enhance Docker Compose Configurations for Bootstrap Domains

ansible/network/files/docker-compose-bootstrap-domain.yml

  • Added configurations for state and blocks pruning set to 'archive'.
  • Introduced new domain IDs and updated network configurations for both
    EVM and AUTO domains.
  • Configured new RPC settings and node reservations for bootstrap nodes.
  • +24/-0   
    docker-compose-domain.yml
    Update Traefik Configurations and Domain IDs in Docker Compose

    ansible/network/files/docker-compose-domain.yml

  • Updated Traefik routing rules to include new domain IDs for EVM and
    AUTO.
  • Added new Traefik configurations for the AUTO domain.
  • Updated domain IDs in existing configurations to specify EVM.
  • +21/-2   

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

    Copy link

    github-actions bot commented May 7, 2024

    PR Description updated to latest commit (500bbf8)

    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 different files and domains, which requires a detailed review to ensure consistency and correctness in the configurations.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Redundancy: The configuration for state and blocks pruning is repeated for both EVM and AUTO domains. This could be simplified or parameterized to avoid redundancy and potential errors in future updates.

    Configuration Clarity: The use of environment variables like ${DOMAIN_ID_EVM} and ${DOMAIN_ID_AUTO} needs clear documentation or examples to ensure they are set correctly in the environment.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileansible/network/files/docker-compose-bootstrap-domain.yml
    suggestion      

    Consider parameterizing the repeated configurations for state and blocks pruning to reduce redundancy and potential errors. This can be achieved by defining these settings in a separate, included YAML file or by using a templating mechanism to apply the settings across different domains. [important]

    relevant line"--state-pruning", "archive"

    relevant fileansible/network/files/docker-compose-domain.yml
    suggestion      

    Add explicit environment variable definitions or checks to ensure that ${DOMAIN_ID_EVM} and ${DOMAIN_ID_AUTO} are defined before they are used in the configurations. This can prevent runtime errors due to undefined variables. [important]

    relevant line"--domain-id", "${DOMAIN_ID_EVM}"

    relevant fileansible/network/files/docker-compose-domain.yml
    suggestion      

    Consider adding health checks for the services defined in the Docker Compose files to ensure that they are running correctly and to facilitate easier debugging and monitoring. [medium]

    relevant line- "traefik.http.services.archival-node.loadbalancer.server.port=8944"

    Copy link

    github-actions bot commented May 7, 2024

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Security
    Enhance security by restricting the CORS policy to specific domains.

    Consider using a more restrictive setting for --rpc-cors to enhance security. Using 'all'
    allows any website to make requests to your service, which might expose sensitive data or
    lead to other security vulnerabilities.

    ansible/network/files/docker-compose-bootstrap-domain.yml [150]

    -"--rpc-cors", "all",
    +"--rpc-cors", "domain-specific.com",
     
    Improve security by binding the RPC server to specific network interfaces.

    The --rpc-listen-on configuration is set to listen on all interfaces (0.0.0.0). It's
    recommended to bind to specific interfaces to reduce the risk of exposing the RPC server
    to potentially malicious traffic.

    ansible/network/files/docker-compose-bootstrap-domain.yml [151]

    -"--rpc-listen-on", "0.0.0.0:8944",
    +"--rpc-listen-on", "192.168.1.1:8944",  # Assuming 192.168.1.1 is the intended interface
     
    Maintainability
    Remove duplicate configuration entries to avoid potential errors.

    The --state-pruning and --blocks-pruning options are duplicated in the configuration.
    Ensure that these settings are intended to be repeated, or consider removing the
    duplicates to avoid configuration errors.

    ansible/network/files/docker-compose-bootstrap-domain.yml [146-159]

     "--state-pruning", "archive",
    -"--blocks-pruning", "archive",
     
    Best practice
    Ensure consistent HTTPS redirection by applying the middleware to all routers.

    The middleware redirect-https is added to the archival-node-auto router but not to the
    archival-node router. To maintain consistency and ensure all traffic is redirected to
    HTTPS, consider adding this middleware to all relevant routers.

    ansible/network/files/docker-compose-domain.yml [97]

    +- "traefik.http.routers.archival-node.middlewares=redirect-https"
     - "traefik.http.routers.archival-node-auto.middlewares=redirect-https"
     
    Possible issue
    Standardize server ports across configurations to prevent misconfigurations.

    The load balancer server port configuration for archival-node-auto is set to 7944, which
    differs from the standard port 8944 used in other configurations. Verify if this is
    intentional and consider standardizing ports for uniformity and to avoid misconfiguration.

    ansible/network/files/docker-compose-domain.yml [98]

    -- "traefik.http.services.archival-node-auto.loadbalancer.server.port=7944"
    +- "traefik.http.services.archival-node-auto.loadbalancer.server.port=8944"
     

    @DaMandal0rian DaMandal0rian self-assigned this May 7, 2024
    @DaMandal0rian DaMandal0rian merged commit 10e8d01 into main Jul 30, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the ansible-extend-auto branch July 30, 2024 13:00
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant