Add a safer version of WithNetNS* #2475

brb · 2016-08-10T16:19:44Z

This PR introduces a safe version of WithNetNS*. The new function executes work() by shelling out weaveutil which before executing it enters a given network namespace via nsenter.

Also, the PR replaces the usage of WithNetNS*Unsafe with the newly introduced function where needed.

Fixes #2419

brb · 2016-08-23T10:43:46Z

PTAL.

rade

^

net/veth.go

 	return err == nil
 }

-func AttachContainer(ns netns.NsHandle, id, ifName, bridgeName string, mtu int, withMulticastRoute bool, cidrs []*net.IPNet, keepTXOn bool) error {
+func AttachContainer(ns netns.NsHandle, nsPath, id, ifName, bridgeName string, mtu int, withMulticastRoute bool, cidrs []*net.IPNet, keepTXOn bool) error {


bboreham · 2016-10-06T15:34:18Z

I squashed the commits in this PR, in branch issues/2419-withnetns
If #2541 lands I will rebase on top of that

The subcommands are going to be used with WithNetNS wrapper.

- Make them safe by using the safe version of WithNetNS. - Improve naming.

brb · 2016-10-19T15:55:57Z

PTAL. A few things:

I'm in favor of getting rid of all occurrences ofWithNetNS*Unsafe as unaware users might trip over it.
ParseNetDev is ugly. Open to suggestions.
If moving NetDev pieces from common/ to net/ clogs reviewing, I will remove the corresponding commit from this PR.

awh

I'm in favor of getting rid of all occurrences of WithNetNS*Unsafe as unaware users might trip over it.

Are you talking about the three remaining usages of WithNetNSLinkUnsafe in cni.go and netns.go? File a separate issue.

ParseNetDev is ugly. Open to suggestions.

As mentioned in the comments, list-netdev should print out JSON encoded Dev structs (actually, I think there is a general principle here that all the utility executables should communicate by receiving and emitting JSON encoded structs the definitions of which are shared with the caller, but that's a separate conversation)

prog/weaveutil/netdev.go

+
+// listNetDevs outputs network ifaces identified by the given indexes
+// in the format of weavenet.Dev.
+func listNetDevs(args []string) error {


net/netdev.go

+	return fmt.Sprintf("%s %s %s", d.Name, d.MAC, d.CIDRs)
+}
+
+func ParseNetDev(netdev string) (Dev, error) {


net/netdev.go

+	if err != nil {
+		return nil, fmt.Errorf("list-netdevs failed: %s", err)
+	}
+	for _, netdevStr := range strings.Split(netdevsStr, "\n") {


bboreham · 2016-10-20T14:45:24Z

We don't really need to change addrs.go or proc-addrs.go; they concentrate all their namespace-changing in one region. And if you don't do that then a lot of the rest of the complexity goes away.

brb · 2016-10-20T14:50:37Z

As mentioned above, I'm in favor of getting rid of WithNetNS*Unsafe for safety reasons. Therefore I made the changes to addrs.go and proc-addrs.go.

brb · 2016-10-20T17:18:54Z

PTAL. I've create #2556 to track removal of WithNetNS*Unsafe.

This reverts commit a5ff7ab, reversing changes made to 04a6e6d.

brb mentioned this pull request Aug 10, 2016

Add a safer version of WithNetNS* #2443

Closed

brb added this to the 1.7.0 milestone Aug 23, 2016

brb assigned awh Aug 23, 2016

brb force-pushed the issues/2419-withnetns branch from 6b3a67f to 4b44bce Compare September 14, 2016 14:09

awh modified the milestones: 1.7.0, 1.8.0 Sep 23, 2016

awh assigned brb and unassigned awh Sep 28, 2016

rade reviewed Oct 5, 2016

View reviewed changes

bboreham self-assigned this Oct 6, 2016

bboreham mentioned this pull request Oct 6, 2016

Change namespace via nsenter, for plugin arp-config step #2541

Merged

bboreham removed their assignment Oct 6, 2016

awh assigned brb and unassigned brb Oct 12, 2016

brb force-pushed the issues/2419-withnetns branch from 4b44bce to 1755ec7 Compare October 19, 2016 15:06

brb added 2 commits October 19, 2016 16:07

Add netdev subcommands to weaveutil

cdd6ed5

The subcommands are going to be used with WithNetNS wrapper.

Refactor NetDev utilities

947ae11

- Make them safe by using the safe version of WithNetNS. - Improve naming.

brb force-pushed the issues/2419-withnetns branch from 1755ec7 to 3492cd9 Compare October 19, 2016 15:15

Use safe version of WithNetNS

3492cd9

brb removed their assignment Oct 19, 2016

brb force-pushed the issues/2419-withnetns branch from cebd490 to e611f04 Compare October 19, 2016 16:02

Move NetDev utils from common/ to net/

e611f04

brb assigned awh Oct 19, 2016

awh suggested changes Oct 20, 2016

View reviewed changes

awh removed their assignment Oct 20, 2016

awh assigned brb Oct 20, 2016

brb mentioned this pull request Oct 20, 2016

Get rid of WithNetNS*Unsafe #2556

Closed

brb force-pushed the issues/2419-withnetns branch from 9751455 to 9754da1 Compare October 20, 2016 17:18

brb removed their assignment Oct 20, 2016

Encode "list-netdevs" output in JSON

9754da1

brb assigned awh Oct 20, 2016

awh approved these changes Oct 26, 2016

View reviewed changes

awh merged commit a5ff7ab into master Oct 26, 2016

awh deleted the issues/2419-withnetns branch October 26, 2016 12:50

brb added a commit that referenced this pull request Nov 3, 2016

Revert "Merge pull request #2475 from /issues/2419-withnetns"

00f8de6

This reverts commit a5ff7ab, reversing changes made to 04a6e6d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a safer version of WithNetNS* #2475

Add a safer version of WithNetNS* #2475

brb commented Aug 10, 2016

brb commented Aug 23, 2016

rade left a comment

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Oct 6, 2016

brb commented Oct 19, 2016

awh left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Oct 20, 2016

brb commented Oct 20, 2016

brb commented Oct 20, 2016 •

edited

Loading

Add a safer version of WithNetNS* #2475

Add a safer version of WithNetNS* #2475

Conversation

brb commented Aug 10, 2016

brb commented Aug 23, 2016

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Oct 6, 2016

brb commented Oct 19, 2016

awh left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Oct 20, 2016

brb commented Oct 20, 2016

brb commented Oct 20, 2016 • edited Loading

brb commented Oct 20, 2016 •

edited

Loading