From 531147372e7f569f9b8b068ca516b0801f62fa58 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 15 Sep 2020 11:49:52 +0100 Subject: [PATCH 1/6] Stale portmappings fix + e2e tests --- e2e/portmap_test.go | 93 ++++++++++++++++++++++++++++++++++++ pkg/network/cni/cni.go | 18 ++++++- pkg/network/docker/docker.go | 2 +- pkg/network/types.go | 2 +- pkg/operations/remove.go | 7 +-- 5 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 e2e/portmap_test.go diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go new file mode 100644 index 000000000..b88f750e3 --- /dev/null +++ b/e2e/portmap_test.go @@ -0,0 +1,93 @@ +// +// This is the e2e package to run tests for Ignite +// Currently, we do local e2e tests only +// we have to wait until the CI setup to allow Ignite to run with sudo and in a KVM environment. +// +// How to run tests: +// sudo IGNITE_E2E_HOME=$PWD $(which go) test ./e2e/. -v -count 1 -run Test +// + +package e2e + +import ( + "fmt" + "os/exec" + "testing" + + "gotest.tools/assert" +) + +func TestPortmapCleanup(t *testing.T) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + + vmName := "e2e_test_ignite_portmap" + mappedPort := 4242 + + runCmd := exec.Command( + igniteBin, + "run", "--name="+vmName, + "--ssh", + "--ports", + fmt.Sprintf("%d:%d", mappedPort, mappedPort), + "weaveworks/ignite-ubuntu", + ) + runOut, runErr := runCmd.CombinedOutput() + assert.Check(t, runErr, fmt.Sprintf("vm run: \n%q\n%s", runCmd.Args, runOut)) + + defer func() { + rmvCmd := exec.Command( + "sudo", + igniteBin, + "rm", "-f", vmName, + ) + rmvCmd.Run() + }() + + // Get the VM ID + idCmd := exec.Command( + "ignite", + "ps", + "--filter", + fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName), + "--quiet", + ) + idOut, idErr := idCmd.CombinedOutput() + assert.Check(t, idErr, fmt.Sprintf("vm id: \n%q\n%s", idCmd.Args, idOut)) + vmID := string(idOut[:len(idOut)-2]) + + // Check that the IPtable rules are installed + grepOut, grepErr := grepIPTables(vmID) + assert.NilError(t, grepErr, fmt.Sprintf("unable to match iptable rules:\n %s", grepOut)) + + rmvCmd := exec.Command( + igniteBin, + "rm", "-f", vmName, + ) + rmvOut, rmvErr := rmvCmd.CombinedOutput() + assert.Check(t, rmvErr, fmt.Sprintf("vm removal: \n%q\n%s", rmvCmd.Args, rmvOut)) + + // Check that the IPtable rules are removed + grepOut, grepErr = grepIPTables(vmID) + assert.Error(t, grepErr, "exit status 1", fmt.Sprintf("unexpected output in grep : \n%s", grepOut)) + assert.Equal(t, len(grepOut), 0, fmt.Sprintf("leftover iptable rules detected:\n%s", string(grepOut))) + +} + +func grepIPTables(search string) ([]byte, error) { + grepCmd := exec.Command( + "grep", + search, + ) + natCmd := exec.Command( + "iptables", + "-t", + "nat", + "-nL", + ) + pipe, _ := natCmd.StdoutPipe() + defer pipe.Close() + + grepCmd.Stdin = pipe + natCmd.Start() + return grepCmd.Output() +} diff --git a/pkg/network/cni/cni.go b/pkg/network/cni/cni.go index fd890f9a4..33e42a6c9 100644 --- a/pkg/network/cni/cni.go +++ b/pkg/network/cni/cni.go @@ -189,7 +189,7 @@ func cniToIgniteResult(r *gocni.CNIResult) *network.Result { return result } -func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string) (err error) { +func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string, portMappings ...meta.PortMapping) (err error) { if err = plugin.initialize(); err != nil { return err } @@ -214,7 +214,21 @@ func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string) (err return nil } - return plugin.cni.Remove(context.Background(), containerID, netnsPath) + pms := make([]gocni.PortMapping, 0, len(portMappings)) + for _, pm := range portMappings { + hostIP := "" + if pm.BindAddress != nil { + hostIP = pm.BindAddress.String() + } + pms = append(pms, gocni.PortMapping{ + HostPort: int32(pm.HostPort), + ContainerPort: int32(pm.VMPort), + Protocol: pm.Protocol.String(), + HostIP: hostIP, + }) + } + + return plugin.cni.Remove(context.Background(), containerID, netnsPath, gocni.WithCapabilityPortMap(pms)) } // cleanupBridges makes the defaultNetworkName CNI network config not leak iptables rules diff --git a/pkg/network/docker/docker.go b/pkg/network/docker/docker.go index 4d4192672..28ffa9073 100644 --- a/pkg/network/docker/docker.go +++ b/pkg/network/docker/docker.go @@ -44,7 +44,7 @@ func (plugin *dockerNetworkPlugin) SetupContainerNetwork(containerID string, _ . }, nil } -func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string) error { +func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string, _ ...meta.PortMapping) error { // no-op for docker, this is handled automatically return nil } diff --git a/pkg/network/types.go b/pkg/network/types.go index f91ce053c..dae3e7e21 100644 --- a/pkg/network/types.go +++ b/pkg/network/types.go @@ -21,7 +21,7 @@ type Plugin interface { SetupContainerNetwork(containerID string, portmappings ...meta.PortMapping) (*Result, error) // RemoveContainerNetwork is the method called before a container using the network plugin can be deleted - RemoveContainerNetwork(containerID string) error + RemoveContainerNetwork(containerID string, portmappings ...meta.PortMapping) error } type Result struct { diff --git a/pkg/operations/remove.go b/pkg/operations/remove.go index 604549845..b2c051f41 100644 --- a/pkg/operations/remove.go +++ b/pkg/operations/remove.go @@ -6,6 +6,7 @@ import ( log "github.com/sirupsen/logrus" api "github.com/weaveworks/ignite/pkg/apis/ignite" + meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1" "github.com/weaveworks/ignite/pkg/client" "github.com/weaveworks/ignite/pkg/dmlegacy/cleanup" "github.com/weaveworks/ignite/pkg/logs" @@ -86,7 +87,7 @@ func StopVM(vm *api.VM, kill, silent bool) error { } // Remove VM networking - if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID())); err != nil { + if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID()), vm.Spec.Network.Ports...); err != nil { log.Warnf("Failed to cleanup networking for stopped container %s %q: %v", vm.GetKind(), vm.GetUID(), err) return err @@ -119,7 +120,7 @@ func StopVM(vm *api.VM, kill, silent bool) error { return nil } -func removeNetworking(containerID string) error { +func removeNetworking(containerID string, portmappings ...meta.PortMapping) error { log.Infof("Removing the container with ID %q from the %q network", containerID, providers.NetworkPlugin.Name()) - return providers.NetworkPlugin.RemoveContainerNetwork(containerID) + return providers.NetworkPlugin.RemoveContainerNetwork(containerID, portmappings...) } From 3a5930e516f4e2938c2fccd66770c86b10022d89 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 15 Sep 2020 11:59:30 +0100 Subject: [PATCH 2/6] Fixing lint errors --- e2e/portmap_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go index b88f750e3..cee8063b3 100644 --- a/e2e/portmap_test.go +++ b/e2e/portmap_test.go @@ -40,7 +40,7 @@ func TestPortmapCleanup(t *testing.T) { igniteBin, "rm", "-f", vmName, ) - rmvCmd.Run() + _ = rmvCmd.Run() }() // Get the VM ID @@ -88,6 +88,6 @@ func grepIPTables(search string) ([]byte, error) { defer pipe.Close() grepCmd.Stdin = pipe - natCmd.Start() + _ = natCmd.Start() return grepCmd.Output() } From c4be3e9de54f2db9787bc4eedc9184d7d371ec08 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 15 Sep 2020 12:21:15 +0100 Subject: [PATCH 3/6] Fixing e2e test failures --- e2e/portmap_test.go | 59 +++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 34 deletions(-) diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go index cee8063b3..fc24d50d3 100644 --- a/e2e/portmap_test.go +++ b/e2e/portmap_test.go @@ -14,6 +14,7 @@ import ( "os/exec" "testing" + "github.com/weaveworks/ignite/e2e/util" "gotest.tools/assert" ) @@ -23,48 +24,38 @@ func TestPortmapCleanup(t *testing.T) { vmName := "e2e_test_ignite_portmap" mappedPort := 4242 - runCmd := exec.Command( - igniteBin, - "run", "--name="+vmName, - "--ssh", - "--ports", - fmt.Sprintf("%d:%d", mappedPort, mappedPort), - "weaveworks/ignite-ubuntu", - ) - runOut, runErr := runCmd.CombinedOutput() - assert.Check(t, runErr, fmt.Sprintf("vm run: \n%q\n%s", runCmd.Args, runOut)) - - defer func() { - rmvCmd := exec.Command( - "sudo", - igniteBin, - "rm", "-f", vmName, - ) - _ = rmvCmd.Run() - }() + igniteCmd := util.NewCommand(t, igniteBin) + + defer igniteCmd.New(). + With("rm", "-f", vmName). + Run() + + igniteCmd.New(). + With("run"). + With("--name=" + vmName). + With("--ssh"). + With("--ports=" + fmt.Sprintf("%d:%d", mappedPort, mappedPort)). + With(util.DefaultVMImage). + Run() // Get the VM ID - idCmd := exec.Command( - "ignite", - "ps", - "--filter", - fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName), - "--quiet", - ) - idOut, idErr := idCmd.CombinedOutput() - assert.Check(t, idErr, fmt.Sprintf("vm id: \n%q\n%s", idCmd.Args, idOut)) + idCmd := igniteCmd.New(). + With("ps"). + With("--filter"). + With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). + With("--quiet") + + idOut, idErr := idCmd.Cmd.CombinedOutput() + assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) vmID := string(idOut[:len(idOut)-2]) // Check that the IPtable rules are installed grepOut, grepErr := grepIPTables(vmID) assert.NilError(t, grepErr, fmt.Sprintf("unable to match iptable rules:\n %s", grepOut)) - rmvCmd := exec.Command( - igniteBin, - "rm", "-f", vmName, - ) - rmvOut, rmvErr := rmvCmd.CombinedOutput() - assert.Check(t, rmvErr, fmt.Sprintf("vm removal: \n%q\n%s", rmvCmd.Args, rmvOut)) + igniteCmd.New(). + With("rm", "-f", vmName). + Run() // Check that the IPtable rules are removed grepOut, grepErr = grepIPTables(vmID) From 62ceda47299fecbc62733b3cc57f03fd4d40e1de Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 15 Sep 2020 12:34:09 +0100 Subject: [PATCH 4/6] Pinning network plugin to CNI --- e2e/portmap_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go index fc24d50d3..8c7d8e76f 100644 --- a/e2e/portmap_test.go +++ b/e2e/portmap_test.go @@ -21,7 +21,7 @@ import ( func TestPortmapCleanup(t *testing.T) { assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") - vmName := "e2e_test_ignite_portmap" + vmName := "e2e_test_ignite_portmap_cleanup" mappedPort := 4242 igniteCmd := util.NewCommand(t, igniteBin) @@ -31,6 +31,7 @@ func TestPortmapCleanup(t *testing.T) { Run() igniteCmd.New(). + WithNetwork("cni"). With("run"). With("--name=" + vmName). With("--ssh"). From 90b3028a550b377a33ca9f056c76f1a5d004b46d Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 15 Sep 2020 12:49:39 +0100 Subject: [PATCH 5/6] Removing defer --- e2e/portmap_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go index 8c7d8e76f..5bbbdb18b 100644 --- a/e2e/portmap_test.go +++ b/e2e/portmap_test.go @@ -26,10 +26,6 @@ func TestPortmapCleanup(t *testing.T) { igniteCmd := util.NewCommand(t, igniteBin) - defer igniteCmd.New(). - With("rm", "-f", vmName). - Run() - igniteCmd.New(). WithNetwork("cni"). With("run"). From 26b2ac81da84a65f51e3585502ed0749dee7aa53 Mon Sep 17 00:00:00 2001 From: networkop Date: Wed, 16 Sep 2020 10:22:44 +0100 Subject: [PATCH 6/6] fix VM name in e2e test --- e2e/portmap_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/portmap_test.go b/e2e/portmap_test.go index 5bbbdb18b..18c4d041a 100644 --- a/e2e/portmap_test.go +++ b/e2e/portmap_test.go @@ -21,7 +21,7 @@ import ( func TestPortmapCleanup(t *testing.T) { assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") - vmName := "e2e_test_ignite_portmap_cleanup" + vmName := "e2e-test-ignite-portmap-cleanup" mappedPort := 4242 igniteCmd := util.NewCommand(t, igniteBin)