plugins icon indicating copy to clipboard operation
plugins copied to clipboard
verified publisher icon containernetworking

bridge: clean ip masq if netns is empty

Open qkboy opened this issue 1 year ago • 11 comments

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 #810

qkboy avatar Aug 23 '24 10:08 qkboy

@AkihiroSuda would you review this pr ? thanks.

qkboy avatar Aug 26 '24 02:08 qkboy

Can we have a test?

AkihiroSuda avatar Aug 26 '24 02:08 AkihiroSuda

I compared this patch between the old 1.2.0 version and the latest 1.5.1. There was nothing different except one blank line that formated by gofumpt. And tested in my local environment. It tested in the latest 1.7.6 nerdctl and the latest 1.5.1 plugins. Here is the version information.

# nerdctl version
Client:
 Version:	v1.7.6
 OS/Arch:	linux/amd64
 Git commit:	845e989f69d25b420ae325fedc8e70186243fd93
 buildctl:
  Version:	v0.10.6
  GitCommit:	0c9b5aeb269c740650786ba77d882b0259415ec7

Server:
 containerd:
  Version:	1.6.14
  GitCommit:	9ba4b250366a5ddde94bb7c9d1def331423aa323
 runc:
  Version:	1.1.4
  GitCommit:	v1.1.4-0-g5fd4c4d

# /opt/cni/bin/bridge version
CNI bridge plugin v1.5.1
CNI protocol versions supported: 0.1.0, 0.2.0, 0.3.0, 0.3.1, 0.4.0, 1.0.0

It still leaked iptables rules if stop and remove a container.

# nerdctl run -d --name nginx-test nginx
# nerdctl stop nginx-test
# nerdctl rm nginx-test
# iptables-save|grep -C2 10.4.0.133
-A POSTROUTING -m comment --comment "kubernetes postrouting rules" -j KUBE-POSTROUTING
-A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE
-A POSTROUTING -s 10.4.0.133/32 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j CNI-80534691afec65b77960bc3e
-A CNI-80534691afec65b77960bc3e -d 10.4.0.0/24 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j ACCEPT
-A CNI-80534691afec65b77960bc3e ! -d 224.0.0.0/4 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j MASQUERADE

After patched, it is ok as expected.

# CGO_ENABLED=0 GOARCH=amd64 ./build_linux.sh -ldflags '-extldflags -static -X github.com/containernetworking/plugins/pkg/utils/buildversion.BuildVersion=v1.5.1-fix.1'
# /opt/cni/bin/bridge version
CNI bridge plugin v1.5.1-fix.1
CNI protocol versions supported: 0.1.0, 0.2.0, 0.3.0, 0.3.1, 0.4.0, 1.0.0

# nerdctl run -d --name nginx-test nginx
# iptables-save|grep -C2 -E '10.4.0.134|10.4.0.133'
-A POSTROUTING -m comment --comment "kubernetes postrouting rules" -j KUBE-POSTROUTING
-A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE
-A POSTROUTING -s 10.4.0.133/32 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j CNI-80534691afec65b77960bc3e
-A POSTROUTING -s 10.4.0.134/32 -m comment --comment "name: \"bridge\" id: \"default-c203713502330103c430254aeac244d004fff0b2a68a488104fe202c81cb26c2\"" -j CNI-8b38442395ce2331296cf53c
-A CNI-80534691afec65b77960bc3e -d 10.4.0.0/24 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j ACCEPT

# nerdctl stop nginx-test
# nerdctl rm nginx-test

# iptables-save|grep -C2 -E '10.4.0.134|10.4.0.133'
-A POSTROUTING -m comment --comment "kubernetes postrouting rules" -j KUBE-POSTROUTING
-A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE
-A POSTROUTING -s 10.4.0.133/32 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j CNI-80534691afec65b77960bc3e
-A CNI-80534691afec65b77960bc3e -d 10.4.0.0/24 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j ACCEPT
-A CNI-80534691afec65b77960bc3e ! -d 224.0.0.0/4 -m comment --comment "name: \"bridge\" id: \"default-f98b8eadd012d3d69c8511e12b0ac8fb667e56606876fa5b072b78afb94a9685\"" -j MASQUERADE

# iptables-save|grep -C2 '10.4.0.134'

qkboy avatar Aug 26 '24 10:08 qkboy

Could you please add unit test as @AkihiroSuda mentioned?

s1061123 avatar Aug 26 '24 11:08 s1061123

/assign @squeed

s1061123 avatar Aug 26 '24 12:08 s1061123

The logic basically makes sense; could you please add some unit tests, especially for getIPCfgs?

As a comment, I wonder if there's a cleaner way to structure the delete function. I don't like how long it is. (This is not this PR's fault, it just becomes clear reading this PR).

squeed avatar Aug 27 '24 08:08 squeed

The logic basically makes sense; could you please add some unit tests, especially for getIPCfgs?

As a comment, I wonder if there's a cleaner way to structure the delete function. I don't like how long it is. (This is not this PR's fault, it just becomes clear reading this PR).

It seems that there is no need to double-check the handling of the ipMasq rule, just to make sure that the prevResult is parsed correctly when the cmdDel is executed.

qkboy avatar Sep 02 '24 13:09 qkboy

@s1061123 @squeed would you review this pr again ? thanks.

qkboy avatar Sep 11 '24 09:09 qkboy

Ping @s1061123 @squeed 🙏

This PR is needed to fix:

  • https://github.com/containerd/nerdctl/issues/3253
  • https://github.com/containerd/nerdctl/issues/3488
  • https://github.com/containerd/nerdctl/issues/3487 (possibly)

AkihiroSuda avatar Oct 02 '24 23:10 AkihiroSuda

@s1061123 @squeed Any chance to merge this?🙏

AkihiroSuda avatar Apr 21 '25 15:04 AkihiroSuda

@squeed PTAL?

s1061123 avatar Apr 23 '25 06:04 s1061123