-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
pkg/ns: verify netns when initialized with GetNS #222
Conversation
|
||
return &netNS{ | ||
file: fd, | ||
mounted: stat.Type == NSFS_MAGIC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't set mounted:true for plain GetNS() calls. The reason is because only the creator of the netns should be disposing of it via ns.Close(), and things that call GetNS() are the creator. I think all you want to do here is return an error if the mountpoint is not an NSFS mount point.
My take on this would be https://github.com/dcbw/cni/commits/namespace-validate ... |
} | ||
|
||
isNSFS, err := IsNSFS(nspath) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to close the FD here on error return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM now, but how about some tests like: It("fails when the path is not a namespace", func() {
tempFile, err := ioutil.TempFile("", "nstest")
Expect(err).NotTo(HaveOccurred())
defer tempFile.Close()
defer os.Remove(tempFile.Name())
_, err = ns.GetNS(tempFile.Name())
Expect(err).To(MatchError(fmt.Sprintf("no network namespace mounted on file %q", tempFile.Name())))
}) |
Right, always good to have more tests! I fleshed your draft out and added a test case. |
Thanks, test LGTM. |
Fixes #221.