Extend ollama connection on linux, extend docs for integration#60
Merged
Extend ollama connection on linux, extend docs for integration#60
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Docker host name resolution mapping for proxy containers, documents Linux-specific Ollama binding and startup/systemd guidance to allow Docker access, and updates a test to assert the new container Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vibepod/core/docker.py (1)
297-323:⚠️ Potential issue | 🟠 MajorRecreate stale running proxy containers that lack
extra_hostsLine 299 returns an already-running proxy without validating config. After upgrading, old containers can keep running without the new
host.docker.internalmapping, so the connectivity fix may never apply until manual removal.Proposed fix
def ensure_proxy(self, image: str, db_path: Path, ca_dir: Path, network: str) -> Any: existing = self.find_proxy() if existing: - if existing.status == "running": - return existing + existing.reload() + extra_hosts = ( + existing.attrs.get("HostConfig", {}).get("ExtraHosts", []) + if hasattr(existing, "attrs") + else [] + ) + has_host_mapping = any( + str(entry).startswith("host.docker.internal:") + for entry in (extra_hosts or []) + ) + if existing.status == "running" and has_host_mapping: + return existing existing.remove(force=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vibepod/core/docker.py` around lines 297 - 323, The current logic returns an existing proxy container if existing.status == "running" without validating that its runtime config includes the required extra_host mapping; update the startup path that uses find_proxy()/existing to inspect the running container's HostConfig ExtraHosts (via existing.attrs['HostConfig']['ExtraHosts'] or equivalent) and if it does not contain an entry for "host.docker.internal" (or the expected "host-gateway" mapping), treat the container as stale and remove it (existing.remove(force=True)) before creating a new one with run_kwargs that include extra_hosts; ensure this check occurs before the early return so old containers lacking the host mapping are recreated.
🧹 Nitpick comments (2)
tests/test_proxy_permissions.py (1)
54-88: Add a regression test for pre-existing running proxy containersNice assertion for new runs. Please also add a case where
find_proxy()returns a running container withouthost.docker.internalinHostConfig.ExtraHosts, and verifyensure_proxy()recreates it. That would lock in upgrade safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_proxy_permissions.py` around lines 54 - 88, Add a regression test that simulates find_proxy() returning an existing running container whose attrs.HostConfig.ExtraHosts does not contain "host.docker.internal" and verify ensure_proxy() recreates it: create a fake container class with attributes id, status="running", attrs={"HostConfig": {"ExtraHosts": []}} and a remove() spy, monkeypatch DockerManager.find_proxy to return that instance, set manager.client.containers.run to record calls, then call DockerManager.ensure_proxy(...) and assert the original container.remove() was called, the new container was started via containers.run (run_kwargs not None), and run_kwargs["extra_hosts"] includes {"host.docker.internal": "host-gateway"} (also keep the existing assertions like user and absence of ports and that db_path.parent exists).docs/llm.md (1)
25-56: Address markdownlint MD046 warning in the new admonition blockThis section currently triggers MD046 (“code block style”). If the project enforces markdownlint, please either restructure this block to satisfy MD046 or add a targeted rule suppression for this admonition section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/llm.md` around lines 25 - 56, The new Ollama admonition triggers markdownlint MD046 (code block style); either make all code blocks in that admonition use a consistent fenced style and language tags (convert any indented/inline blocks to fenced triple-backtick blocks for the bash snippets like "OLLAMA_HOST=0.0.0.0 ollama serve", the systemctl commands, the ini block containing Environment="OLLAMA_HOST=0.0.0.0", and the ss check) or wrap the whole admonition with a targeted markdownlint suppression by adding <!-- markdownlint-disable MD046 --> before the block and <!-- markdownlint-enable MD046 --> after it so the linter ignores this section. Ensure the chosen change is applied to the same admonition that contains the ollama serve, sudo systemctl edit ollama, the ini Environment line, and the ss -tlnp | grep 11434 example.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/llm.md`:
- Line 23: The sentence "On Linux, Ollama binds to `127.0.0.1` by default.
Docker containers reach the host via a different network interface
(`172.17.0.1`), so the default binding will refuse connections." hard-codes a
single gateway IP; update that sentence in docs/llm.md to avoid a specific
static IP and instead refer to the Docker bridge gateway generically (e.g.,
"Docker bridge gateway (for example `172.17.0.1` on the default Docker
bridge)"), so replace the parenthetical IP with wording like "for example
`172.17.0.1` on the default Docker bridge" or similar to indicate it is
setup-dependent.
---
Outside diff comments:
In `@src/vibepod/core/docker.py`:
- Around line 297-323: The current logic returns an existing proxy container if
existing.status == "running" without validating that its runtime config includes
the required extra_host mapping; update the startup path that uses
find_proxy()/existing to inspect the running container's HostConfig ExtraHosts
(via existing.attrs['HostConfig']['ExtraHosts'] or equivalent) and if it does
not contain an entry for "host.docker.internal" (or the expected "host-gateway"
mapping), treat the container as stale and remove it
(existing.remove(force=True)) before creating a new one with run_kwargs that
include extra_hosts; ensure this check occurs before the early return so old
containers lacking the host mapping are recreated.
---
Nitpick comments:
In `@docs/llm.md`:
- Around line 25-56: The new Ollama admonition triggers markdownlint MD046 (code
block style); either make all code blocks in that admonition use a consistent
fenced style and language tags (convert any indented/inline blocks to fenced
triple-backtick blocks for the bash snippets like "OLLAMA_HOST=0.0.0.0 ollama
serve", the systemctl commands, the ini block containing
Environment="OLLAMA_HOST=0.0.0.0", and the ss check) or wrap the whole
admonition with a targeted markdownlint suppression by adding <!--
markdownlint-disable MD046 --> before the block and <!-- markdownlint-enable
MD046 --> after it so the linter ignores this section. Ensure the chosen change
is applied to the same admonition that contains the ollama serve, sudo systemctl
edit ollama, the ini Environment line, and the ss -tlnp | grep 11434 example.
In `@tests/test_proxy_permissions.py`:
- Around line 54-88: Add a regression test that simulates find_proxy() returning
an existing running container whose attrs.HostConfig.ExtraHosts does not contain
"host.docker.internal" and verify ensure_proxy() recreates it: create a fake
container class with attributes id, status="running", attrs={"HostConfig":
{"ExtraHosts": []}} and a remove() spy, monkeypatch DockerManager.find_proxy to
return that instance, set manager.client.containers.run to record calls, then
call DockerManager.ensure_proxy(...) and assert the original container.remove()
was called, the new container was started via containers.run (run_kwargs not
None), and run_kwargs["extra_hosts"] includes {"host.docker.internal":
"host-gateway"} (also keep the existing assertions like user and absence of
ports and that db_path.parent exists).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6710f1cb-96c2-4c5b-91b1-b37d6423e13b
📒 Files selected for processing (3)
docs/llm.mdsrc/vibepod/core/docker.pytests/test_proxy_permissions.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/llm.md`:
- Around line 25-56: The docs currently instruct setting OLLAMA_HOST=0.0.0.0 but
lack a security warning; update the llm.md content around the
"OLLAMA_HOST=0.0.0.0" examples to add a clear caution that binding to 0.0.0.0
exposes Ollama to all interfaces and may be unsafe on public networks, and
provide alternatives: recommend using 0.0.0.0 only on trusted networks or behind
a firewall, suggest binding to a specific interface (e.g., the Docker bridge IP)
instead of 0.0.0.0, and mention configuring network-level access controls or
authentication before doing so—place this warning immediately after the
manual/systemd examples referencing OLLAMA_HOST to ensure visibility.
- Line 54: The example command "ss -tlnp | grep 11434" uses the -p flag which
requires elevated privileges; update the docs to either prefix with sudo ("sudo
ss -tlnp | grep 11434") or drop the -p flag ("ss -tln | grep 11434"), and add a
short parenthetical note explaining that process names shown by -p require
root/sudo to avoid confusing readers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60c403b5-e7ef-4d38-b88b-3a128bda795d
📒 Files selected for processing (3)
docs/llm.mdsrc/vibepod/core/docker.pytests/test_proxy_permissions.py
✅ Files skipped from review due to trivial changes (1)
- src/vibepod/core/docker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_proxy_permissions.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/llm.md`:
- Around line 73-75: Update the guidance so it does not conflate CORS with
authentication: remove or reword the sentence that groups OLLAMA_ORIGINS with
"Add authentication" and instead state that OLLAMA_ORIGINS is only a
CORS/origins control, not an auth mechanism; explicitly recommend using a
reverse proxy (e.g., nginx/traefik) with proper authentication or network
ACLs/firewall rules to protect the service, and note that API-key support is
only available for Ollama's cloud API (not the local server). Reference the
OLLAMA_ORIGINS variable and the general "Add authentication" guidance (reverse
proxy/API-key mentions) so the change is applied to that exact sentence in
docs/llm.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45228d8a-a788-4507-9f21-0929b2ad8311
📒 Files selected for processing (3)
docs/llm.mdsrc/vibepod/core/docker.pytests/test_proxy_permissions.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_proxy_permissions.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/vibepod/core/docker.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/llm.md (1)
59-80: LGTM! Comprehensive security guidance.The security warning appropriately addresses the risks of binding to
0.0.0.0, and the three safer alternatives are well-explained:
- Binding to the Docker bridge gateway IP is a sound recommendation
- Firewall rules provide network-level protection
- The clarification that
OLLAMA_ORIGINSis CORS control (not authentication) is accurate and prevents confusionThe note that local Ollama has no built-in auth and the reverse proxy recommendation are both correct.
💡 Optional: Add specific command to extract bridge gateway IP
For users following the safer alternative to bind to the Docker bridge gateway (lines 66-69), you could add a concrete example command to extract the gateway IP:
- **Bind to the Docker bridge gateway only** (e.g., `OLLAMA_HOST=172.17.0.1`) so only containers on the default Docker bridge can reach Ollama while the service remains unreachable from other interfaces. Find your bridge gateway IP: ```bash docker network inspect bridge -f '{{(index .IPAM.Config 0).Gateway}}'This is optional—the current "substitute the actual gateway IP" guidance is adequate for users choosing this advanced option. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/llm.mdaround lines 59 - 80, Add a concrete example command to the
"Bind to the Docker bridge gateway only" bullet in the "Security: binding to
0.0.0.0exposes Ollama on all interfaces" warning: update the docs/llm.md
warning block to include a short, copy-pasteable docker network inspect command
(with its template flag) that shows how to extract the bridge gateway IP so
users can easily substitute the gateway (mention that it uses docker network
inspect bridge with a template to print the Gateway).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/llm.md:
- Around line 59-80: Add a concrete example command to the "Bind to the Docker
bridge gateway only" bullet in the "Security: binding to0.0.0.0exposes
Ollama on all interfaces" warning: update the docs/llm.md warning block to
include a short, copy-pasteable docker network inspect command (with its
template flag) that shows how to extract the bridge gateway IP so users can
easily substitute the gateway (mention that it uses docker network inspect
bridge with a template to print the Gateway).</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `f747bb02-2c15-46ac-a3b1-de116c566718` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between eb364a41c5a9f60116346a8d1459bc6f4cba723a and cd6b467a72f290ebc2fa437bbc77a9cf82704ce1. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `docs/llm.md` * `src/vibepod/core/docker.py` * `tests/test_proxy_permissions.py` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * tests/test_proxy_permissions.py </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/vibepod/core/docker.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves Docker networking support for Ollama integration and updates related documentation and tests. The main focus is to ensure that Docker containers can reliably connect to Ollama on Linux systems by configuring the correct network interface and adding the necessary Docker options.
Documentation updates:
0.0.0.0) for Linux users, including instructions for both manual and systemd-managed setups. This ensures Docker containers can connect to Ollama, addressing common connectivity issues.Docker networking improvements:
extra_hosts={"host.docker.internal": "host-gateway"}, enabling containers to resolve the host machine ashost.docker.internaland connect to services running on the host.Summary by CodeRabbit
Documentation
Bug Fixes
Tests