Skip to content

Conversation

@keivenchang
Copy link
Contributor

@keivenchang keivenchang commented Sep 25, 2025

Overview:

Add network isolation modes to container run script with comprehensive configuration options. This enables secure CI/CD pipelines and flexible networking for different deployment scenarios.

Details:

  • Add --network flag to run.sh supporting host, bridge, none, and container sharing modes
  • Update container README with detailed network configuration documentation and use cases
  • Add CI/CD workflow examples demonstrating bridge networking for test isolation
  • Document performance/security tradeoffs and limitations for each network mode
  • Include comparison table and warnings for restricted functionality modes

Where should the reviewer start?

  • container/run.sh - New network flag implementation and argument parsing
  • container/README.md - Comprehensive network configuration documentation and examples

/coderabbit profile chill

Summary by CodeRabbit

  • New Features

    • Added a configurable networking option to the container runner with modes: host (default), bridge, none, and container sharing. Help text updated with usage and examples.
  • Documentation

    • Expanded network configuration guidance with detailed use cases and examples for each mode, including custom networks.
    • Updated usage examples to be network-aware; removed outdated GPU-focused example.
    • Introduced a CI/CD Workflow section emphasizing network isolation and in-container integration tests.
    • Augmented examples for workspace mounting, volume/cache flags, and a multi-service startup sequence.

- Add --network flag to run.sh supporting host, bridge, none, and container sharing
- Update README with comprehensive network configuration documentation
- Add CI/CD workflow examples with bridge networking for isolation
- Document limitations and use cases for each network mode

Signed-off-by: Keiven Chang <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 25, 2025

Walkthrough

Adds a configurable --network option to container/run.sh (default: host) and updates container/README.md with expanded network configuration documentation, examples, and CI/CD workflow guidance. Docker run now uses the selected network value instead of a hardcoded host network.

Changes

Cohort / File(s) Summary
Documentation: Network & CI/CD updates
container/README.md
Rewrites network configuration section with modes (host, bridge, none, container:name, custom), replaces prior examples with network-aware usage, adds CI/CD workflow emphasizing network isolation, updates examples for mounts/volumes/cache, includes multi-service startup sequence.
Script: Network option in runner
container/run.sh
Introduces NETWORK variable (default host), parses --network <mode> in get_options, expands help to document values and examples, and uses --network "$NETWORK" in docker run.

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant run.sh as run.sh
  participant Docker as docker run

  User->>run.sh: Invoke with args (e.g., --network bridge)
  run.sh->>run.sh: Parse options (get_options)
  run.sh-->>run.sh: Set NETWORK (default host or provided value)
  run.sh->>Docker: docker run --network "$NETWORK" ...
  Docker-->>run.sh: Container started with selected network
  run.sh-->>User: Exit with status
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

In wires and waves, I thump with glee,
Flip host to bridge—a hop for me!
CI drums hum, containers align,
A carrot-shaped ping on every line.
With NETWORK set, I bound and run—
Docked and linked, the job is done. 🥕🐇

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description follows the repository template for Overview, Details, and Where to start sections but omits the required “Related Issues” section with an action keyword and issue reference, making it incomplete against the specified template. Please add a “#### Related Issues:” section listing any closing or related issues using an action keyword (e.g., “closes #123”) to fully comply with the repository’s description template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change of adding network isolation modes to the container/run.sh script, matching the core implementation and its intent. It is concise, specific, and clearly communicates what the pull request introduces without ambiguous or extraneous phrasing. While it omits documentation updates, titles need only highlight the main feature. Therefore, it effectively summarizes the most important change from the developer’s perspective.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- Add warning about host networking port sharing (only one NATS/etcd instance)
- Update network mode comparison table with Port Sharing column
- Fix production workflow to use non-local-dev image (runs as root)
- Improve CI/CD examples to use appropriate image types
- Clarify bridge networking setup instructions for full stack testing

Signed-off-by: Keiven Chang <[email protected]>
Copy link
Contributor

@kthui kthui left a comment

Choose a reason for hiding this comment

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

Nice! We have network isolation to the container.

One thing people might ask in the future is adding port publish

-p HOST_PORT:CONTAINER_PORT

because the host or other containers no longer have access to network services at the container if bridge mode is used, so network service ports will need to be explicitly published, but I think this can be done in a separate PR given the default option is still "host".

…D workflow

- Add detailed port sharing limitations and warnings for host networking
- Restructure CI/CD workflow into clear numbered steps
- Add comprehensive framework-specific GPU memory configuration
- Include inline argument explanations for vLLM, SGLang, and TensorRT-LLM
- Improve code block formatting with proper bash syntax highlighting
- Remove redundant sections and streamline documentation flow
- Add bridge networking isolation benefits and setup instructions

Signed-off-by: Keiven Chang <[email protected]>
- Unify CI/CD workflow format to match Development/Production workflows
- Use single code block with numbered comments for consistency
- Remove framework bias by treating vLLM, SGLang, TensorRT-LLM equally
- Uncomment all framework alternatives for user choice
- Consolidate framework-specific arguments into compact summary
- Improve documentation consistency across all workflow sections

Signed-off-by: Keiven Chang <[email protected]>
- Add --port|-p option to run.sh for Docker port mapping
- Integrate PORT_MAPPINGS variable into docker run command
- Update help text with port mapping documentation
- Add cache volume mount (-v /home/ubuntu/.cache:/home/ubuntu/.cache) to all examples
- Simplify bridge networking examples and remove redundant port examples
- Add port sharing note with host_port:container_port format clarification
- Enhance network mode comparison table with port publishing column
- Standardize container examples across all networking modes

Signed-off-by: Keiven Chang <[email protected]>
@keivenchang
Copy link
Contributor Author

Nice! We have network isolation to the container.

One thing people might ask in the future is adding port publish

-p HOST_PORT:CONTAINER_PORT

because the host or other containers no longer have access to network services at the container if bridge mode is used, so network service ports will need to be explicitly published, but I think this can be done in a separate PR given the default option is still "host".

Ok, I just added the --port|-p option and mentioned in the README.md about it.

@keivenchang keivenchang changed the title feat: add network isolation modes to container run script feat: add network isolation modes to container/run.sh script Sep 25, 2025
@keivenchang keivenchang merged commit a0f845e into main Sep 26, 2025
18 of 19 checks passed
@keivenchang keivenchang deleted the keivenchang/run.sh_add-network-isolation-mode branch September 26, 2025 04:50
saturley-hall pushed a commit that referenced this pull request Oct 3, 2025
Signed-off-by: Keiven Chang <[email protected]>
Signed-off-by: Harrison King Saturley-Hall <[email protected]>
saturley-hall added a commit that referenced this pull request Oct 3, 2025
…3402)

Signed-off-by: Keiven Chang <[email protected]>
Signed-off-by: Harrison King Saturley-Hall <[email protected]>
Co-authored-by: Keiven C <[email protected]>
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.

4 participants