fix false positives when probing local ports already used

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"),

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::<port>-` 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

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) {

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,