fix: add network cleanup for kill and stop cmd

[alegrey91]

Fix #2699

This PR provides code to cleanup CNI network setup when kill and stop command are typed.
Additionally, the onStartContainer hook was added in order to restore the CNI network setup when start command is typed (as suggested in the issue by @yankay).

How to test

In order to test this PR, follow these commands:

# compile the binary with the latest changes
make

# create the first container with the built nerdctl version
sudo _output/nerdctl run --name ngx1 -d -p 9999:80 nginx:alpine

# check iptables rules have been setup
sudo iptables-save | grep 9999

# kill the first container previosly created
sudo _output/nerdctl kill ngx1

# (now the iptables rules should disappeared
# create the second container bound on the same port
sudo _output/nerdctl run --name ngx2 -d -p 9999:80 nginx:alpine

# check iptables rules have been setup
sudo iptables-save | grep 9999

# access the second container forward
curl 127.0.0.1:9999

Credits

Thanks to @yankay and @89luca89 for the help

[AkihiroSuda]

Can we have an integration test? (See cmd/nerdctl/*_test.go)

[alegrey91]

Can we have an integration test? (See cmd/nerdctl/*_test.go)

Sure!

[alegrey91]

Can we have an integration test? (See cmd/nerdctl/*_test.go)

@AkihiroSuda just added

[AkihiroSuda]

Thanks, looks good, but please squash commits

[alegrey91]

Thanks, looks good, but please squash commits

Sure and thanks for the accurate review. I learned something new :)

[AkihiroSuda]

CI is failing, probably you need to wrap the clean up function with WithDetachedNetNSIfAny
https://github.com/search?q=repo%3Acontainerd%2Fnerdctl%20WithDetachedNetNSIfAny&type=code

[alegrey91]

CI is failing, probably you need to wrap the clean up function with WithDetachedNetNSIfAny https://github.com/search?q=repo%3Acontainerd%2Fnerdctl%20WithDetachedNetNSIfAny&type=code

Fixed!
Any idea why the test-integration-docker-compatibility test is failing?

[AkihiroSuda]

Any idea why the test-integration-docker-compatibility test is failing?

Because the iptables names are different for Docker.

=== RUN   TestStopCleanupForwards
    iptables.go:31: error listing rules in chain: "running [/usr/sbin/iptables -t nat -S CNI-HOSTPORT-DNAT --wait]: exit status 1: iptables v1.8.7 (nf_tables): chain `CNI-HOSTPORT-DNAT' in table `nat' is incompatible, use 'nft' tool.\n\n"
    iptables.go:35: not enough rules: 0
    container_stop_linux_test.go:117: assertion failed: false (bool) != true (true bool)
--- FAIL: TestStopCleanupForwards (0.51s)

[alegrey91]

Any idea why the test-integration-docker-compatibility test is failing?

Because the iptables names are different for Docker.

=== RUN   TestStopCleanupForwards
    iptables.go:31: error listing rules in chain: "running [/usr/sbin/iptables -t nat -S CNI-HOSTPORT-DNAT --wait]: exit status 1: iptables v1.8.7 (nf_tables): chain `CNI-HOSTPORT-DNAT' in table `nat' is incompatible, use 'nft' tool.\n\n"
    iptables.go:35: not enough rules: 0
    container_stop_linux_test.go:117: assertion failed: false (bool) != true (true bool)
--- FAIL: TestStopCleanupForwards (0.51s)

What if I edit the ForwardExists function to pass the chain as argument?
The new signature will be something like this:

func ForwardExists(t *testing.T, ipt *iptables.IPTables, chain, containerIP string, port int) bool

Then we could add a condition in the tests to check if target is docker or not and pass the chain name accordingly.
eg:

var chain string
if testutil.GetTarget() == testutil.Docker {
        // docker chain
	chain = "DOCKER"
} else {
        // nerdctl chain
	chain = cniplugin.MustFormatChainNameWithPrefix(netName, containerID, "DN-")
}

assert.Equal(t, iptablesutil.ForwardExists(t, ipt, chain, testutil.Namespace, containerID), true)

WDYT?

[alegrey91]

@AkihiroSuda I just pushed the code with the changes I described above.
One job of the rootless suite is failing. Not sure if is due to my code or different error.
Can you please rerun it in order to check this?
I've added another commit with latest changes. If you think this is good, I can squash the last commit with the previous one.

[AkihiroSuda]

Thanks, looks good, please squash

[AkihiroSuda]

Thanks