Make miren server docker install handle taken ports gracefully#867
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/server_install_docker_test.go`:
- Around line 181-199: The Docker port conflict tests only verify that
dockerPortConflictError recognizes the error string, but they do not assert the
translated user-facing guidance. Update TestDockerPortConflictError to also
check the returned error/output from dockerPortConflictError and confirm it
includes the friendly 443 conflict messaging, such as the port-443-in-use
guidance and the suggested --ingress-mode behind-proxy-http path, using the
existing ctx helper and dockerPortConflictError symbol.
- Around line 147-152: The render helper in server_install_docker_test.go is
using the outer test handle instead of the active subtest, so its assertion
failures are reported against the wrong test. Update render to accept the
current *testing.T from the calling subtest, call t.Helper() inside render, and
use that t for require.Error so failures are attributed to the correct subtest.
In `@cli/commands/server_install_docker.go`:
- Around line 374-399: The host-network path in publishedPorts() is skipping the
new preflight because it returns nil, so preflightPortCheck() never validates
the ports Miren still needs. Split the required host ports from the Docker
publish list in server_install_docker.go, and update
publishedPorts()/preflightPortCheck() so --host-network still checks 80, 443,
and 8443 early while only omitting Docker port mappings.
- Around line 551-563: The Docker bind-conflict fallback in
dockerPortConflictError currently treats all port errors the same and always
prints the behind-proxy-http advice, which loses the port-specific remediation
from portConflict(). Update dockerPortConflictError to inspect the conflicting
host port from the error, then branch to the same per-role guidance used by
portConflict() so 80, 443, and 8443 each get the correct message and suggested
fix.
- Line 30: The ingress-mode help text is misleading because it implies both
behind-proxy-http and behind-proxy-https are for TLS-terminating proxies, but
only behind-proxy-http should be described that way. Update the IngressMode
description in server_install_docker.go and any generated command docs to
clearly state that behind-proxy-https still terminates TLS in Miren via
dockerIngressArgs() and uses :443, while behind-proxy-http is the mode for
nginx/tailscale-style TLS-terminating proxies.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b96a233-bd8c-4e30-b2cd-f72b82d59fd7
📒 Files selected for processing (5)
cli/commands/commands.gocli/commands/server_install_docker.gocli/commands/server_install_docker_test.godocs/docs/command/server-docker-install.mddocs/docs/tls.md
This command published its ports by running `docker run` and hoping for the best. If something already held 443 (the reporter runs `tailscale serve` there), the install died with a raw docker daemon stack trace, and only after the user had sat through cloud registration. Add a pre-flight check that verifies every host port we're about to publish is free before any expensive setup, failing with guidance tailored to which port conflicted. A taken HTTP port can move with --http-port; a taken 443 is best handed to a TLS-terminating proxy, so we wire --ingress-mode behind-proxy-http into the installer. Miren then serves plain HTTP and lets the proxy own 443, turning "tailscale already has 443" from a dead end into a one-flag fix. A backstop translates docker's own conflict error into the same guidance for cases the pre-flight can't see (an unprivileged CLI can't test-bind ports < 1024). Deliberately absent is a --https-port flag, which the issue asked for: moving HTTPS off 443 silently breaks automatic HTTPS since browsers expect it there. behind-proxy mode is the honest answer. Closes MIR-1241
3756d88 to
4ee7714
Compare
evanphx
left a comment
There was a problem hiding this comment.
Great checking and feedback
Installing the Miren server via Docker assumed it could grab ports 80, 443, and 8443. If something already held one of them,
docker runfailed with a raw daemon error likePorts are not available: ... bind: address already in use, and you'd only hit it after sitting through cloud registration. The person who filed MIR-1241 runstailscale serveon 443, so this was a hard wall on a pretty reasonable setup.The fix has three parts. First, a pre-flight check runs before any expensive setup and verifies every port we're about to publish is actually free, failing with guidance tailored to which port conflicted. A taken HTTP port can just be moved with
--http-port. A taken 443 is a different story: moving HTTPS off the port browsers expect would quietly break the automatic-HTTPS promise, so the right move there isn't relocation, it's letting whatever already owns 443 keep it.That's why the second part wires
--ingress-mode behind-proxy-httpinto the docker installer.tailscale serve(and nginx, Caddy, Cloudflare Tunnel) are TLS-terminating proxies, so the architecturally-correct answer is to run Miren behind them: Miren serves plain HTTP, the proxy owns 443 and TLS, and there's no conflict to fight over. The friendly error now hands you that exact command, turning a dead end into a one-flag fix.Third, a backstop translates docker's own port-conflict error into the same guidance, covering the cases the pre-flight can't see (an unprivileged CLI can't test-bind ports below 1024, so it leans on docker to discover the clash there).
One deliberate non-change worth calling out: the issue literally asked for an
--https-portflag, mirroring--http-port. We didn't add it. It looks like a tidy parallel but it's a footgun, since moving HTTPS to a nonstandard port silently breaks the thing people install Miren for. behind-proxy mode is the honest version of "443 is taken, now what."Closes MIR-1241