Add ingress.http_address for plain-HTTP ingress on a custom address#789
Add ingress.http_address for plain-HTTP ingress on a custom address#789jcasimir wants to merge 3 commits into
Conversation
Introduce a new [ingress] section for ingress-listener settings and add its first field, http_address. When set, it directs a plain-HTTP ingress to a host:port — intended for deployments behind a TLS-terminating proxy (nginx, Caddy, an ALB) where Miren itself shouldn't speak TLS. Schema-only; no behavior change yet — the field is plumbed through TOML, env (MIREN_INGRESS_HTTP_ADDRESS), and CLI (--ingress-http-address), and validated as host:port at config load. The runtime branch that consumes it lands in the next commit. The codegen output (*.gen.go) is regenerated from schema.yml via go generate; do not hand-edit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an ingress HTTP option and wiring throughout the codebase: a new Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/commands/server_test.go (1)
14-98: ⚡ Quick winAdd a test case asserting
tls.standard_tlsdoes not produce a warning.The PR description explicitly calls out: "tls.standard_tls is intentionally excluded from warnings." None of the table cases verifies this invariant. If the exclusion is accidentally dropped from
warnIngressTLSOverride, every existing test would still pass.✅ Proposed additional test case
{ name: "all warn together, each named individually", ... }, + { + name: "standard_tls set: no warning emitted", + notWantContain: []string{"standard_tls", "ignored"}, + // StandardTLS is set directly below after TLSConfig construction + },Then in the loop body, add a branch for the new case (or expose
standardTLS boolin the table struct) that callstls.SetStandardTLS(true)and asserts no warning is emitted.🤖 Prompt for 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. In `@cli/commands/server_test.go` around lines 14 - 98, Add a test case to ensure tls.standard_tls is excluded from warnings: extend the test table in TestWarnIngressTLSOverride with a case (e.g., name "standard_tls silent") that sets standardTLS true, then in the t.Run setup call tls.SetStandardTLS(true) before invoking warnIngressTLSOverride(log, tls) and assert the output does NOT contain "tls.standard_tls" (add it to notWantContain); this ensures warnIngressTLSOverride still omits standard_tls.
🤖 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.go`:
- Around line 832-853: The ingress server goroutines are started detached and
can keep serving after startup failures; replace the two detached go funcs with
eg.Go calls so their lifecycles propagate through the errgroup `eg`: use eg.Go
to run the Serve call for `ingressServer` (returning the error unless it's
http.ErrServerClosed) and use another eg.Go to wait on `sub.Done()` and call
`ingressServer.Shutdown(shutdownCtx)` (returning any shutdown error),
referencing `ingressServer`, `ln`, `ingressAddr`, `sub`, and `eg` so the
errgroup observes Serve/shutdown errors instead of only logging them.
---
Nitpick comments:
In `@cli/commands/server_test.go`:
- Around line 14-98: Add a test case to ensure tls.standard_tls is excluded from
warnings: extend the test table in TestWarnIngressTLSOverride with a case (e.g.,
name "standard_tls silent") that sets standardTLS true, then in the t.Run setup
call tls.SetStandardTLS(true) before invoking warnIngressTLSOverride(log, tls)
and assert the output does NOT contain "tls.standard_tls" (add it to
notWantContain); this ensures warnIngressTLSOverride still omits standard_tls.
🪄 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: e467fb41-8df7-4029-8249-38cf633d8f1e
📒 Files selected for processing (10)
cli/commands/server.gocli/commands/server_test.godocs/docs/server-config.mdpkg/serverconfig/cli.gen.gopkg/serverconfig/config.gen.gopkg/serverconfig/defaults.gen.gopkg/serverconfig/env.gen.gopkg/serverconfig/loader.gen.gopkg/serverconfig/schema.ymlpkg/serverconfig/validation.gen.go
When ingress.http_address is set, take a new branch in Server() that binds plain HTTP on the configured address — bypassing both the TLS path and the legacy bare-:80 fallback. Listen synchronously so a port conflict surfaces as a startup error instead of an orphaned goroutine; the supervisor needs to know if the ingress isn't really up. The accept loop runs in a goroutine after a successful bind, and a sibling goroutine waits on context cancellation to call server.Shutdown so in-flight requests drain cleanly on SIGTERM. A small warnIngressTLSOverride helper logs one warning per non-default tls.* field that has no effect in this mode (self_signed, acme_email, acme_dns_provider, additional_ips, additional_names). standard_tls is intentionally skipped: it defaults to true, so warning whenever it's "set" would cry wolf on every default config. Each warning names the specific field so operators can grep and remove without ambiguity. When ingress.http_address is empty, the existing standard_tls and plain-HTTP paths are unchanged.
Add a [ingress] section to the server config reference covering the use case (Miren behind nginx/Caddy/ALB), the configuration shape, and the precedence list — ingress.http_address overrides both standard_tls and the legacy bare-:80 fallback when set.
jcasimir
force-pushed
the
jcasimir/configurable-http-ingress
branch
from
15d4bac to
95306bb
Compare
|
Thanks @coderabbitai — both findings addressed in the latest force-push:
Verified end-to-end on the local containerized smoke runner: all five smoke scenarios (validation paths + full standalone-mode boot + listener probe + warn-helper assertion) still pass against the updated branch. |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Thanks for the PR @jcasimir! I'm 100% behind the idea that our default http listener port needs to be configurable. Where I think we need to figure out the right balance is that this ALSO disables TLS entirely. It seems like it would be better if it were 2 flags, one to set the http ingress port and another to disable TLS listening. |
|
Ok, so just to clarify you're saying you might want to enable TLS and non-TLS at the same time? I could see that being interesting. |
…FD-84) Replaces the GetStandardTLS branching in server.go with a switch on cfg.Ingress.GetMode(). Each non-autoprovision mode resolves its bind address from cfg.Ingress.GetAddress() (with a localhost default), and dispatches to the right serve function: - tls-autoprovision: existing ServeTLSWithController or ServeTLSSelfSigned (binds :443 + :80 as before) - behind-proxy-https: new ServeTLSWithControllerOnAddr or ServeTLSSelfSignedOnAddr (single TLS listener at the configured address; adopted from Jeff Casimir's PR mirendev#790 essentially unchanged) - behind-proxy-http: plain http.ListenAndServe at the configured address Also drops the Host-based exception in ServeTLSWithController's :80 handler that used to route localhost / 127.0.0.1 / ::1 / raw-IP Host requests directly to the default route app over plain HTTP. That was a dev-convenience hack with production security smell. Operators who want plain-HTTP access for dev workflow now pick behind-proxy-http explicitly; the autoprovision :80 listener now does only what it says (redirect + ACME). ValidateIngressCoherence runs right after Load() in both Server() and ServerConfigValidate() so config errors surface before any listener starts. Thanks to Jeff Casimir (https://github.com/jcasimir) for surfacing the use case in PRs mirendev#789/mirendev#790/mirendev#791. The autotls helpers in this commit are the OnAddr variants from his PR mirendev#790 essentially unchanged.
3 participants
Standing up a Miren server on a host that already serves another application on 80/443 currently leaves no way to put the Miren ingress somewhere else. Setting
--serve-tls=falsefalls back to a hard-coded:80, which still collides. The intended deployment shape is the standard one — TLS-terminating proxy out front, plain HTTP to Miren on a loopback port inward — but Miren can't bind anywhere except 0.0.0.0.This PR introduces a new
[ingress]config section with one field,http_address. When set, Miren binds plain HTTP on that address and skips the standard 80/443 binding entirely.tls.*settings have no effect in this mode; awarnIngressTLSOverridehelper logs one warning per non-defaulttls.*field, naming each so operators can grep and clean up.tls.standard_tlsis intentionally not in the warn list — it defaults totrue, so warning whenever it's "set" would fire on every default config.The listener binds synchronously so a port conflict surfaces as a startup error instead of an orphaned goroutine after the supervisor thinks we're up. The accept loop and graceful shutdown run in background goroutines wired to the parent context.
When
ingress.http_addressis empty (default), all existing paths are unchanged.Non-goals
HTTPS on a custom port is a related but distinct change — a follow-up PR adds
ingress.https_address. A third PR introduces aselectIngressModeresolver that rejects setting both ingress addresses simultaneously, once there are two fields to be exclusive with.Testing
TestWarnIngressTLSOverrideincli/commands/server_test.gotable-tests each warn-trigger field, including the silent-on-defaultstandard_tlscase.curl http://127.0.0.1:18080/against the custom address returns Miren's 404;ss -tlnconfirms no listeners on 80 or 443; the warn line fires as expected when--self-signed-tlsis set alongside.