From 52c0ed1e77f7448debf7ff3777e8e89819a5adf5 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Tue, 30 Jun 2026 08:59:52 +1000 Subject: [PATCH] fix: free host-netns :80 for ECS bridge/host tasks via cred-endpoint DNAT (#446) * fix: free host-netns :80 for ECS bridge/host tasks via cred-endpoint DNAT bridge and host network-mode tasks share the VM host netns where the ecs-agent credential endpoint bound 169.254.170.2:80, so a task binding 0.0.0.0:80 hit EADDRINUSE and crash-looped. Bind the listener on the high proxy port 51679 and DNAT 169.254.170.2:80 to it (OUTPUT for host/bridge task traffic, PREROUTING for the awsvpc credential veth), keeping the dst IP on the endpoint so no route_localnet is needed. Nothing holds a socket on :80, so host/bridge tasks can bind it. e2e runs nginx on :80 in all three network modes; the settle guard catches a regression where a crash-looping :80 bind flaps RUNNING then STOPPED. * test: validate ECS host-netns :80 binding via bridge task The bridge subtest now runs nginx on :80 to prove the cred-endpoint DNAT frees the host netns (pre-fix it crash-looped on EADDRINUSE). host mode keeps a no-port command: it shares the same host netns, so two :80 tasks cannot coexist on one node (one :80 per host, as on AWS), and StopTask does not synchronously reap the prior container's port. --- cmd/ecs-agent/credendpoint.go | 71 +++++++++++++++++++++++++++--- cmd/ecs-agent/credendpoint_test.go | 39 ++++++++++++++++ tests/e2e/ecs/ecs_test.go | 20 +++++---- 3 files changed, 117 insertions(+), 13 deletions(-) diff --git a/cmd/ecs-agent/credendpoint.go b/cmd/ecs-agent/credendpoint.go index b242af8da..2fb863a28 100644 --- a/cmd/ecs-agent/credendpoint.go +++ b/cmd/ecs-agent/credendpoint.go @@ -22,6 +22,10 @@ const ( // container credentials; defaultCredEndpointPort is the standard HTTP port. defaultCredEndpointIP = "169.254.170.2" defaultCredEndpointPort = 80 + // defaultCredProxyPort is the high port the listener actually binds; a nat + // DNAT rule rewrites 169.254.170.2:80 to it so nothing holds a socket on :80 + // and host/bridge tasks sharing the netns can bind :80 themselves. + defaultCredProxyPort = 51679 // credRefreshMargin refreshes a cached credential set this long before expiry // so a container never starts a request with a token about to lapse. credRefreshMargin = 5 * time.Minute @@ -58,6 +62,7 @@ type credEndpoint struct { caPath string ip string port int + proxyPort int run netCmdRunner // assume mints fresh credentials for a role; defaults to AssumeRole over the @@ -90,6 +95,7 @@ func newCredEndpoint(provider credentials.CredentialsProvider, region, gatewayUR caPath: caPath, ip: ip, port: port, + proxyPort: defaultCredProxyPort, run: run, roles: map[string]string{}, cache: map[string]credEntry{}, @@ -217,16 +223,21 @@ func (c *credEndpoint) client() (*http.Client, error) { return hc, nil } -// Start adds the endpoint IP to a dummy interface in the host netns and serves -// HTTP on it. A bind or address failure is returned so the caller can log it -// without aborting register/heartbeat. +// Start adds the endpoint IP to a dummy interface in the host netns, serves HTTP +// on the high proxy port, and DNATs the advertised :80 to it. A bind, address, +// or nat failure is returned so the caller can log it without aborting +// register/heartbeat. func (c *credEndpoint) Start() error { if err := c.addEndpointAddr(); err != nil { return err } - ln, err := net.Listen("tcp", net.JoinHostPort(c.ip, strconv.Itoa(c.port))) + ln, err := net.Listen("tcp", net.JoinHostPort(c.ip, strconv.Itoa(c.listenPort()))) if err != nil { - return fmt.Errorf("listen %s:%d: %w", c.ip, c.port, err) + return fmt.Errorf("listen %s:%d: %w", c.ip, c.listenPort(), err) + } + if err := c.addRedirect(); err != nil { + _ = ln.Close() + return err } c.ln = ln c.srv = &http.Server{Handler: c, ReadHeaderTimeout: 5 * time.Second} @@ -250,10 +261,60 @@ func (c *credEndpoint) Stop() error { defer cancel() _ = c.srv.Shutdown(ctx) } + c.delRedirect() c.delEndpointAddr() return nil } +// listenPort is the port the socket binds. Loopback test mode binds the +// caller-supplied (ephemeral) port directly; the real endpoint binds the high +// proxy port and reaches :80 via the DNAT rules. +func (c *credEndpoint) listenPort() int { + if c.ip == "127.0.0.1" { + return c.port + } + return c.proxyPort +} + +// addRedirect installs nat DNAT rules rewriting the advertised :80 to the proxy +// port, for both locally generated traffic (host/bridge task containers, OUTPUT) +// and traffic arriving over the awsvpc credential veth (PREROUTING). The dst IP +// is left on the endpoint IP so no route_localnet handling is needed. Skipped in +// loopback test mode, which binds the port directly. +func (c *credEndpoint) addRedirect() error { + if c.run == nil || c.ip == "127.0.0.1" { + return nil + } + for _, chain := range []string{"OUTPUT", "PREROUTING"} { + if _, err := c.run.Run("iptables", c.natArgs("-C", chain)...); err == nil { + continue // rule already present + } + if _, err := c.run.Run("iptables", c.natArgs("-A", chain)...); err != nil { + return fmt.Errorf("add nat redirect %s: %w", chain, err) + } + } + return nil +} + +func (c *credEndpoint) delRedirect() { + if c.run == nil || c.ip == "127.0.0.1" { + return + } + for _, chain := range []string{"OUTPUT", "PREROUTING"} { + _, _ = c.run.Run("iptables", c.natArgs("-D", chain)...) + } +} + +// natArgs builds the iptables nat rule for op (-A/-C/-D) on chain: DNAT the +// advertised endpoint :80 to the proxy port on the same IP. +func (c *credEndpoint) natArgs(op, chain string) []string { + return []string{ + "-t", "nat", op, chain, + "-d", c.ip, "-p", "tcp", "--dport", strconv.Itoa(c.port), + "-j", "DNAT", "--to-destination", net.JoinHostPort(c.ip, strconv.Itoa(c.proxyPort)), + } +} + // addEndpointAddr brings up the dummy interface holding the endpoint IP. Skipped // for loopback (tests bind 127.0.0.1 directly, no interface plumbing needed). func (c *credEndpoint) addEndpointAddr() error { diff --git a/cmd/ecs-agent/credendpoint_test.go b/cmd/ecs-agent/credendpoint_test.go index c62a350a6..0266816d6 100644 --- a/cmd/ecs-agent/credendpoint_test.go +++ b/cmd/ecs-agent/credendpoint_test.go @@ -151,6 +151,45 @@ func TestCredEndpoint_StartServesOverLoopback(t *testing.T) { } } +func TestCredEndpoint_RedirectInstallsAndRemovesDNAT(t *testing.T) { + // failOn the -C existence check so addRedirect falls through to -A. + f := &fakeNetRunner{failOn: "-t nat -C"} + c := newCredEndpoint(nil, "us-east-1", "https://gw", "", "169.254.170.2", 80, f) + if c.listenPort() != defaultCredProxyPort { + t.Fatalf("listenPort = %d, want %d (proxy)", c.listenPort(), defaultCredProxyPort) + } + if err := c.addRedirect(); err != nil { + t.Fatalf("addRedirect: %v", err) + } + target := "-d 169.254.170.2 -p tcp --dport 80 -j DNAT --to-destination 169.254.170.2:51679" + for _, chain := range []string{"-A OUTPUT", "-A PREROUTING"} { + if !f.sawAny("iptables -t nat " + chain + " " + target) { + t.Errorf("missing add rule for %s; calls=%v", chain, f.joined()) + } + } + c.delRedirect() + for _, chain := range []string{"-D OUTPUT", "-D PREROUTING"} { + if !f.sawAny("iptables -t nat " + chain + " " + target) { + t.Errorf("missing delete rule for %s; calls=%v", chain, f.joined()) + } + } +} + +func TestCredEndpoint_RedirectSkippedOnLoopback(t *testing.T) { + f := &fakeNetRunner{} + c := newCredEndpoint(nil, "us-east-1", "https://gw", "", "127.0.0.1", 0, f) + if err := c.addRedirect(); err != nil { + t.Fatalf("addRedirect: %v", err) + } + c.delRedirect() + if f.sawAny("iptables") { + t.Errorf("loopback mode must not touch iptables; calls=%v", f.joined()) + } + if c.listenPort() != c.port { + t.Errorf("loopback listenPort = %d, want caller port %d", c.listenPort(), c.port) + } +} + func TestCredRelativeURI(t *testing.T) { if got := credRelativeURI("abc"); got != "/v2/credentials/abc" { t.Errorf("credRelativeURI = %q", got) diff --git a/tests/e2e/ecs/ecs_test.go b/tests/e2e/ecs/ecs_test.go index fbd36993c..a9900ffab 100644 --- a/tests/e2e/ecs/ecs_test.go +++ b/tests/e2e/ecs/ecs_test.go @@ -32,10 +32,14 @@ const ( ecsSubnetBCIDR = "10.41.2.0/24" ecsClusterPfx = "ecs-e2e" ecsInstanceTyp = "t3.small" - // awsvpc tasks serve HTTP on 80 (isolated ENI netns) so the ALB target group - // health check (GET /) passes. bridge/host tasks share the host netns, where - // the agent credential endpoint owns 169.254.170.2:80, so they run a durable - // no-port command instead and only assert placement + mode, not serving. + // awsvpc and bridge tasks serve nginx on :80. bridge shares the VM host netns + // where the agent credential endpoint used to own 169.254.170.2:80; it now + // binds :80 cleanly because the endpoint moved to a high proxy port + DNAT, so + // the bridge subtest is the regression proof (the settle guard in + // waitTaskRunning catches a crash-looping :80 bind that flaps RUNNING then + // STOPPED). host mode also shares that netns, so only one host-netns task can + // hold :80 at a time (one :80 per host, as on AWS); the host subtest runs a + // durable no-port command to assert placement without contending for :80. ecsTaskImage = "docker.io/library/nginx:1.27-alpine" ecsHostNetnsCmd = "sleep 600" ) @@ -86,7 +90,7 @@ func TestECS(t *testing.T) { }) t.Run("TaskBridge", func(t *testing.T) { - tdArn := registerTaskDef(t, c, fx, "bridge", strings.Fields(ecsHostNetnsCmd)) + tdArn := registerTaskDef(t, c, fx, "bridge", nil) task := runStandaloneTask(t, c, fx, tdArn, nil) assert.Empty(t, task.Attachments, "bridge task must not allocate an ENI") }) @@ -513,9 +517,9 @@ func waitContainerInstanceActive(t *testing.T, c *harness.AWSClient, cluster, ec // --- Task definitions & standalone tasks ---------------------------------- // registerTaskDef registers a one-container task definition. command overrides -// the image entrypoint; when set the container exposes no port (used by the -// host-netns modes to avoid the credential endpoint's :80). When command is nil -// the container runs nginx and publishes 80 for ALB/awsvpc serving. +// the image entrypoint and exposes no port (the host subtest uses it so two +// host-netns tasks don't contend for :80); when command is nil the container +// runs nginx and publishes 80 for awsvpc/bridge serving. func registerTaskDef(t *testing.T, c *harness.AWSClient, fx *ecsFixture, networkMode string, command []string) string { t.Helper() family := fmt.Sprintf("%s-%s", fx.ClusterName, networkMode)