From d5df9726581061902dfbf5db7d9c4b3c6a3b3cdf Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Mon, 31 Aug 2026 09:17:34 +0200 Subject: [PATCH] fix(preflight): probe the address the provider actually binds checkHostPort bound 127.0.0.1 and reported every failure as "host port N in use (likely another cluster)". Two distinct conflations came out of that, each of which blocked ystack's docker acceptance run on a cluster whose ports were entirely free. A refused bind is not a taken port. Ports under 1024 need privilege, which the probe does not have, so a non-root provision of ystack's `{host: "80", guest: "80"}` forward failed preflight on a host where `docker run -p 80:80` publishes fine. Split EADDRINUSE from EACCES and let the caller say who does the binding: under PortBinderDaemon (docker) a refused probe is evidence of nothing, since dockerd or Docker Desktop's root helper does the bind; under PortBinderSelf (qemu, which binds hostfwd in-process as the invoking user) it is a real blocker, now reported as a privilege problem rather than as a phantom second cluster. Where the probe can't answer, a connect probe asks the weaker question any user may ask. The probe address was wrong too. Both providers bind the wildcard -- docker's HostIP is 0.0.0.0, qemu's `hostfwd=tcp::-` leaves the host address empty -- and Go sets SO_REUSEADDR on every listener, so on BSD a loopback probe binds happily beside a wildcard listener that already owns the port. A host-local `y-cluster serve` on *:8944 thus passed preflight and took the provision down with the daemon's own "address already in use". Probe 0.0.0.0, plus loopback for the mirror-image case where a 127.0.0.1 listener leaves the provider's bind intact but steals the traffic the cluster is reached on. The two platforms disagree in ways the tests now pin down. Darwin skips the reserved-port check for INADDR_ANY only, so any user binds 0.0.0.0:80 while 127.0.0.1:80 is refused; Linux enforces the restriction on every address. The privileged-port tests skip where the environment cannot produce EACCES and run on Linux CI. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/provision/docker/docker.go | 1 + pkg/provision/preflight.go | 120 ++++++++++++++++++++++++++++---- pkg/provision/preflight_test.go | 112 ++++++++++++++++++++++++++++- pkg/provision/qemu/qemu.go | 1 + 4 files changed, 218 insertions(+), 16 deletions(-) diff --git a/pkg/provision/docker/docker.go b/pkg/provision/docker/docker.go index 4f66043..86858da 100644 --- a/pkg/provision/docker/docker.go +++ b/pkg/provision/docker/docker.go @@ -117,6 +117,7 @@ func Provision(ctx context.Context, cfg config.DockerConfig, logger *zap.Logger) // names the port but not what to change in y-cluster's config). pf := provision.Preflight{ HostPorts: dockerHostPorts(cfg), + PortBinder: provision.PortBinderDaemon, ContextName: cfg.Context, ContextCluster: cfg.Name, KubeconfigPath: os.Getenv("KUBECONFIG"), diff --git a/pkg/provision/preflight.go b/pkg/provision/preflight.go index 254d2fb..a37f14b 100644 --- a/pkg/provision/preflight.go +++ b/pkg/provision/preflight.go @@ -7,10 +7,34 @@ import ( "os" "path/filepath" "strings" + "syscall" + "time" "github.com/Yolean/y-cluster/pkg/kubeconfig" ) +// PortBinder names the process that ends up binding the host ports, +// which is what decides how to read a bind probe that comes back +// "permission denied". +type PortBinder int + +const ( + // PortBinderDaemon: a privileged daemon binds the host port and + // hands the socket to an unprivileged process -- dockerd, or + // Docker Desktop's root helper passing the fd to + // com.docker.backend. y-cluster's own privileges have no bearing + // on whether that bind succeeds, so a refused probe is evidence + // of nothing. + PortBinderDaemon PortBinder = iota + + // PortBinderSelf: the provisioner spawns the binding process as + // this user -- qemu's `-netdev user,hostfwd=tcp::80-:80` binds + // the host side in-process. Here the probe is exactly the bind + // the provision will attempt, so a refusal is a real blocker + // worth failing fast on. + PortBinderSelf +) + // Preflight runs cross-provisioner checks BEFORE any state-mutating // step in Provision. The point is to fail fast with an actionable // message ("host port 6443 already bound; change portForwards in @@ -20,7 +44,9 @@ import ( // Two classes of check: // // - HostPorts: every entry must currently be free. Empty values -// skip (provider auto-assigns). +// skip (provider auto-assigns). PortBinder says who does the +// binding, which is what makes an unbindable privileged port +// either a hard blocker or none of our business. // - KubeconfigContext: the context name must be either absent or // already pointing at clusterName. A second cluster that // reuses an existing context name would clobber the first @@ -30,10 +56,11 @@ import ( // running") layer on top in the per-provider Provision; they're // not generalisable. type Preflight struct { - HostPorts []string - ContextName string - ContextCluster string - KubeconfigPath string // empty -> kubectl-style env+default search + HostPorts []string + PortBinder PortBinder + ContextName string + ContextCluster string + KubeconfigPath string // empty -> kubectl-style env+default search } // Run executes every check, accumulating errors so the caller @@ -43,7 +70,7 @@ type Preflight struct { func (p Preflight) Run() error { var problems []string for _, port := range p.HostPorts { - if err := checkHostPort(port); err != nil { + if err := checkHostPort(port, p.PortBinder); err != nil { problems = append(problems, err.Error()) } } @@ -58,23 +85,90 @@ func (p Preflight) Run() error { return fmt.Errorf("preflight checks failed:\n - %s", strings.Join(problems, "\n - ")) } +// hostPortDialTimeout bounds the fallback connect probe. The target +// is loopback, so anything that hasn't answered by then isn't going +// to. +const hostPortDialTimeout = 250 * time.Millisecond + // checkHostPort verifies port (a string for cobra-friendliness) is -// not currently bound on 127.0.0.1. Probes by binding briefly and +// free for the provider to bind. Probes by binding briefly and // closing immediately. Race window is negligible for human-driven // provisions. -func checkHostPort(port string) error { +// +// The authoritative probe is against the IPv4 wildcard, because +// that is what both providers bind: docker sets HostIP to 0.0.0.0, +// and qemu's `hostfwd=tcp::-` leaves the host address empty, +// which slirp reads as 0.0.0.0. +// +// Go sets SO_REUSEADDR on every listener, and on BSD that lets a +// wildcard and a loopback bind of one port coexist, so neither +// address alone sees every conflict. A second, loopback probe +// covers the other half: a listener on 127.0.0.1 doesn't block the +// provider's wildcard bind, but it does take the loopback traffic +// the cluster is reached on. Only EADDRINUSE counts there -- +// Darwin refuses every loopback bind under port 1024 whether or +// not the port is free, because XNU skips the reserved-port check +// for INADDR_ANY only. +// +// Ports below 1024 need privilege to bind on Linux (and on Darwin +// off the wildcard), which the probe usually lacks, so "bind +// refused" and "port taken" are different answers: EACCES means +// "can't tell from here", not "in use". What's left is the weaker +// question any user may ask -- is something accepting connections +// there -- plus, under PortBinderSelf, an error that names the +// privilege as the problem instead of blaming another cluster. +func checkHostPort(port string, binder PortBinder) error { if port == "" { return nil // provider auto-assigns } - addr := net.JoinHostPort("127.0.0.1", port) - l, err := net.Listen("tcp", addr) - if err != nil { - return fmt.Errorf("host port %s in use (likely another cluster); change the binding in the config", port) + l, err := net.Listen("tcp", net.JoinHostPort("0.0.0.0", port)) + switch { + case err == nil: + _ = l.Close() + if lo, loErr := net.Listen("tcp", net.JoinHostPort("127.0.0.1", port)); loErr == nil { + _ = lo.Close() + } else if errors.Is(loErr, syscall.EADDRINUSE) { + return errHostPortInUse(port) + } + return nil + case errors.Is(err, syscall.EADDRINUSE): + return errHostPortInUse(port) + case !errors.Is(err, os.ErrPermission): + return fmt.Errorf("host port %s: bind probe failed: %w", port, err) + } + // A privileged port can't be bind-probed, but a wildcard + // listener on it still answers on loopback. + if hostPortAnswers(net.JoinHostPort("127.0.0.1", port)) { + return errHostPortInUse(port) + } + if binder == PortBinderSelf { + return fmt.Errorf( + "host port %s: nothing is listening, but this user may not bind a "+ + "privileged port and the provider binds host ports as you. Run as "+ + "root, grant the binary CAP_NET_BIND_SERVICE, or map the forward to "+ + "a host port above 1023 in the config", port) } - _ = l.Close() return nil } +func errHostPortInUse(port string) error { + return fmt.Errorf("host port %s in use (likely another cluster); change the binding in the config", port) +} + +// hostPortAnswers reports whether something is accepting TCP +// connections on addr. Weaker than the bind probe -- a listener with +// a full backlog, or one bound to a single non-loopback interface, +// escapes it -- but it needs no privilege, so it is the only probe +// left once the bind is refused. +func hostPortAnswers(addr string) bool { + c, err := net.DialTimeout("tcp", addr, hostPortDialTimeout) + if err != nil { + return false + } + _ = c.Close() + return true +} + // checkKubeconfigContext returns nil when the context is absent // or already points at expectedCluster. Otherwise the context // belongs to a different cluster and re-using it for a new diff --git a/pkg/provision/preflight_test.go b/pkg/provision/preflight_test.go index 3f037dc..50fa3a5 100644 --- a/pkg/provision/preflight_test.go +++ b/pkg/provision/preflight_test.go @@ -1,6 +1,7 @@ package provision import ( + "errors" "net" "os" "path/filepath" @@ -12,7 +13,7 @@ import ( // nobody is listening on passes the check. func TestPreflight_PortFree(t *testing.T) { port := pickFreePort(t) - if err := checkHostPort(port); err != nil { + if err := checkHostPort(port, PortBinderDaemon); err != nil { t.Fatalf("free port %s: %v", port, err) } } @@ -26,7 +27,7 @@ func TestPreflight_PortInUse(t *testing.T) { } defer func() { _ = l.Close() }() port := portFromAddr(l.Addr().String()) - err = checkHostPort(port) + err = checkHostPort(port, PortBinderDaemon) if err == nil { t.Fatalf("port %s should report in-use", port) } @@ -35,14 +36,119 @@ func TestPreflight_PortInUse(t *testing.T) { } } +// TestPreflight_WildcardListenerInUse is the ystack 8944 +// regression: a host-local `y-cluster serve` holds *:8944, the +// provision then dies on the daemon's "address already in use". +// A loopback probe misses it -- SO_REUSEADDR lets 127.0.0.1:port +// bind alongside the wildcard on BSD -- so the check has to probe +// the wildcard, which is what the provider binds anyway. +func TestPreflight_WildcardListenerInUse(t *testing.T) { + l, err := net.Listen("tcp", "0.0.0.0:0") + if err != nil { + t.Fatal(err) + } + defer func() { _ = l.Close() }() + port := portFromAddr(l.Addr().String()) + + // Establish that this is the case a loopback probe lets through, + // so the test keeps meaning what it says if the probe address + // ever changes back. + if lo, err := net.Listen("tcp", net.JoinHostPort("127.0.0.1", port)); err == nil { + _ = lo.Close() + } else { + t.Logf("loopback bind alongside wildcard already refused here (%v); "+ + "this platform would have caught the conflict either way", err) + } + + err = checkHostPort(port, PortBinderDaemon) + if err == nil { + t.Fatalf("wildcard listener on port %s should report in-use", port) + } + if !strings.Contains(err.Error(), port) || !strings.Contains(err.Error(), "in use") { + t.Fatalf("error should name the port and 'in use': %v", err) + } +} + // TestPreflight_PortEmpty: an empty Host (provider auto-assigns) // must not error -- there's nothing to check. func TestPreflight_PortEmpty(t *testing.T) { - if err := checkHostPort(""); err != nil { + if err := checkHostPort("", PortBinderDaemon); err != nil { t.Fatalf("empty port should pass: %v", err) } } +// TestPreflight_PrivilegedPortDaemonBinder is the ystack regression: +// `portForwards: [{host: "80", guest: "80"}]` on the docker +// provider, provisioned by a non-root user. The bind probe is +// refused for lack of privilege while the port is in fact free and +// dockerd would publish it happily, so the check must pass. +func TestPreflight_PrivilegedPortDaemonBinder(t *testing.T) { + port := unbindablePortOrSkip(t) + if err := checkHostPort(port, PortBinderDaemon); err != nil { + t.Fatalf("free privileged port %s under a daemon binder: %v", port, err) + } +} + +// TestPreflight_PrivilegedPortSelfBinder: the same refused probe, +// but qemu binds hostfwd ports as this user, so the port really is +// unusable. Fail -- and say why, rather than pinning it on another +// cluster that doesn't exist. +func TestPreflight_PrivilegedPortSelfBinder(t *testing.T) { + port := unbindablePortOrSkip(t) + err := checkHostPort(port, PortBinderSelf) + if err == nil { + t.Fatalf("privileged port %s under a self binder should error", port) + } + if !strings.Contains(err.Error(), port) { + t.Fatalf("error should name the port: %v", err) + } + if strings.Contains(err.Error(), "in use") { + t.Fatalf("permission denied must not be reported as in-use: %v", err) + } +} + +// TestPreflight_HostPortAnswers covers the fallback probe on its +// own, since the privileged-port path can't be set up both ways in +// one unprivileged test: it has to spot a live listener without +// binding anything, and stop spotting it once the listener closes. +func TestPreflight_HostPortAnswers(t *testing.T) { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := l.Addr().String() + if !hostPortAnswers(addr) { + t.Fatalf("live listener at %s should answer", addr) + } + if err := l.Close(); err != nil { + t.Fatal(err) + } + if hostPortAnswers(addr) { + t.Fatalf("closed listener at %s should not answer", addr) + } +} + +// unbindablePortOrSkip returns a privileged port that this process +// can neither bind nor reach, so checkHostPort's permission branch +// is the one under test. Skips where the environment can't produce +// that: Darwin, which lets any user bind a reserved port on the +// wildcard; running as root; an unprivileged range reaching this +// far down (containers default net.ipv4.ip_unprivileged_port_start +// to 0); or something already listening. +func unbindablePortOrSkip(t *testing.T) string { + t.Helper() + const port = "1023" // highest privileged port, least likely to be claimed + l, err := net.Listen("tcp", net.JoinHostPort("0.0.0.0", port)) + if err == nil { + _ = l.Close() + t.Skipf("port %s is bindable here; no permission denial to test", port) + } + if !errors.Is(err, os.ErrPermission) { + t.Skipf("port %s unavailable for a reason other than permission: %v", port, err) + } + return port +} + // TestPreflight_ContextAbsent: a context name that doesn't exist // in kubeconfig is fine -- there's nothing to clobber. func TestPreflight_ContextAbsent(t *testing.T) { diff --git a/pkg/provision/qemu/qemu.go b/pkg/provision/qemu/qemu.go index 9be2177..4fc4caa 100644 --- a/pkg/provision/qemu/qemu.go +++ b/pkg/provision/qemu/qemu.go @@ -244,6 +244,7 @@ func Provision(ctx context.Context, cfg Config, logger *zap.Logger) (*Cluster, e // the user fixes them in one config edit, not three. pf := provision.Preflight{ HostPorts: preflightHostPorts(cfg), + PortBinder: provision.PortBinderSelf, ContextName: cfg.Context, ContextCluster: clusterName(cfg.Name), KubeconfigPath: cfg.Kubeconfig,