search

containernetworking / plugins

Some reference and example networking plugins, maintained by the CNI team.
Apache License 2.0
2.23k stars 788 forks source link

bridge: clean ip masq if netns is empty #1078

Open qkboy opened 3 months ago

qkboy commented 3 months ago

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 commented 3 months ago

@AkihiroSuda would you review this pr ? thanks.

AkihiroSuda commented 3 months ago

Can we have a test?

qkboy commented 3 months ago

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'
s1061123 commented 3 months ago

Could you please add unit test as @AkihiroSuda mentioned?

s1061123 commented 3 months ago

/assign @squeed

squeed commented 3 months ago

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).

qkboy commented 2 months ago

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 commented 2 months ago

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

AkihiroSuda commented 1 month ago

Ping @s1061123 @squeed 🙏

This PR is needed to fix: