diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c5dce2..712a41e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`EngineOptions.DisableHostPortPublishing`** — drops the host side of compose + `ports:` entries. Services still start and nothing else about them changes; each + dropped entry raises an `engine.warn` event (`compose_host_port_publish_skipped`) + naming the service and where it is reachable instead. Set it when the daemon's "host" + is a namespace shared with other workloads — the engine running inside a Kubernetes + pod next to sidecars — where a service publishing a common port (`8080`) loses the + race against a sidecar already listening there and the boot fails on a raw + `bind: address already in use`. Nothing in that shape consumes the publish anyway: + service-to-service traffic goes over the compose network by service name, and an + embedder forwarding a port out of the namespace dials the container on that network, + which is why dropping it changes no connectivity. It is an `EngineOptions` field + rather than a `devcontainer.json` one because it describes where the engine is + deployed, not the project. Entries are dropped from the in-memory project, so + `ConfigHash` sees them go and a container created before the option was turned on is + recreated rather than reused with its publishes intact. `ComposeBackendNative` only — + under `ComposeBackendShellout`, `docker compose` owns the publish, so a compose-source + `Up` is refused rather than run with the option silently ignored. `network_mode: host` + has the same collision and is not covered here; it needs a refusal rather than a drop + ([#136](https://github.com/crunchloop/devcontainer/issues/136)). + ### Removed - **BREAKING — `runtime.Capabilities` is cut from six fields to three.** The struct diff --git a/README.md b/README.md index 1dfc040..54dbbf8 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,7 @@ field/behavior the library covers. Legend: ✅ acted on · ⚠️ parsed but not | `forwardPorts` | ⚠️ parsed | Not actuated; [#7](https://github.com/crunchloop/devcontainer/issues/7) | | `portsAttributes`, `otherPortsAttributes` | ⚠️ parsed | Surfaced on `ResolvedConfig`; not enforced | | `appPort` (deprecated) | ✅ translated | Folded into `forwardPorts` (skipping container ports already declared); deprecation warning still emitted | +| compose `ports:` | ✅ published | Host publishes come from the compose file, not `devcontainer.json`. `EngineOptions.DisableHostPortPublishing` drops the host side of every entry — for an embedder whose daemon host is a namespace shared with other workloads (the engine inside a Kubernetes pod), where the binding collides with a sidecar and nothing consumes it. Each dropped entry raises an `engine.warn` event. Requires `ComposeBackendNative`. | **Other** diff --git a/compose/apply_drop_host_ports_test.go b/compose/apply_drop_host_ports_test.go new file mode 100644 index 0000000..36f337a --- /dev/null +++ b/compose/apply_drop_host_ports_test.go @@ -0,0 +1,119 @@ +package compose + +import ( + "testing" + + composetypes "github.com/compose-spec/compose-go/v2/types" +) + +func portProject() *composetypes.Project { + return &composetypes.Project{ + Services: composetypes.Services{ + "app": { + Name: "app", + Image: "app:dev", + Ports: []composetypes.ServicePortConfig{ + {Target: 8080, Published: "8080", Protocol: "tcp"}, + {Target: 9229, Published: "9229", HostIP: "127.0.0.1", Protocol: "tcp"}, + }, + Environment: composetypes.MappingWithEquals{}, + }, + "db": { + Name: "db", + Image: "postgres:17", + Ports: []composetypes.ServicePortConfig{ + // `ports: ["5432"]` — no published side; the daemon + // picks an ephemeral host port, in the same namespace. + {Target: 5432}, + }, + }, + "cache": { + Name: "cache", + Image: "redis:7", + }, + }, + } +} + +// Every entry goes, on every service, and the caller learns what was +// taken so it can say so. Service order is sorted: the engine turns +// each entry into a warning, and warning order must not depend on map +// iteration. +func TestApplyDropHostPorts_DropsEveryEntry(t *testing.T) { + proj := portProject() + + dropped := ApplyDropHostPorts(proj) + + for name, svc := range proj.Services { + if len(svc.Ports) != 0 { + t.Errorf("service %q kept %d ports, want 0", name, len(svc.Ports)) + } + } + if len(dropped) != 3 { + t.Fatalf("dropped = %d entries, want 3; got %+v", len(dropped), dropped) + } + want := []string{"app", "app", "db"} + for i, svc := range want { + if dropped[i].Service != svc { + t.Errorf("dropped[%d].Service = %q, want %q (sorted by service name)", i, dropped[i].Service, svc) + } + } + if got := dropped[0].String(); got != "8080:8080/tcp" { + t.Errorf("dropped[0] = %q, want 8080:8080/tcp", got) + } + if got := dropped[1].String(); got != "127.0.0.1:9229:9229/tcp" { + t.Errorf("dropped[1] = %q, want 127.0.0.1:9229:9229/tcp", got) + } + // Published unset: still dropped, and rendered without a host side + // rather than as ":5432/tcp". + if got := dropped[2].String(); got != "5432/tcp" { + t.Errorf("dropped[2] = %q, want 5432/tcp", got) + } +} + +// The drop is surgical: nothing but ports: moves. A service that +// declared none is not rewritten at all. +func TestApplyDropHostPorts_LeavesTheRestOfTheServiceAlone(t *testing.T) { + proj := portProject() + + ApplyDropHostPorts(proj) + + if got := proj.Services["app"].Image; got != "app:dev" { + t.Errorf("app image = %q, want app:dev", got) + } + if got := proj.Services["cache"].Image; got != "redis:7" { + t.Errorf("cache image = %q, want redis:7", got) + } + if len(proj.Services) != 3 { + t.Errorf("services = %d, want 3", len(proj.Services)) + } +} + +func TestApplyDropHostPorts_NoPortsIsANoOp(t *testing.T) { + proj := &composetypes.Project{ + Services: composetypes.Services{"app": {Name: "app", Image: "app:dev"}}, + } + if dropped := ApplyDropHostPorts(proj); dropped != nil { + t.Errorf("dropped = %+v, want nil", dropped) + } + if dropped := ApplyDropHostPorts(nil); dropped != nil { + t.Errorf("nil project: dropped = %+v, want nil", dropped) + } +} + +// The reason the drop happens here and not inside the orchestrator's +// portsOf: ConfigHash reads the project's ServiceConfig, so removing +// the entries is visible to the recreate check. A container created +// before the option was turned on published the ports and must be +// replaced, not reused. +func TestApplyDropHostPorts_ChangesConfigHash(t *testing.T) { + proj := portProject() + before := ConfigHash("sha256:x", proj.Services["app"]) + + ApplyDropHostPorts(proj) + + after := ConfigHash("sha256:x", proj.Services["app"]) + if before == after { + t.Error("ApplyDropHostPorts did not change ConfigHash; a container created with the publishes would be reused with them intact") + } +} diff --git a/compose/apply_override.go b/compose/apply_override.go index 2bb1665..8753bf5 100644 --- a/compose/apply_override.go +++ b/compose/apply_override.go @@ -2,6 +2,8 @@ package compose import ( "fmt" + "sort" + "strconv" composetypes "github.com/compose-spec/compose-go/v2/types" ) @@ -135,3 +137,94 @@ func ApplyRunOverride(project *composetypes.Project, primaryService string, ov O project.Services[primaryService] = svc return nil } + +// DroppedHostPort records one `ports:` entry removed by +// ApplyDropHostPorts, so the caller can report what it took away. +type DroppedHostPort struct { + // Service is the compose service that declared the entry. + Service string + + // HostIP is the address the entry asked to bind, empty for + // compose's all-interfaces default. + HostIP string + + // Published is the host-side port, empty when the entry left it + // to the daemon (`ports: ["8080"]` — an ephemeral host port). + Published string + + // Target is the container-side port the entry published. + Target int + + // Protocol is "tcp" or "udp", empty for compose's tcp default. + Protocol string +} + +// String renders the entry in compose's own short syntax +// (`127.0.0.1:8080:8080/tcp`), so a diagnostic can quote the line the +// user wrote rather than a shape only this package knows. +func (d DroppedHostPort) String() string { + proto := d.Protocol + if proto == "" { + proto = "tcp" + } + out := strconv.Itoa(d.Target) + "/" + proto + if d.Published != "" { + out = d.Published + ":" + out + } + if d.HostIP != "" { + out = d.HostIP + ":" + out + } + return out +} + +// ApplyDropHostPorts mutates project so no service publishes a host +// port, and returns every entry it removed in service-name order. +// +// For an engine whose daemon's "host" is a namespace shared with +// workloads it does not own — the engine running inside a Kubernetes +// pod next to sidecars — a published port is not the project's to +// take: it collides with whatever else already listens there, and +// nothing consumes it, because service-to-service traffic goes over +// the compose network by service name and an embedder forwarding a +// port out of the namespace dials the container on that network. +// +// Dropping the entries here rather than inside the orchestrator is +// deliberate: ConfigHash reads the project's ServiceConfig, so a +// container created with the bindings drifts from one created +// without them and is recreated, instead of being reused with its +// publishes intact. +// +// Entries are removed whole, including ones that left `published` +// unset — those bind an ephemeral host port, which is the same +// namespace and the same problem. +func ApplyDropHostPorts(project *composetypes.Project) []DroppedHostPort { + if project == nil { + return nil + } + + names := make([]string, 0, len(project.Services)) + for name := range project.Services { + names = append(names, name) + } + sort.Strings(names) + + var dropped []DroppedHostPort + for _, name := range names { + svc := project.Services[name] + if len(svc.Ports) == 0 { + continue + } + for _, p := range svc.Ports { + dropped = append(dropped, DroppedHostPort{ + Service: name, + HostIP: p.HostIP, + Published: p.Published, + Target: int(p.Target), + Protocol: p.Protocol, + }) + } + svc.Ports = nil + project.Services[name] = svc + } + return dropped +} diff --git a/engine.go b/engine.go index 755b54f..3ae3389 100644 --- a/engine.go +++ b/engine.go @@ -84,6 +84,38 @@ type EngineOptions struct { // Native; then the default flips and the shellout path is // deleted. ComposeBackend ComposeBackend + + // DisableHostPortPublishing drops the host side of compose + // `ports:` entries. Services still start and nothing else about + // them changes; they are simply not published. Each dropped entry + // is reported as an events.WarnEvent with code + // "compose_host_port_publish_skipped". + // + // Set it when the daemon's "host" is a namespace shared with other + // workloads — the engine running inside a Kubernetes pod next to + // sidecars — where binding a host port is neither useful nor safe: + // a service publishing a common port (8080) loses the race against + // a sidecar already listening there and the boot fails on a raw + // "bind: address already in use". Nothing consumes the publish in + // that shape anyway, so the binding only takes a scarce shared + // port. + // + // Dropping it leaves connectivity intact — service-to-service + // traffic goes over the compose network by service name, and an + // embedder forwarding a port out of the namespace dials the + // container on that network, never the host publish. + // + // It lives here rather than in devcontainer.json because it is a + // property of where the engine is deployed, not of the project: a + // user cannot know what their compose stack is scheduled next to, + // and a config field would let them opt back into a binding that + // cannot work. + // + // ComposeBackendNative only. The shellout backend hands `ports:` + // to `docker compose`, which publishes them regardless, so a + // compose-source Up under ComposeBackendShellout is refused rather + // than run with the option silently ignored. + DisableHostPortPublishing bool } // ComposeBackend selects between the legacy shellout and the new diff --git a/test/integration/compose_host_port_publishing_test.go b/test/integration/compose_host_port_publishing_test.go new file mode 100644 index 0000000..51aefc3 --- /dev/null +++ b/test/integration/compose_host_port_publishing_test.go @@ -0,0 +1,95 @@ +//go:build integration + +package integration + +import ( + "context" + "path/filepath" + "strings" + "testing" + "time" + + devcontainer "github.com/crunchloop/devcontainer" +) + +// writeComposePortsWorkspace lays out a project whose primary service +// publishes a host port and whose sidecar answers on the compose +// network. The two together are what DisableHostPortPublishing +// claims: the publish is removable, the service-name path is not. +func writeComposePortsWorkspace(t *testing.T) string { + t.Helper() + dir := t.TempDir() + + mustWrite(t, filepath.Join(dir, "docker-compose.yml"), ` +services: + app: + image: `+testImage+` + command: ["sh", "-c", "while sleep 1000; do :; done"] + ports: + - "39080:8080" + db: + image: `+testImage+` + command: ["sh", "-c", "while true; do echo db-reachable | nc -l -p 5000; done"] +`) + + mustWrite(t, filepath.Join(dir, ".devcontainer", "devcontainer.json"), `{ + "dockerComposeFile": "../docker-compose.yml", + "service": "app", + "runServices": ["app", "db"], + "workspaceFolder": "/workspaces/proj" + }`) + return dir +} + +// The option's claim is that dropping the host side of `ports:` is +// semantically safe. This is the half a fake runtime cannot make: the +// project still boots against a real daemon, and the sidecar is still +// reachable by service name over the compose network — which is how +// an embedder reaches it, and why nothing is lost by not publishing. +// +// What the host binding itself does (or, here, does not do) is +// asserted at the RunSpec boundary in the unit tests; the daemon-side +// translation of RunSpec.Ports is covered in runtime/docker. +func TestComposeSource_DisableHostPortPublishing_BootsAndKeepsServiceDNS(t *testing.T) { + if testing.Short() { + t.Skip() + } + + eng, rt := newEngineWith(t, devcontainer.EngineOptions{ + ComposeBackend: devcontainer.ComposeBackendNative, + DisableHostPortPublishing: true, + }) + defer rt.Close() + + ws := writeComposePortsWorkspace(t) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + wsObj, err := eng.Up(ctx, devcontainer.UpOptions{ + LocalWorkspaceFolder: ws, + Recreate: true, + SkipLifecycle: true, + }) + if err != nil { + t.Fatalf("Up: %v", err) + } + defer func() { + _ = eng.Down(context.Background(), wsObj, devcontainer.DownOptions{ + Remove: true, + RemoveVolumes: true, + }) + }() + + // Retry: nothing orders the sidecar's listener against this exec, + // and busybox nc serves one connection per accept. + res, err := eng.Exec(ctx, wsObj, devcontainer.ExecOptions{ + Cmd: []string{"sh", "-c", `for i in $(seq 1 30); do out=$(nc -w 5 db 5000); if [ -n "$out" ]; then echo "$out"; exit 0; fi; sleep 1; done; exit 1`}, + }) + if err != nil { + t.Fatalf("Exec nc db: %v", err) + } + if !strings.Contains(res.Stdout, "db-reachable") { + t.Errorf("sidecar unreachable by service name: stdout=%q stderr=%q", res.Stdout, res.Stderr) + } +} diff --git a/up.go b/up.go index 44523b8..9f87d7b 100644 --- a/up.go +++ b/up.go @@ -647,6 +647,20 @@ func (e *Engine) upComposeShellout( runOverride compose.Override, existingContainer bool, ) (*Workspace, error) { + // DisableHostPortPublishing cannot be honored here: this path + // hands the user's compose files to `docker compose`, which + // publishes `ports:` into the namespace we were told not to bind, + // and stripping them would mean re-implementing merge, extends and + // override resolution against the YAML. Refuse loudly instead of + // running with the option quietly ignored — the failure it exists + // to prevent is a boot-time bind collision, which is worse to + // debug than this. + if e.opts.DisableHostPortPublishing { + return nil, fmt.Errorf( + "compose source: EngineOptions.DisableHostPortPublishing requires ComposeBackendNative; " + + "the shellout backend passes ports: to `docker compose`, which publishes them on the daemon's host") + } + cr, ok := e.runtime.(runtime.ComposeRuntime) if !ok { return nil, fmt.Errorf("compose source: runtime does not support compose: %w", runtime.ErrNotImplemented) @@ -750,6 +764,7 @@ func (e *Engine) upComposeNative( if err := compose.ApplyRunOverride(project, src.Service, runOverride); err != nil { return nil, err } + e.dropHostPortPublishes(project, opts) orch := compose.NewOrchestrator(e.runtime) res, err := orch.Up(ctx, &compose.Plan{ @@ -771,6 +786,33 @@ func (e *Engine) upComposeNative( return e.buildWorkspace(ctx, containerID, cfg, opts.LocalEnv) } +// dropHostPortPublishes strips the host side of every compose +// `ports:` entry under EngineOptions.DisableHostPortPublishing, and +// warns once per removed entry. +// +// The removal is semantically safe — service-to-service traffic goes +// over the compose network by service name, and an embedder +// forwarding a port out of the namespace dials the container there — +// which is what makes a silent default acceptable. It still warns, +// because trading a confusing "address already in use" for a silent +// no-op is not a fix: the message says where the service is reachable +// instead, so the user can delete the entry rather than wonder why it +// did nothing. +func (e *Engine) dropHostPortPublishes(project *composetypes.Project, opts UpOptions) { + if !e.opts.DisableHostPortPublishing { + return + } + for _, d := range compose.ApplyDropHostPorts(project) { + opts.bus.Emit(events.WarnEvent{ + Code: "compose_host_port_publish_skipped", + Message: fmt.Sprintf( + "compose service %q: not publishing %s on the host — host port publishing is disabled for this engine, whose host is shared with workloads the project does not own. "+ + "Reach the service as %s:%d from inside the project, or at the address your platform forwards; the ports: entry is unnecessary here and can be removed.", + d.Service, d, d.Service, d.Target), + }) + } +} + // composeBindMounts assembles the workspace + cfg + extra mounts in // the order the engine has always used them, kept as a helper so // both compose backends share the exact same set. Only bind mounts diff --git a/up_compose_host_ports_test.go b/up_compose_host_ports_test.go new file mode 100644 index 0000000..5c43b57 --- /dev/null +++ b/up_compose_host_ports_test.go @@ -0,0 +1,150 @@ +package devcontainer + +import ( + "context" + "strings" + "testing" + + composetypes "github.com/compose-spec/compose-go/v2/types" + + "github.com/crunchloop/devcontainer/compose" + "github.com/crunchloop/devcontainer/config" + "github.com/crunchloop/devcontainer/events" + "github.com/crunchloop/devcontainer/runtime" +) + +func hostPortEngine(t *testing.T, disablePublishing bool) (*Engine, *buildRecorder) { + t.Helper() + rt := &buildRecorder{fakeRuntime: newFakeRuntime()} + eng, err := New(EngineOptions{ + Runtime: &composeFake{buildRecorder: rt}, + ComposeBackend: ComposeBackendNative, + DisableHostPortPublishing: disablePublishing, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + return eng, rt +} + +// upComposeNative's inputs, minimal but real: one service publishing a +// host port, plus the workspace bind the run override always carries. +func hostPortUp(t *testing.T, eng *Engine, out chan events.Event) { + t.Helper() + project := sidecarProject(map[string]composetypes.ServiceConfig{ + "app": { + Image: "app:dev", + Ports: []composetypes.ServicePortConfig{ + {Target: 8080, Published: "8080", Protocol: "tcp"}, + }, + }, + }) + cfg := &config.ResolvedConfig{ + DevcontainerID: "dc-test", + LocalWorkspaceFolder: t.TempDir(), + ContainerWorkspaceFolder: "/workspaces/test", + } + opts := UpOptions{LocalEnv: map[string]string{}} + opts.bus = newEventBus(events.NewEmitter(nil), out) + + src := &config.ComposeSource{Service: "app"} + runOverride := compose.Override{ + Service: "app", + ExtraBindMounts: []compose.BindMount{ + {Source: cfg.LocalWorkspaceFolder, Target: cfg.ContainerWorkspaceFolder}, + }, + } + if _, err := eng.upComposeNative(context.Background(), cfg, opts, project, src, + "dc-test", t.TempDir(), "app:dev", runOverride); err != nil { + t.Fatalf("upComposeNative: %v", err) + } +} + +// The consumer check: the option has to reach the RunSpec the +// orchestrator hands the backend, not just the project it reads. +func TestUpComposeNative_DisableHostPortPublishingDropsPublishes(t *testing.T) { + eng, rt := hostPortEngine(t, true) + out := make(chan events.Event, 8) + + hostPortUp(t, eng, out) + + if got := rt.createdSpec.Ports; len(got) != 0 { + t.Errorf("RunSpec.Ports = %+v, want no host publish to reach the backend", got) + } + + close(out) + var warned *events.WarnEvent + for ev := range out { + if w, ok := ev.(events.WarnEvent); ok && w.Code == "compose_host_port_publish_skipped" { + warned = &w + break + } + } + if warned == nil { + t.Fatal("no compose_host_port_publish_skipped warning; a silent no-op is what this option must not be") + } + // The message has to point somewhere, not just report a removal. + for _, want := range []string{`"app"`, "8080:8080/tcp", "app:8080"} { + if !strings.Contains(warned.Message, want) { + t.Errorf("warning %q does not mention %q", warned.Message, want) + } + } +} + +// The drop is opt-in: without the option the publish still reaches the +// backend, which is what the CLI and any engine that owns its host +// depend on. +func TestUpComposeNative_PublishesByDefault(t *testing.T) { + eng, rt := hostPortEngine(t, false) + + hostPortUp(t, eng, nil) + + ports := rt.createdSpec.Ports + if len(ports) != 1 { + t.Fatalf("RunSpec.Ports = %+v, want the declared 8080 publish", ports) + } + if ports[0].HostPort != "8080" || ports[0].ContainerPort != 8080 { + t.Errorf("RunSpec.Ports[0] = %+v, want host 8080 -> container 8080", ports[0]) + } +} + +// R2 path parity: the shellout backend cannot honor the option — it +// hands ports: to `docker compose` — so it refuses instead of running +// with the option quietly ignored. The guard is the first thing on +// that path, ahead of any work. +func TestUpComposeShellout_RefusesDisableHostPortPublishing(t *testing.T) { + rt := &buildRecorder{fakeRuntime: newFakeRuntime()} + eng, err := New(EngineOptions{Runtime: rt, DisableHostPortPublishing: true}) + if err != nil { + t.Fatalf("New: %v", err) + } + + _, err = eng.upComposeShellout(context.Background(), nil, sidecarUpOptions(), nil, nil, + "dc-test", t.TempDir(), "app:dev", compose.Override{}, false) + if err == nil { + t.Fatal("upComposeShellout accepted DisableHostPortPublishing; it would publish on the daemon's host anyway") + } + if !strings.Contains(err.Error(), "ComposeBackendNative") { + t.Errorf("error %q does not say which backend honors the option", err) + } +} + +// composeFake answers the orchestrator primitives fakeRuntime leaves +// unimplemented, so a compose Up runs end to end in a unit test: +// no pre-existing containers, a network that creates, an image that +// resolves to a digest. +type composeFake struct { + *buildRecorder +} + +func (c *composeFake) CreateNetwork(ctx context.Context, spec runtime.NetworkSpec) (string, error) { + return "net-" + spec.Name, nil +} + +func (c *composeFake) ListContainers(ctx context.Context, filter runtime.LabelFilter) ([]runtime.Container, error) { + return nil, nil +} + +func (c *composeFake) InspectImage(ctx context.Context, ref string) (*runtime.ImageDetails, error) { + return &runtime.ImageDetails{ID: "sha256:" + ref}, nil +}