libnetwork: fail the join when ingress rules fail

A task on the ingress network needs rules in its own network namespace
that redirect each published port to its target port and filter all
other traffic addressed to its ingress endpoint. Adding them can fail,
for example when the kernel lacks a feature the rules use.
populateLoadBalancers logged that failure and let the join succeed. The
task then ran, and was reported as running, without serving its
published ports and with every port it listens on reachable from the
rest of the ingress network. The only trace was the daemon log.

Return the error instead, so the join fails and the task fails with the
reason. This applies to both firewall backends.

Signed-off-by: Cory Snider <csnider@mirantis.com>

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

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

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

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

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.