Add --host flag to docker model install-runner command #215
Conversation
Add clarifying comment about host security for auto-installation Add tests for --host flag in install-runner command Signed-off-by: Eric Curtin <[email protected]>
Reviewer's GuideThis PR introduces a new --host flag to the install-runner command, updating the container creation logic to bind to the specified host instead of a hardcoded address, ensures secure defaults for auto-installation, updates the CLI reference docs, and adds tests for the new flag. Class diagram for updated CreateControllerContainer and install-runner commandclassDiagram
class CreateControllerContainer {
+CreateControllerContainer(ctx, dockerClient, port, host, environment, doNotTrack, gpu, modelStorageVolume, printer, engineKind)
}
class InstallRunnerCommand {
+port: uint16
+host: string
+gpuMode: string
+doNotTrack: bool
+newInstallRunner()
}
InstallRunnerCommand --> CreateControllerContainer: calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a --host flag to the Docker Model Runner install command to allow users to specify the host address for binding the container. The changes enable configuring the network binding while maintaining security defaults.
- Adds
--hostflag with default value "127.0.0.1" to install-runner command - Updates container creation logic to use the configurable host parameter
- Adds comprehensive test coverage for the new flag functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/cli/pkg/standalone/containers.go | Updates CreateControllerContainer function to accept host parameter and conditionally bind bridge gateway IP only for localhost |
| cmd/cli/commands/install-runner.go | Adds host flag to CLI command and passes it to container creation functions |
| cmd/cli/docs/reference/model_install-runner.md | Documents the new --host flag in CLI reference |
| cmd/cli/commands/install-runner_test.go | Adds comprehensive test coverage for the new host flag functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary of ChangesHello @ericcurtin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/install-runner.go:165` </location>
<code_context>
func newInstallRunner() *cobra.Command {
var port uint16
+ var host string
var gpuMode string
var doNotTrack bool
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Defaulting the host flag to '127.0.0.1' is a good security measure, but users may not realize the implications.
Please update the CLI help text to clearly warn users about the security risks of binding to non-localhost addresses.
Suggested implementation:
```golang
var host string
```
```golang
c := &cobra.Command{
```
```golang
// ... other flag definitions ...
c.Flags().StringVar(
&host,
"host",
"127.0.0.1",
"Host address to bind the model runner. WARNING: Binding to non-localhost addresses (e.g., 0.0.0.0) may expose your model runner to the network and pose security risks. Use '127.0.0.1' to restrict access to localhost only.",
)
```
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/install-runner.go:263` </location>
<code_context>
}
c.Flags().Uint16Var(&port, "port", 0,
"Docker container port for Docker Model Runner (default: 12434 for Docker CE, 12435 for Cloud mode)")
+ c.Flags().StringVar(&host, "host", "127.0.0.1", "Host address to bind Docker Model Runner")
c.Flags().StringVar(&gpuMode, "gpu", "auto", "Specify GPU support (none|auto|cuda)")
c.Flags().BoolVar(&doNotTrack, "do-not-track", false, "Do not track models usage in Docker Model Runner")
</code_context>
<issue_to_address>
**🚨 suggestion (security):** CLI flag description for 'host' could clarify security implications.
Consider updating the 'host' flag description to note that using '0.0.0.0' or a public IP will make the service accessible externally, which may pose a security risk.
```suggestion
c.Flags().StringVar(&host, "host", "127.0.0.1", "Host address to bind Docker Model Runner (use '0.0.0.0' or a public IP to allow external access; this may pose a security risk)")
```
</issue_to_address>
### Comment 3
<location> `cmd/cli/commands/install-runner_test.go:88-89` </location>
<code_context>
+ }
+}
+
+func TestInstallRunnerValidArgsFunction(t *testing.T) {
+ cmd := newInstallRunner()
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for command argument rejection.
Please add a test that passes arguments to the command and confirms they are rejected as intended.
```suggestion
func TestInstallRunnerValidArgsFunction(t *testing.T) {
cmd := newInstallRunner()
}
func TestInstallRunnerRejectsArguments(t *testing.T) {
cmd := newInstallRunner()
// Simulate passing an unexpected argument
cmd.SetArgs([]string{"unexpected-arg"})
err := cmd.Execute()
if err == nil {
t.Error("Expected error when passing arguments to install-runner, got nil")
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --host flag to the install-runner command, allowing users to specify the host address for the Docker Model Runner. The changes are well-implemented, defaulting to 127.0.0.1 for security, especially during auto-installation. The implementation correctly propagates the new host parameter down to the container creation logic and includes appropriate updates to documentation and new unit tests. My feedback focuses on improving the robustness of the new tests for clearer failure reporting.
Add clarifying comment about host security for auto-installation
Add tests for --host flag in install-runner command
Summary by Sourcery
Introduce a --host flag to the install-runner command to let users specify the host address for binding the Docker Model Runner, defaulting to localhost for security.
New Features:
Enhancements:
Documentation:
Tests: