Support multiple ports and non-HTTP services per app

[phinze]

Users have been asking to deploy non-HTTP services — the immediate motivation was wanting to run an IRC server on Miren with ports 6667 and 6697 exposed. Today the app config layer only supports a single port per service, and there's no way to specify node_port or protocol.

The fun part is that all the hard infrastructure work was already done. The sandbox container spec, firewall rules, and service controller all handle multi-port just fine. The bottleneck was the app config layer: one port integer per service, no array, no node_port, no protocol.

Config layer (appconfig)

Adds a ports[] array to the config spec schema and wires it through the full pipeline — TOML parsing with validation in appconfig, mapping through the build server's buildServicesConfig, config version conversion, and the deployment launcher's buildSandboxSpec. The existing scalar port/port_name/port_type fields are preserved for backward compat; ports[] takes precedence when present, and mixing the two styles is a validation error. The PORT env var picks the first HTTP-typed port, or falls back to the first port in the array.

Also adds a deploy-time guardrail: since node_port is now a first-class config field, two apps could silently claim the same host port and only find out when nftables blew up at runtime. The new validateNodePorts check runs during BuildFromTar and catches both intra-app duplicates and cross-app conflicts with clear error messages.

Empty protocol is now normalized to "tcp" before duplicate (port, protocol) validation.

Service entity lifecycle (controllers/deployment/launcher)

HTTP services are proxied at L7 via httpingress, but non-HTTP services (TCP/UDP) need L4 routing through the ipalloc → ServiceController → nftables pipeline. That pipeline requires a network_v1alpha.Service entity to exist — without one, no IP is allocated and no firewall rules are created.

The launcher now manages Service entities for app services that have at least one non-HTTP port:

• ensureServiceForPorts — creates or updates a Service entity with the correct ports, match labels, and metadata. On update, preserves existing IPs so ipalloc doesn't need to re-allocate.
• cleanupStaleServices — deletes orphaned Service entities when services are removed from config or become HTTP-only.
• Deterministic entity IDs (svc/<appname>-<servicename>) for stable identity across deploys.

ServiceController per-port endpoint chain fix (controllers/service)

The ServiceController had a bug where Create() used srv.Port[0] to determine the DNAT target port for all endpoint chains. For multi-port services, this meant all traffic was forwarded to the first port regardless of the original destination port — e.g., TCP traffic arriving on port 7000 would get DNAT'd to port 3000.

Moved endpoint chain setup inside the per-port loop so each service port gets its own endpoint chains targeting the correct port.

Test app

Adds testdata/tcp-echo/ — a minimal Go app with a TCP echo server on port 7000 and an HTTP health endpoint on port 3000, for validating multi-port service behavior end-to-end.

Example app config for an IRC server:

[services.irc]
command = "./ircd"

[[services.irc.ports]]
port = 6667
name = "irc"
type = "tcp"

[[services.irc.ports]]
port = 6697
name = "irc-tls"
type = "tcp"
node_port = 6697

Closes MIR-745

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds repeated ports components to schema, generated types, and app config (fields: port, name, protocol, type, node_port). AppConfig parsing and validation were extended; ConfigSpec conversion now maps ports. Build and deployment convert ports into container and network ports, validate node_port conflicts against SandboxPools, and manage Service entities for non-HTTP ports. Service controller and launcher create per-port, per-protocol chains and update service lifecycle. Tests and a tcp-echo example exercising multi-port and protocol behavior were added.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 `@appconfig/appconfig.go`:
- Around line 243-257: Duplicate detection for ports uses the raw p.Protocol
value, so an empty protocol and "tcp" are treated as different even though empty
should default to "tcp"; update the normalization before duplicate checks by
canonicalizing p.Protocol (e.g., set protocol := p.Protocol; if protocol=="" {
protocol = "tcp" }) and use that normalized value when constructing pp :=
portProto{p.Port, protocol} and when formatting error messages; ensure
seenPortProto and duplicate checks use the normalized protocol while seenNames
logic remains unchanged and still references p.Name and serviceName.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 97e2fb5 and 02375ed.

📒 Files selected for processing (9)

• api/core/config_version.go
• api/core/core_v1alpha/schema.gen.go
• api/core/schema.yml
• appconfig/appconfig.go
• appconfig/appconfig_test.go
• controllers/deployment/launcher.go
• controllers/deployment/launcher_test.go
• servers/build/build.go
• servers/build/build_test.go

[evanphx]

This all looks good, but have it work, a network_v1alpha.Service has to be created but I don't see it here. Something like this: https://github.com/mirendev/runtime/blob/main/pkg/addon/framework.go#L139

[phinze]

lol you're right - all this config wiring missed the last mile! fixing

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (1)

testdata/tcp-echo/main.go (1)

13-13: Trim redundant inline comments.

At Line 13 and Line 33, the comments restate what the code already clearly expresses. Removing them would keep the sample tighter.

Proposed cleanup

-	// TCP echo server on port 7000
 	go func() {
@@
-	// HTTP health endpoint on port 3000
 	port := os.Getenv("PORT")

As per coding guidelines, "Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does".

Also applies to: 33-33

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@testdata/tcp-echo/main.go` at line 13, Remove the redundant inline comments
that merely restate the code: delete the comment on the TCP port declaration and
the comment inside the echo handler so the sample stays concise; locate the
comments near the top of the file around the TCP port setup (in main or any
net.Listen call) and the comment inside the echo loop/handler (within func main
or the goroutine handling connections) and remove them, leaving only comments
that add non-obvious context.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@appconfig/appconfig.go`:
- Around line 246-252: Validation currently accepts "udp" but the dataplane only
supports TCP; update appconfig validation to reject "udp" (treat any "udp"
port.protocol as invalid) so configs cannot declare unsupported protocols. In
appconfig.go change the checks around p.Protocol (the block setting proto :=
p.Protocol and the similar block at the other occurrence) to return an error
when p.Protocol == "udp" (allow only "" or "tcp"), and keep the default proto
logic as "tcp" for empty values; reference the existing symbols p.Protocol and
proto so the change aligns with the other occurrence at the second block.
- Around line 229-230: The mixed-style detection currently checks svcConfig.Port
> 0 which misses negative scalar port values; update the condition in the
validation that returns the error for mixed 'ports' array and scalar fields so
it uses svcConfig.Port != 0 instead of > 0, keeping the existing checks for
svcConfig.PortName != "" and svcConfig.PortType != "" (this is the block that
constructs the error message using serviceName), so any non-zero scalar port
(including negatives) triggers the mixed-style error.

In `@controllers/deployment/launcher.go`:
- Around line 237-253: The current cleanup can delete services when
ensureServiceForPorts transiently fails because desiredServices only contains
names from successful ensures; change the loop that builds desiredServices so it
collects svc.Name for every spec.Services entry regardless of
ensureServiceForPorts outcome (i.e., append svc.Name before or unconditionally
after calling ensureServiceForPorts), keep the existing error logging and
continue on error, and then call cleanupStaleServices with that full list so
cleanup does not remove active Service entities on partial reconcile failures;
reference ensureServiceForPorts, desiredServices, svc.Name, and
cleanupStaleServices to locate the change.
- Around line 1039-1059: The function ensureServiceForPorts currently only
checks svc.Ports (core_v1alpha.ConfigSpecServicesPorts) and misses scalar
fields; update ensureServiceForPorts to treat scalar svc.Port and svc.PortType
as a fallback (or normalize them into svc.Ports at start) so non-HTTP scalar
ports trigger Service creation: when iterating spec.Services in
ensureServiceForPorts, if svc.Ports is empty but svc.Port or svc.PortType is
set, create a Ports entry from those scalar fields (or treat svc.PortType !=
"http" as hasNonHTTP) before the hasNonHTTP check; reference
ensureServiceForPorts, core_v1alpha.ConfigSpecServicesPorts, svc.Ports,
svc.Port, svc.PortType and ConfigSpecFromConfig in your change to keep behavior
consistent and add unit tests covering scalar non-HTTP ports.

---

Nitpick comments:
In `@testdata/tcp-echo/main.go`:
- Line 13: Remove the redundant inline comments that merely restate the code:
delete the comment on the TCP port declaration and the comment inside the echo
handler so the sample stays concise; locate the comments near the top of the
file around the TCP port setup (in main or any net.Listen call) and the comment
inside the echo loop/handler (within func main or the goroutine handling
connections) and remove them, leaving only comments that add non-obvious
context.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2dcdcbc and 14d97b4.

📒 Files selected for processing (14)

• api/core/config_version.go
• api/core/core_v1alpha/schema.gen.go
• api/core/schema.yml
• appconfig/appconfig.go
• appconfig/appconfig_test.go
• controllers/deployment/launcher.go
• controllers/deployment/launcher_test.go
• controllers/service/service.go
• controllers/service/service_test.go
• servers/build/build.go
• servers/build/build_test.go
• testdata/tcp-echo/.miren/app.toml
• testdata/tcp-echo/go.mod
• testdata/tcp-echo/main.go

🚧 Files skipped from review as they are similar to previous changes (3)

• testdata/tcp-echo/go.mod
• api/core/schema.yml
• testdata/tcp-echo/.miren/app.toml

[phinze]

Ok this does the thing now. Ready for re-review! (I'll knock out the last few rabbit issues in parallel.)

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

appconfig/appconfig_test.go (1)

926-1137: Add node_port boundary tests to lock in the new validation path.

Validate() now enforces node_port range, but this suite doesn’t assert the -1/65536 failure cases yet.

✅ Suggested test additions

 func TestValidatePortsConfig(t *testing.T) {
+	t.Run("node_port negative", func(t *testing.T) {
+		config := `
+name = "test-app"
+
+[[services.web.ports]]
+port = 8080
+name = "http"
+node_port = -1
+`
+		_, err := Parse([]byte(config))
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "node_port must be between 0 and 65535")
+	})
+
+	t.Run("node_port out of range", func(t *testing.T) {
+		config := `
+name = "test-app"
+
+[[services.web.ports]]
+port = 8080
+name = "http"
+node_port = 70000
+`
+		_, err := Parse([]byte(config))
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "node_port must be between 0 and 65535")
+	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@appconfig/appconfig_test.go` around lines 926 - 1137, Add two subtests inside
TestValidatePortsConfig mirroring the existing port-range tests that call Parse
on configs with [[services.web.ports]] entries using node_port = -1 and
node_port = 65536, require an error, and assert the error message contains the
node_port range failure (e.g. "node_port must be between 1 and 65535"). Locate
the TestValidatePortsConfig function and add t.Run blocks similar to the "port
out of range" and "port zero" cases that use Parse to trigger validation; keep
assertions consistent with other cases (require.Error and assert.Contains on
err.Error()).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@controllers/deployment/launcher_test.go`:
- Around line 2076-2079: The comment describing
"TestNoServiceEntityForHTTPOnlyService" is placed above the wrong test; move the
HTTP-only comment block so it immediately precedes the
TestNoServiceEntityForHTTPOnlyService function and ensure the comment for
TestServiceEntityCreatedForScalarNonHTTPPort sits directly above that test;
update both comment texts if needed to add useful context (not restating code)
and verify the comment blocks reference the exact test names
TestNoServiceEntityForHTTPOnlyService and
TestServiceEntityCreatedForScalarNonHTTPPort so intent is clear when scanning
the tests.

---

Nitpick comments:
In `@appconfig/appconfig_test.go`:
- Around line 926-1137: Add two subtests inside TestValidatePortsConfig
mirroring the existing port-range tests that call Parse on configs with
[[services.web.ports]] entries using node_port = -1 and node_port = 65536,
require an error, and assert the error message contains the node_port range
failure (e.g. "node_port must be between 1 and 65535"). Locate the
TestValidatePortsConfig function and add t.Run blocks similar to the "port out
of range" and "port zero" cases that use Parse to trigger validation; keep
assertions consistent with other cases (require.Error and assert.Contains on
err.Error()).

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 14d97b4 and e831f59.

📒 Files selected for processing (6)

• appconfig/appconfig.go
• appconfig/appconfig_test.go
• controllers/deployment/launcher.go
• controllers/deployment/launcher_test.go
• controllers/service/service.go
• controllers/service/service_test.go

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Misplaced test comment currently documents the wrong function.

The HTTP-only comment block is placed above the scalar non-HTTP test, which makes test intent harder to scan.

✏️ Suggested comment placement fix

-// TestNoServiceEntityForHTTPOnlyService verifies that services with only HTTP
-// ports do not get a Service entity created (they use httpingress instead).
 // TestServiceEntityCreatedForScalarNonHTTPPort tests that the launcher creates a
 // Service entity when scalar port fields (Port/PortType) specify a non-HTTP port.
 func TestServiceEntityCreatedForScalarNonHTTPPort(t *testing.T) {
@@
 }
 
+// TestNoServiceEntityForHTTPOnlyService verifies that services with only HTTP
+// ports do not get a Service entity created (they use httpingress instead).
 func TestNoServiceEntityForHTTPOnlyService(t *testing.T) {

As per coding guidelines: "Only add comments when they provide valuable context or explain 'why' something is done - avoid redundant comments that restate what the code does".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// TestNoServiceEntityForHTTPOnlyService verifies that services with only HTTP
	// ports do not get a Service entity created (they use httpingress instead).
	// TestServiceEntityCreatedForScalarNonHTTPPort tests that the launcher creates a
	// Service entity when scalar port fields (Port/PortType) specify a non-HTTP port.
	// TestServiceEntityCreatedForScalarNonHTTPPort tests that the launcher creates a
	// Service entity when scalar port fields (Port/PortType) specify a non-HTTP port.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controllers/deployment/launcher_test.go` around lines 2076 - 2079, The
comment describing "TestNoServiceEntityForHTTPOnlyService" is placed above the
wrong test; move the HTTP-only comment block so it immediately precedes the
TestNoServiceEntityForHTTPOnlyService function and ensure the comment for
TestServiceEntityCreatedForScalarNonHTTPPort sits directly above that test;
update both comment texts if needed to add useful context (not restating code)
and verify the comment blocks reference the exact test names
TestNoServiceEntityForHTTPOnlyService and
TestServiceEntityCreatedForScalarNonHTTPPort so intent is clear when scanning
the tests.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

docs/docs/traffic-routing.md (1)

77-77: Clarify the gRPC example wording.

“gRPC without HTTP/2” is self-contradictory and may confuse readers; consider rephrasing to indicate gRPC traffic that bypasses HTTP ingress and is exposed as raw L4.

Suggested wording tweak

-For non-HTTP services — databases, game servers, IRC, gRPC without HTTP/2, raw TCP/UDP protocols — Miren routes traffic at Layer 4 using nftables NAT rules.
+For non-HTTP services — databases, game servers, IRC, and other raw TCP/UDP protocols (including gRPC services exposed via L4 passthrough) — Miren routes traffic at Layer 4 using nftables NAT rules.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/docs/traffic-routing.md` at line 77, The phrase "gRPC without HTTP/2" is
confusing; update the sentence that currently reads "For non-HTTP services —
databases, game servers, IRC, gRPC without HTTP/2, raw TCP/UDP protocols — Miren
routes traffic at Layer 4 using nftables NAT rules." to clarify gRPC that
bypasses HTTP ingress and is exposed as raw L4 (e.g., "gRPC traffic exposed
directly at Layer 4 (bypassing HTTP ingress/HTTP/2)") so readers understand you
mean gRPC carried as raw L4 rather than implying gRPC lacks HTTP/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/docs/traffic-routing.md`:
- Around line 25-27: The fenced diagram blocks (e.g., the blocks containing
"Client → :443 → HTTP Ingress → Route Lookup → Activator → Sandbox :3000" and
"Client → :6697 (node_port) → nftables PREROUTING → Service Chain → Load Balance
→ Endpoint Chain → DNAT → Sandbox :6697", and the similar block at lines
127–130) are missing language identifiers and trigger MD040; update each
triple-backtick fence to include a language tag such as text (e.g., replace ```
with ```text) so the diagrams are annotated and the markdown linter passes.

---

Nitpick comments:
In `@docs/docs/traffic-routing.md`:
- Line 77: The phrase "gRPC without HTTP/2" is confusing; update the sentence
that currently reads "For non-HTTP services — databases, game servers, IRC, gRPC
without HTTP/2, raw TCP/UDP protocols — Miren routes traffic at Layer 4 using
nftables NAT rules." to clarify gRPC that bypasses HTTP ingress and is exposed
as raw L4 (e.g., "gRPC traffic exposed directly at Layer 4 (bypassing HTTP
ingress/HTTP/2)") so readers understand you mean gRPC carried as raw L4 rather
than implying gRPC lacks HTTP/2.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e831f59 and ff45ec6.

📒 Files selected for processing (3)

• docs/docs/firewall.md
• docs/docs/services.md
• docs/docs/traffic-routing.md

✅ Files skipped from review due to trivial changes (1)

• docs/docs/services.md

[evanphx]

Looks great! I suspect there will be some "service in app.toml" vs "service entity" confusion until we change the name of the later, but that's fine for now.

[evanphx]

Open and reachable in what sense? That the host machine doesn't have firewall rules prevent them?

[phinze]

Yeah the section this is in sets the context:

Required External Ports

If you're running Miren on a cloud provider, you'll need to configure security groups or network ACLs to allow external traffic to reach the Miren server.

[phinze]

I suspect there will be some "service in app.toml" vs "service entity" confusion until we change the name of the later, but that's fine for now.

Yeah we can finalize a name and land #611 soon