Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bridge plugin leak ip masq if netns is empty #810

Closed
qkboy opened this issue Jan 18, 2023 · 1 comment · May be fixed by #1078
Closed

Bridge plugin leak ip masq if netns is empty #810

qkboy opened this issue Jan 18, 2023 · 1 comment · May be fixed by #1078
Labels
Stale

Comments

@qkboy
Copy link

qkboy commented Jan 18, 2023 •
edited
Loading

When calling bridge to create a container through nerdctl, if set "ipMasq": true , three iptables rules will be created. These rules are not deleted after the container is deleted, but remain on the system.

:CNI-4f6dd6c08c7e1a901788c768 - [0:0]
-A POSTROUTING -s 10.4.0.29/32 -m comment --comment "name: \"bridge\" id: \"default-237eaf51aa0c1411433db10b2c8dac0f826f87ac6f48c62084ee84c3d9c20384\"" -j CNI-4f6dd6c08c7e1a901788c768
-A CNI-4f6dd6c08c7e1a901788c768 -d 10.4.0.0/24 -m comment --comment "name: \"bridge\" id: \"default-237eaf51aa0c1411433db10b2c8dac0f826f87ac6f48c62084ee84c3d9c20384\"" -j ACCEPT
-A CNI-4f6dd6c08c7e1a901788c768 ! -d 224.0.0.0/4 -m comment --comment "name: \"bridge\" id: \"default-237eaf51aa0c1411433db10b2c8dac0f826f87ac6f48c62084ee84c3d9c20384\"" -j MASQUERADE

The reason is that, nerdctl calling cni Del will put Netns="" into the StdinData.

// The third parameter means Netns path , it's empty
if err := opts.cni.Remove(ctx, opts.fullID, "", namespaceOpts...); err != nil {
	logrus.WithError(err).Errorf("failed to call cni.Remove")
	return err
}

and ctr also do the same action.

defer func() {
	if enableCNI {
		// The third parameter means Netns path , it's empty
		if err := network.Remove(ctx, commands.FullID(ctx, container), ""); err != nil {
			logrus.WithError(err).Error("network review")
		}
	}
	task.Delete(ctx)
}()

But bridge plugin only execute ipamDel() and didn't teardown ip masq when Netns is empty .

	if args.Netns == "" {
		return ipamDel()
	}

In another case, even if Netns is not empty, but the container has stopped. Bridge plugin try to obtain the ip address from container's netns will be fail too. So teardown ip masq will not execute too. The current handling method is to skip.

	// If the device isn't there then don't try to clean up IP masq either.
	var ipnets []*net.IPNet
	err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
		var err error
		ipnets, err = ip.DelLinkByNameAddr(args.IfName)
		if err != nil && err == ip.ErrLinkNotFound {
			return nil
		}
		return err
	})

When nerdctl calls cni Del, the container has been stopped. This problem is difficult to solve on nerdctl or ctr.
Actually, nerdctl or ctr will inserts the network metadata into the prevResult structure of the StdinData.

"prevResult":{"cniVersion":"1.0.0","interfaces":[{"name":"nerdctl0","mac":"12:02:11:fb:08:a7"},{"name":"vethd4a25238","mac":"96:56:42:de:0f:bf"},{"name":"eth0","mac":"6e:af:15:d5:2c:d8","sandbox":"/proc/2538456/ns/net"}],"ips":[{"interface":2,"address":"10.4.0.102/24","gateway":"10.4.0.1"}]

We can solve this problem by getting the IP address from prevResult and adjusting the order of executing ip masq.
qkboy pushed a commit to qkboy/plugins that referenced this issue Jan 18, 2023
bridge: clean ip masq if netns is empty
e7d5b5b 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810
@qkboy qkboy mentioned this issue Jan 18, 2023
Closed
qkboy pushed a commit to qkboy/plugins that referenced this issue Jan 18, 2023
bridge: clean ip masq if netns is empty
b65130c 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
@qkboy qkboy mentioned this issue Jan 18, 2023
Open
@github-actions github-actions bot added the Stale label Mar 20, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 28, 2023
@apostasie apostasie mentioned this issue Jul 28, 2024
Open
@AkihiroSuda
Copy link
Contributor

@s1061123 @squeed Could you reopen this issue and the fix #811 ?

qkboy pushed a commit to qkboy/plugins that referenced this issue Aug 23, 2024
bridge: clean ip masq if netns is empty
3e6c62d 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
@qkboy qkboy mentioned this issue Aug 23, 2024
Open
qkboy pushed a commit to qkboy/plugins that referenced this issue Aug 23, 2024
bridge: clean ip masq if netns is empty
d4b8819 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
qkboy pushed a commit to qkboy/plugins that referenced this issue Aug 26, 2024
bridge: clean ip masq if netns is empty
e58ec8a 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
qkboy pushed a commit to qkboy/plugins that referenced this issue Aug 26, 2024
bridge: clean ip masq if netns is empty
053be10 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
qkboy pushed a commit to qkboy/plugins that referenced this issue Sep 2, 2024
bridge: clean ip masq if netns is empty
47725c6 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
qkboy pushed a commit to qkboy/plugins that referenced this issue Sep 8, 2024
@qkboy
bridge: clean ip masq if netns is empty
bbf8920 
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix containernetworking#810

Signed-off-by: hyphen.wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
2 participants
@AkihiroSuda @qkboy