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: clean ip masq if netns is empty #1078

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bridge: clean ip masq if netns is empty #1078

wants to merge 1 commit into from
+104 −17

Conversation

qkboy
Copy link

@qkboy qkboy commented Aug 23, 2024

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 qkboy force-pushed the bridge-clean-masq-if-netns-empty branch from 3e6c62d to d4b8819 Compare August 23, 2024 10:45
@qkboy qkboy marked this pull request as ready for review August 23, 2024 10:46
@AkihiroSuda AkihiroSuda mentioned this pull request Aug 23, 2024
Closed
@qkboy
Copy link
Author

qkboy commented Aug 26, 2024

@AkihiroSuda would you review this pr ? thanks.

@AkihiroSuda
Copy link
Contributor

Can we have a test?

@qkboy
Copy link
Author

qkboy commented Aug 26, 2024

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
Copy link
Contributor

Could you please add unit test as @AkihiroSuda mentioned?

s1061123
s1061123 requested changes Aug 26, 2024
View reviewed changes
plugins/main/bridge/bridge.go Outdated Show resolved Hide resolved
@s1061123
Copy link
Contributor

/assign @squeed

@qkboy qkboy force-pushed the bridge-clean-masq-if-netns-empty branch 2 times, most recently from e58ec8a to 053be10 Compare August 26, 2024 13:31
squeed
squeed reviewed Aug 27, 2024
View reviewed changes
plugins/main/bridge/bridge.go Outdated
var debugPostIPAMError error
var (
debugPostIPAMError error
logger = log.New(os.Stdout, "", log.Ldate|log.Ltime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stdout is used as part of the response protocol, so we can't log to it. We need to use stderr.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little confused about it, and the absence of logs didn't affect functionality, so I deleted it.

squeed
squeed reviewed Aug 27, 2024
View reviewed changes
plugins/main/bridge/bridge.go
// so don't return an error if the device is already removed.
// https://github.com/kubernetes/kubernetes/issues/43014#issuecomment-287164444
_, ok := err.(ns.NSPathNotExistErr)
if ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style suggestion:

if _, isNotExists := err.(ns.NSPathNotExistErr); !isNotExist {
    return err
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@squeed
Copy link
Member

squeed commented Aug 27, 2024

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

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 qkboy force-pushed the bridge-clean-masq-if-netns-empty branch from 053be10 to 47725c6 Compare September 2, 2024 12:51
@qkboy
Copy link
Author

qkboy commented Sep 2, 2024

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 qkboy force-pushed the bridge-clean-masq-if-netns-empty branch 4 times, most recently from 47725c6 to bbf8920 Compare September 8, 2024 15:32
@qkboy
Copy link
Author

qkboy commented Sep 11, 2024

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

@qkboy qkboy force-pushed the bridge-clean-masq-if-netns-empty branch from 2189ddb to 47725c6 Compare September 20, 2024 07:52
@AkihiroSuda
Copy link
Contributor

Ping @s1061123 @squeed 🙏

This PR is needed to fix:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1061123 s1061123 Awaiting requested review from s1061123

@squeed squeed Awaiting requested review from squeed

Requested changes must be addressed to merge this pull request.

Assignees

@squeed squeed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bridge plugin leak ip masq if netns is empty
4 participants
@qkboy @AkihiroSuda @s1061123 @squeed