Reshape ingress config around named modes (RFD-84)#799
Merged
Conversation
Introduces the [ingress] section with mode and address fields, and retires tls.standard_tls. The mode is a three-value enum (tls-autoprovision, behind-proxy-http, behind-proxy-https) that names the supported deployment shapes directly so invalid combinations become unrepresentable rather than rejected at load time. The cross-field validation that the previous shape required (warning when [tls] knobs become no-ops, the HTTP-01-cant-pair-with-custom-https rule, the mutual-exclusion check between two address fields) collapses to a small ValidateIngressCoherence method: enforce that [tls] is empty under behind-proxy-http, reject the reserved-but-unsupported unix: address prefix with a clear message, and require that ingress.address remain empty under tls-autoprovision since the :443 + :80 pair is structural for HTTP-01 ACME. Constants in ingress_modes.go give callers and downstream code a typed name to switch on rather than bare strings. See mirendev/rfd#134 for the design and rationale.
…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 #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 #789/#790/#791. The autotls helpers in this commit are the OnAddr variants from his PR #790 essentially unchanged.
- server-config.md: new [ingress] section documenting mode + address, with the three-mode reference table. [tls] section reframed as cert-only and no longer lists the removed standard_tls field. - tls.md: new "Ingress Modes and TLS" section that introduces the three modes and how cert sourcing varies across them. Old TLS reference table no longer mentions standard_tls. - command/server.md: regenerated to pick up --ingress-mode and --ingress-address flags and drop --serve-tls.
…eshape The systemd install path, the embedded systemd entrypoint test script, and the example server.toml all still referenced the removed flag and field. Drop the explicit --serve-tls since tls-autoprovision (the default mode) is exactly what those callers want. Update the example config to point at the new [ingress] section and link RFD-84.
📝 WalkthroughWalkthroughThis pull request introduces a configurable ingress mode system to replace the boolean Comment |
This was referenced May 11, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 872-883: The behind-proxy-http branch currently spawns a detached
goroutine that calls http.ListenAndServe(addr, hs) and only logs errors; change
it to register the listener with the existing errgroup by calling eg.Go(func()
error { ctx.Log.Info("starting HTTP server", "addr", addr); return
http.ListenAndServe(addr, hs) }) so that startup failures are propagated through
eg.Wait(); keep the same logging context (addr and hs) but remove the
fire-and-forget go func and ensure the returned error is handled by the
surrounding code that calls eg.Wait().
In `@configs/server.toml.example`:
- Around line 49-52: The comment for ingress.address incorrectly states the
value is "ignored" under tls-autoprovision; update the docs to reflect actual
behavior: state that setting ingress.address is invalid in tls-autoprovision and
will cause configuration validation to fail (rather than being ignored).
Reference the ingress.address config key and tls-autoprovision mode in the
comment so users know the validator will reject this value; alternatively, if
you prefer to change behavior instead of docs, adjust the validation logic that
checks mode == "tls-autoprovision" to accept/ignore ingress.address (edit the
validator function that enforces ingress.address rules).
In `@pkg/serverconfig/cli.gen.go`:
- Around line 25-26: The CLI help for the ingress flags is stale: update the
descriptions for the IngressConfigAddress field/flag (--ingress-address) to
reflect that it is validated (not ignored) for tls-autoprovision and clarify the
behavior/constraints, and correct the IngressConfigMode description to
accurately describe the current TLS model (replace the outdated
"behind-proxy-https" wording with the actual mode semantics used by the TLS
configuration); make these changes at the schema/source level where
IngressConfigAddress and IngressConfigMode are defined and then regenerate
pkg/serverconfig/cli.gen.go so the compiled help text matches runtime validation
and current TLS behavior.
In `@pkg/serverconfig/schema.yml`:
- Around line 197-200: Update the description string for the ingress-mode CLI
option (cli.long: ingress-mode, env: MIREN_INGRESS_MODE) so the
"behind-proxy-https" mode no longer incorrectly claims "no ACME provisioning";
specifically edit the text for the behind-proxy-https entry to state that TLS is
terminated by Miren with externally-provided certificates and remove or soften
the absolute "no ACME provisioning" claim (e.g., mention that ACME is not
performed by Miren for externally-provided certs or omit ACME detail entirely).
🪄 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: f0d36a27-21bc-4300-b275-d7b535be59d4
📒 Files selected for processing (20)
cli/commands/server.gocli/commands/server_config_cmds.gocli/commands/server_install.gocomponents/autotls/autotls.gocomponents/autotls/selfsigned.goconfigs/server.toml.exampledocs/docs/command/server.mddocs/docs/server-config.mddocs/docs/tls.mdhack/systemd/entrypoint.shpkg/serverconfig/cli.gen.gopkg/serverconfig/config.gen.gopkg/serverconfig/defaults.gen.gopkg/serverconfig/env.gen.gopkg/serverconfig/ingress_modes.gopkg/serverconfig/loader.gen.gopkg/serverconfig/schema.ymlpkg/serverconfig/validate.gopkg/serverconfig/validate_test.gopkg/serverconfig/validation.gen.go
Four findings, all real: - behind-proxy-http listener now goes through the existing errgroup (eg.Go) for both serve and graceful shutdown, so startup failures propagate via eg.Wait() instead of getting logged and ignored. The fire-and-forget pattern is still present in the autotls helpers; that's a separate cleanup to do across all the ingress paths in a follow-up. - Help text and config example said 'ignored' under tls-autoprovision for ingress.address, but the validator hard-errors. Fixed both the schema description and the example comment to say 'rejected by validation' so users know what to expect. - behind-proxy-https mode description claimed 'no ACME provisioning', which conflicts with DNS-01 support called out in the RFD and in ValidateIngressCoherence. Softened to 'certs from self-signed or DNS-01 ACME, since :80 isn't bound for HTTP-01'. cli.gen.go regenerated from the schema changes.
The previous commit updated the schema descriptions but rebuilt with a stale binary, so the generated docs/docs/command/server.md still had the old text. CI's go-generate-check caught it.
evanphx
requested changes
May 12, 2026
Contributor
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
Looks good, but we need to accept --start-tls as a noop (maybe mark it hidden so it's not normally used), so that upgrades don't break.
| TLSConfigAdditionalIPs []string `long:"ips" description:"Additional IPs assigned to the server cert"` | ||
| TLSConfigAdditionalNames []string `long:"dns-names" description:"Additional DNS names assigned to the server cert"` | ||
| TLSConfigSelfSigned *bool `long:"self-signed-tls" description:"Use self-signed certificates for TLS (for development/testing only)"` | ||
| TLSConfigStandardTLS *bool `long:"serve-tls" description:"Expose the http ingress on standard TLS ports"` |
Contributor
There was a problem hiding this comment.
We should still include this option but ignore it so that upgrades don't break.
Contributor
Author
|
Yeah fair call I was waffling on the hard-line removal but better to be nicer |
…afety Per Evan's review on #799: existing systemd unit files from pre-RFD-84 installs pass --serve-tls in ExecStart, and operators may have standard_tls = true in their config.toml or MIREN_TLS_STANDARD_TLS set in their environment. The previous commit removed the field outright, which would break those upgrades by either rejecting the unknown flag or (for env/TOML cases) silently dropping a value the operator expected to take effect. Re-adding the field as a real (non-cli_only) bool on TLSConfig so: - TOML standard_tls = true parses cleanly (go-toml/v2 is lenient about unknown fields anyway, but the explicit field is clearer) - MIREN_TLS_STANDARD_TLS env var is read into the field - --serve-tls CLI flag is accepted The field is never read by any code path. ingress.mode is the source of truth for the deployment shape. A deprecation warning fires at startup when the field is explicitly set (i.e. StandardTLS != nil), pointing operators at RFD-84. Also extended configgen with a cli.hidden schema knob and used it here. Note: mflags (Miren's CLI library) doesn't currently read the hidden tag, so --serve-tls still appears in --help. The 'Deprecated and ignored' description is loud enough on its own; if we want true hiding later, that's a small mflags PR.
Contributor
Author
|
Addressed in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/serverconfig/validate.go (1)
69-73: ⚡ Quick winGuard
WarnDeprecatedConfigagainst nil logger.
log.Warn(...)is unconditional; a nil logger would panic. A small defaulting guard makes this safer.Suggested patch
func (c *Config) WarnDeprecatedConfig(log *slog.Logger) { + if log == nil { + log = slog.Default() + } if c.TLS.StandardTLS != nil { log.Warn("tls.standard_tls (also --serve-tls / MIREN_TLS_STANDARD_TLS) is deprecated and ignored; use ingress.mode to pick the deployment shape. See RFD-84 at rfd.miren.garden/rfd/84.", "value", *c.TLS.StandardTLS) } }🤖 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 `@pkg/serverconfig/validate.go` around lines 69 - 73, The WarnDeprecatedConfig method calls log.Warn unconditionally and will panic if the passed slog.Logger is nil; update WarnDeprecatedConfig to guard against a nil logger (e.g., if log == nil { return } or initialize a no-op/default logger) before using log.Warn, keeping the existing conditional on c.TLS.StandardTLS and the deprecated message text (referencing WarnDeprecatedConfig, the log parameter, and c.TLS.StandardTLS).
🤖 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.
Nitpick comments:
In `@pkg/serverconfig/validate.go`:
- Around line 69-73: The WarnDeprecatedConfig method calls log.Warn
unconditionally and will panic if the passed slog.Logger is nil; update
WarnDeprecatedConfig to guard against a nil logger (e.g., if log == nil { return
} or initialize a no-op/default logger) before using log.Warn, keeping the
existing conditional on c.TLS.StandardTLS and the deprecated message text
(referencing WarnDeprecatedConfig, the log parameter, and c.TLS.StandardTLS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84716c44-3e01-4a8b-a460-7e7a1fc8e607
📒 Files selected for processing (12)
cli/commands/server.gocli/commands/server_config_cmds.godocs/docs/command/server.mdpkg/serverconfig/cli.gen.gopkg/serverconfig/cmd/configgen/main.gopkg/serverconfig/config.gen.gopkg/serverconfig/defaults.gen.gopkg/serverconfig/env.gen.gopkg/serverconfig/loader.gen.gopkg/serverconfig/schema.ymlpkg/serverconfig/toml_strict_test.gopkg/serverconfig/validate.go
✅ Files skipped from review due to trivial changes (5)
- pkg/serverconfig/defaults.gen.go
- pkg/serverconfig/loader.gen.go
- pkg/serverconfig/cli.gen.go
- pkg/serverconfig/config.gen.go
- pkg/serverconfig/env.gen.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/commands/server.go
evanphx
approved these changes
May 12, 2026
Contributor
Author
|
@jcasimir I clicked merge but still very open to any feedback you have at any point! Definitely want to make sure this works for your use case 👍 |
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
2 participants
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.
This work started because @jcasimir wanted to run Miren behind nginx in production, and Miren didn't quite know how to be run that way yet. He opened #789, #790, and #791 to make it work, extending the existing TLS configuration along the lines its surface seemed to invite. Reading the three PRs as a set, though, surfaced a deeper tension: the ingress and TLS sections together were trying to answer three separate questions (where to listen, whether to terminate TLS, where certs come from) through knobs that each spanned multiple of those answers, and the cross-axis coupling was forcing compensating validation helpers to keep things coherent. We landed on a mode-based redesign that names each deployment shape directly, which RFD-84 walks through in full.
This PR is the implementation. Three modes now:
tls-autoprovision(default, behavior unchanged from today),behind-proxy-http(plain HTTP, defaults to127.0.0.1:80), andbehind-proxy-https(TLS terminated by Miren on a single listener, defaults to127.0.0.1:443). An optionalingress.addressfield replaces any mode's default bind, so operators on multi-homed hosts, Tailscale interfaces, IPv6, or non-standard ports can bind wherever they need. Unix sockets are reserved as a futureaddressvalue and rejected today with a clear "not yet supported" error.One real behavior change comes with this. The autoprovision
:80handler used to have a Host-based exception that routedlocalhost,127.0.0.1,::1, and any raw-IP Host directly to the default route app over plain HTTP, bypassing the HTTPS redirect. That was a dev-convenience shortcut that became a small production smell, since operators didn't always realize that raw-IP access could reach apps without TLS. It's gone now. The autoprovision path does only what its name suggests, and operators who want plain-HTTP access for dev workflow pickbehind-proxy-httpexplicitly.One small ergonomic regression worth flagging in dev:
curl http://localhost/(no scheme, defaults to HTTP) used to hit the default route directly via that side door. Now it 302s tohttps://localhost/. The other dev shortcuts still work, though:curl -k https://localhost/andcurl -kL http://localhost/both reach the default route as before. If the friction proves annoying, RFD-84 leaves room for a dev-only opt-in flag in a follow-up.tls.standard_tlsretires entirely. The dev environment already used--self-signed-tlsrather than that field, so the dev loop is unchanged; I caught and updated a few stranded references in the systemd install path, the embedded systemd entrypoint test script, and the exampleserver.toml.Big thanks to @jcasimir for the use case and the implementation work; both set the stage for this change. His autotls helpers from #790 are the backbone of the new
behind-proxy-httpsmode. The three PRs close out once this lands.Closes MIR-1115.