Browse code

Merge pull request #49778 from robmry/fix_delete_legacy_link

Fix removal of legacy links

Rob Murray authored on 2025/04/10 23:16:00
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