diff --git a/daemon/libnetwork/sandbox_linux.go b/daemon/libnetwork/sandbox_linux.go index 58d89e95e1..d2abad5d17 100644 --- a/daemon/libnetwork/sandbox_linux.go +++ b/daemon/libnetwork/sandbox_linux.go @@ -459,7 +459,9 @@ func (sb *Sandbox) populateNetworkResourcesOS(ctx context.Context, ep *Endpoint) // information including gateway and other routes so that // loadbalancers are populated all the network state is in // place in the sandbox. - sb.populateLoadBalancers(ep) + if err := sb.populateLoadBalancers(ep); err != nil { + return err + } // Only update the store if we did not come here as part of // sandbox delete. If we came here as part of delete then do diff --git a/daemon/libnetwork/service_linux.go b/daemon/libnetwork/service_linux.go index 470db44599..c679d058d9 100644 --- a/daemon/libnetwork/service_linux.go +++ b/daemon/libnetwork/service_linux.go @@ -43,10 +43,16 @@ type osLoadBalancer struct { // Populate all loadbalancers on the network that the passed endpoint // belongs to, into this sandbox. -func (sb *Sandbox) populateLoadBalancers(ep *Endpoint) { +// +// On the ingress network that means the rules that redirect published ports to +// target ports and filter everything else addressed to the endpoint. Without +// them the task can't serve its published ports, and other endpoints on the +// ingress network can reach any port it listens on. So a failure is returned, +// to fail the join, rather than leaving the task running without them. +func (sb *Sandbox) populateLoadBalancers(ep *Endpoint) error { // This is an interface less endpoint. Nothing to do. if ep.Iface() == nil { - return + return nil } n := ep.getNetwork() @@ -54,9 +60,10 @@ func (sb *Sandbox) populateLoadBalancers(ep *Endpoint) { if n.ingress { if err := sb.addRedirectRules(eIP, ep.ingressPorts); err != nil { - log.G(context.TODO()).Errorf("Failed to add redirect rules for ep %s (%.7s): %v", ep.Name(), ep.ID(), err) + return fmt.Errorf("failed to add ingress redirect rules for endpoint %s (%.7s): %w", ep.Name(), ep.ID(), err) } } + return nil } func (n *Network) findLBEndpointSandbox() (*Endpoint, *Sandbox, error) { diff --git a/daemon/libnetwork/service_unsupported.go b/daemon/libnetwork/service_unsupported.go index 0afefa7b62..c7409a7026 100644 --- a/daemon/libnetwork/service_unsupported.go +++ b/daemon/libnetwork/service_unsupported.go @@ -25,6 +25,6 @@ func (c *Controller) rmServiceBinding(name, sid, nid, eid string, vip net.IP, in return errors.New("not supported") } -func (sb *Sandbox) populateLoadBalancers(*Endpoint) {} +func (sb *Sandbox) populateLoadBalancers(*Endpoint) error { return nil } func arrangeIngressFilterRule() {} diff --git a/daemon/libnetwork/service_windows.go b/daemon/libnetwork/service_windows.go index 6789a9e6ce..dc2f4f4d72 100644 --- a/daemon/libnetwork/service_windows.go +++ b/daemon/libnetwork/service_windows.go @@ -168,7 +168,8 @@ func numEnabledBackends(lb *loadBalancer) int { return nEnabled } -func (sb *Sandbox) populateLoadBalancers(ep *Endpoint) { +func (sb *Sandbox) populateLoadBalancers(ep *Endpoint) error { + return nil } func arrangeIngressFilterRule() { diff --git a/integration/service/network_linux_test.go b/integration/service/network_linux_test.go index 068018be45..a097cb51a9 100644 --- a/integration/service/network_linux_test.go +++ b/integration/service/network_linux_test.go @@ -688,10 +688,10 @@ func TestIngressPortsAfterFailedPublish(t *testing.T) { // daemon needs a working firewall for everything from bringing up the bridge to // publishing the first port. // - // With nftables, the port also appears in the task's own ingress table and in - // the ingress load balancer's table. Those are let through: a load balancer - // that can't be programmed never gets as far as publishing, and this test is - // about the publish on the host. + // The port also appears in the task's own ingress rules and, with nftables, in + // the ingress load balancer's table. Those are let through: a task that can't + // add its rules fails to start, a load balancer that can't be programmed never + // gets as far as publishing, and this test is about the publish on the host. refusedPort := strconv.Itoa(refusedIngressPort) binDir := t.TempDir() refusals := filepath.Join(binDir, "refusals") @@ -717,8 +717,10 @@ case "$in" in *docker-bridges*) esac printf '%%s\n' "$in" | exec %[3]s "$@"`, refusedPort, refusals, realFwTool), 0o755)) } else { + // The task's ingress rule is the only one using REDIRECT. assert.NilError(t, os.WriteFile(filepath.Join(binDir, fwTool), fmt.Appendf(nil, `#!/bin/sh +case "$*" in *REDIRECT*) exec %[3]s "$@" ;; esac for arg in $@; do if [ $arg = %[1]q ]; then echo "$*" >> %[2]q @@ -794,6 +796,74 @@ for arg in $@; do }) } +// TestIngressTaskFailsWithoutRedirectRules checks that a task whose ingress +// rules can't be added to its network namespace fails to start, rather than +// running without them. +func TestIngressTaskFailsWithoutRedirectRules(t *testing.T) { + skip.If(t, testEnv.IsRemoteDaemon) + skip.If(t, testEnv.IsRootless, "rootless mode doesn't support Swarm-mode") + skip.If(t, networking.FirewalldRunning(), "can't use firewalld in host netns to add rules in L3Segment") + ctx := setupTest(t) + + const l3SegHost = "itfr" + l3 := networking.NewL3Segment(t, "test-"+l3SegHost) + defer l3.Destroy(t) + l3.AddHost(t, l3SegHost, "ns-"+l3SegHost, "eth0", netip.MustParsePrefix("192.168.114.222/24")) + + // The daemon finds iptables and nft on its PATH, so a wrapper ahead of the + // real one can refuse the task's ingress rules and nothing else. + fwTool := "iptables" + // Only a task's ingress rules use REDIRECT. + wrapper := `#!/bin/sh +case "$*" in *REDIRECT*) echo "iptables: refused by test wrapper" >&2; exit 1 ;; esac +exec %s "$@"` + if strings.HasPrefix(testEnv.FirewallBackendDriver(), "nftables") { + fwTool = "nft" + // Each update is an nft script on stdin. A task's ingress rules are the + // docker-container-ingress table. + wrapper = `#!/bin/sh +in=$(cat) +case "$in" in *docker-container-ingress*) echo "nft: refused by test wrapper" >&2; exit 1 ;; esac +printf '%%s\n' "$in" | exec %s "$@"` + } + realFwTool, err := exec.LookPath(fwTool) + assert.NilError(t, err) + binDir := t.TempDir() + assert.NilError(t, os.WriteFile(filepath.Join(binDir, fwTool), fmt.Appendf(nil, wrapper, realFwTool), 0o755)) + + l3.Hosts[l3SegHost].Do(t, func() { + d := swarm.NewSwarm(ctx, t, testEnv, daemon.WithSwarmIptables(true), + daemon.WithEnvVars("PATH="+binDir+":"+os.Getenv("PATH"))) + defer d.Stop(t) + c := d.NewClientT(t) + defer c.Close() + + serviceID := swarm.CreateService(ctx, t, d, + swarm.ServiceWithName("test-"+l3SegHost), + swarm.ServiceWithCommand([]string{"httpd", "-f"}), + swarm.ServiceWithEndpoint(ingressPortSpec(8080)), + swarm.ServiceWithReplicas(1), + ) + poll.WaitOn(t, func(_ poll.LogT) poll.Result { + tasks, err := c.TaskList(ctx, client.TaskListOptions{ + Filters: make(client.Filters).Add("service", serviceID), + }) + if err != nil { + return poll.Error(err) + } + for _, task := range tasks.Items { + if task.Status.State == swarmtypes.TaskStateRunning { + return poll.Error(fmt.Errorf("task %s is running without its ingress rules", task.ID)) + } + if strings.Contains(task.Status.Err, "ingress redirect rules") { + return poll.Success() + } + } + return poll.Continue("waiting for a task to fail, tasks: %d", len(tasks.Items)) + }, swarm.ServicePoll) + }) +} + // TestSwarmPublishedPortsOverIPv6 checks that the ports a Swarm service publishes // are reachable over IPv6 - and that when the daemon is configured so they can't // be, an IPv6 client is refused rather than left waiting.