Do not skip bridge creation if bridge exists

common/odp/odp.go

@@ -75,6 +75,14 @@ func DeleteDatapath(dpname string) error {
 }
 
 func AddDatapathInterface(dpname string, ifname string) error {
+	return addDatapathInterface(dpname, ifname, false)
+}
+
+func AddDatapathInterfaceIfNotExist(dpname string, ifname string) error {
+	return addDatapathInterface(dpname, ifname, true)
+}
+
+func addDatapathInterface(dpname, ifname string, ignoreIfExist bool) error {
 	dpif, err := odp.NewDpif()
 	if err != nil {
 		return err
@@ -87,5 +95,9 @@ func AddDatapathInterface(dpname string, ifname string) error {
 	}
 
 	_, err = dp.CreateVport(odp.NewNetdevVportSpec(ifname))
+	if ignoreIfExist && err == odp.NetlinkError(syscall.EEXIST) {
+		return nil
+	}
+
 	return err
 }

net/bridge.go

@@ -227,12 +227,19 @@ func (config *BridgeConfig) configuredBridgeType() Bridge {
 }
 
 func EnsureBridge(procPath string, config *BridgeConfig, log *logrus.Logger) (Bridge, error) {
-	bridgeType, err := ExistingBridgeType(config.WeaveBridgeName, config.DatapathName)
-	if bridgeType != nil || err != nil {
-		return bridgeType, err
+	existingBridgeType, err := ExistingBridgeType(config.WeaveBridgeName, config.DatapathName)
+	if err != nil {
+		return nil, err
+	}
+
+	bridgeType := config.configuredBridgeType()
+
+	if existingBridgeType != nil && bridgeType.String() != existingBridgeType.String() {
+		return nil,
+			fmt.Errorf("Existing bridge type %q is different than requested %q. Please do 'weave reset' and try again",
+				existingBridgeType, bridgeType)
 	}
 
-	bridgeType = config.configuredBridgeType()
 	for {
 		if err := bridgeType.init(config); err != nil {
 			if errors.Cause(err) == errBridgeNotSupported {
@@ -288,7 +295,7 @@ func (b bridgeImpl) initPrep(config *BridgeConfig) error {
 		config.MTU = 65535
 	}
 	b.bridge = &netlink.Bridge{LinkAttrs: linkAttrs}
-	if err = netlink.LinkAdd(b.bridge); err != nil {
+	if err := LinkAddIfNotExist(b.bridge); err != nil {
 		return errors.Wrapf(err, "creating bridge %q", config.WeaveBridgeName)
 	}
 	if err := netlink.LinkSetHardwareAddr(b.bridge, mac); err != nil {
@@ -320,7 +327,7 @@ func (b bridgeImpl) init(config *BridgeConfig) error {
 	if err := b.initPrep(config); err != nil {
 		return err
 	}
-	if _, err := CreateAndAttachVeth(BridgeIfName, PcapIfName, config.WeaveBridgeName, config.MTU, true, func(veth netlink.Link) error {
+	if _, err := CreateAndAttachVeth(BridgeIfName, PcapIfName, config.WeaveBridgeName, config.MTU, true, false, func(veth netlink.Link) error {
 		return netlink.LinkSetUp(veth)
 	}); err != nil {
 		return errors.Wrap(err, "creating pcap veth pair")
@@ -370,11 +377,11 @@ func (bf bridgedFastdpImpl) init(config *BridgeConfig) error {
 	if err := bf.bridgeImpl.initPrep(config); err != nil {
 		return err
 	}
-	if _, err := CreateAndAttachVeth(BridgeIfName, DatapathIfName, config.WeaveBridgeName, config.MTU, true, func(veth netlink.Link) error {
+	if _, err := CreateAndAttachVeth(BridgeIfName, DatapathIfName, config.WeaveBridgeName, config.MTU, true, false, func(veth netlink.Link) error {
 		if err := netlink.LinkSetUp(veth); err != nil {
 			return errors.Wrapf(err, "setting link up on %q", veth.Attrs().Name)
 		}
-		if err := odp.AddDatapathInterface(bf.datapathName, veth.Attrs().Name); err != nil {
+		if err := odp.AddDatapathInterfaceIfNotExist(bf.datapathName, veth.Attrs().Name); err != nil {
 			return errors.Wrapf(err, "adding interface %q to datapath %q", veth.Attrs().Name, bf.datapathName)
 		}
 		return nil
@@ -394,7 +401,7 @@ func (bf bridgedFastdpImpl) attach(veth *netlink.Veth) error {
 }
 
 func (f fastdpImpl) attach(veth *netlink.Veth) error {
-	return odp.AddDatapathInterface(f.datapathName, veth.Attrs().Name)
+	return odp.AddDatapathInterfaceIfNotExist(f.datapathName, veth.Attrs().Name)
 }
 
 func configureIPTables(config *BridgeConfig) error {
@@ -404,10 +411,18 @@ func configureIPTables(config *BridgeConfig) error {
 	}
 	if config.DockerBridgeName != "" {
 		if config.WeaveBridgeName != config.DockerBridgeName {
-			err := ipt.Insert("filter", "FORWARD", 1, "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP")
+			// This is not ideal, as it does not check whether the rule is at the top
+			// of the chain.
+			found, err := ipt.Exists("filter", "FORWARD", "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP")
 			if err != nil {
 				return err
 			}
+			if !found {
+				err := ipt.Insert("filter", "FORWARD", 1, "-i", config.DockerBridgeName, "-o", config.WeaveBridgeName, "-j", "DROP")
+				if err != nil {
+					return err
+				}
+			}
 		}
 
 		dockerBridgeIP, err := FindBridgeIP(config.DockerBridgeName, nil)
@@ -435,48 +450,46 @@ func configureIPTables(config *BridgeConfig) error {
 		}
 	}
 
+	// The order among weave filter/FORWARD rules is important!
+	fwdRules := make([][]string, 0)
+
 	if config.NPC {
 		// Steer traffic via the NPC
 		_ = ipt.NewChain("filter", "WEAVE-NPC") // ignore error because it might already exist
-		// If WEAVE-NPC chain doesn't exist then next line will fail
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"); err != nil {
-			return err
-		}
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"); err != nil {
-			return err
-		}
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "DROP"); err != nil {
-			return err
-		}
+		// If WEAVE-NPC chain doesn't exist then creating a rule in the chain will fail
+		fwdRules = append(fwdRules,
+			[][]string{
+				{"-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"},
+				{"-o", config.WeaveBridgeName, "-m", "state", "--state", "NEW", "-j", "NFLOG", "--nflog-group", "86"},
+				{"-o", config.WeaveBridgeName, "-j", "DROP"},
+			}...)
 	} else {
 		// Work around the situation where there are no rules allowing traffic
 		// across our bridge. E.g. ufw
-		if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil {
-			return err
-		}
+		fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "-o", config.WeaveBridgeName, "-j", "ACCEPT"})
 	}
 
 	if !config.NPC {
 		// Create a chain for allowing ingress traffic when the bridge is exposed
 		_ = ipt.NewChain("filter", "WEAVE-EXPOSE")
-		if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"); err != nil {
-			return err
-		}
+		fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-j", "WEAVE-EXPOSE"})
 	}
 
 	// Forward from weave to the rest of the world
-	if err = ipt.AppendUnique("filter", "FORWARD", "-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"); err != nil {
-		return err
-	}
+	fwdRules = append(fwdRules, []string{"-i", config.WeaveBridgeName, "!", "-o", config.WeaveBridgeName, "-j", "ACCEPT"})
 	// and allow replies back
-	if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"); err != nil {
+	fwdRules = append(fwdRules, []string{"-o", config.WeaveBridgeName, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"})
+
+	if err := ensureRules("filter", "FORWARD", fwdRules, ipt); err != nil {
 		return err
 	}
 
 	// create a chain for masquerading
-	if err = ipt.ClearChain("nat", "WEAVE"); err != nil {
-		return errors.Wrap(err, "clearing WEAVE chain")
-	}
+	//
+	// NB: we do not clear the chain to preserve existing rules
+	// inserted by "weave expose".
+	_ = ipt.NewChain("nat", "WEAVE")
+
 	return ipt.AppendUnique("nat", "POSTROUTING", "-j", "WEAVE")
 }
 
@@ -487,3 +500,40 @@ func linkSetUpByName(linkName string) error {
 	}
 	return netlink.LinkSetUp(link)
 }
+
+// ensureRules ensures the presence of given iptables rules.
+//
+// If any rule from the list is missing, the function deletes all given
+// rules and re-appends them to ensure the order of the rules.
+func ensureRules(table, chain string, rulespecs [][]string, ipt *iptables.IPTables) error {
+	allFound := true
+
+	for _, rs := range rulespecs {
+		found, err := ipt.Exists(table, chain, rs...)
+		if err != nil {
+			return errors.Wrapf(err, "ipt.Exists(%s, %s, %s)", table, chain, rs)
+		}
+		if !found {
+			allFound = false
+			break
+		}
+	}
+
+	// All rules exist, do nothing.
+	if allFound {
+		return nil
+	}
+
+	for _, rs := range rulespecs {
+		// If any is missing, then delete all, as we need to preserve the order of
+		// given rules. Ignore errors, as rule might not exist.
+		if !allFound {
+			ipt.Delete(table, chain, rs...)
+		}
+		if err := ipt.Append(table, chain, rs...); err != nil {
+			return errors.Wrapf(err, "ipt.Append(%s, %s, %s)", table, chain, rs)
+		}
+	}
+
+	return nil
+}

net/netlink.go

@@ -0,0 +1,16 @@
+package net
+
+import (
+	"syscall"
+
+	"github.com/pkg/errors"
+	"github.com/vishvananda/netlink"
+)
+
+func LinkAddIfNotExist(link netlink.Link) error {
+	err := netlink.LinkAdd(link)
+	if err != nil && err == syscall.EEXIST {
+		return nil
+	}
+	return errors.Wrapf(err, "creating link %q", link)
+}

net/veth.go

@@ -13,7 +13,7 @@ import (
 )
 
 // create and attach a veth to the Weave bridge
-func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bool, init func(peer netlink.Link) error) (*netlink.Veth, error) {
+func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bool, errIfLinkExist bool, init func(peer netlink.Link) error) (*netlink.Veth, error) {
 	bridge, err := netlink.LinkByName(bridgeName)
 	if err != nil {
 		return nil, fmt.Errorf(`bridge "%s" not present; did you launch weave?`, bridgeName)
@@ -28,7 +28,12 @@ func CreateAndAttachVeth(name, peerName, bridgeName string, mtu int, keepTXOn bo
 			MTU:  mtu},
 		PeerName: peerName,
 	}
-	if err := netlink.LinkAdd(veth); err != nil {
+
+	linkAdd := LinkAddIfNotExist
+	if errIfLinkExist {
+		linkAdd = netlink.LinkAdd
+	}
+	if err := linkAdd(veth); err != nil {
 		return nil, fmt.Errorf(`could not create veth pair %s-%s: %s`, name, peerName, err)
 	}
 
@@ -116,7 +121,7 @@ func AttachContainer(netNSPath, id, ifName, bridgeName string, mtu int, withMult
 			id = id[:maxIDLen] // trim passed ID if too long
 		}
 		name, peerName := vethPrefix+"pl"+id, vethPrefix+"pg"+id
-		veth, err := CreateAndAttachVeth(name, peerName, bridgeName, mtu, keepTXOn, func(veth netlink.Link) error {
+		veth, err := CreateAndAttachVeth(name, peerName, bridgeName, mtu, keepTXOn, true, func(veth netlink.Link) error {
 			if err := netlink.LinkSetNsFd(veth, int(ns)); err != nil {
 				return fmt.Errorf("failed to move veth to container netns: %s", err)
 			}

plugin/net/driver.go

@@ -134,7 +134,7 @@ func (driver *driver) CreateEndpoint(create *api.CreateEndpointRequest) (*api.Cr
 
 	// create veths. note we assume endpoint IDs are unique in the first 9 chars
 	name, peerName := vethPair(create.EndpointID)
-	if _, err := weavenet.CreateAndAttachVeth(name, peerName, weavenet.WeaveBridgeName, 0, false, nil); err != nil {
+	if _, err := weavenet.CreateAndAttachVeth(name, peerName, weavenet.WeaveBridgeName, 0, false, true, nil); err != nil {
 		return nil, driver.error("JoinEndpoint", "%s", err)
 	}
 

prog/weaver/main.go

@@ -197,6 +197,7 @@ func main() {
 	mflag.StringVar(&dnsConfig.ResolvConf, []string{"-resolv-conf"}, "", "path to resolver configuration for fallback DNS lookups")
 	mflag.StringVar(&bridgeConfig.DatapathName, []string{"-datapath"}, "", "ODP datapath name")
 	mflag.BoolVar(&bridgeConfig.NoFastdp, []string{"-no-fastdp"}, false, "Disable Fast Datapath")
+	mflag.BoolVar(&bridgeConfig.NoBridgedFastdp, []string{"-no-bridged-fastdp"}, false, "Disable Bridged Fast Datapath")
 	mflag.StringVar(&trustedSubnetStr, []string{"-trusted-subnets"}, "", "comma-separated list of trusted subnets in CIDR notation")
 	mflag.StringVar(&dbPrefix, []string{"-db-prefix"}, "/weavedb/weave", "pathname/prefix of filename to store data")
 	mflag.StringVar(&procPath, []string{"-proc-path"}, "/proc", "path to reach host /proc filesystem")

prog/weaveutil/attach.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"net"
 	"os"
-	"strconv"
 	"strings"
 	"syscall"
 
@@ -15,8 +14,8 @@ import (
 )
 
 func attach(args []string) error {
-	if len(args) < 4 {
-		cmdUsage("attach-container", "[--no-multicast-route] [--keep-tx-on] [--hairpin-mode=true|false] <container-id> <bridge-name> <mtu> <cidr>...")
+	if len(args) < 3 {
+		cmdUsage("attach-container", "[--no-multicast-route] [--keep-tx-on] [--hairpin-mode=true|false] <container-id> <bridge-name> <cidr>...")
 	}
 
 	keepTXOn := false
@@ -52,16 +51,12 @@ func attach(args []string) error {
 	} else if nsHost.Equal(nsContainer) {
 		return fmt.Errorf("Container is running in the host network namespace, and therefore cannot be\nconnected to weave - perhaps the container was started with --net=host")
 	}
-	mtu, err := strconv.Atoi(args[2])
-	if err != nil && args[3] != "" {
-		return fmt.Errorf("unable to parse mtu %q: %s", args[2], err)
-	}
-	cidrs, err := parseCIDRs(args[3:])
+	cidrs, err := parseCIDRs(args[2:])
 	if err != nil {
 		return err
 	}
 
-	err = weavenet.AttachContainer(weavenet.NSPathByPid(pid), fmt.Sprint(pid), weavenet.VethName, args[1], mtu, withMulticastRoute, cidrs, keepTXOn, hairpinMode)
+	err = weavenet.AttachContainer(weavenet.NSPathByPid(pid), fmt.Sprint(pid), weavenet.VethName, args[1], 0, withMulticastRoute, cidrs, keepTXOn, hairpinMode)
 	// If we detected an error but the container has died, tell the user that instead.
 	if err != nil && !processExists(pid) {
 		err = fmt.Errorf("Container %s died", args[0])

prog/weaveutil/bridge.go

@@ -2,9 +2,7 @@ package main
 
 import (
 	"fmt"
-	"strconv"
 
-	"github.com/weaveworks/weave/common"
 	weavenet "github.com/weaveworks/weave/net"
 )
 
@@ -22,36 +20,3 @@ func detectBridgeType(args []string) error {
 	}
 	return nil
 }
-
-func createBridge(args []string) error {
-	if len(args) != 10 {
-		cmdUsage("create-bridge", "<docker-bridge-name> <weave-bridge-name> <datapath-name> <mtu> <port> <mac> <no-fastdp> <no-bridged-fastdp> <proc-path> <expect-npc>")
-	}
-
-	mtu, err := strconv.Atoi(args[3])
-	if err != nil && args[3] != "" {
-		return err
-	}
-	port, err := strconv.Atoi(args[4])
-	if err != nil {
-		return err
-	}
-	config := weavenet.BridgeConfig{
-		DockerBridgeName: args[0],
-		WeaveBridgeName:  args[1],
-		DatapathName:     args[2],
-		MTU:              mtu,
-		Port:             port,
-		Mac:              args[5],
-		NoFastdp:         args[6] != "",
-		NoBridgedFastdp:  args[7] != "",
-		NPC:              args[9] == "--expect-npc",
-	}
-	procPath := args[8]
-	bridgeType, err := weavenet.EnsureBridge(procPath, &config, common.Log)
-	if err != nil {
-		return err
-	}
-	fmt.Println(bridgeType.String())
-	return nil
-}

prog/weaveutil/main.go

@@ -17,7 +17,6 @@ func init() {
 		"help":                     help,
 		"netcheck":                 netcheck,
 		"docker-tls-args":          dockerTLSArgs,
-		"create-bridge":            createBridge,
 		"detect-bridge-type":       detectBridgeType,
 		"create-datapath":          createDatapath,
 		"delete-datapath":          deleteDatapath,