Skip to content

Add bindIp support for network servers #498

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

BelfordZ
Copy link
Contributor

@BelfordZ BelfordZ commented May 22, 2025

User description

Summary

  • add externalBindIp and internalBindIp to IP configuration
  • use bind IPs for external and internal servers
  • document bind IP fields in NodeIpInfo and examples

Testing

  • npm test

PR Type

Enhancement, Documentation


Description

  • Added externalBindIp and internalBindIp to server IP configuration.

  • Updated network server setup to use bind IPs.

  • Documented new bind IP fields in types and examples.

  • Enhanced configuration and reporting documentation for bind IPs.


Changes walkthrough 📝

Relevant files
Enhancement
server.ts
Add bind IP fields to server configuration defaults           

src/config/server.ts

  • Added externalBindIp and internalBindIp to default server IP config.
  • Ensured both fields are initialized with default values.
  • +2/-0     
    index.ts
    Use bind IPs for network server setup and config                 

    src/network/index.ts

  • Extended IPInfo interface with bind IP fields and documentation.
  • Modified external server setup to use externalBindIp if provided.
  • Modified internal server setup to use internalBindIp if provided.
  • Updated initialization logic to include bind IPs in ipInfo.
  • +21/-7   
    shardus-types.ts
    Add bind IP fields to server configuration types                 

    src/shardus/shardus-types.ts

  • Added externalBindIp and internalBindIp to ServerConfiguration.ip.
  • Documented new bind IP fields in type definitions.
  • +4/-0     
    Documentation
    lost-detection-planning.txt
    Document bind IPs in node config example                                 

    docs/lost-detection-planning.txt

  • Added externalBindIp and internalBindIp to node configuration example.
  • +2/-0     
    reporting.md
    Document bind IP fields in NodeIpInfo                                       

    docs/reporting.md

  • Added externalBindIp and internalBindIp to NodeIpInfo documentation.
  • +2/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 93
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Optional Parameter Handling

    The new externalBindIp and internalBindIp parameters are used directly in server binding. Ensure that these values are always defined (i.e., never undefined) at runtime, or provide safe defaults to avoid runtime errors when not set in configuration.

    this.extServer = this.app.listen(
      this.ipInfo.externalPort,
      this.ipInfo.externalBindIp,
      () => {
        const msg = `External server running on port ${this.ipInfo.externalPort}...`
        console.log(msg)
        this.mainLogger.info('Network: ' + msg)
      }
    )
    Documentation Consistency

    The new bind IP fields are documented in several places. Double-check that all user-facing documentation, config examples, and type definitions are consistent and clear about the purpose and usage of externalBindIp and internalBindIp.

      /** Optional IP/interface to bind the external server */
      externalBindIp?: string
      /** Optional IP/interface to bind the internal server */
      internalBindIp?: string
    }

    Comment on lines +170 to +178
    this.extServer = this.app.listen(
    this.ipInfo.externalPort,
    this.ipInfo.externalBindIp,
    () => {
    const msg = `External server running on port ${this.ipInfo.externalPort}...`
    console.log(msg)
    this.mainLogger.info('Network: ' + msg)
    }
    )

    Choose a reason for hiding this comment

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

    Suggestion: If this.ipInfo.externalBindIp is undefined, passing it as the second argument to listen may cause unexpected behavior or errors. Add logic to only include the bind IP if it is defined. [possible issue, importance: 8]

    Suggested change
    this.extServer = this.app.listen(
    this.ipInfo.externalPort,
    this.ipInfo.externalBindIp,
    () => {
    const msg = `External server running on port ${this.ipInfo.externalPort}...`
    console.log(msg)
    this.mainLogger.info('Network: ' + msg)
    }
    )
    this.extServer = this.ipInfo.externalBindIp
    ? this.app.listen(
    this.ipInfo.externalPort,
    this.ipInfo.externalBindIp,
    () => {
    const msg = `External server running on port ${this.ipInfo.externalPort}...`
    console.log(msg)
    this.mainLogger.info('Network: ' + msg)
    }
    )
    : this.app.listen(
    this.ipInfo.externalPort,
    () => {
    const msg = `External server running on port ${this.ipInfo.externalPort}...`
    console.log(msg)
    this.mainLogger.info('Network: ' + msg)
    }
    )

    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