Browse code

Fix removal of legacy links

It's possible to remove a legacy link from running containers.
When that happens, the Sandbox's Endpoints are removed and
re-added.

Since commit 65120d5 ("Create bridge veth in container netns")
the veth device has been created in the container's netns. When
that happens, a flag is set on the Endpoint to note that it
does not need to be moved into the netns.

But, during the Leave/Join (Sandbox.Refresh) the veth is moved
out of the netns. So, clear the flag during the Leave, to note
that it needs to be moved back in during the Join.

Signed-off-by: Rob Murray <rob.murray@docker.com>

Rob Murray authored on 2025/04/10 03:28:52
Showing 3 changed files
... ...
@@ -31,7 +31,6 @@ import (
31 31
 	"github.com/docker/docker/integration-cli/cli/build"
32 32
 	"github.com/docker/docker/integration-cli/daemon"
33 33
 	"github.com/docker/docker/libnetwork/drivers/bridge"
34
-	"github.com/docker/docker/libnetwork/iptables"
35 34
 	"github.com/docker/docker/opts"
36 35
 	"github.com/docker/docker/testutil"
37 36
 	testdaemon "github.com/docker/docker/testutil/daemon"
... ...
@@ -795,44 +794,6 @@ func (s *DockerDaemonSuite) TestDaemonICCLinkExpose(c *testing.T) {
795 795
 	assert.NilError(c, err, out)
796 796
 }
797 797
 
798
-func (s *DockerDaemonSuite) TestDaemonLinksIpTablesRulesWhenLinkAndUnlink(c *testing.T) {
799
-	// make sure the default docker0 bridge doesn't interfere with the test,
800
-	// which may happen if it was created with the same IP range.
801
-	deleteInterface(c, "docker0")
802
-
803
-	bridgeName := "ext-bridge7"
804
-	bridgeIP := "192.169.1.1/24"
805
-
806
-	createInterface(c, "bridge", bridgeName, bridgeIP)
807
-	defer deleteInterface(c, bridgeName)
808
-
809
-	s.d.StartWithBusybox(testutil.GetContext(c), c, "--bridge", bridgeName, "--icc=false")
810
-	defer s.d.Restart(c)
811
-
812
-	out, err := s.d.Cmd("run", "-d", "--name", "child", "--publish", "8080:80", "busybox", "top")
813
-	assert.NilError(c, err, out)
814
-	out, err = s.d.Cmd("run", "-d", "--name", "parent", "--link", "child:http", "busybox", "top")
815
-	assert.NilError(c, err, out)
816
-
817
-	childIP := s.d.FindContainerIP(c, "child")
818
-	parentIP := s.d.FindContainerIP(c, "parent")
819
-
820
-	sourceRule := []string{"-i", bridgeName, "-o", bridgeName, "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"}
821
-	destinationRule := []string{"-i", bridgeName, "-o", bridgeName, "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"}
822
-	iptable := iptables.GetIptable(iptables.IPv4)
823
-	if !iptable.Exists("filter", "DOCKER", sourceRule...) || !iptable.Exists("filter", "DOCKER", destinationRule...) {
824
-		c.Fatal("Iptables rules not found")
825
-	}
826
-
827
-	s.d.Cmd("rm", "--link", "parent/http")
828
-	if iptable.Exists("filter", "DOCKER", sourceRule...) || iptable.Exists("filter", "DOCKER", destinationRule...) {
829
-		c.Fatal("Iptables rules should be removed when unlink")
830
-	}
831
-
832
-	s.d.Cmd("kill", "child")
833
-	s.d.Cmd("kill", "parent")
834
-}
835
-
836 798
 func (s *DockerDaemonSuite) TestDaemonUlimitDefaults(c *testing.T) {
837 799
 	s.d.StartWithBusybox(testutil.GetContext(c), c, "--default-ulimit", "nofile=42:42", "--default-ulimit", "nproc=1024:1024")
838 800
 
... ...
@@ -558,3 +558,65 @@ func TestFirewalldReloadNoZombies(t *testing.T) {
558 558
 	assert.Check(t, !strings.Contains(resAfterReload.Combined(), bridgeName),
559 559
 		"After deletes: did not expect rules for %s in: %s", bridgeName, resAfterReload.Combined())
560 560
 }
561
+
562
+// TestRemoveLegacyLink checks that a legacy link can be deleted while the
563
+// linked containers are running.
564
+//
565
+// Replacement for DockerDaemonSuite/TestDaemonLinksIpTablesRulesWhenLinkAndUnlink
566
+func TestRemoveLegacyLink(t *testing.T) {
567
+	ctx := setupTest(t)
568
+
569
+	// Tidy up after the test by starting a new daemon, which will remove the icc=false
570
+	// rules this test will create for docker0.
571
+	defer func() {
572
+		d := daemon.New(t)
573
+		d.StartWithBusybox(ctx, t)
574
+		defer d.Stop(t)
575
+	}()
576
+
577
+	d := daemon.New(t)
578
+	d.StartWithBusybox(ctx, t, "--icc=false")
579
+	defer d.Stop(t)
580
+	c := d.NewClientT(t)
581
+
582
+	// Run an http server.
583
+	const svrName = "svr"
584
+	svrId := ctr.Run(ctx, t, c,
585
+		ctr.WithExposedPorts("80/tcp"),
586
+		ctr.WithName(svrName),
587
+		ctr.WithCmd("httpd", "-f"),
588
+	)
589
+	defer ctr.Remove(ctx, t, c, svrId, containertypes.RemoveOptions{Force: true})
590
+
591
+	// Run a container linked to the http server.
592
+	const svrAlias = "thealias"
593
+	const clientName = "client"
594
+	clientId := ctr.Run(ctx, t, c,
595
+		ctr.WithName(clientName),
596
+		ctr.WithLinks(svrName+":"+svrAlias),
597
+	)
598
+	defer ctr.Remove(ctx, t, c, clientId, containertypes.RemoveOptions{Force: true})
599
+
600
+	// Check the link works.
601
+	res := ctr.ExecT(ctx, t, c, clientId, []string{"wget", "-T3", "http://" + svrName})
602
+	assert.Check(t, is.Contains(res.Stderr(), "404 Not Found"))
603
+
604
+	// Remove the link ("docker rm --link client/thealias").
605
+	err := c.ContainerRemove(ctx, clientName+"/"+svrAlias, containertypes.RemoveOptions{RemoveLinks: true})
606
+	assert.Check(t, err)
607
+
608
+	// Check both containers are still running.
609
+	inspSvr := ctr.Inspect(ctx, t, c, svrId)
610
+	assert.Check(t, is.Equal(inspSvr.State.Running, true))
611
+	inspClient := ctr.Inspect(ctx, t, c, clientId)
612
+	assert.Check(t, is.Equal(inspClient.State.Running, true))
613
+
614
+	// Check the link's alias doesn't work.
615
+	res = ctr.ExecT(ctx, t, c, clientId, []string{"wget", "-T3", "http://" + svrName})
616
+	assert.Check(t, is.Contains(res.Stderr(), "bad address"))
617
+
618
+	// Check the icc=false rules now block access by address.
619
+	svrAddr := inspSvr.NetworkSettings.Networks["bridge"].IPAddress
620
+	res = ctr.ExecT(ctx, t, c, clientId, []string{"wget", "-T3", "http://" + svrAddr})
621
+	assert.Check(t, is.Contains(res.Stderr(), "download timed out"))
622
+}
... ...
@@ -899,6 +899,12 @@ func (ep *Endpoint) sbLeave(ctx context.Context, sb *Sandbox, force bool) error
899 899
 		log.G(ctx).WithError(err).Warn("Failed to clean up network resources on container disconnect")
900 900
 	}
901 901
 
902
+	// Even if the interface was initially created in the container's namespace, it's
903
+	// now been moved out. When a legacy link is deleted, the Endpoint is removed and
904
+	// then re-added to the Sandbox. So, to make sure the re-add works, note that the
905
+	// interface is now outside the container's netns.
906
+	ep.iface.createdInContainer = false
907
+
902 908
 	// Update the store about the sandbox detach only after we
903 909
 	// have completed sb.clearNetworkResources above to avoid
904 910
 	// spurious logs when cleaning up the sandbox when the daemon