fix 714-plugin add netns validation reinforcement #890

Poor12 · 2022-03-31T07:07:05Z

fix-714: containernetworking/plugins#714

Poor12 · 2022-04-01T01:40:32Z

Is something like this? Need your review. :) @squeed

squeed · 2022-04-13T15:48:10Z

pkg/skel/skel.go

+
+func checkNetNS(nsPath string) (bool, *types.Error) {
+	ns, err := netns.GetFromPath(nsPath)
+	if err != nil {


We need to support CNI DEL for empty nsPath as well as already-deleted nsPath. So, paradoxically, I think we can return true here if err != nil.

Do you mean we just pass the check and let plugins deal with empty plugins as well as already-deleted nsPath？

dcbw · 2022-04-13T15:55:05Z

pkg/skel/skel.go

+		if err != nil {
+			return err
+		}
+		if cmdArgs.NetnsOverride != "TRUE" {


@Poor12 could we make this a case-insensitive comparison for "true", and also add a check for "1"?

squeed · 2022-04-13T15:57:58Z

pkg/skel/skel.go

@@ -241,6 +277,17 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 		return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow CHECK", "")
 	case "DEL":
 		err = t.checkVersionAndCall(cmdArgs, versionInfo, cmdDel)
+		if err != nil {


Should we check this for ADD as well? I don't see why not...

Add check for ADD.

coveralls · 2022-04-19T12:43:32Z

Coverage increased (+0.02%) to 72.228% when pulling c31383f on Poor12:fix-714 into 8dba382 on containernetworking:main.

squeed · 2022-04-19T12:53:26Z

@Poor12 surprise: this doesn't build on Windows :). We'll need to stub out the function with build flags.

Poor12 · 2022-04-20T01:46:12Z

@Poor12 surprise: this doesn't build on Windows :). We'll need to stub out the function with build flags.

I am not familiar with this. Could you do me a favor？Thanks a lot.

dcbw · 2022-04-27T15:48:24Z

I am not familiar with this. Could you do me a favor？Thanks a lot.

 # github.com/vishvananda/netns
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:28:13: undefined: unix.Stat_t
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:29:12: undefined: unix.Fstat
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:32:12: undefined: unix.Fstat
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:43:8: undefined: unix.Stat_t
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:44:12: undefined: unix.Fstat
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:56:8: undefined: unix.Stat_t
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:57:12: undefined: unix.Fstat
Error: C:\Users\runneradmin\go\pkg\mod\github.com\vishvananda\[email protected]\netns.go:71:12: undefined: unix.Close

That just means that because the netlink library and Unix stuff doesn't work on Windows, you need to move that code to a different file. Here's what you do.

Make a new file called netns_linux.go and put getCurrentNS(), getCurrentThreadNetNSPath(), and checkNetNS() into that.
Make a new file called netns_windows.go and stub out the checkNetNS() function there like:

func checkNetNS(nsPath string) (bool, *types.Error) {
    return false, nil
}

Files with _ at the end are automatically only built on the platform in question. It's magic :) That way Linux builds will use the full implementation, and the Windows builds will use the stub implementation that just returns OK. @MikeZappa87 says that Windows doesn't have a good way (that we know of) to get the root compartment, so let's just ignore this on Windows for now.

Poor12 · 2022-04-28T02:58:49Z

Thanks for informing this. I have not seen _windows files before. And Happy Labor Day :) @dcbw

dcbw · 2022-05-04T16:04:33Z

pkg/ns/ns_windows.go

+
+package ns
+
+func CheckNetNS(nsPath string) (bool, *types.Error) {


@Poor12 you need to import "github.com/containernetworking/cni/pkg/types" here to get types.Error.

Poor12 · 2022-05-20T02:05:06Z

/cc @squeed @dcbw

kkBill · 2022-06-14T15:13:15Z

Is there any progress about this issue？

Poor12 · 2022-06-20T13:52:41Z

Excuse me, several months ago I started a issue about containernetworking/plugins#714 and have proposed a tentative solution but got very few replies lately. Actually our productive environment has had this problem for a while and we really hope to fix this problem in the community in order to avoid the problem of upstream and downstream inconsistencies caused by our private modification.

But during several months waiting for a response from the community, I have several confusion about this problem.

Do we still need to fix this problem？Or we don't think it's a problem？
Is there a more reasonable solution for this problem？

cc @squeed @dcbw @mars1024 @matthewdupre @mccv1r0 @MikeZappa87 @jellonek
Really sorry if disturbing you.

Signed-off-by: Poor12 <[email protected]>

Poor12 force-pushed the fix-714 branch from 0ee63cd to 074bd0e Compare March 31, 2022 07:08

squeed reviewed Apr 13, 2022

View reviewed changes

dcbw reviewed Apr 13, 2022

View reviewed changes

squeed reviewed Apr 13, 2022

View reviewed changes

Poor12 force-pushed the fix-714 branch from 074bd0e to bbfac72 Compare April 14, 2022 03:10

Poor12 force-pushed the fix-714 branch from bbfac72 to 7ca247e Compare April 28, 2022 02:53

Poor12 changed the title ~~[WIP]fix 714-plugin add netns validation reinforcement~~ fix 714-plugin add netns validation reinforcement Apr 28, 2022

dcbw reviewed May 4, 2022

View reviewed changes

Poor12 force-pushed the fix-714 branch from 7ca247e to 45e92ad Compare May 5, 2022 01:09

fix 714-plugin add netns validation reinforcement

c31383f

Signed-off-by: Poor12 <[email protected]>

squeed force-pushed the fix-714 branch from 45e92ad to c31383f Compare October 10, 2022 15:16

squeed merged commit dbf33e2 into containernetworking:main Oct 10, 2022

s1061123 mentioned this pull request Nov 13, 2023

Add CNI_NETNS_OVERRIDE for upcoming CNI change containernetworking/plugins#969

Merged

Icarus9913 mentioned this pull request May 13, 2024

supplement ns building files for darwin OS #1089

Merged


		package ns

		func CheckNetNS(nsPath string) (bool, *types.Error) {

fix 714-plugin add netns validation reinforcement #890

fix 714-plugin add netns validation reinforcement #890

Uh oh!

Conversation

Poor12 commented Mar 31, 2022

Uh oh!

Poor12 commented Apr 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squeed commented Apr 19, 2022

Uh oh!

Poor12 commented Apr 20, 2022

Uh oh!

dcbw commented Apr 27, 2022

Uh oh!

Poor12 commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Poor12 commented May 20, 2022

Uh oh!

kkBill commented Jun 14, 2022

Uh oh!

Poor12 commented Jun 20, 2022

Uh oh!

Uh oh!

coveralls commented Apr 19, 2022 •

edited

Loading